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

contrib/net/http: fix flaky TestRoundTripperNetworkError #1292

Merged
merged 2 commits into from
May 13, 2022

Conversation

nsrip-dd
Copy link
Contributor

The test was intended to cause an HTTP request to time out on the client
side, but this timeout was induced with a short sleep in the HTTP
handler. This caused the test to sometimes fail in CI. Removing the
sleep made the test fail consistently.

To force the client to time out, make the handler wait on a channel that
doesn't close until the end of the test.

This fix can be verified locally by setting the client timeout to a very
long value like 1 minute. The test will still pass.

Fixes #1291.

The test was intended to cause an HTTP request to time out on the client
side, but this timeout was induced with a short sleep in the HTTP
handler. This caused the test to sometimes fail in CI. Removing the
sleep made the test fail consistently.

To force the client to time out, make the handler wait on a channel that
doesn't close until the end of the test.

This fix can be verified locally by setting the client timeout to a very
long value like 1 minute. The test will still pass.

Fixes #1291.
@nsrip-dd nsrip-dd requested a review from a team May 13, 2022 14:00
@nsrip-dd nsrip-dd added this to the 1.39.0 milestone May 13, 2022
Copy link
Contributor

@ajgajg1134 ajgajg1134 left a comment

Choose a reason for hiding this comment

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

Thanks! This is definitely better than the sleep

@nsrip-dd nsrip-dd merged commit 86a56ed into v1 May 13, 2022
@nsrip-dd nsrip-dd deleted the nick.ripley/fix-flaky-roundtripper-test branch May 13, 2022 14:12
@felixge
Copy link
Member

felixge commented May 13, 2022

Awesome. Thanks @nsrip-dd and @ajgajg1134 for review 🙇

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.

contrib/net/http: TestRoundTripperNetworkError is (still) flaky
3 participants