Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Check for valid user token in integration tests #21520

Merged
merged 6 commits into from
Oct 20, 2022
Merged

Check for valid user token in integration tests #21520

merged 6 commits into from
Oct 20, 2022

Conversation

nagos
Copy link
Contributor

@nagos nagos commented Oct 20, 2022 •
edited
Loading

Added checks for logged user token.

Some builds fail at unrelated tests, due to missing token.

Example:
https://drone.gitea.io/go-gitea/gitea/62011/2/14

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 20, 2022
@nagos
Copy link
Contributor Author

nagos commented Oct 20, 2022

My bad. fixed.

@GiteaBot 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 Oct 20, 2022
@silverwind
Copy link
Member

silverwind commented Oct 20, 2022 •
edited
Loading

Will it fix #19961?

Not sure I understand correctly, but I think this won't reduce random failures, but will change the point of failure to this new assertion, right?

@nagos
Copy link
Contributor Author

nagos commented Oct 20, 2022 •
edited
Loading

Yes, #19961 is the same problem.

I think i found a root cause of this problem.
Go tests are run in parallel, and we get race condition on tokenCounter variable.
https://github.com/go-gitea/gitea/blob/main/tests/integration/integration_test.go#L262

My last commit changed counter to random value. This should prevent duplicate token ids.

@lunny
Copy link
Member

lunny commented Oct 20, 2022

Yes, #19961 is the same problem.

I think i found a root cause of this problem. Go tests are run in parallel, and we get race condition on tokenCounter variable. https://github.com/go-gitea/gitea/blob/main/tests/integration/integration_test.go#L262

My last commit changed counter to random value. This should prevent duplicate token ids.

Use an atomic maybe better. Random cannot ensure there is no duplicated token name.

@silverwind silverwind mentioned this pull request Oct 20, 2022
Closed
zeripath
zeripath approved these changes Oct 20, 2022
View reviewed changes
Copy link
Contributor

@zeripath zeripath left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we run tests generally in parallel - many things would be broken in that case - but I do suspect that there are things that are run in parallel and so do suspect that this will solve the problem.

@GiteaBot 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 Oct 20, 2022
@zeripath zeripath merged commit ffa4f4b into go-gitea:main Oct 20, 2022
zjjhot added a commit to zjjhot/gitea that referenced this pull request Oct 21, 2022
@zjjhot
Merge remote-tracking branch 'upstream/main'
8c21404 
* upstream/main:
  [skip ci] Updated translations via Crowdin
  Check for valid user token in integration tests (go-gitea#21520)
  Ignore error when retrieving changed PR review files (go-gitea#21487)
  move invite by mail to services package (go-gitea#21513)
  Enable Monaco automaticLayout (go-gitea#21515)
  Update macOS install command (go-gitea#21507)
  [skip ci] Updated translations via Crowdin
  Suppress `ExternalLoginUserNotExist` error (go-gitea#21504)
  Revert increased width on pull pages (go-gitea#21470)
  Add team member invite by email (go-gitea#20307)
  Disable the 'Add File' button when not able to edit repo (go-gitea#21503)
  Remove vitest globals (go-gitea#21505)
  Fix branch dropdown shifting on page load (go-gitea#21428)
@lunny
Copy link
Member

lunny commented Oct 21, 2022

Please send backport to v1.17

silverwind pushed a commit to silverwind/gitea that referenced this pull request Oct 21, 2022
@nagos @wxiaoguang @silverwind
Check for valid user token in integration tests (go-gitea#21520)
c1be31d 
Added checks for logged user token.

Some builds fail at unrelated tests, due to missing token.

Example:
https://drone.gitea.io/go-gitea/gitea/62011/2/14

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
@silverwind silverwind mentioned this pull request Oct 21, 2022
Merged
@silverwind silverwind added the skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. label Oct 21, 2022
@silverwind silverwind mentioned this pull request Oct 21, 2022
Closed
lunny pushed a commit that referenced this pull request Oct 22, 2022
@silverwind @nagos @wxiaoguang
Check for valid user token in integration tests (#21520) (#21529)
8043fbc 
Backport #21520

Added checks for logged user token.

Some builds fail at unrelated tests, due to missing token.

Co-authored-by: Vladimir Yakovlev <nagos@inbox.ru>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
@lunny lunny added the backport/done All backports for this PR have been created label Oct 24, 2022
vanhoang1107 added a commit to vanhoang1107/gitea that referenced this pull request Nov 7, 2022
@vanhoang1107
Merge remote-tracking branch 'src/release/v1.17' into release/v1.17
dc24c81 
* src/release/v1.17: (48 commits)
  Fix repository adoption on Windows (go-gitea#21646) (go-gitea#21651)
  Sync git hooks when config file path changed (go-gitea#21619) (go-gitea#21625)
  Fix package access for admins and inactive users (go-gitea#21580) (go-gitea#21592)
  Fix `Timestamp.IsZero` (go-gitea#21593) (go-gitea#21604)
  Added check for disabled Packages (go-gitea#21540) (go-gitea#21614)
  Fix issues count bug (go-gitea#21600)
  Update milestone counters when issue is deleted (go-gitea#21459) (go-gitea#21586)
  Suppress `ExternalLoginUserNotExist` error (go-gitea#21504) (go-gitea#21572)
  support binary deploy in npm packages (go-gitea#21589)
  SessionUser protection against nil pointer dereference (go-gitea#21581)
  Case-insensitive NuGet symbol file GUID (go-gitea#21409) (go-gitea#21575)
  Prevent Authorization header for presigned LFS urls (go-gitea#21531) (go-gitea#21569)
  Update binding to fix bugs (go-gitea#21560)
  Check for valid user token in integration tests (go-gitea#21520) (go-gitea#21529)
  Fix generating compare link (go-gitea#21519) (go-gitea#21530)
  Ignore error when retrieving changed PR review files (go-gitea#21487) (go-gitea#21524)
  Enable Monaco automaticLayout (go-gitea#21516)
  Fix incorrect notification commit url (go-gitea#21479) (go-gitea#21483)
  Display total commit count in hook message (go-gitea#21400) (go-gitea#21481)
  Enforce grouped NuGet search results (go-gitea#21442) (go-gitea#21480)
  ...
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@zeripath zeripath zeripath approved these changes

@lunny lunny lunny approved these changes

Assignees
No one assigned
Labels
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@nagos @silverwind @lunny @zeripath @GiteaBot @wxiaoguang