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

Make all integration tests safe to run in parallel #2300

Closed
wants to merge 18 commits into from

Conversation

corylanou
Copy link
Contributor

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:

  • Track Graphite listeners so we could close them properly and assign 0 port for testing
  • Track OpenTSDB listeners so we could close them properly and assign 0 port for testing
  • Change they way we do default config values for Graphite/OpenTSDB endpoints as to allow for testing. All other default styles should also likely move to this new style.

Before the change:

$ time go test ./... -p=1
ok      github.com/influxdb/influxdb    3.597s
ok      github.com/influxdb/influxdb/admin      0.011s
ok      github.com/influxdb/influxdb/client     0.009s
ok      github.com/influxdb/influxdb/cmd/influx 4.647s
ok      github.com/influxdb/influxdb/cmd/influxd        111.165s
ok      github.com/influxdb/influxdb/collectd   0.007s
ok      github.com/influxdb/influxdb/graphite   0.006s
ok      github.com/influxdb/influxdb/httpd      1.539s
ok      github.com/influxdb/influxdb/influxql   0.013s
ok      github.com/influxdb/influxdb/messaging  3.766s
?       github.com/influxdb/influxdb/opentsdb   [no test files]
ok      github.com/influxdb/influxdb/raft       8.218s
?       github.com/influxdb/influxdb/statik     [no test files]
?       github.com/influxdb/influxdb/test       [no test files]
?       github.com/influxdb/influxdb/tests/urlgen       [no test files]
?       github.com/influxdb/influxdb/udp        [no test files]
?       github.com/influxdb/influxdb/uuid       [no test files]

real    2m21.503s
user    0m20.528s
sys     0m7.652s

After the change:

$ time go test ./... -parallel 100
ok      github.com/influxdb/influxdb    3.605s
ok      github.com/influxdb/influxdb/admin      0.012s
ok      github.com/influxdb/influxdb/client     0.011s
ok      github.com/influxdb/influxdb/cmd/influx 3.458s
ok      github.com/influxdb/influxdb/cmd/influxd        57.129s
ok      github.com/influxdb/influxdb/collectd   0.012s
ok      github.com/influxdb/influxdb/graphite   0.006s
ok      github.com/influxdb/influxdb/httpd      1.497s
ok      github.com/influxdb/influxdb/influxql   0.012s
ok      github.com/influxdb/influxdb/messaging  3.739s
?       github.com/influxdb/influxdb/opentsdb   [no test files]
ok      github.com/influxdb/influxdb/raft       7.043s
?       github.com/influxdb/influxdb/statik     [no test files]
?       github.com/influxdb/influxdb/test       [no test files]
?       github.com/influxdb/influxdb/tests/urlgen       [no test files]
?       github.com/influxdb/influxdb/udp        [no test files]
?       github.com/influxdb/influxdb/uuid       [no test files]

real    0m59.438s
user    0m20.251s
sys     0m5.510s

@@ -137,13 +149,27 @@ func TestQuery_Auth(t *testing.T) {
path := tempfile()
defer os.Remove(path)

// spin up a server
config, _ := influxd.NewTestConfig()

Copy link
Contributor

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?

@otoolep
Copy link
Contributor

otoolep commented Apr 15, 2015

I like the change -- especially the randomization of ports.

@otoolep otoolep force-pushed the server-integration-test-close-race branch from 44a43f1 to 8d9aa56 Compare April 15, 2015 22:13
if err != nil {
panic(err)
}
return p
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

@corylanou corylanou force-pushed the server-integration-test-close-race branch from 8d9aa56 to 2fb4996 Compare April 16, 2015 17:47
return s.clusterListener.Addr()
}

func (s *Node) ClusterURL() *url.URL {
Copy link
Contributor Author

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.

Copy link
Contributor

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(),
}

@otoolep
Copy link
Contributor

otoolep commented Apr 16, 2015

Some small feedback, +1. Don't forget the changelog.

@corylanou corylanou force-pushed the server-integration-test-close-race branch from 01dc316 to 38a71cb Compare April 16, 2015 23:59
@otoolep
Copy link
Contributor

otoolep commented Apr 21, 2015

All done in a different PR.

@otoolep otoolep closed this Apr 21, 2015
@corylanou corylanou deleted the server-integration-test-close-race branch April 23, 2015 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants