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.

Sign up for GitHub

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

Jump to bottom

Add connection pool pre-warming tests #8358

Merged
merged 1 commit into from
Apr 12, 2024
Merged

Add connection pool pre-warming tests #8358

merged 1 commit into from
Apr 12, 2024

Conversation

ean5533
Copy link
Collaborator

@ean5533 ean5533 commented Apr 11, 2024

  1. The first test just needed to be uncommented now that Redo TaskFaker's internal queue #8348 is merged
  2. A second test was added to exercise http2-specific functionality

@ean5533
Add connection pool pre-warming tests
510b46a 
1. The first test just needed to be uncommented now that #8348 is merged
2. A second test was added to exercise http2-specific functionality
@ean5533 ean5533 requested a review from swankjesse April 11, 2024 23:53
yschimke
yschimke approved these changes Apr 12, 2024
View reviewed changes
Copy link
Collaborator

@yschimke yschimke left a comment

Choose a reason for hiding this comment

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

Nice, very readable.

@ean5533 ean5533 merged commit 73c3ea5 into master Apr 12, 2024
20 checks passed
@ean5533 ean5533 deleted the enelson-pool-test branch April 12, 2024 18:57
swankjesse
swankjesse reviewed Apr 13, 2024
View reviewed changes
okhttp/src/test/java/okhttp3/internal/connection/ConnectionPoolTest.kt
forceConnectionsToExpire(pool, routePlanner, expireTime)
assertThat(pool.connectionCount()).isEqualTo(2)

// Excess connections aren't removed until they idle out, even if no longer needed
Copy link
Member

Choose a reason for hiding this comment

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

Great comments here

swankjesse
swankjesse approved these changes Apr 13, 2024
View reviewed changes
okhttp/src/test/java/okhttp3/internal/connection/ConnectionPoolTest.kt
assertThat(pool.connectionCount()).isEqualTo(1)
}

@Test fun connectionPreWarmingHttp2() {
Copy link
Member

Choose a reason for hiding this comment

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

This is so cool

@yschimke
Copy link
Collaborator 

ConnectionPoolTest > connectionPreWarmingHttp2() FAILED
    org.opentest4j.AssertionFailedError: expected:<[2]> but was:<[1]>
        at app//okhttp3.internal.connection.ConnectionPoolTest.connectionPreWarmingHttp2(ConnectionPoolTest.kt:256)

This is still 50% flaky.

@ean5533
Copy link
Collaborator Author

ean5533 commented Apr 20, 2024

Hm, interesting, I don't immediately know what would cause any non-determinism in this test. I'll take a look on Monday.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@swankjesse swankjesse swankjesse approved these changes

@yschimke yschimke yschimke approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ean5533 @yschimke @swankjesse