Page MenuHomePhabricator

Merge #12926: Run unit tests in parallel
ClosedPublic

Authored by markblundeberg on Jul 3 2019, 00:28.

Details

Summary

PR12926 backport https://github.com/bitcoin/bitcoin/pull/12926/files
7ef9cd8 Increase entropy in test temp directory name (Pieter Wuille)
f6dfb0f Reorder travis builds (Pieter Wuille)
156db42 tests: run tests in parallel (Cory Fields)
66f3255 tests: split up actual tests and helper files (Cory Fields)

Pull request description:

This runs the unit tests (`src/test/test_bitcoin`) in 4 separate simultaneous processes, significantly speeding up some Travis runs (over 2x for win32).

This uses an approach by @theuni that relies on `make` as the mechanism for distributing tests over processes (through `-j`). For every test .cpp file, we search for `BOOST_FIXTURE_TEST_SUITE` or `BOOST_AUTO_TEST_SUITE`, and then invoke the test binary for just that suite (using `-t`). The (verbose) output is stored in a temporary file, and only shown in the case of failure.

Some makefile reshuffling is necessary to avoid trying to run tests from `src/test/test_bitcoin.cpp` for example, which contains framework/utility code but no real tests.

Finally, order the Travis jobs from slow to fast (apart from the arm/doc job which goes first, for fast failure). This should help reducing the total wall clock time before opening a PR and finishing Travis, in case where not all jobs are started simultaneously.

This is an alternative to #12831.
Test Plan

make check and pay attention to the output

Diff Detail

Repository
rABC Bitcoin ABC
Branch
patch
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 6647
Build 11341: Bitcoin ABC Buildbot (legacy)
Build 11340: arc lint + arc unit

Event Timeline

markblundeberg created this revision.Jul 3 2019, 00:28
Herald added a reviewer: Restricted Project. · View Herald TranscriptJul 3 2019, 00:28

This is pretty neat but it still has some issues.

  • The testsuite summary prints out too early! (with TOTAL: 0).
  • It no longer runs in parallel with the secp256k1 tests!

As a result it's barely faster:

$ time make -j4 check
...
real	2m15.085s
user	4m5.065s
sys	1m19.582s

Before:

$ time make -j4 check
...
real	2m31.073s
user	4m0.694s
sys	1m15.051s
markblundeberg edited the summary of this revision. (Show Details)Jul 3 2019, 00:46

If we take this, we should also do this fix for OpenBSD compat (why not): https://github.com/bitcoin/bitcoin/pull/13355

Note that if accepted, this (and the trivial fix mentioned) would bring us up to sync with Core regarding the way tests are done. All other changes in this makefile seem to be regarding fuzzing.

So, maybe we could do it a better way...

We could benefit from properly using the parallel test harness:
https://www.gnu.org/software/automake/manual/html_node/Parallel-Test-Harness.html

If TESTS= were filled up with all the individual tests, then they would run in parallel, and also run parallel to the secp256k1 tests. Moreover, they would get properly logged together into the test-suite.log AND the test summary would be informative. I tried playing around with automake to achieve this but I gave up quickly, as I don't really understand automake.

Fabien notes that the test summary actually prints out one test: the bitcoin-qt tests. For me it was 0 because I was using a non-qt build.

Fabien accepted this revision.Jul 3 2019, 17:49

Agreed that there are better solutions, but this is an improvement with few drawbacks.
I'll take care to get cmake up with something similar.

This revision is now accepted and ready to land.Jul 3 2019, 17:49