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

Parallelise test suite #1041

Merged
merged 9 commits into from
Sep 23, 2016
Merged

Parallelise test suite #1041

merged 9 commits into from
Sep 23, 2016

Conversation

alyssais
Copy link
Contributor

@alyssais alyssais commented Sep 20, 2016

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew tests with your changes locally?

Integrates parallel_tests with brew tests and brew cask-tests, as suggested in #933.

  • brew tests now runs from Library/Homebrew, not Library/Homebrew/test, because parallel_tests demands it be so. I had to modify one test to make this work.
  • Can somebody look at brew-cask-tests and check the code coverage is still set up right?
  • The test output is weird because the different processes all output on top of each other.
  • On my machine, there's a slight speed improvement, but a pretty small one.

@MikeMcQuaid
Copy link
Member

Nice work again @penman! Looks like https://travis-ci.org/Homebrew/brew/builds/161295814 shows a slight improvement over https://travis-ci.org/Homebrew/brew.

The test output is weird because the different processes all output on top of each other.

This is the only blocker on this, I think. Would be good to get this output a bit nicer if possible.

@MikeMcQuaid
Copy link
Member

On my machine, there's a slight speed improvement, but a pretty small one.

Does --group-by runtime produce any speed-up on your machine on repeated runs?

@reitermarkus
Copy link
Member

brew tests now runs from Library/Homebrew

This probably breaks coverage reporting, as I think the .simplecov file has to be in the directory the tests are run in. So you will need to move this to Library/Homebrew also, and update the Library/Homebrew/cask/.simplecov symlink.

@MikeMcQuaid
Copy link
Member

We can wait and see what the results say here when this CI job is finished.

@alyssais
Copy link
Contributor Author

@MikeMcQuaid turns out I didn't need --group-by runtime, it just wasn't recording the run times.

@MikeMcQuaid
Copy link
Member

@penman Cool. Was just wondering if that made you see any more speedup after running a few times.

@alyssais
Copy link
Contributor Author

I could get Minitest to print nothing but the dots, so no wordy output would get in the way. Would that be enough?

@MikeMcQuaid
Copy link
Member

I could get Minitest to print nothing but the dots, so no wordy output would get in the way. Would that be enough?

@penman Sounds perfect. Would errors be handled nicely at the end?

@alyssais
Copy link
Contributor Author

@MikeMcQuaid oh, uh, with that approach errors aren't reported at all.

Perhaps I could get them reporting without all of Minitest's other reporting, but even with that I think they'd still spit out in the middle of the output when the process running them ended.

I don't know that I can think of a good way to do this…

@alyssais
Copy link
Contributor Author

Here's how they do it for RSpec: https://github.com/grosser/parallel_tests/blob/03d87ea93047e930296cf1e3549cb7bf76947619/lib/parallel_tests/rspec/logger_base.rb

@alyssais
Copy link
Contributor Author

Oh, but there's the --serialize-stdout option that will just wait for each process to finish, and print all of the output for each process one after the other. It does mean though, that even if the first test fails, you might not see it until the very end.

@MikeMcQuaid
Copy link
Member

Oh, but there's the --serialize-stdout option that will just wait for each process to finish, and print all of the output for each process one after the other. It does mean though, that even if the first test fails, you might not see it until the very end.

@penman That sounds like at least what we want for CI (e.g. if ENV["CI"] or ENV["JENKINS_HOME"] and then we could punt on solving the developer output until people decide how we want to handle that.

@alyssais
Copy link
Contributor Author

Aborted? Hmm…

@MikeMcQuaid
Copy link
Member

@penman Timed out after 45m. Let's try it again.

@alyssais
Copy link
Contributor Author

Aborted again…
I guess it's time for me to install Sierra!

@alyssais
Copy link
Contributor Author

It's hanging because of SimpleCov trying to filter a hash in (at least) O(n²). I'm going to submit a PR to them to make this more efficient.

But, the reason this has suddenly become a problem is that we seem to have started doing the coverage for >10000 Ruby files. I can try to investigate this, but I'm not very familiar with SimpleCov so it might take a while. If anybody else wants to take a look, you can reproduce with

brew cask-tests --coverage

@MikeMcQuaid
Copy link
Member

@penman Yeh, the performance with --coverage is like 10x worse. Glad you're submitting a PR there; was on my list to dig into. Make use of UoE's ADS class 😉

@alyssais
Copy link
Contributor Author

@MikeMcQuaid

Make use of UoE's ADS class 😉

I don't start it until January 😞

@alyssais
Copy link
Contributor Author

GREEEEEEEEEN. I'm skeptical that code coverage has jumped 11% though, so I'd still like somebody who knows about that to take a look at it.

@MikeMcQuaid
Copy link
Member

Want to play around with this locally to investigate the speed differences. Thoughts on the simplecov issues? Thanks @penman!

@reitermarkus
Copy link
Member

Will also play around a bit locally to see if I can get the simplecov issues sorted.

@alyssais
Copy link
Contributor Author

Is the Sierra CI machine any different to the El Capitan / Yosemite ones?

@MikeMcQuaid
Copy link
Member

@penman It's running --coverage and the others are not. Identical hardware, though.

@alyssais
Copy link
Contributor Author

The test times are similar with and without coverage, with the exception of test_integration_cmds, which takes almost nine times as long with coverage.

Test times (without coverage)

test/test_integration_cmds.rb             122.87
test/test_cmd_audit.rb                    8.45
test/test_sandbox.rb                      5.56
test/test_ENV.rb                          3.04
test/test_formula_installer.rb            2.52
test/test_os_mac_language.rb              1.14
test/test_tap.rb                          1.09
test/test_patching.rb                     1.04
test/test_formula.rb                      0.85
test/test_gpg.rb                          0.56
test/test_migrator.rb                     0.47
test/test_keg.rb                          0.40
test/test_download_strategies.rb          0.33
test/test_versions.rb                     0.31
test/test_cmd_testbot.rb                  0.29
test/test_cleaner.rb                      0.26
test/test_formulary.rb                    0.25
test/test_bash.rb                         0.25
test/test_cleanup.rb                      0.24
test/test_utils.rb                        0.18
test/test_pathname.rb                     0.17
test/test_resource.rb                     0.15
test/test_diagnostic.rb                   0.14
test/test_tab.rb                          0.13
test/test_checksum_verification.rb        0.10
test/test_options.rb                      0.09
test/test_formula_installer_bottle.rb     0.09
test/test_update_report.rb                0.08
test/test_software_spec.rb                0.07
test/test_os_mac_mach.rb                  0.07
test/test_os_mac_bottle_tag.rb            0.07
test/test_requirement.rb                  0.07
test/test_build_environment.rb            0.06
test/test_dependency_expansion.rb         0.06
test/test_commands.rb                     0.05
test/test_inreplace.rb                    0.04
test/test_exceptions.rb                   0.04
test/test_dependency_collector.rb         0.04
test/test_os_mac_keg.rb                   0.04
test/test_blacklist.rb                    0.04
test/test_compiler_selector.rb            0.04
test/test_patch.rb                        0.04
test/test_language_python.rb              0.03
test/test_stdlib.rb                       0.02
test/test_os_mac_version.rb               0.02
test/test_gpg2_requirement.rb             0.02
test/test_formula_validation.rb           0.02
test/test_formula_spec_selection.rb       0.02
test/test_dependencies.rb                 0.02
test/test_os_mac_diagnostic.rb            0.02
test/test_os_mac_dependency_collector.rb  0.01
test/test_pkg_version.rb                  0.01
test/test_dependency.rb                   0.01
test/test_bottle_collector.rb             0.01
test/test_caveats.rb                      0.01
test/test_bottle_hooks.rb                 0.01
test/test_formula_support.rb              0.01
test/test_formula_lock.rb                 0.01
test/test_x11_requirement.rb              0.01
test/test_compiler_failure.rb             0.01
test/test_formula_pin.rb                  0.01
test/test_descriptions.rb                 0.01
test/test_language_module_requirement.rb  0.01
test/test_os_mac_bottle_collector.rb      0.00
test/test_os_mac_blacklist.rb             0.00
test/test_os_mac_x11_requirement.rb       0.00
test/test_cmd_info.rb                     0.00
test/test_language_go.rb                  0.00
test/test_json.rb                         0.00
test/test_string.rb                       0.00
test/test_bottle_filename.rb              0.00
test/test_checksum.rb                     0.00
test/test_build_options.rb                0.00

Test times (with coverage)

test/test_integration_cmds.rb             1082.18
test/test_cmd_audit.rb                    8.42
test/test_sandbox.rb                      4.81
test/test_formula_installer.rb            3.47
test/test_patching.rb                     2.39
test/test_ENV.rb                          1.94
test/test_formula.rb                      1.68
test/test_tap.rb                          1.00
test/test_gpg.rb                          0.53
test/test_keg.rb                          0.49
test/test_download_strategies.rb          0.39
test/test_migrator.rb                     0.39
test/test_cmd_testbot.rb                  0.34
test/test_formulary.rb                    0.31
test/test_bash.rb                         0.28
test/test_utils.rb                        0.24
test/test_cleanup.rb                      0.24
test/test_versions.rb                     0.22
test/test_update_report.rb                0.15
test/test_os_mac_language.rb              0.14
test/test_pathname.rb                     0.13
test/test_cleaner.rb                      0.13
test/test_formula_installer_bottle.rb     0.12
test/test_checksum_verification.rb        0.12
test/test_software_spec.rb                0.10
test/test_diagnostic.rb                   0.10
test/test_dependency_expansion.rb         0.10
test/test_os_mac_mach.rb                  0.09
test/test_tab.rb                          0.09
test/test_exceptions.rb                   0.07
test/test_dependencies.rb                 0.06
test/test_os_mac_keg.rb                   0.06
test/test_patch.rb                        0.06
test/test_dependency_collector.rb         0.06
test/test_requirement.rb                  0.05
test/test_options.rb                      0.04
test/test_resource.rb                     0.04
test/test_dependency.rb                   0.04
test/test_inreplace.rb                    0.04
test/test_os_mac_bottle_tag.rb            0.04
test/test_gpg2_requirement.rb             0.03
test/test_os_mac_dependency_collector.rb  0.03
test/test_os_mac_diagnostic.rb            0.03
test/test_formula_spec_selection.rb       0.03
test/test_compiler_selector.rb            0.03
test/test_language_python.rb              0.03
test/test_formula_validation.rb           0.02
test/test_build_environment.rb            0.02
test/test_stdlib.rb                       0.02
test/test_commands.rb                     0.02
test/test_hardware.rb                     0.02
test/test_ARGV.rb                         0.02
test/test_shell.rb                        0.01
test/test_bottle_collector.rb             0.01
test/test_checksum.rb                     0.01
test/test_descriptions.rb                 0.01
test/test_x11_requirement.rb              0.01
test/test_formula_support.rb              0.01
test/test_os_mac_blacklist.rb             0.01
test/test_bottle_filename.rb              0.01
test/test_os_mac_bottle_collector.rb      0.01
test/test_build_options.rb                0.01
test/test_bottle_hooks.rb                 0.01
test/test_compiler_failure.rb             0.01
test/test_formula_lock.rb                 0.01
test/test_language_module_requirement.rb  0.01
test/test_os_mac_version.rb               0.01
test/test_pkg_version.rb                  0.01
test/test_string.rb                       0.01
test/test_formula_pin.rb                  0.01
test/test_caveats.rb                      0.01
test/test_language_go.rb                  0.00
test/test_mpi_requirement.rb              0.00
test/test_os_mac_x11_requirement.rb       0.00
test/test_cmd_info.rb                     0.00


# TODO: setting the --seed here is an ugly temporary hack, to remain only
# until test-suite glitches are fixed.
ENV["TESTOPTS"] = "--seed=14830" if ENV["TRAVIS"]
Copy link
Member

Choose a reason for hiding this comment

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

You can remove this as the cask tests are not run on Travis anymore anyway.

This hack has been in Homebrew Cask for more than two years
(since 51f93e6), and it originated even
earlier (6d2f7bc).

Cask tests apparently aren't even run on Travis anymore,
so this can be safely removed.
@MikeMcQuaid
Copy link
Member

The test times are similar with and without coverage, with the exception of test_integration_cmds, which takes almost nine times as long with coverage.

@penman Thanks for digging in there. This will be why it's so slow:
https://github.com/Homebrew/brew/blob/bbed7246bc5c5b7acb8c1d427d10b43e090dfd39/Library/Homebrew/test/Gemfile

I'm tempted to just 💀 the coverage on those commands until we can get the times more sane.

@alyssais
Copy link
Contributor Author

alyssais commented Sep 22, 2016

I'm tempted to just 💀 the coverage on those commands until we can get the times more sane.

@MikeMcQuaid What if I told you I could get them to run with coverage more than twice as fast as they do now? 😉

Feels like it should be a separate PR, though…

@reitermarkus
Copy link
Member

This will be why it's so slow: https://github.com/Homebrew/brew/blob/bbed7246bc5c5b7acb8c1d427d10b43e090dfd39/Library/Homebrew/test/Gemfile

Thanks for pointing to this Gemfile! Now I know why cask coverage is at 100 % at all times: cask-tests is using SimpleCov 0.12.0.

Copy link
Member

@reitermarkus reitermarkus left a comment

Choose a reason for hiding this comment

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

Everything seems to be working, except coverage, which I'll fix in a follow-up PR. So 👍 to merging this.

@MikeMcQuaid
Copy link
Member

@MikeMcQuaid What if I told you I could get them to run with coverage more than twice as fast as they do now? 😉

Feels like it should be a separate PR, though…

@penman Feel free to make it this PR! Still thinking about whether parallel tests provides a decent, measurable speedup? Your thoughts?

@alyssais
Copy link
Contributor Author

@penman Feel free to make it this PR!

@MikeMcQuaid already did! #1084

@alyssais
Copy link
Contributor Author

Still thinking about whether parallel tests provides a decent, measurable speedup? Your thoughts?

@MikeMcQuaid I think it's a good thing to have. I think (but haven't researched, this is a hunch) that we're not seeing a lot of the potential for speedup because parallel_tests will try to spread different tests across different processes, but different tests in the same file are always grouped together. When I look at the output, I get four processes. Three finish at the same time, and one goes on much longer, slowing things down. That final process is the one that runs test_integration_cmds. So, if we split that up into smaller files, we'd probably get a much bigger speed up.

@alyssais
Copy link
Contributor Author

Even without any further changes, brew tests; brew cask-tests runs a full minute faster on this branch than master for me on the first run.

@MikeMcQuaid
Copy link
Member

Even without any further changes, brew tests; brew cask-tests runs a full minute faster on this branch than master for me on the first run.

Good enough for me! Nice work again @penman (I'm almost bored of saying that now 😆)

@MikeMcQuaid MikeMcQuaid merged commit 5cf3838 into Homebrew:master Sep 23, 2016
@alyssais alyssais deleted the parallel_tests branch September 23, 2016 07:57
@MatzFan
Copy link
Contributor

MatzFan commented Sep 26, 2016

if we split that [test_integration_cmds] up into smaller files, we'd probably get a much bigger speed up.

Big time. On my ancient Core 2 Duo machine I get this: 2 processes for 77 tests, ~ 38 tests per process. As @penman says, the gem apportions tests proportionately across cores by file. For me, the first process exits after 29s and the other one - with the integration tests - after 190s - so not getting full benefit of @penman's excellent work! Splitting the integration tests so they likely get evenly spread across each core would be the way forward. Best done when no merge conflicts likely, I guess. Be happy to do this one weekend maybe.

@reitermarkus
Copy link
Member

reitermarkus commented Sep 26, 2016

Be happy to do this one weekend maybe.

@MatzFan, sorry to ruin your weekend plans, but I was curious and went ahead with it: #1156 😄

@Homebrew Homebrew locked and limited conversation to collaborators May 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants