Skip to content

Fix potentially flaky HTTPClientTests.testNoBytesSentOverBodyLimit #426

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

Conversation

fabianfett
Copy link
Member

Motivation

If we call

context.write(ByteBuffer(string: "foo"), promise: nil)
context.flush()
context.close()

there is no guarantee that "foo" will be written to the network. Currently the test testNoBytesSentOverBodyLimit assumes this however.

Changes

  • testNoBytesSentOverBodyLimit does not assume anymore, that bytes will be sent before a close of the connection.
  • Use XCTAssertEqual to compare Equatable errors.

@fabianfett fabianfett requested a review from Lukasa September 14, 2021 07:36
@fabianfett fabianfett added the semver/none No version bump required. label Sep 14, 2021
@fabianfett fabianfett added this to the HTTP/2 support milestone Sep 14, 2021
@fabianfett fabianfett force-pushed the ff-fix-potnetially-flaky-testNoBytesSentOverBodyLimit branch from 393ada5 to dc8ad93 Compare September 14, 2021 07:43
}
} catch {
// TBD: We sadly can't verify the error type, since it is private in `NIOTestUtils`:
// NIOTestUtils.BlockingQueue<Element>.TimeoutError
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should probably find a way to make this public so we can be confident that we're throwing the right error.

@fabianfett fabianfett merged commit b0180d1 into swift-server:main Sep 14, 2021
@fabianfett fabianfett deleted the ff-fix-potnetially-flaky-testNoBytesSentOverBodyLimit branch September 14, 2021 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/none No version bump required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants