Followup to pinned Issues

[JakobDev]

This addressees some things from #24406 that came up after the PR was merged. Mostly from @delvh.

[silverwind]

Test failure unrelated.

[silverwind]

Unpin works now for me.

[silverwind]

The TestAPIListPinnedIssues failure is now showing on other PRs as well, so it's definitely a flaky test:

https://github.com/go-gitea/gitea/actions/runs/5098326031/jobs/9165402897?pr=24948

@JakobDev can you check it? If you think you can not fix it, please skip the test on CI. We can not tolerate flaky tests.

router: completed POST /api/v1/repos/user2/repo1/issues/3/pin?token=707257d4850c020f16fee26af4032734a2d96761 for test-mock:12345, 204 No Content in 126.3ms @ repo/issue_pin.go:16(repo.PinIssue)
    testlogger.go:60: 2023/05/27 10:33:47 ...eb/routing/logger.go:102:func1() [I] router: completed GET /api/v1/repos/user2/repo1/issues/pinned for test-mock:12345, 200 OK in 6.0ms @ repo/issue_pin.go:175(repo.ListPinnedIssues)
    api_issue_pin_test.go:170: 
        	Error Trace:	/home/runner/work/gitea/gitea/tests/integration/api_issue_pin_test.go:170
        	Error:      	Not equal: 
        	            	expected: 1
        	            	actual  : 0
        	Test:       	TestAPIListPinnedIssues
panic: runtime error: index out of range [0] with length 0 [recovered]
	panic: runtime error: index out of range [0] with length 0

[silverwind]

I've seen this TestAPIListPinnedIssues failure a few times now, it seems like every other CI run fails with it, definitely a important issue currently.

[JakobDev]

Is there any Way to cause this failure replaceable?

[silverwind]

Is there any Way to cause this failure replaceable?

If you can run integration locally (I personally have not figured it out yet how to), try running it a few hundret times, maybe while creating a bit of load in the background:

while true; do GITEA_CONF=tests/sqlite.ini go test -tags sqlite,sqlite_unlock_notify -count=1 -run TestAPIListPinnedIssues ./tests/integration done

Otherwise, try adding some debug prints here maybe and run it on the CI.

[wxiaoguang]

About CI failure, I guess it's caused by this line:

issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{RepoID: repo.ID})

Sometimes you get a plain issue, sometimes you get a PR issue (IsPullRequest=true), database doesn't guarantee the result's order if the query is only "SELECT" without "ORDER BY"

-> Test query must have order by explicilty to avoid unstable results #24963

---

And I can see there are also some (unrelated to the failure) bugs in old code, eg: return is missing for these ctx.Error. Similar mistakes happen quite frequently in many PRs, that's why I suggested #24120 but it seems not getting enough interest. Maybe it needs to mention more maintainers to take a look.

	// If we don't do this, it will crash when trying to add the unpin event to the comment history
	err = issue.LoadRepo(ctx)
	if err != nil {
		ctx.Error(http.StatusInternalServerError, "LoadRepo", err)
		// FIXME: return here
	}

	err = issue.Unpin(ctx, ctx.Doer)
	if err != nil {
		ctx.Error(http.StatusInternalServerError, "UnpinIssue", err)
		// FIXME: return here
	}

	ctx.Status(http.StatusNoContent)

[JakobDev]

It is really weird, that the test suite gives IsPull a random value if you don't set it. I have now ut explicit to false. Let's see if that fixes the Problem.

[wxiaoguang]

It is really weird, that the test suite gives IsPull a random value if you don't set it. I have now ut explicit to false. Let's see if that fixes the Problem.

See my comment above, there are more than one issue records for issues_model.Issue{RepoID: repo.ID} in database, some of them are plain issues, some of them are PR issues.

Sometimes you get a plain issue, sometimes you get a PR issue (IsPullRequest=true), database doesn't guarantee the result's order if the query is only "SELECT" without "ORDER BY"
-> Test query must have order by explicilty to avoid unstable results #24963

#24963 will fix this problem for all tests, including this TestAPIListPinnedIssues

[wxiaoguang]

Overall LGTM. Two nits:

• log.Error(err.Error()) could be improved to log.Error("{detailed message}: %v", err), to provide more useful information in log.
  • And log.Error(userString) is not right in some cases, if there is a % in userString, it breaks the formatter.
• , IsPull: false could be reverted to before, because by default IsPull is false, this statement is a no-op.

[delvh]

If I had to guess, there are still some routes that don't handle the case "issue cannot pinned as too many are pinned" correctly.
Either way, I can accept this PR as it is now.

[silverwind]

Need to run make fmt.

[JakobDev]

All requested changes have been done!

Need to run make fmt.

Why do I forget this so often?