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

Possible Race Condition in stdlib tests if using t.Parallel() #8

Open
eduncan911 opened this issue Jun 23, 2016 · 1 comment
Open
Assignees
Labels

Comments

@eduncan911
Copy link
Owner

eduncan911 commented Jun 23, 2016

As mentioned above, Go 1.5+ now executes all tests concurrently since GOMAXPROCS is set to the number of cores on the machine and if you set the t.Parallel() flag.

This package was originally written with a global state to keep track of the current context being run synchronously.

I have not seen the race issue come up with about a dozen projects I use this package on testing on an 8 core (16 thread) machine, with the largest one using about 80 specs (tests). Nor I have received reports from others I have asked to test it. But it is coded in such a way that it could cause an issue with tests running concurrently with the t.Parallel() flag set.

Therefore, a refactor is required to move the state up into each Given() instead of globally and to make use of channels where items must remain global (I frown on mutex.Lock()s everything).

It will most likely loosen the ability to track the Feature context with this change, and the output may be out of order. But if you are using the t.Parallel() flag, you are already accepting things to be out of order.

In the interim, I could slap a big ol' a mutex lock on the global context which would resolve the issue if it ever comes up. But, that would greatly slow things down and I would prefer to refactor around it as mentioned above.

See #4 for details.

@eduncan911 eduncan911 changed the title Possible Race Condition in stdlib tests if using t.Parallel() Refactoring: Possible Race Condition in stdlib tests if using t.Parallel() Jun 23, 2016
@eduncan911 eduncan911 self-assigned this Jun 23, 2016
@eduncan911 eduncan911 added the bug label Jun 23, 2016
@eduncan911 eduncan911 changed the title Refactoring: Possible Race Condition in stdlib tests if using t.Parallel() Possible Race Condition in stdlib tests if using t.Parallel() Jun 23, 2016
@eduncan911
Copy link
Owner Author

The major refactoring is shaping up to have the same restriction because of the new As a() and Feature() functionality I've added. This is because of the flow of writing code/BDD tests, but keeping it in the same context.

There are some tricks I am thinking of to keep it at least per func Test_...() function. But, it would involve reflection - and I am trying to stay away from reflection.

The Go stdlib does not provide a way to know if a test has the t.Parallel() function set to True.

Another thought is to make it a Flag you can pass in, that if you do use t.Parallel(), it could fall back to using Reflection to ensure it runs in parallel.

Finally.. There's Go 1.7 just around the corner. It now allows for Sub-Tests in a group. I may want to re-evaluate BDD style testing when it is released.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant