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: remove use of testing.T.FailNow() inside goroutine #1601

Merged
merged 1 commit into from Feb 6, 2020

Conversation

alrs
Copy link
Contributor

@alrs alrs commented Feb 5, 2020

This fixes TestClientAutoRefreshShutdownRace() to not fire t.Fatal() inside a goroutine.

t.Fatal() is a convenience function that calls t.Log() and t.FailNow(). t.FailNow() should not be called from inside a goroutine. From the godoc:

FailNow must be called from the goroutine running the test or benchmark function, not from other goroutines created during the test.

This replaces the done channel with an error channel, and tests the error variable outside the goroutine.

@alrs alrs requested a review from bai as a code owner February 5, 2020 17:22
@ghost ghost added the cla-needed label Feb 5, 2020
@alrs
Copy link
Contributor Author

alrs commented Feb 5, 2020

I'm happy to fill out a CLA, should someone provide a link to one.

@alrs alrs changed the title fix: remove use of t.testing.FailNow() inside goroutine fix: remove use of testing.T.FailNow() inside goroutine Feb 5, 2020
@bai
Copy link
Contributor

bai commented Feb 5, 2020

Here we go, it's actually one of the failing checks: https://github.com/Shopify/sarama/pull/1601/checks?check_run_id=428029640 👀

@alrs
Copy link
Contributor Author

alrs commented Feb 5, 2020

@bai CLA is complete.

@ghost ghost removed the cla-needed label Feb 5, 2020
@bai
Copy link
Contributor

bai commented Feb 6, 2020

Thanks for contributing.

@bai bai merged commit 9d2cdb2 into IBM:master Feb 6, 2020
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.

None yet

2 participants