Integration of property based testing into Bitcoin Core #12775

pull Christewart wants to merge 1 commits into bitcoin:master from Christewart:rapidcheck_build changing 9 files +196 −1
  1. Christewart commented at 11:39 PM on March 24, 2018: member

    This PR is a subset of the changes in #8469. It's meant to be easier to review. This PR contains all of the build instructions needed for travis to pass. It includes one property call key_properties.cpp along with a generator file called crypto_gen.{h,cpp}.

  2. fanquake added the label Tests on Mar 24, 2018
  3. practicalswift commented at 6:10 PM on March 25, 2018: contributor

    Strong concept ACK

    Very nice work!

  4. randolf commented at 8:05 PM on March 25, 2018: contributor

    @Christewart I noticed that depends/packages/rapidcheck.mk references MarcoFalke:

    $(package)_download_path=https://github.com/MarcoFalke/rapidcheck/archive

    Do you think it might be better to reference directly from "bitcoin" instead? If so, should MarcoFalke's "rapidcheck" project be added to the "bitcoin" project to facilitate this?

  5. randolf commented at 8:08 PM on March 25, 2018: contributor

    @Christewart In src/test/key_properties.cpp there are new references to the BOOST libraries. As I understand it, one of the goals is to move away from BOOST, so I'm curious: Is it possible to accomplish the same goals in this code without the additional reliance on BOOST?

  6. MarcoFalke requested review from theuni on Mar 25, 2018
  7. MarcoFalke commented at 8:24 PM on March 25, 2018: member

    We don't plan to get rid of the boost unit test framework any time soon.

  8. Christewart commented at 11:24 AM on March 26, 2018: member

    @randolf I'm not sure if it is too much of a concern right now who hosts the package. If we want to move it under the bitcoin project in the future i have no problem with that. Marco was kind enough to help me out with the setup which is why his repo is currently referenced.

  9. MarcoFalke commented at 12:39 PM on March 26, 2018: member

    @randolf Ideally it should reference the proper upstream, but since they don't have any releases that might not work.

  10. laanwj commented at 7:53 PM on March 27, 2018: member

    We don't plan to get rid of the boost unit test framework any time soon.

    Indeed. @randolf In general: please don't comment on boost usage of PRs in reviews if it concerns libraries that are already used in other parts of our code (such as boost test), only if it concerns using new boost libaries (such as lexical cast and whatnot.)

  11. in src/test/gen/crypto_gen.cpp:4 in 900a441eea outdated 
       0 | @@ -0,0 +1,23 @@
   1 | +#include "test/gen/crypto_gen.h"
    fanquake commented at 8:14 AM on April 19, 2018:

    This and the .h are missing a copyright header:

    // Copyright (c) 2018 The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
  12. in src/test/key_properties.cpp:3 in 900a441eea outdated 
       0 | @@ -0,0 +1,53 @@
   1 | +// Copyright (c) 2012-2016 The Bitcoin Core developers
   2 | +// Distributed under the MIT software license, see the accompanying
   3 | +// file COPYING or http://www.opensource.org/licenses/mit-license.php.
    fanquake commented at 8:15 AM on April 19, 2018:

    This can just be:

    // Copyright (c) 2018 The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
  13. in depends/packages/rapidcheck.mk:8 in 900a441eea outdated 
       0 | @@ -0,0 +1,18 @@
   1 | +package=rapidcheck
   2 | +$(package)_version=10fc0cb
   3 | +$(package)_download_path=https://github.com/MarcoFalke/rapidcheck/archive
   4 | +$(package)_file_name=$(package)-$($(package)_version).tar.gz
   5 | +$(package)_sha256_hash=9640926223c00af45bce4c7df8b756b5458a89b2ba74cfe3e404467f13ce26df
   6 | +
   7 | +define $(package)_config_cmds
   8 | +  cmake -DCMAKE_POSITION_INDEPENDENT_CODE:BOOL=true .
    fanquake commented at 8:16 AM on April 19, 2018:

    Looks like we'll need some documentation about installing/having cmake available

    Extracting rapidcheck...
/Users/xxx/GitHub/bitcoin/depends/sources/rapidcheck-10fc0cb.tar.gz: OK
Preprocessing rapidcheck...
Configuring rapidcheck...
/bin/sh: cmake: command not found
  14. fanquake commented at 8:27 AM on April 19, 2018: member

    Testing https://github.com/bitcoin/bitcoin/pull/12775/commits/900a441eeacd9f936a2495e878174f9aeead66d2 on macOS 10.13.4:

    After installing cmake I could successfully build rapidcheck in depends:

    bash-3.2$ make NO_QT=1 RAPIDCHECK=1
Configuring rapidcheck...
-- The C compiler identification is AppleClang 9.1.0.9020039
-- The CXX compiler identification is AppleClang 9.1.0.9020039
-- Check for working C compiler: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/cc
-- Check for working C compiler: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/cc -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
-- Check for working CXX compiler: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++
-- Check for working CXX compiler: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Configuring done
-- Generating done
-- Build files have been written to: /Users/xxx/GitHub/bitcoin/depends/work/build/x86_64-apple-darwin17.5.0/rapidcheck/10fc0cb-3719dbaacbb
Building rapidcheck...
Scanning dependencies of target rapidcheck
[  2%] Building CXX object CMakeFiles/rapidcheck.dir/src/BeforeMinimalTestCase.cpp.o
[  5%] Building CXX object CMakeFiles/rapidcheck.dir/src/Check.cpp.o
[  8%] Building CXX object CMakeFiles/rapidcheck.dir/src/Classify.cpp.o
[ 11%] Building CXX object CMakeFiles/rapidcheck.dir/src/GenerationFailure.cpp.o
[ 14%] Building CXX object CMakeFiles/rapidcheck.dir/src/Log.cpp.o
[ 17%] Building CXX object CMakeFiles/rapidcheck.dir/src/Random.cpp.o
[ 20%] Building CXX object CMakeFiles/rapidcheck.dir/src/Show.cpp.o
[ 22%] Building CXX object CMakeFiles/rapidcheck.dir/src/detail/Any.cpp.o
[ 25%] Building CXX object CMakeFiles/rapidcheck.dir/src/detail/Assertions.cpp.o
[ 28%] Building CXX object CMakeFiles/rapidcheck.dir/src/detail/Base64.cpp.o
[ 31%] Building CXX object CMakeFiles/rapidcheck.dir/src/detail/Configuration.cpp.o
[ 34%] Building CXX object CMakeFiles/rapidcheck.dir/src/detail/DefaultTestListener.cpp.o
[ 37%] Building CXX object CMakeFiles/rapidcheck.dir/src/detail/FrequencyMap.cpp.o
[ 40%] Building CXX object CMakeFiles/rapidcheck.dir/src/detail/ImplicitParam.cpp.o
[ 42%] Building CXX object CMakeFiles/rapidcheck.dir/src/detail/LogTestListener.cpp.o
[ 45%] Building CXX object CMakeFiles/rapidcheck.dir/src/detail/MapParser.cpp.o
[ 48%] Building CXX object CMakeFiles/rapidcheck.dir/src/detail/MulticastTestListener.cpp.o
[ 51%] Building CXX object CMakeFiles/rapidcheck.dir/src/detail/ParseException.cpp.o
[ 54%] Building CXX object CMakeFiles/rapidcheck.dir/src/detail/Platform.cpp.o
[ 57%] Building CXX object CMakeFiles/rapidcheck.dir/src/detail/Property.cpp.o
[ 60%] Building CXX object CMakeFiles/rapidcheck.dir/src/detail/PropertyContext.cpp.o
[ 62%] Building CXX object CMakeFiles/rapidcheck.dir/src/detail/ReproduceListener.cpp.o
[ 65%] Building CXX object CMakeFiles/rapidcheck.dir/src/detail/Results.cpp.o
[ 68%] Building CXX object CMakeFiles/rapidcheck.dir/src/detail/Serialization.cpp.o
[ 71%] Building CXX object CMakeFiles/rapidcheck.dir/src/detail/StringSerialization.cpp.o
[ 74%] Building CXX object CMakeFiles/rapidcheck.dir/src/detail/TestMetadata.cpp.o
[ 77%] Building CXX object CMakeFiles/rapidcheck.dir/src/detail/TestParams.cpp.o
[ 80%] Building CXX object CMakeFiles/rapidcheck.dir/src/detail/Testing.cpp.o
[ 82%] Building CXX object CMakeFiles/rapidcheck.dir/src/gen/Numeric.cpp.o
[ 85%] Building CXX object CMakeFiles/rapidcheck.dir/src/gen/Text.cpp.o
[ 88%] Building CXX object CMakeFiles/rapidcheck.dir/src/gen/detail/ExecHandler.cpp.o
[ 91%] Building CXX object CMakeFiles/rapidcheck.dir/src/gen/detail/GenerationHandler.cpp.o
[ 94%] Building CXX object CMakeFiles/rapidcheck.dir/src/gen/detail/Recipe.cpp.o
[ 97%] Building CXX object CMakeFiles/rapidcheck.dir/src/gen/detail/ScaleInteger.cpp.o
[100%] Linking CXX static library librapidcheck.a
[100%] Built target rapidcheck
Staging rapidcheck...
Postprocessing rapidcheck...
Caching rapidcheck...

    Did a ./configure --prefix=`pwd`/depends/x86_64-apple-darwin17.5.0:

    checking rapidcheck.h usability... yes
checking rapidcheck.h presence... yes
checking for rapidcheck.h... yes

    You could add rapidcheck under the "Options used to compile and link:" at the end of ./configure

    make check -j6 failed with:

    Running tests: from test/gen/crypto_gen.cpp
Missing an argument value for the parameter run_test in the argument 

Parameter: run_test
 Filters, which test units to include or exclude from test module execution.
 Command line formats:
   --run_test=<test unit filter>
   -t <test unit filter>
 Environment variable: BOOST_TEST_RUN_FILTERS

For detailed help on Boost.Test parameters use:
  test_bitcoin --help
or
  test_bitcoin --help=<parameter name>
make[3]: *** [test/gen/crypto_gen.cpp.test] Error 1
make[3]: *** Waiting for unfinished jobs....
make[2]: *** [check-am] Error 2
make[1]: *** [check-recursive] Error 1
make: *** [check-recursive] Error 1
  15. fanquake added this to the milestone 0.17.0 on May 31, 2018
  16. MarcoFalke commented at 11:55 AM on July 14, 2018: member

    Could you set RAPIDCHECK for the depends build and --with-rapidcheck for the configure, such that travis runs it?

  17. Christewart force-pushed on Jul 14, 2018
  18. Christewart force-pushed on Jul 14, 2018
  19. Christewart force-pushed on Jul 15, 2018
  20. Christewart commented at 12:20 AM on July 15, 2018: member

    @fanquake I fixed the issue with make check. I was accidentally classifying generator files as actual unit test files

  21. Christewart force-pushed on Jul 15, 2018
  22. Christewart force-pushed on Jul 15, 2018
  23. Christewart commented at 1:23 PM on July 15, 2018: member

    one build is still failing from a timeout issue it appears? I don't think it is related to this pr?

  24. MarcoFalke removed this from the milestone 0.17.0 on Jul 29, 2018
  25. MarcoFalke added this to the milestone 0.18.0 on Jul 29, 2018
  26. DrahtBot added the label Needs rebase on Aug 5, 2018
  27. laanwj commented at 10:50 AM on August 27, 2018: member

    Probably a good time to move this forward and finally get it in now that 0.17 is branched off.

  28. Integration of property based testing into Bitcoin Core
    update copyright headers

attempt to fix linting errors

Fixing issue with make check classifying generator files as actual unit tests

Wrapping gen files in ENABLE_PROPERTY_TESTS macro

Make macro better
    b2f49bd732
  29. Christewart force-pushed on Aug 27, 2018
  30. Christewart commented at 2:17 PM on August 27, 2018: member

    @laanwj Rebased

  31. DrahtBot removed the label Needs rebase on Aug 27, 2018
  32. in configure.ac:1157 in b2f49bd732 
    1152 | +RAPIDCHECK_LIBS=
1153 | +if test "x$enable_property_tests" = xyes; then
1154 | +   RAPIDCHECK_LIBS=-lrapidcheck
1155 | +fi
1156 | +AC_SUBST(RAPIDCHECK_LIBS)
1157 | +AM_CONDITIONAL([ENABLE_PROPERTY_TESTS], [test x$enable_property_tests = xyes])
    laanwj commented at 9:55 AM on August 31, 2018:

    I think something should be logged to the output depending on whether rapidcheck is used or not, currently there is nothing in configure output that suggests this

    oh, just noticed in travis that without specifying explicit --with-rapidcheck there is:

    checking rapidcheck.h usability... no
checking rapidcheck.h presence... no
checking for rapidcheck.h... no
  33. in depends/packages/rapidcheck.mk:13 in b2f49bd732 
       8 | +  cmake -DCMAKE_POSITION_INDEPENDENT_CODE:BOOL=true .
   9 | +endef
  10 | +
  11 | +define $(package)_build_cmds
  12 | +  $(MAKE) && \
  13 | +  mkdir -p $($(package)_staging_dir)$(host_prefix)/include && \
    laanwj commented at 10:07 AM on August 31, 2018:

    indeed, this is necessary because rapidcheck has no make install

    Christewart commented at 11:55 AM on August 31, 2018:

    This is an open PR on the repo

    https://github.com/emil-e/rapidcheck/pull/152

    Empact commented at 12:44 AM on October 9, 2018:

    Now that the PR is merged, should this be switched over?

  34. laanwj commented at 10:09 AM on August 31, 2018: member

    I'm surprised that debian/ubuntu doesn't package rapidcheck, the only way to get it is to build it from source.

  35. laanwj commented at 10:14 AM on August 31, 2018: member

    This PR contains all of the build instructions needed for travis to pass.

    it is not enabled in travis.yml though, is that intentional?

  36. laanwj commented at 10:23 AM on August 31, 2018: member

    ACK

    tested both without and with rapidcheck, and can confirm that the tests run in the latter case:

    .../bitcoin/src/test/key_properties.cpp(22): Entering test suite "key_properties"
.../bitcoin/src/test/key_properties.cpp(25): Entering test case "key_uniqueness"
Using configuration: seed=9557370667331148370
.../bitcoin/src/test/key_properties.cpp(25): Leaving test case "key_uniqueness"; testing time: 3049us
.../bitcoin/src/test/key_properties.cpp(31): Entering test case "key_generates_correct_pubkey"
.../bitcoin/src/test/key_properties.cpp(31): Leaving test case "key_generates_correct_pubkey"; testing time: 36435us
.../bitcoin/src/test/key_properties.cpp(38): Entering test case "key_set_symmetry"
.../bitcoin/src/test/key_properties.cpp(38): Leaving test case "key_set_symmetry"; testing time: 2357us
.../bitcoin/src/test/key_properties.cpp(46): Entering test case "key_sign_symmetry"
.../bitcoin/src/test/key_properties.cpp(46): Leaving test case "key_sign_symmetry"; testing time: 28370us
.../bitcoin/src/test/key_properties.cpp(22): Leaving test suite "key_properties"; testing time: 70316us
  37. Christewart commented at 11:57 AM on August 31, 2018: member

    it is not enabled in travis.yml though, is that intentional?

    Frankly I haven't got that far. It's not intentional, just extra work that I don't have time to do ATM :-).

  38. fanquake commented at 12:32 PM on August 31, 2018: member

    @Christewart If you’re currently short on time, I can take this over.

    On Fri, 31 Aug 2018 at 19:57, Chris Stewart notifications@github.com wrote:

    it is not enabled in travis.yml though, is that intentional?

    Frankly I haven't got that far. It's not intentional, just extra work that I don't have time to do ATM :-).

    — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/12775#issuecomment-417642035, or mute the thread https://github.com/notifications/unsubscribe-auth/AA0t8vbkQrtybajNY4SDN2fDlhmUMWgNks5uWSSxgaJpZM4S58ml .

  39. laanwj commented at 12:36 PM on August 31, 2018: member

    Frankly I haven't got that far. It's not intentional, just extra work that I don't have time to do ATM :-).

    It's fine, doesn't have to be done in this PR. It's just that your opening post mentions "This PR contains all of the build instructions needed for travis to pass."

  40. MarcoFalke commented at 3:04 PM on August 31, 2018: member

    Enabling this on travis should just be setting RAPIDCHECK=1 in DEP_OPTS (and also installing cmake, i.e. add it to PACKAGES)

  41. in src/test/gen/crypto_gen.cpp:14 in b2f49bd732 
       9 | +#include <rapidcheck/Gen.h>
  10 | +#include <rapidcheck/gen/Predicate.h>
  11 | +#include <rapidcheck/gen/Container.h>
  12 | +
  13 | +/** Generates 1 to 20 keys for OP_CHECKMULTISIG */
  14 | +rc::Gen<std::vector<CKey>> MultisigKeys()
    practicalswift commented at 7:43 AM on September 6, 2018:

    MultisigKeys() seems unused? Is that intentional?

    Christewart commented at 3:57 PM on September 6, 2018:

    I don't think it is used on this PR, but I believe it is used with the remaining test cases in #8469. Basically this PR splits #8469 in half -- there is still a lot of test cases in that PR. Basically once this PR is merged i'm going to open another one with those test cases

    Empact commented at 12:47 AM on October 9, 2018:

    I think we should be more careful about adding unused code like this under the expectation that it will be used in the future. YAGNI + concerns about maintaining an informative record in the code history.

  42. laanwj deleted a comment on Sep 6, 2018
  43. pull[bot] referenced this in commit eea87ef537 on Sep 8, 2018
  44. laanwj merged this on Sep 8, 2018
  45. laanwj closed this on Sep 8, 2018
  46. in src/Makefile.test.include:11 in b2f49bd732 
       7 | @@ -8,6 +8,7 @@ TEST_SRCDIR = test
   8 |  TEST_BINARY=test/test_bitcoin$(EXEEXT)
   9 |  
  10 |  JSON_TEST_FILES = \
  11 | +  test/data/script_tests.json \
    MarcoFalke commented at 2:03 PM on September 9, 2018:

    Why is this change required?

  47. in depends/packages/rapidcheck.mk:17 in b2f49bd732 
      12 | +  $(MAKE) && \
  13 | +  mkdir -p $($(package)_staging_dir)$(host_prefix)/include && \
  14 | +  cp -a include/* $($(package)_staging_dir)$(host_prefix)/include/ && \
  15 | +  cp -a extras/boost_test/include/rapidcheck/* $($(package)_staging_dir)$(host_prefix)/include/rapidcheck/ && \
  16 | +  mkdir -p $($(package)_staging_dir)$(host_prefix)/lib && \
  17 | +  cp -a librapidcheck.a $($(package)_staging_dir)$(host_prefix)/lib/
    MarcoFalke commented at 6:02 PM on September 9, 2018:

    Might want to update to the latest rapidcheck and use the make install target?

  48. deadalnix referenced this in commit f9d5b98ef5 on Feb 27, 2020
  49. deadalnix referenced this in commit 254e3daef8 on Feb 28, 2020
  50. ftrader referenced this in commit 9613af3b3e on May 19, 2020
  51. MarcoFalke locked this on Sep 8, 2021
Contributors
ChristewartpracticalswiftrandolfMarcoFalkelaanwjfanquakeEmpact


theuni

Labels
Tests

Milestone
0.18.0

Linked (view graph)
#8469 [POC] Introducing property based testing to Core#14430 Add more property based tests for basic bitcoin data structures
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-02 03:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me