Parallel make check #865

scravy · 2019-03-28T12:20:35Z

This is a backport of bitcoin/bitcoin#12926, which unveils how master is currently broken as reported in #861

Here is the original 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.

For bitcoin this improved in quite a nice speedup for builds (especially the Win32 unit test only one):

OSX 10.13.4 (2.9 GHz Intel Core i7): time make -j5 V=1 check

This PR:
real	1m28.438s
user	3m42.053s
sys	0m45.473s
versus master:
real	3m5.687s
user	2m57.911s
sys	0m46.390s

src/Makefile.test.include

scravy · 2019-03-28T13:07:25Z

Something is not working nicely in the windows build:

Running tests: amount_tests from test/amount_tests.cpp
Running tests: allocator_tests from test/allocator_tests.cpp
Running tests: base16_tests from test/base16_tests.cpp
Running tests: base32_tests from test/base32_tests.cpp
Running tests: base58_tests from test/base58_tests.cpp
Running tests: base64_tests from test/base64_tests.cpp
Running tests: bech32_tests from test/bech32_tests.cpp
Running tests: better_enum_set_tests from test/better_enum_set_tests.cpp
Running tests: bip32_tests from test/bip32_tests.cpp
Running tests: blockchain_behavior_test from test/blockchain/blockchain_behavior_tests.cpp
Running tests: blockchain_custom_parameters_tests from test/blockchain/blockchain_custom_parameters_tests.cpp
Running tests: blockchain_parameters_tests from test/blockchain/blockchain_parameters_tests.cpp
Running tests: blockchain_difficulty_tests from test/blockchain_tests.cpp
Running tests: blockencodings_tests from test/blockencodings_tests.cpp
Running tests: bloom_tests from test/bloom_tests.cpp
Running tests: bswap_tests from test/bswap_tests.cpp
Running tests: checkqueue_tests from test/checkqueue_tests.cpp
Running tests: coins_tests from test/coins_tests.cpp
Running tests: coins_view_db_tests from test/coins_view_db_tests.cpp
Running tests: compress_tests from test/compress_tests.cpp
Running tests: crypto_tests from test/crypto_tests.cpp
Running tests: cuckoocache_tests from test/cuckoocache_tests.cpp
Running tests: dependency_injector_tests from test/dependency_injector_tests.cpp
Running tests: DoS_tests from test/DoS_tests.cpp
Running tests: embargoman_tests from test/embargoman_tests.cpp
Running tests: admincommand_tests from test/esperanza/admincommand_tests.cpp
Running tests: adminstate_tests from test/esperanza/adminstate_tests.cpp
Running tests: finalization_checks_tests from test/esperanza/checks_tests.cpp
Running tests: from test/esperanza/finalization_utils.cpp
Missing an argument value for the parameter run_test in the argument 
Parameter: run_test
 Filters, which test units to include or exclude from test module execution.
 Command line formats:
   --run_test=<test unit filter>
   -t <test unit filter>
 Environment variable: BOOST_TEST_RUN_FILTERS
For detailed help on Boost.Test parameters use:
  test_unite.exe --help
or
  test_unite.exe --help=<parameter name>

(this is a different failure from the expected one).

The linux builds all error out properly:

unknown location(0): fatal error: in "coins_view_db_tests/ccoins_view_cache_clear_coins": DependencyInitializationError: Database I/O error
test/coins_view_db_tests.cpp(11): last checkpoint: "ccoins_view_cache_clear_coins" fixture entry.
test/coins_view_db_tests.cpp(11): Leaving test case "ccoins_view_cache_clear_coins"; testing time: 23642us
test/coins_view_db_tests.cpp(9): Leaving test suite "coins_view_db_tests"; testing time: 23654us
Leaving test module "Unit-e Test Suite"; testing time: 23772us

I would like to point out that the functionality in here does not replace the one provided by run-unit-tests as that script runs all tests no matter what, whereas this will still error out on first blow-up. run-unit-tests is used to give an accurate overview of how many tests out of all tests are failing.

Currently unit tests can not be run in parallel. This prevents us from speeding up ci builds (see bitcoin/bitcoin#12926, #865) and won't work with the updated build definitions from bitcoin 0.17 (#860). This is a regression which was introduced in #793. This pull request fixes #861. The problem is that the `StateDB` component does something actively when being initialized (not something a component should do), which is to access disk and so on, which can fail. Since there is a `UnitEInjector` for every `BasicTestingSetup` running the unit tests in parallel creates a database _in the same directory_ per unit tests suite which clashes and breaks and throws and aborts and dies. This can be prevented by using an in-memory database, but the problem shifts: How to get that configuration in? I did not want to expose this as a user-definable setting and also not as a chain parameter, so these two configuration options are not available. Which is why I created `UnitEInjectorConfiguration`. This is something which should not be necessary as usually you would not expose the same module as in production in unit tests but use a subset or just the mocked version, but since the rest of bitcoin is not in well-defined components and accesses them using `GetComponent` a global injector has to be available in some tests. The change ultimately does not affect the prod version of things, but the tests, which inject a different `UnitEInjectorConfiguration`. I made this a `struct` rather then just a flag `unit_tests` such that it is extensible in case we have other cases like this. In order to make the injector be able to access fields from it's own instance in the definition of a component I altered the definition of `UNMANAGED_COMPONENT` a bit. I would like to point out that these initialization issues would also have happened with an Application class (#723), as that is exactly what `UnitEInjector` is, just you would inject things manually. Signed-off-by: Julian Fleischer <julian@thirdhash.com>

Signed-off-by: Julian Fleischer <julian@thirdhash.com>

scravy · 2019-04-01T12:13:17Z

Rebased with the change from #870 which should allow the CI build to pass, give us parallel unit tests, and decrease ci run times :-)

src/test/test_unite.cpp

scravy · 2019-04-01T12:49:22Z

@castarco @Gnappuraz I answered your comments/questions. What do you otherwise think about this change?

Gnappuraz

utACK ab8fa35

src/Makefile.test.include

frolosofsky

Besides two missed files, utACK 7651432

Signed-off-by: Julian Fleischer <julian@thirdhash.com>

scravy added the upstream sync Related to upstream merges label Mar 28, 2019

scravy self-assigned this Mar 28, 2019

scravy requested a review from a team March 28, 2019 12:20

castarco reviewed Mar 28, 2019

View reviewed changes

src/Makefile.test.include Show resolved Hide resolved

scravy force-pushed the parallel-make-check branch from b96fc42 to 5572e7c Compare March 28, 2019 16:09

This was referenced Mar 29, 2019

possible build issues not covered by CI #868

Open

Allow unit tests being run in parallel again #870

Merged

scravy added 4 commits April 1, 2019 14:01

Run unit tests in parallel

7651432

Signed-off-by: Julian Fleischer <julian@thirdhash.com>

Increase entropy in test temp directory name

023da1b

Signed-off-by: Julian Fleischer <julian@thirdhash.com>

Run all test suites from all test files

c806490

Signed-off-by: Julian Fleischer <julian@thirdhash.com>

Properly split tests and util files

ab8fa35

Signed-off-by: Julian Fleischer <julian@thirdhash.com>

scravy force-pushed the parallel-make-check branch from 1a90464 to ab8fa35 Compare April 1, 2019 12:01

scravy requested a review from a team April 1, 2019 12:42

Gnappuraz reviewed Apr 1, 2019

View reviewed changes

src/test/test_unite.cpp Show resolved Hide resolved

Gnappuraz approved these changes Apr 1, 2019

View reviewed changes

frolosofsky reviewed Apr 1, 2019

View reviewed changes

src/Makefile.test.include Show resolved Hide resolved

frolosofsky approved these changes Apr 1, 2019

View reviewed changes

Wrap tests in BOOST_AUTO_TEST_SUITE

71c896a

Signed-off-by: Julian Fleischer <julian@thirdhash.com>

scravy merged commit ccdeb51 into dtr-org:master Apr 1, 2019

scravy deleted the parallel-make-check branch April 6, 2019 02:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Parallel make check #865

Parallel make check #865

Uh oh!

scravy commented Mar 28, 2019 •

edited

Loading

Uh oh!

Uh oh!

scravy commented Mar 28, 2019

Uh oh!

scravy commented Apr 1, 2019

Uh oh!

Uh oh!

scravy commented Apr 1, 2019

Uh oh!

Gnappuraz left a comment

Uh oh!

Uh oh!

frolosofsky left a comment

Uh oh!

Uh oh!

Parallel make check #865

Parallel make check #865

Uh oh!

Conversation

scravy commented Mar 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

scravy commented Mar 28, 2019

Uh oh!

scravy commented Apr 1, 2019

Uh oh!

Uh oh!

scravy commented Apr 1, 2019

Uh oh!

Gnappuraz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

frolosofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

scravy commented Mar 28, 2019 •

edited

Loading