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

Rewrite Http2 test to be less flaky #6818

Merged
merged 2 commits into from Aug 2, 2021
Merged

Conversation

yschimke
Copy link
Collaborator

@yschimke yschimke commented Aug 2, 2021

No description provided.

@yschimke yschimke added android Relates to usage specifically on Android jdk8 labels Aug 2, 2021
@yschimke
Copy link
Collaborator Author

yschimke commented Aug 2, 2021

Existing Failure

HttpOverHttp2Test > responseHeadersAfterGoaway(Protocol, MockWebServer) > okhttp3.internal.http2.HttpOverHttp2Test.responseHeadersAfterGoaway(Protocol, MockWebServer)[1] FAILED
    org.opentest4j.AssertionFailedError: 
    expected: "ABC"
     but was: null
        at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
        at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
        at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
        at okhttp3.internal.http2.HttpOverHttp2Test.responseHeadersAfterGoaway(HttpOverHttp2Test.java:1814)

Ongoing saga of #4836

@yschimke
Copy link
Collaborator Author

yschimke commented Aug 2, 2021

New failure

HttpOverHttp2Test > responseHeadersAfterGoaway(Protocol, MockWebServer) > okhttp3.internal.http2.HttpOverHttp2Test.responseHeadersAfterGoaway(Protocol, MockWebServer)[2] FAILED
    okhttp3.internal.http2.StreamResetException: stream was reset: PROTOCOL_ERROR
        at okhttp3.internal.http2.Http2Stream.takeHeaders(Http2Stream.kt:148)
        at okhttp3.internal.http2.Http2ExchangeCodec.readResponseHeaders(Http2ExchangeCodec.kt:97)
        at okhttp3.internal.connection.Exchange.readResponseHeaders(Exchange.kt:110)
        at okhttp3.internal.http.CallServerInterceptor.intercept(CallServerInterceptor.kt:93)
        at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.kt:109)

@yschimke yschimke marked this pull request as draft August 2, 2021 18:08
@yschimke yschimke added the tests Fix relates to tests not code label Aug 2, 2021
@yschimke yschimke marked this pull request as ready for review August 2, 2021 19:58
@yschimke
Copy link
Collaborator Author

yschimke commented Aug 2, 2021

@swankjesse landing as it seems to reduce flakes after some builds. Will observe, and follow up with any concerns or review comments.

@yschimke yschimke merged commit e58548c into square:master Aug 2, 2021

assertThat(bodies.remove()).isEqualTo("DEF");

if (errors.isEmpty()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know the root cause for the flakiness in this case, but I suggest that you look at a few techniques I used in the past to make similar tests deterministic and that may apply here: #4792, #4728, #4772.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, will do. I wanted to get something in so we have PR and master with working builds. So I hope you forgive my hackish attempt to start!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will get familiar with QueueDispatcher, there are a couple more flaky tests here.

Copy link
Member

@swankjesse swankjesse left a comment

Choose a reason for hiding this comment

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

LGTM

I prefer LinkedBlockingDeque over CountDownLatch for tests like this; I think it’s easier to follow.

@yschimke
Copy link
Collaborator Author

I'm due to come back and fix this, it's sitting in my github notification queue taunting me for months now. But glad I've fixed for now, even in a hacky way, as my priority was green PR builds.

I will get to it though :)

@yschimke yschimke deleted the http2flakes branch January 6, 2022 08:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
android Relates to usage specifically on Android tests Fix relates to tests not code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants