search

franela / goblin

Minimal and Beautiful Go testing framework
MIT License
884 stars 79 forks source link

Fixed a few race conditions #92

Closed Neirpyc closed 3 years ago

Neirpyc commented 3 years ago

List of changes

Why?

go test -race on this package revealed multiple race conditions, which should be fixed by this PR.

Now, the default tests of this package pass with the -race flag without issues.

marcosnils commented 3 years ago

Thx for the change @Neirpyc . How about if we add the -race flag to the CI scripts since that after merging this PR goblin will be race-free?

Neirpyc commented 3 years ago

@marcosnils I'd rather suggest that we add a new test file with build flags // +build race containing only test matching TestSomething_Race so running go test isn't any slower, go test -run=Race says that there is no tests, and go test -race -run=Race would run only a few specific test, to generate theses three race conditions.

marcosnils commented 3 years ago

I'd rather suggest that we add a new test file with build flags // +build race containing only test matching TestSomething_Race so running go test isn't any slower, go test -run=Race says that there is no tests, and go test -race -run=Race would run only a few specific test, to generate theses three race conditions.

I don't think that's necessary for now.. doesn't it? I believe we're over-optimizing at this point since goblin tests usually don't take that much time to run..

Neirpyc commented 3 years ago

Yeah, I guess you're right. Yet, this solution would give the choice to the user as running go test -race would behave the same in both cases. But it turns out that, as we have to compile two times if we run both tests separately, it's not worth it. Just do what you think is the best, and feel free to revert my second commit, I don't really care ^^.

marcosnils commented 3 years ago

Just do what you think is the best, and feel free to revert my second commit, I don't really care ^^.

:+1: I think I'll merge all tests for the moment and make go test -race ./... run everything since goblin tests usually run very fast for now.

Neirpyc commented 3 years ago

All right! Another thing to do could be finding projects using Goblin, and running their tests with -race to make sure there is no data race left in the Goblin's code which are not covered by Goblin's tests ^^

marcosnils commented 3 years ago

Another thing to do could be finding projects using Goblin, and running their tests with -race to make sure there is no data race left in the Goblin's code which are not covered by Goblin's tests ^^

That's a very good idea.. unfortunately.. I don't have the time for such quest ATM :(