Skip to content

Conversation

@nhooyr
Copy link
Contributor

@nhooyr nhooyr commented Jan 22, 2018

FatalF must not be called from outside the testing goroutine or this leads to a race condition within the testing package.

See https://golang.org/pkg/testing/#T.FailNow

@nhooyr nhooyr self-assigned this Jan 22, 2018
@nhooyr
Copy link
Contributor Author

nhooyr commented Jan 22, 2018

This fixes the CI failure in #233

@nhooyr nhooyr changed the title fix race condition in test fix race condition in integration test Jan 22, 2018
@nhooyr nhooyr changed the title fix race condition in integration test fix race condition in a integration test Jan 22, 2018
@nhooyr
Copy link
Contributor Author

nhooyr commented Jan 24, 2018

Something is wrong with CI hmm.

go func() {
for err := range watcher.Errors {
t.Fatalf("error received: %s", err)
t.Error("error received: %s", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be t.Errorf("error received: %s",err)

gdey added a commit that referenced this pull request Aug 30, 2018
The documentation for t.FailNow (which is called by t.Fatalf) states
that it should not be called in go routines outside of the test go
routine.
@gdey gdey closed this Aug 30, 2018
@nathany
Copy link
Contributor

nathany commented Aug 30, 2018

@nhooyr thanks for bringing this to our attention. #266 provides the fix.

nathany pushed a commit that referenced this pull request Aug 30, 2018
The documentation for t.FailNow (which is called by t.Fatalf) states
that it should not be called in go routines outside of the test go
routine.
radu-munteanu pushed a commit to radu-munteanu/fsnotify that referenced this pull request Feb 25, 2019
The documentation for t.FailNow (which is called by t.Fatalf) states
that it should not be called in go routines outside of the test go
routine.
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.

3 participants