Skip to content

Conversation

@Kami
Copy link
Member

@Kami Kami commented Jul 24, 2018

This pull request cherry picks two big test performance improvements from #4263:

  1. Don't drop each collection before dropping the whole database (this was needed with old, pre-WiredTiger MongoDB versions)
  2. Don't synchronously ensure indexes for all the collections inside the tests base classes. Asynchronous index ensure should work just fine with tests since they don't run in a distributed environment where races can occur (and multiple tests can't run in parallel at the same time anyway, because all the tests utilize the same database). Synchronous index ensure is only needed for production deployments which multiple services can start up at the same time. This should offer significant test speed up.

All those changes are independent from other MongoDB start up and download related changes in #4263.

Based on the local testing, this should speed Travis tests, by 20% at the minimum. More accurate numbers are coming up soon once the Travis builds finishes.

Changes include:

1. No need to drop collections anymore before dropping the MongoDB database
   (only needed with older versions)
2. No need to synchronously ensure indexes for tests. Async index ensure
   is fine for tests and offers significat test speeds ups.
@Kami Kami added this to the 2.9.0 milestone Jul 24, 2018
@Kami
Copy link
Member Author

Kami commented Jul 24, 2018

It looks like the slowest build now takes around ~18 minutes and it used to take around ~22 minutes. I'm also looking into some other improvements (coverage seems to have a huge overhead, so perhaps we could only run it on master branches and not for all the PRs).

Before: https://travis-ci.org/StackStorm/st2/builds/407625613
After: https://travis-ci.org/StackStorm/st2/builds/407657639

@Kami
Copy link
Member Author

Kami commented Jul 24, 2018

It looks like the coverage changes in #4147 broke codecov coverage reporting - https://travis-ci.org/StackStorm/st2/jobs/407657640#L9507, https://codecov.io/github/StackStorm/st2?branch=master (/cc @blag).

Having said that, I'm looking if it's possible to only run coverage on master and version branches and not for pull request since coverage has huge negative performance overhead on the tests run time.

@Kami
Copy link
Member Author

Kami commented Jul 24, 2018

It turns out that code coverage related changes in #4147 have a massive negative performance overhead - build time went from ~10 minutes to ~20+ minutes after that PR has been merged.

Because of that, I'm working with @blag to only instrument, generate and submit coverage report to coveralls on master branch changes.

@blag blag force-pushed the test_speed_ups branch from d012a85 to fc49d30 Compare July 24, 2018 18:02
@Kami
Copy link
Member Author

Kami commented Jul 24, 2018

Also, to back my claim that the coverage PR introduced a massive 40 or so percent test slow down with some actual data:

Travis build page before the change was merged: https://travis-ci.org/StackStorm/st2/builds/385220238
And after the change was merged: https://travis-ci.org/StackStorm/st2/builds/407625613

So ~22 vs ~14 minutes.

And now with those changes tests take less than 10 minutes. Big chunk of that is due to the coverage change and other 3-5 minutes speed up is due to my changes in 2dd5b78.

@blag blag force-pushed the test_speed_ups branch from 576a5ea to 92366e5 Compare July 24, 2018 20:01
Copy link
Member

@arm4b arm4b left a comment

Choose a reason for hiding this comment

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

This IS Awesome! 🎉
Back to 1yr. ago 10mins!

Great work @Kami @blag!

coverage combine .coverage.integration.*
@if [ -n "$(NOSE_COVERAGE_FLAGS)" ]; then \
. $(VIRTUALENV_DIR)/bin/activate; COVERAGE_FILE=.coverage.integration \
coverage combine .coverage.integration.*; \
Copy link
Member

@arm4b arm4b Jul 24, 2018

Choose a reason for hiding this comment

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

BTW, combine itself looks suspicious, because codecov already combines coverage natively, sent from different jobs within one build: https://docs.codecov.io/docs/merging-reports

Copy link
Contributor

Choose a reason for hiding this comment

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

That's true, but I also wanted to combine the tests locally. I can remove that if we want to.

@Kami
Copy link
Member Author

Kami commented Jul 24, 2018

@blag Thanks - LGTM, but it would be great if you can also make the environment variable change we discussed on Slack so it's not tied / coupled to Travis (using new ENABLE_COVERAGE environment variable to control everything coverage related and setting this variable inside .travis.yml based on the value of TRAVIS_PULL_REQUEST environment variable).

In addition to that it would also be great if you can have a look in coverage combining and submitting coverage to coveralls which I mentioned on Slack and which @armab mentioned above.

Kami added 2 commits July 24, 2018 23:56
dependency won't be updated locally when running "make requirements"
and either virtualenv needs to be re-created for new version to be
installed or pip uninstall orchestra ran before make requirements.
Copy link
Contributor

@m4dcoder m4dcoder left a comment

Choose a reason for hiding this comment

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

LGTM

@blag blag mentioned this pull request Jul 24, 2018
@blag blag closed this Jul 25, 2018
@blag blag reopened this Jul 25, 2018
@blag
Copy link
Contributor

blag commented Jul 25, 2018

Sorry hit the wrong button there. I'll implement notifying coveralls in a separate PR, since that's not really related to test speed ups at all.

@Kami
Copy link
Member Author

Kami commented Jul 25, 2018

While at it, I also pushed a small change (3991bc1) for more user-friendly named builds - https://travis-ci.org/StackStorm/st2/builds/407952313

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants