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.

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

Jump to bottom

Fix nat integration tests #13683

Merged
merged 1 commit into from
Jun 3, 2015
Merged

Fix nat integration tests #13683

merged 1 commit into from
Jun 3, 2015

Conversation

crosbymichael
Copy link
Contributor

This removes complexity of current implementation and makes the test
correct and assert the right things.

Fixes #13556

Signed-off-by: Michael Crosby crosbymichael@gmail.com

@crosbymichael crosbymichael force-pushed the nat-test-fixes branch 2 times, most recently from 9209164 to 7fe387c Compare June 3, 2015 01:12
@jessfraz
Copy link
Contributor

jessfraz commented Jun 3, 2015

trying on my machine for the ultimate test, brace yoself

@crosbymichael
Fix nat integration tests
4f42097 
This removes complexity of current implementation and makes the test
correct and assert the right things.

Signed-off-by: Michael Crosby <crosbymichael@gmail.com>
@icecrime icecrime added this to the 1.7.0 milestone Jun 3, 2015
@icecrime
Copy link
Contributor

icecrime commented Jun 3, 2015

Used to fail consistently for me, now green both with and without --userland-proxy.

LGTM!

@mrjana
Copy link
Contributor

mrjana commented Jun 3, 2015

@crosbymichael This is what the test was doing before somebody changed the test to echo bye. But this method of testing was also flaky because once in a while nc will accept the connection and will just exit without printing the message. I believe the only consistent way of testing with nc is to make sure that connection gets established and then assert if it fails. We can't rely on the message being printed.

@jessfraz
Copy link
Contributor

jessfraz commented Jun 3, 2015

LGTM

@crosbymichael
Copy link
Contributor Author

@mrjana i changed it so the nc outputs like before so it's robust and we don't have to worry about it dropping bytes. I made sure that was addressed

@mrjana
Copy link
Contributor

mrjana commented Jun 3, 2015

@crosbymichael cool. I just realized that your piping in the actual message in the server side to nc so that it gets served to the client all the time. I think that should work. Nice job :-) LGTM

@crosbymichael
Copy link
Contributor Author

No problem, thanks for taking a look

jessfraz pushed a commit that referenced this pull request Jun 3, 2015
Merge pull request #13683 from crosbymichael/nat-test-fixes
63823cd 
Fix nat integration tests
@jessfraz jessfraz merged commit 63823cd into moby:master Jun 3, 2015
@crosbymichael crosbymichael deleted the nat-test-fixes branch June 3, 2015 01:54
@LK4D4 LK4D4 mentioned this pull request Jun 3, 2015
Closed
@jessfraz
Copy link
Contributor

cherry-picked

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
status/2-code-review
Projects
None yet
Milestone
1.7.0
Development

Successfully merging this pull request may close these issues.

libnetwork: TestDaemonICCPing
5 participants
@crosbymichael @jessfraz @icecrime @mrjana @GordonTheTurtle