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.

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

artemredkin
Copy link
Collaborator

Fixes test crashing on precondition.

Motivation:
When tests are executed on macOS client ELG is actually a NIOTS ELG, so it's incompatible with NIOHTTP1TestServer

Modifications:
Use separate ELG in testUncleanCloseThrows

Result:
Closes #256

@artemredkin artemredkin added the kind/bug Feature doesn't work as expected. label Jun 15, 2020
@artemredkin artemredkin added this to the 1.2.0 milestone Jun 15, 2020
@artemredkin artemredkin requested a review from weissi June 15, 2020 12:53
@@ -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

Copy link
Contributor

@weissi weissi left a comment

Choose a reason for hiding this comment

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

awesome, thkna

@artemredkin artemredkin merged commit f2aef45 into swift-server:master Jun 15, 2020
@artemredkin artemredkin deleted the fix_unclean_close_test_again branch June 15, 2020 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Feature doesn't work as expected.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tests fail on macOS
2 participants