Skip to content

fix test faling with NIOTS #257

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

Merged
merged 4 commits into from
Jun 15, 2020
Merged

fix test faling with NIOTS #257

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
422a966
fix test faling with NIOTS
artemredkin Jun 15, 2020
313b3dd
review fix
artemredkin Jun 15, 2020
1bd5751
review fixes
artemredkin Jun 15, 2020
4fffe55
Merge branch 'master' into fix_unclean_close_test_again
artemredkin Jun 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -797,7 +797,7 @@ class HTTPClientInternalTests: XCTestCase {
}

func testUncleanCloseThrows() {
let server = NIOHTTP1TestServer(group: self.clientGroup)
let server = NIOHTTP1TestServer(group: MultiThreadedEventLoopGroup(numberOfThreads: 1))
Copy link
Contributor

Choose a reason for hiding this comment

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

who shuts this MTELG down?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we have self.serverGroup which could be used

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oooops, no one, good catch, thank you! no, we don't have that in internal tests class

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

and its only monday... 😔

Copy link
Contributor

Choose a reason for hiding this comment

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

oooops, no one, good catch, thank you! no, we don't have that in internal tests class

We should. I was probably lazy and changed only all of the non-internal tests

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

so far we won't be using it anywhere else in that class (it usually just creates HTTPBin), still worth it?

Copy link
Contributor

Choose a reason for hiding this comment

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

@artemredkin I think we should have a defaultHTTPBin and defaultClient and client/serverGroup all set up in setUp/tearDown. That makes tests more concise and therefore better

Copy link
Contributor

Choose a reason for hiding this comment

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

but we don't need to do it right in this PR, up to you

defer {
XCTAssertNoThrow(try server.stop())
}
Expand Down