Page MenuHomePhabricator
Differential D2494

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

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

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Commits
rSTAGINGb97a82b0f9a8: [CMAKE] Run test/util/bitcoin-util-test.py with ninja check
rABCb97a82b0f9a8: [CMAKE] Run test/util/bitcoin-util-test.py with ninja check
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

Fabien created this revision.Feb 4 2019, 11:07
Herald added a reviewer: Restricted Project. · View Herald TranscriptFeb 4 2019, 11:07
Herald added a reviewer: deadalnix. · View Herald Transcript
Herald added a subscriber: schancel. · View Herald Transcript
teamcity added a subscriber: teamcity.Feb 4 2019, 11:07

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

teamcity added a comment.Feb 4 2019, 11:21

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B4861: Diff 7154.Feb 4 2019, 11:21
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
Fabien updated this revision to Diff 7161.Feb 4 2019, 15:44

Remove wrapping the script with ctest

Fabien edited the summary of this revision. (Show Details)Feb 4 2019, 15:44
Fabien edited the test plan for this revision. (Show Details)
teamcity added a comment.Feb 4 2019, 15:45

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Fabien added inline comments.Feb 4 2019, 15:46
test/CMakeLists.txt
60 ↗(On Diff #7154)

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

Harbormaster completed remote builds in B4866: Diff 7161.Feb 4 2019, 15:59
teamcity added a comment.Feb 4 2019, 15:59

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

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
Fabien updated this revision to Diff 7167.Feb 4 2019, 17:35

Remove now useless add_test

teamcity added a comment.Feb 4 2019, 17:36

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

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

This is fixed in D2498

teamcity added a comment.Feb 4 2019, 17:50

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B4870: Diff 7167.Feb 4 2019, 17:50
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
Fabien updated this revision to Diff 7168.Feb 4 2019, 18:59

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

teamcity added a comment.Feb 4 2019, 19:00

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

teamcity added a comment.Feb 4 2019, 19:14

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B4871: Diff 7168.Feb 4 2019, 19:14
deadalnix accepted this revision.Feb 5 2019, 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.Feb 5 2019, 00:19
Fabien added inline comments.Feb 5 2019, 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 ?

Closed by commit rABCb97a82b0f9a8: [CMAKE] Run test/util/bitcoin-util-test.py with ninja check (authored by Fabien). · Explain WhyFeb 9 2019, 08:15
This revision was automatically updated to reflect the committed changes.
Fabien added a commit: rSTAGINGb97a82b0f9a8: [CMAKE] Run test/util/bitcoin-util-test.py with ninja check.Sep 18 2019, 22:21

Revision Contents
Changeset List

PathSize
test/
15 lines

Diff 7161

View Options

test/CMakeLists.txt

Loading...