Fix parallel creating commit status bug with tests #21911

lunny · 2022-11-23T10:26:12Z

This PR is a follow up of #21469

delvh

The second test in the PR title seems redundant.

Also, should we label this PR as skip-changelog, or is that implied in kind/testing?

tests/integration/repo_commits_test.go

delvh · 2022-11-24T16:11:10Z

Also, a comment why we ignore Sqlite would be good.

delvh · 2022-11-28T15:53:15Z

Is this PR alive or dead?

lunny · 2022-11-29T12:12:59Z

Is this PR alive or dead?

Please review again.

lunny · 2022-11-29T12:13:20Z

Is this PR alive or dead?

Please review again.

lunny · 2022-11-30T16:41:13Z

make L-G-T-M work

wxiaoguang · 2022-11-30T17:07:17Z

Some hints:

For postgresql, if the INSERT INTO encounters duplicated key and reports error, the transaction can not continue any more. So this solution is flawed for postgresql (in rare cases). For other databases (mssql, mysql, sqlite3), if the INSERT INTO fails, the transaction can still continue, so the approach could work.

(Although I use this approach a lot, I just realized that there is a problem with postgresql recently .....)

harryzcy · 2022-12-01T04:46:46Z

Does this PR introduce a DATA RACE error in the integration test?

==================
WARNING: DATA RACE
Read at 0x000008cb6420 by goroutine 239777:
  code.gitea.io/gitea/tests/integration.getTokenForLoggedInUser()
      /drone/src/tests/integration/integration_test.go:268 +0x54
  code.gitea.io/gitea/tests/integration.NewAPITestContext()
      /drone/src/tests/integration/api_helper_for_declarative_test.go:36 +0x8c
  code.gitea.io/gitea/tests/integration.TestRepoCommitsStatusParallel.func1.1()
      /drone/src/tests/integration/repo_commits_test.go:142 +0x84
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1446 +0x188
  testing.(*T).Run.func1()
      /usr/local/go/src/testing/testing.go:1493 +0x40

Previous write at 0x000008cb6420 by goroutine 239776:
  code.gitea.io/gitea/tests/integration.getTokenForLoggedInUser()
      /drone/src/tests/integration/integration_test.go:268 +0x70
  code.gitea.io/gitea/tests/integration.NewAPITestContext()
      /drone/src/tests/integration/api_helper_for_declarative_test.go:36 +0x8c
  code.gitea.io/gitea/tests/integration.TestRepoCommitsStatusParallel.func1.1()
      /drone/src/tests/integration/repo_commits_test.go:142 +0x84
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1446 +0x188
  testing.(*T).Run.func1()
      /usr/local/go/src/testing/testing.go:1493 +0x40

Goroutine 239777 (running) created at:
  testing.(*T).Run()
      /usr/local/go/src/testing/testing.go:1493 +0x564
  code.gitea.io/gitea/tests/integration.TestRepoCommitsStatusParallel.func1()
      /drone/src/tests/integration/repo_commits_test.go:141 +0x138
  code.gitea.io/gitea/tests/integration.TestRepoCommitsStatusParallel.func2()
      /drone/src/tests/integration/repo_commits_test.go:147 +0x54

Goroutine 239776 (running) created at:
  testing.(*T).Run()
      /usr/local/go/src/testing/testing.go:1493 +0x564
  code.gitea.io/gitea/tests/integration.TestRepoCommitsStatusParallel.func1()
      /drone/src/tests/integration/repo_commits_test.go:141 +0x138
  code.gitea.io/gitea/tests/integration.TestRepoCommitsStatusParallel.func2()
      /drone/src/tests/integration/repo_commits_test.go:147 +0x54
==================
--- FAIL: TestRepoCommitsStatusParallel (0.88s)

lunny · 2022-12-01T09:50:12Z

I think you are right.

 testlogger.go:77: 2022/12/01 06:47:08 ...pi/v1/repo/status.go:65:NewCommitStatus() [E] [63884dec-12] CreateCommitStatus: NewCommitStatus[repo_id: 1, user_id: 2, sha: 65f1bf27bc3bf70f64657658635e66094edbcb4d]: generate commit status index failed: pq: current transaction is aborted, commands ignored until end of transaction block
    testlogger.go:77: 2022/12/01 06:47:08 ...eb/routing/logger.go:98:func1() [I] [63884dec-12] router: completed POST /api/v1/repos/user2/repo1/statuses/65f1bf27bc3bf70f64657658635e66094edbcb4d?token=44f685f4ffd298711136bc5065ee155259e15ba8 for , 500 Internal Server Error in 181.6ms @ repo/status.go:20(repo.NewCommitStatus)
    --- FAIL: TestRepoCommitsStatusParallel/ParallelCreateStatus_2 (0.47s)
        pull_status_test.go:104: 
            	Error Trace:	/drone/src/integration_test.go:336
            	            				/drone/src/integration_test.go:165
            	            				/drone/src/pull_status_test.go:104
            	            				/drone/src/repo_commits_test.go:141
            	Error:      	Not equal: 
            	            	expected: 201
            	            	actual  : 500
            	Test:       	TestRepoCommitsStatusParallel/ParallelCreateStatus_2
            	Messages:   	Request: POST /api/v1/repos/user2/repo1/statuses/65f1bf27bc3bf70f64657658635e66094edbcb4d?token=44f685f4ffd298711136bc5065ee155259e15ba8
        pull_status_test.go:104: Response: {"message":"","url":"http:https://localhost:3002/api/swagger"}

lunny · 2022-12-02T03:37:35Z

How did you test that? I think we have enabled RACE_ENABLED in our CI, but I couldn't find the log

harryzcy · 2022-12-02T03:43:59Z

It's in my PR, https://drone.gitea.io/go-gitea/gitea/63992. I got the issue after merging from main.

* giteaofficial/main: docs: add `Edit this page` (go-gitea#21981) refactor some functions to support ctx as first parameter (go-gitea#21878) Remove deprecated packages & staticcheck fixes (go-gitea#22012) Add pnpm to packages/overview (go-gitea#22008) Update to Alpine 3.17 (go-gitea#21904) Update gitea-vet to check FSFE REUSE (go-gitea#22004) Multiple improvements for comment edit diff (go-gitea#21990) Remove session in api tests (go-gitea#21984) Remove duplicate "Actions" label in mobile view (go-gitea#21974) Fix generate index failure possibility on postgres (go-gitea#21998) Use path not filepath in template filenames (go-gitea#21993) Update chroma to v2.4.0 (go-gitea#22000) Util type to parse ref name (go-gitea#21969) Skip initing LFS storage if disabled (go-gitea#21996) Fix parallel creating commit status bug with tests (go-gitea#21911) Skip initing disabled storages (go-gitea#21985) Fix leaving organization bug on user settings -> orgs (go-gitea#21983) Fix typos (go-gitea#21979) Normalize `AppURL` according to RFC 3986 (go-gitea#21950)

backport #21911 backport #21998 Co-authored-by: silverwind <[email protected]>

Add test for parallel creating commit status tests

12519e8

lunny added the type/testing label Nov 23, 2022

delvh reviewed Nov 24, 2022

View reviewed changes

tests/integration/repo_commits_test.go Outdated Show resolved Hide resolved

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 24, 2022

ignore sqlite3

d0408c4

lunny added 4 commits November 25, 2022 00:12

Merge branch 'main' into lunny/commit_status

e2b4c2a

Merge branch 'main' into lunny/commit_status

fd79a0b

Merge branch 'main' into lunny/commit_status

9df1f90

Fix test

a1e3418

lunny added 4 commits November 29, 2022 18:03

Fix test

0fb5677

Merge branch 'main' into lunny/commit_status

09cd8bb

Fix bug

0d4c6b3

remove sqlite condition in test

bf50a91

lunny added type/bug backport/v1.17 outdated/backport/v1.18 This PR should be backported to Gitea 1.18 labels Nov 29, 2022

lunny added this to the 1.19.0 milestone Nov 29, 2022

lunny changed the title ~~Add test for parallel creating commit status tests~~ Fix parallel creating commit status bug with tests Nov 29, 2022

Remove unused const variable

a46e7e2

wolfogre approved these changes Nov 29, 2022

View reviewed changes

GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Nov 29, 2022

Merge branch 'main' into lunny/commit_status

63a51f0

lafriks approved these changes Nov 30, 2022

View reviewed changes

GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Nov 30, 2022

Merge branch 'main' into lunny/commit_status

a295bf9

lunny merged commit b2c4870 into go-gitea:main Nov 30, 2022

lunny deleted the lunny/commit_status branch November 30, 2022 16:41

This was referenced Nov 30, 2022

Fix parallel creating commit status bug with tests (#21911) #21989

Merged

Fix parallel creating commit status bug with tests (#21911) #21991

Closed

lunny added the backport/done All backports for this PR have been created label Nov 30, 2022

lunny removed the backport/v1.17 label Dec 6, 2022

lunny added a commit that referenced this pull request Dec 13, 2022

Fix parallel creating commit status bug with tests (#21911) (#21989)

c36a1bc

backport #21911 backport #21998 Co-authored-by: silverwind <[email protected]>

AbdulrhmnGhanem mentioned this pull request Dec 30, 2022

Fork updates from go-gitea/gitea main #58 kitspace/gitea#59

Merged

go-gitea locked and limited conversation to collaborators May 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix parallel creating commit status bug with tests #21911

Fix parallel creating commit status bug with tests #21911

lunny commented Nov 23, 2022 •

edited

Loading

delvh left a comment

delvh commented Nov 24, 2022

delvh commented Nov 28, 2022

lunny commented Nov 29, 2022

lunny commented Nov 29, 2022

lunny commented Nov 30, 2022

wxiaoguang commented Nov 30, 2022 •

edited

Loading

harryzcy commented Dec 1, 2022

lunny commented Dec 1, 2022

lunny commented Dec 2, 2022

harryzcy commented Dec 2, 2022 •

edited

Loading

Fix parallel creating commit status bug with tests #21911

Fix parallel creating commit status bug with tests #21911

Conversation

lunny commented Nov 23, 2022 • edited Loading

delvh left a comment

Choose a reason for hiding this comment

delvh commented Nov 24, 2022

delvh commented Nov 28, 2022

lunny commented Nov 29, 2022

lunny commented Nov 29, 2022

lunny commented Nov 30, 2022

wxiaoguang commented Nov 30, 2022 • edited Loading

harryzcy commented Dec 1, 2022

lunny commented Dec 1, 2022

lunny commented Dec 2, 2022

harryzcy commented Dec 2, 2022 • edited Loading

lunny commented Nov 23, 2022 •

edited

Loading

wxiaoguang commented Nov 30, 2022 •

edited

Loading

harryzcy commented Dec 2, 2022 •

edited

Loading