Page MenuHomePhabricator

Integration of property based testing into Bitcoin ABC
ClosedPublic

Authored by Fabien on Feb 25 2020, 14:48.

Details

Summary

This is an initial integration of the rapidcheck property based tests.
The original test has been slightly modified to adapt to our codebase.
The feature has been added to CMake and the documentation updated
accordingly.
Note that as opposed to autotools, which enables the test automatically
if rapidcheck is installed, it is disabled by default with cmake and
requires a flag to be set by the user.

Backport of core PR12775, PR16622 and PR16645.

Test Plan

Read the doc and install rapidcheck.

../configure

Ensure that rapidcheck is used by looking at the summary.

make check

Ensure there is no regression:

cmake -GNinja ..
ninja check


cmake -GNinja .. -DENABLE_PROPERTY_BASED_TESTS=ON

Check in the output that rapidcheck is enabled by searching for a
couple lines similar to:

-- Found Rapidcheck: /usr/local/include
-- Found Rapidcheck: /usr/local/lib/librapidcheck.a
ninja check

Assuming you are running a 64-bit Linux machine:

cd depends
RAPIDCHECK=1 make build-linux64

Check the rapidcheck package is built.

cd .. && mkdir buildLinux64 && cd buildLinux64
cmake -GNinja .. \
  -DCMAKE_TOOLCHAIN_FILE=../cmake/platforms/Linux64.cmake \
  -DENABLE_PROPERTY_BASED_TESTS=ON

Check the rapidcheck lib found is the one from the depends.

ninja check

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

[Bot Message]
One or more PR numbers were detected in the summary.
Links to those Bitcoin Core PRs have been inserted into the summary for reference.

Use all the CPU cores to build the depends package.

jasonbcox requested changes to this revision.Feb 25 2020, 22:42
jasonbcox added a subscriber: jasonbcox.

Test plan is incomplete. Other than the nits and other comments, everything else looks good.

cmake/modules/FindRapidcheck.cmake
11 ↗(On Diff #16487)

Nit: s/::/:

13 ↗(On Diff #16487)

s/Rapicheck/Rapidcheck

17 ↗(On Diff #16487)

Nit: s/::/:

32 ↗(On Diff #16487)

I noticed this too. There's an open issue for rapidcheck not having releases: https://github.com/emil-e/rapidcheck/issues/235 Consider adding this to the comment.

depends/packages/rapidcheck.mk
3 ↗(On Diff #16487)

This doesn't need to be fixed immediately, but flagging this since it's dependent on an old pre-release of MarcoFalke's rapidcheck repo.

src/test/CMakeLists.txt
90 ↗(On Diff #16487)

I assume this being null by default is ok, but probably best to add checking the default, rapidcheck-disabled case to the test plan.

This revision now requires changes to proceed.Feb 25 2020, 22:42
cmake/modules/FindRapidcheck.cmake
11 ↗(On Diff #16487)
17 ↗(On Diff #16487)

Dito

depends/packages/rapidcheck.mk
3 ↗(On Diff #16487)

It's on the stack, see D5324

src/test/CMakeLists.txt
90 ↗(On Diff #16487)

Yes it is fine because it is not quoted. I'll add it to the test plan.

Fabien edited the test plan for this revision. (Show Details)

Fix typo and link the github issue.

jasonbcox added inline comments.
cmake/modules/FindRapidcheck.cmake
11 ↗(On Diff #16487)

TIL

This revision is now accepted and ready to land.Feb 26 2020, 17:59