-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Make all integration tests safe to run in parallel #2300
Conversation
@@ -137,13 +149,27 @@ func TestQuery_Auth(t *testing.T) { | |||
path := tempfile() | |||
defer os.Remove(path) | |||
|
|||
// spin up a server | |||
config, _ := influxd.NewTestConfig() | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style: any reason there are multiple blank lines here?
I like the change -- especially the randomization of ports. |
44a43f1
to
8d9aa56
Compare
if err != nil { | ||
panic(err) | ||
} | ||
return p |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a tiny window here where the port has been freed, and could then be re-used by the next caller of newPort()
, and still be used by the first caller?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When watching the ports log out it always went up continuously. Could it create a race condition? I'm not sure. I suspect it has more to do with how the underlying platform implements it. I will ask on Gopher slack though to see if anyone knows if there is some type of delay at the system level before it puts that port back on the market.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately we confirmed that on Ubuntu 14.04, port collision occur in this scenario.
8d9aa56
to
2fb4996
Compare
2fb4996
to
d041424
Compare
return s.clusterListener.Addr() | ||
} | ||
|
||
func (s *Node) ClusterURL() *url.URL { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not super excited about this. I'm open to suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about?
return &url.URL{
Scheme: "http",
host: s.ClusterAddr().String(),
}
Some small feedback, +1. Don't forget the changelog. |
01dc316
to
38a71cb
Compare
All done in a different PR. |
Packages test in parallel by default, and with the recent addition of integration tests in other packages, this is now creating intermittent port collision, and hence, test failures.
This change also required the following changes:
0
port for testing0
port for testingdefault
config values for Graphite/OpenTSDB endpoints as to allow for testing. All otherdefault
styles should also likely move to this new style.Before the change:
After the change: