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.

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

api: fix intermittent test failures #6461

Conversation

rogpeppe
Copy link
Contributor

The test code assumed that we'd get dial requests in the
order they are specified, but in a heavily loaded test server
this is not necessarily the case, leading to failures like this one:

http://reports.vapour.ws/releases/4490/job/run-unit-tests-xenial-amd64/attempt/1427#highlight​

This fixes the test so that we don't rely on the sequential try ordering - the
concurrent retry mechanism isn't something we need to test in this test,
so just connect to a single address in each test.

Copy link
Contributor

@frobware frobware left a comment

Choose a reason for hiding this comment

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

LGTM - though one comment against the documentation.

for i, test := range openWithSNIHostnameTests {
c.Logf("test %d: %v", i, test.about)
s.testSNIHostName(c, test.info, test.expectDial)
}
}

// testSNIHostName tests that when the API is dialed with the given info,
// api.newWebsocketDialer is called with the expected information
// (one element for each call to newWebsocketDialer)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the 'one element for each...' still hold true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, thanks. Fixed.

@rogpeppe rogpeppe force-pushed the 119-fix-snihostname-test-intermittent-failure branch from fc140bd to ba5e377 Compare October 17, 2016 16:46
Copy link
Contributor

@mhilton mhilton left a comment

Choose a reason for hiding this comment

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

LGTM

@rogpeppe
Copy link
Contributor Author

$$merge$$

@jujubot
Copy link
Collaborator

jujubot commented Oct 17, 2016

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

@jujubot jujubot merged commit 809dc26 into juju:develop Oct 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants