Skip to content

Improve HTTPClient tests #424

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

Merged

Conversation

fabianfett
Copy link
Member

Motivation

We have some tests that make it hard to see the reason for a test failure. Since HTTPClientError is equatable, let's use XCTAssertEqual

Modifications

  • Use XCTAssertEqual to compare HTTPClientErrors
  • Don't call some server on the internet expecting it will not respond. Create a server and shut it down directly, use the now unused port.

Result

  • Better diagnostics on failing tests
  • Less outside dependencies on tests

@fabianfett fabianfett requested a review from Lukasa September 13, 2021 14:18
}
func testDeadline() {
XCTAssertThrowsError(try self.defaultClient.get(url: self.defaultHTTPBinURLPrefix + "wait", deadline: .now() + .milliseconds(150)).wait()) {
XCTAssertEqual($0 as? HTTPClientError, .readTimeout)
Copy link
Member Author

Choose a reason for hiding this comment

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

Getting a readTimeout error on a deadline is weird. This will be actually fixed with the new implementation

@fabianfett fabianfett added this to the HTTP/2 support milestone Sep 13, 2021
@fabianfett fabianfett added the semver/none No version bump required. label Sep 13, 2021
@fabianfett fabianfett force-pushed the ff-better-http-client-tests branch from b041fe3 to 10fff8d Compare September 13, 2021 15:41
@fabianfett fabianfett force-pushed the ff-better-http-client-tests branch from 10fff8d to 18c0a5b Compare September 13, 2021 15:41
@fabianfett fabianfett merged commit 75b716e into swift-server:main Sep 13, 2021
@fabianfett fabianfett deleted the ff-better-http-client-tests branch September 13, 2021 15:49
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