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

Improve test and build time by using -i flag #4758

Merged
merged 1 commit into from
Jul 26, 2017

Conversation

ruflin
Copy link
Contributor

@ruflin ruflin commented Jul 26, 2017

See #4755 for details.

@ruflin ruflin added the review label Jul 26, 2017
@ruflin ruflin force-pushed the improve-test-and-build-time branch from 3a41172 to 8cf1cc3 Compare July 26, 2017 07:00
Copy link
Contributor

@tsg tsg left a comment

Choose a reason for hiding this comment

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

WFG

@ruflin
Copy link
Contributor Author

ruflin commented Jul 26, 2017

Curious to see if this also speeds up tests on Jenkins as libbeat packages are only built once for all beats. On Travis it probably doesn't make a difference as it is a new environment each time.

@ruflin
Copy link
Contributor Author

ruflin commented Jul 26, 2017

Strangely the Jenkins build took longer them previous builds.

@ruflin
Copy link
Contributor Author

ruflin commented Jul 26, 2017

I just reliased this does not apply when running the testsuite because there the gotestcover package is used. I will check if I can use the -i flag there too.

@ruflin ruflin force-pushed the improve-test-and-build-time branch from 8cf1cc3 to c32c53c Compare July 26, 2017 08:04
@tsg tsg merged commit 30ee527 into elastic:master Jul 26, 2017
@ruflin ruflin deleted the improve-test-and-build-time branch July 26, 2017 12:31
@@ -142,15 +143,18 @@ prepare-tests:
.PHONY: unit-tests
unit-tests: ## @testing Runs the unit tests with coverage. Race is not enabled for unit tests because tests run much slower.
unit-tests: prepare-tests
go test -i ${GOPACKAGES}
Copy link
Contributor

Choose a reason for hiding this comment

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

From the help of go test:

	-i
	    Install packages that are dependencies of the test.
	    Do not run the test.

And indeed go test -i doesn't actually run the tests. However, in master make unit-tests seems to work fine, so I'm not quite sure what's going on. But this makes me think adding -i to the test commands might not do what we expect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We actually always run go test -i followed by go test. Just in the case here it is in the line below. Also see line 151 as an example. My tests showed that running go test -i followed by go test takes the same time together as just running go test alone. So why do it? If you run it more then once, go test -i is very quick and the run time will be much shorter.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha, I missed that we run the go test without -i afterwards. Thanks!

@cyrilleverrier
Copy link
Contributor

Will you merge this improvement to the 5.x branches? it is a very nice improvement.

@ruflin
Copy link
Contributor Author

ruflin commented Jul 31, 2017

@cyrilleverrier No plan to backport this as it's not directly a feature. I would also give this some more time to be tested. I kind of still worry about side effects as there must be a reason this is not enabled in Golang by default.

@ruflin ruflin mentioned this pull request Nov 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants