Page MenuHomePhabricator

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

Authored by Fabien on Mon, Feb 4, 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
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.Mon, Feb 4, 11:07
Herald added a reviewer: Restricted Project. · View Herald TranscriptMon, Feb 4, 11:07
Herald added a subscriber: schancel. · View Herald Transcript
deadalnix requested changes to this revision.Mon, Feb 4, 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.Mon, Feb 4, 12:14
Fabien updated this revision to Diff 7161.Mon, Feb 4, 15:44

Remove wrapping the script with ctest

Fabien edited the summary of this revision. (Show Details)Mon, Feb 4, 15:44
Fabien edited the test plan for this revision. (Show Details)
Fabien added inline comments.Mon, Feb 4, 15:46
test/CMakeLists.txt
60 ↗(On Diff #7154)

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

deadalnix requested changes to this revision.Mon, Feb 4, 16:31
deadalnix added inline comments.
test/CMakeLists.txt
60 ↗(On Diff #7161)

Is that correct ?

76 ↗(On Diff #7161)

Why is this dependency present twice ?

This revision now requires changes to proceed.Mon, Feb 4, 16:31
Fabien updated this revision to Diff 7167.Mon, Feb 4, 17:35

Remove now useless add_test

Fabien added inline comments.Mon, Feb 4, 17:36
test/CMakeLists.txt
60 ↗(On Diff #7167)

This is fixed in D2498

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

check-bitoin-util

This revision now requires changes to proceed.Mon, Feb 4, 18:52
Fabien updated this revision to Diff 7168.Mon, Feb 4, 18:59

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

deadalnix accepted this revision.Tue, Feb 5, 00:19
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.Tue, Feb 5, 00:19
Fabien added inline comments.Tue, Feb 5, 08:09
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.