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.

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

fix issue #34 #35

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

pauleibye
Copy link

Add stepDefs mutex and lock in appropriate places to prevent races.

Enables a test to be switched back to use the default parallel = true flag, the example api and hooks tests still fail with parallelization, but after looking it seems that's an issue with how they were written (using package global variables) rather than a bug with concurrent execution.

#34

Copy link
Member

@aaronc aaronc left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution @pauleibye ! Have you run tests with the -race flag on this branch? When I checkout this branch locally and run go test ./... -race, I'm still getting race errors.

@pauleibye
Copy link
Author

pauleibye commented Aug 26, 2024

Thanks for your contribution @pauleibye ! Have you run tests with the -race flag on this branch? When I checkout this branch locally and run go test ./... -race, I'm still getting race errors.

that's a good point, no I was not. This commit did fix the race conditions in our codebase, and our test suite uses the race flag. The test that I removed the NonParallel option from might be the problem still.

edit: it looks like the race happens in simple_test.go, which might be expected given that this test uses a global that the cases all try to overwrite.
image

@pauleibye
Copy link
Author

pauleibye commented Aug 26, 2024

reverted simple_test.go to main, which sets the non parallel flag on the non pointer test since it relies on overwriting a shared variable in a specific order

@aaronc
Copy link
Member

aaronc commented Aug 27, 2024

I'm still getting race errors locally. I'm going to look at this a bit and see if I can figure out what's going on.

@aaronc aaronc self-assigned this Aug 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants