fix flaky mssql test

[techknowlogick]

In one of my other PRs I noticed that the mssql test was flaky (output here: https://github.com/go-gitea/gitea/actions/runs/18435115337/job/52527538242 ) and it was possible in rare-cases, due to timings, that the else branch below my code was triggered and c.Issue.Repo wasn't loaded causing a nil reference.

[wxiaoguang]

No , the fix is not right. There is no data race for "comment.Repo", it should NOT be used in this test.

[lunny]

It should fix the test like https://github.com/go-gitea/gitea/actions/runs/18446226509/job/52553328391?pr=35644 but it's not a RACE problem.

[wxiaoguang]

So please propose a correct fix to show your ability.

[techknowlogick]

Here is another example: https://github.com/go-gitea/gitea/actions/runs/18446226509/job/52553328391

I figured it was a race due to it being flaky, because it only matches the comment type occasionally which I thought meant it was a race due to the comments being made and which one is currently the "latest" for this test. Either way, the code below attempts ensure that when issue&repo are loaded below that they are loaded.

[wxiaoguang]

Either way, the code below attempts ensure that when issue&repo are loaded below that they are loaded.

No , the fix is not right. There is no data race for "comment.Repo", it should NOT be used in this test.

Please read the code again.

[wxiaoguang]

It should fix the test like https://github.com/go-gitea/gitea/actions/runs/18446226509/job/52553328391?pr=35644 but it's not a RACE problem.

So please propose a correct fix to show your ability.

To be clear: this PR doesn't fix. If you rerun many times, the flaky test still occur. Because you will get IsForcePush=false, then next assert fails.

Since you have wrote so many bugs and don't really understand the code base, I think I should act as a teacher to teach you how to program correctly, but not as a baby sitter to do every fix for you.

[lunny]

I guess this mostly caused by loadcomments order sequence

[wxiaoguang]

I guess this mostly caused by loadcomments order sequence

No

[wxiaoguang]

PR status change and comment creation are in 2 steps.

Even if the PR's status check passes, the comment might haven't been created. So this PR only makes the assert.True(t, lastComment.IsForcePush) fail.

[techknowlogick]

c.Issue.Repo in that function has the possibility of being nil, and depending on the conditional re: force push the tree could take us to the branch where it is referenced. With this PR, it loads the issue&repo, so that reference is no longer nil.
This PR achieves my goal of fixing the thing that was broken, in this case a nil reference which is what the logs were reporting. I did not look at other asserts, which as you pointed out will need to be looked at, but this is a fix that needs to be in place as clearly there is a chance for a panic due to a nil reference.

Regardless of my assumption on it being a race condition, which is something that could reasonably be assumed about a flaky test of this nature, and others made similar assumptions, this fixes a real problem. It is inappropriate to questions someone ability, and dismiss contributions based on previous bugs. Everyone writes bugs, regardless of their talent, and we shouldn't shame others about it, but rather build others up so that we are all better in the future.

[wxiaoguang]

Sure, everyone writes bugs, including me.

But would you respect the reviewer's work? For example: I have left many review suggestions in "Manage User Badges in the UI #31262", I didn't see you have any response or progress.

And would you please be responsible? For example: I have asked many times in "ACME certificate fails to renew (incorrect directory) #32191" and "Try to fix ACME path when renew #33668".

There are still many similar examples.

It seems that you selectively ignore my reviews and questions, it makes me feel that my time is wasted. Is it inappropriate? Why should I spend more time on you?

[lunny]

c.Issue.Repo in that function has the possibility of being nil, and depending on the conditional re: force push the tree could take us to the branch where it is referenced. With this PR, it loads the issue&repo, so that reference is no longer nil. This PR achieves my goal of fixing the thing that was broken, in this case a nil reference which is what the logs were reporting. I did not look at other asserts, which as you pointed out will need to be looked at, but this is a fix that needs to be in place as clearly there is a chance for a panic due to a nil reference.

Regardless of my assumption on it being a race condition, which is something that could reasonably be assumed about a flaky test of this nature, and others made similar assumptions, this fixes a real problem. It is inappropriate to questions someone ability, and dismiss contributions based on previous bugs. Everyone writes bugs, regardless of their talent, and we shouldn't shame others about it, but rather build others up so that we are all better in the future.

OK. FindComments has the correct ordering. But why do tests pass on other databases but fail on MSSQL? I think @wxiaoguang is right — the issue is that the push comments are created asynchronously, so their completion order can vary when FindComments is called. The test only waits for the status change, not for the comment creation to complete. Therefore, this PR doesn’t address the root cause.

---

I understand that everyone has their own strengths and weaknesses. Please remember to address or discuss them privately.

This PR doesn’t address the root cause

[wxiaoguang]

I understand that everyone has their own strengths and weaknesses. Please remember to address or discuss them privately.

What are your strengths? I don't understand why the same problems happen again and again (copy-paste duplicate code, dirty patch without test, fragile change without design), and you even blindly copied the GPL code without changing their comment, what can I say? If you believe these are the normal behaviors of a project's owner with twenty-years experiences, then that's fine, just keep doing them.

• Add theme global configuration from admin UI #35558 (review)
• Move some service configuration from app.ini to database #35607 (comment)
• Clean up the lock files created 24 hours ago #35399 (comment)
• Fix issue where commenting with a status change could partially fail. #35317 (comment)
• Fix the bug when the last code comment deleted, a review comment type comment is still in the activity list of the pull request #35133 (review)
• Fix new file bug when sign rule is parentsigned #35291 (comment)
• Fix git diff hunk 0 based start line #35047 (comment)
• Add running with falure status #35025 (comment)
• Fix users' noreply email address could be verified when sign with ssh key #35043 (comment)
• When a repository is broken but git repository could be open, then mark it as normal #34443 (comment)
• Fix bug when generating repository #34149 (comment)
• Error when pushing to renamed repository #28460 (comment)
• Fix contributors related page may leak resources #34016

[wxiaoguang]

Replaced by #35647, feel free to delete this PR or my comments if you'd like to.