Page MenuHomePhabricator

[CMAKE] Run the leveldb tests serially
ClosedPublic

Authored by Fabien on Jun 26 2020, 19:10.

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Commits
rABCf0323812f55b: [CMAKE] Run the leveldb tests serially
Summary

This adds a facility to create a job pool with an arbitrary number of
concurrent jobs and assign it to a test suite. This is used to limit the
leveldb suite to a single job and make the tests run serially.

This adds about 20% to the leveldb tests duration on my machine, but
since they are only running as part of the check-all (or higher) global
target the impact is minor.

This is expected to prevent spurious leveldb failures like
https://build.bitcoinabc.org/viewLog.html?tab=buildLog&buildTypeId=BitcoinABC_Master_BitcoinAbcMasterCoverage&buildId=87936&guest=1

Depends on D7073.

Test Plan
ninja all check-all
ninja check-leveldb

Check it still works with make, even if the pool has no effect:

cmake ..
make -j42 check-leveldb

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Fabien created this revision.Jun 26 2020, 19:10
Herald added a reviewer: Restricted Project. · View Herald TranscriptJun 26 2020, 19:10
Fabien requested review of this revision.Jun 26 2020, 19:10
deadalnix requested changes to this revision.Jun 26 2020, 19:14
deadalnix added a subscriber: deadalnix.

You can create a specific job pool for this and set the job count to 1 rather than simply use terminal.

This revision now requires changes to proceed.Jun 26 2020, 19:14
Fabien requested review of this revision.Jun 26 2020, 19:20

Setting the job pool for custom targets is only possible with cmake >= 3.15 and we enforce cmake >= 3.13 (debian 10 default): https://gitlab.kitware.com/cmake/cmake/-/merge_requests/3308.
Note that due to the wrapper, USES_TERMINAL will not dump the test to the console unless there is a failure. It's still not equivalent to a dedicated pool but close enough.

deadalnix requested changes to this revision.Jun 27 2020, 12:37

The difference is very relevent because this adds to the serial path for no good reasons.

This revision now requires changes to proceed.Jun 27 2020, 12:37
Fabien updated this revision to Diff 22603.Wed, Jul 29, 13:05

Use a JOB_POOL mechanism to limit the number of concurrent jobs.

Fabien edited the summary of this revision. (Show Details)Wed, Jul 29, 13:06
Fabien edited the test plan for this revision. (Show Details)
deadalnix requested changes to this revision.Thu, Jul 30, 00:30
deadalnix added inline comments.
cmake/modules/TestSuite.cmake
89 ↗(On Diff #22603)

BELONG_TO_POOL is a boolean name, either it belongs or not. But this is not a boolean.

This revision now requires changes to proceed.Thu, Jul 30, 00:30
Fabien updated this revision to Diff 22638.Thu, Jul 30, 09:41

Pick a better name for BELONG_TO_POOL

deadalnix accepted this revision.Thu, Jul 30, 12:22
deadalnix added inline comments.
cmake/modules/TestSuite.cmake
101 ↗(On Diff #22638)

This is not a parameter, but an argument. Parameters are declarations.

int foo(int a) { ... }
foo(3);

In here, a is a parameter, 3 is an argument.

cmake_parse_arguments does parse the arguments. what you give is is a description of the parameters.

This revision is now accepted and ready to land.Thu, Jul 30, 12:22
Fabien updated this revision to Diff 22640.Thu, Jul 30, 12:28

PARAM => ARG

This revision was automatically updated to reflect the committed changes.