Page MenuHomePhabricator

[CMAKE] Run test/util/bitcoin-util-test.py with ninja check
ClosedPublic

Authored by Fabien on Feb 4 2019, 11:07.

Details

Summary

Add a few new rules to cmake:

  • bitcoin-util-test will run bitcoin-util-test.py
  • check now includes bitcoin-util-test

This mimics the behaviour of make check with autotools

Test Plan
mkdir buildcmake && cd buildcmake
cmake -GNinja ..
ninja bitcoin-util-test
ninja check

Should return an unknown target error, as bitcoin-tx is required:

cmake -DBUILD_BITCOIN_TX=OFF -GNinja .. && ninja bitcoin-util-test

Should run the tests but not bitcoin-util-test:

cmake -DBUILD_BITCOIN_TX=OFF -GNinja .. && ninja check

Diff Detail

Repository
rABC Bitcoin ABC
Branch
cmake_bitcoin_utils_test
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 4866
Build 7795: Bitcoin ABC Buildbot (legacy)
Build 7794: arc lint + arc unit

Event Timeline

deadalnix requested changes to this revision.Feb 4 2019, 12:14
deadalnix added inline comments.
test/CMakeLists.txt
60 ↗(On Diff #7154)

Is that correct ?

77 ↗(On Diff #7154)

This is almost certainly not correct. Running the test depends on running the test with a custom target.

This revision now requires changes to proceed.Feb 4 2019, 12:14

Remove wrapping the script with ctest

Fabien edited the test plan for this revision. (Show Details)
test/CMakeLists.txt
60 ↗(On Diff #7154)

No, it's useless, I will remove it in another diff

deadalnix requested changes to this revision.Feb 4 2019, 16:31
deadalnix added inline comments.
test/CMakeLists.txt
60

Is that correct ?

76

Why is this dependency present twice ?

This revision now requires changes to proceed.Feb 4 2019, 16:31

Remove now useless add_test

test/CMakeLists.txt
60 ↗(On Diff #7167)

This is fixed in D2498

deadalnix requested changes to this revision.Feb 4 2019, 18:52
deadalnix added inline comments.
test/CMakeLists.txt
66 ↗(On Diff #7167)

check-bitoin-util

This revision now requires changes to proceed.Feb 4 2019, 18:52

bitcoin-util-test => check-bitcoin-util

deadalnix added inline comments.
test/CMakeLists.txt
68 ↗(On Diff #7168)

You should remove this. If all the tasks did this, the only thing you'd get is an intermingled dump of meaningless logs.

This revision is now accepted and ready to land.Feb 5 2019, 00:19
test/CMakeLists.txt
68 ↗(On Diff #7168)

Because I removed the ctest wrapper for this, there is zero output that tells you whether it's running or not, until it fails.
The same is done in configure.ac, I suppose for the same reason.
Is it OK to assume that the tests are silently running, with no way to be sure from the logs ?

This revision was automatically updated to reflect the committed changes.