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

Make tests faster, by reducing timeout margins #516

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

fabianfett
Copy link
Member

Motivation

We like fast tests. For this reason we enabled parallel testing recently. However there are a number of tests that have unnecessary high timeout margins. If we reduce those tests will be even faster.

Changes

  • Reduce connection timeout numbers from 10sec (default) down to 2sec (explicit)
  • Clean up a number of tests. (Removed last instance of should fail)

Result

Faster and cleaner tests

@fabianfett fabianfett added the semver/none For PRs that when merged do not need a bump in version number. label Dec 4, 2021
@fabianfett
Copy link
Member Author

Nightly fails because of Sendable requirements.

@@ -76,7 +76,10 @@ class HTTPConnectionPool_FactoryTests: XCTestCase {
let factory = HTTPConnectionPool.ConnectionFactory(
key: .init(request),
tlsConfiguration: nil,
clientConfiguration: .init(proxy: .socksServer(host: "127.0.0.1", port: server!.localAddress!.port!)),
clientConfiguration: .init(
timeout: .init(connect: .seconds(3), read: nil),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we use 3 seconds instead of 2 seconds here?

Copy link
Collaborator

@dnadoba dnadoba left a comment

Choose a reason for hiding this comment

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

Great! This will decrease runtime significantly.

Nevertheless, I think the end goal should be to use an EmbeddedEventLoop in combination with advanceTime(to:). With that we should be able to reduce test time to a couple of milliseconds and make it independent of actual time passing.

@fabianfett
Copy link
Member Author

fabianfett commented Dec 6, 2021

Nevertheless, I think the end goal should be to use an EmbeddedEventLoop in combination with advanceTime(to:).

While I agree in general, I'm not sure this actually works for integration tests. In all our unit tests, we should already be using EmbeddedEventLoop or just being able to call some timeout method (in the state machine tests)

@fabianfett
Copy link
Member Author

@swift-server-bot test please.

@dnadoba
Copy link
Collaborator

dnadoba commented Dec 6, 2021

@swift-server-bot test this please

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/none For PRs that when merged do not need a bump in version number.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants