Add test for ExtendCommentTreePathLength migration and fix bugs

[mithileshgupta12]

No description provided.

[wxiaoguang]

I don't think it needs AI to write redundant code to test a well-tested framework function ModifyColumn.

v322 only calls ModifyColumn clearly.

[mithileshgupta12]

@wxiaoguang I did not write this with AI. I was just looking at the migrations and saw that we have test for v321 which is using ModifyColumn as well. So wrote similar for v322. But fair enough

[wxiaoguang]

Thank you for the PR, it just reads like some similar PRs I have read 🤣

Could you elaborate why you started the PR? Since I don't think it's necessary to test a framework function. For example: we don't test the file content after we call os.WriteFile, we just assume the framework should work as expected.

[mithileshgupta12]

I agree with your point about not testing framework functions. But v321_test.go exists and also tests ModifyColumn. I was just following same pattern for consistency. Should we remove v321_test.go or is there a reason it needs testing that
doesn't apply to v322?

[wxiaoguang]

Maybe some people just copied&pasted code at that time .... and actually v321_test is not quite right: it incorrectly used LONGTEXT for the Notice.Description, so that case asserts nothing ....

Anyway, I am neutral for such tests. So either is fine to me.

[wxiaoguang]

v321 exists due to another reason:

This PR upgrade xorm to v1.3.10 which fixed a bug when both longtext json tags in the struct field. The longtext will be ignored and json will be considered as text

So its test makes sure "LONGTEXT JSON" works.

[mithileshgupta12]

Thanks for the explanation @wxiaoguang. Should I close this pr as we don't need to test framework functions. Learned a lot in the process though.

[wxiaoguang]

Or I think we can keep it, and by the way fix the bug in v321_test ?

Actually I found a new bug: the v323 should be in v1_26 package because it (#32751) doesn't belong to 1.25

I can also help if there is anything unclear.

[mithileshgupta12]

Yes I will fix the issues. Can you please explain what needs to be fixed in v321_test. You mentioned it
incorrectly uses LONGTEXT for Notice.Description - should I update the
test to properly verify the LONGTEXT JSON behavior?

[wxiaoguang]

v321_test: the Notice struct needs to have a "Description TEXT" field, but not a "LONGTEXT" field. Because the task is to convert the TEXT to LONGTEXT.

---

should I update the test to properly verify the LONGTEXT JSON behavior?

No to the Notice. The TEXT JSON only belongs to another struct.

[mithileshgupta12]

Got it. Will push the fix shortly

[mithileshgupta12]

@wxiaoguang pushed the fix for v321_test.go

[wxiaoguang]

Thank you very much. I will make some more changes to address "Actually I found a new bug: the v323 should be in v1_26 package"

[wxiaoguang]

Made some more changes:

1. Move 323 to the correct package
2. Avoid using for-loop to check tables (if there is any typo in table name, the for-loop asserts nothing)