Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integration of property based testing into Bitcoin Core #12775

Merged
merged 1 commit into from Sep 8, 2018

Conversation

Projects
None yet
8 participants
@Christewart
Copy link
Contributor

commented Mar 24, 2018

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}.

@fanquake fanquake added the Tests label Mar 24, 2018

@practicalswift

This comment has been minimized.

Copy link
Member

commented Mar 25, 2018

Strong concept ACK

Very nice work!

@randolf

This comment has been minimized.

Copy link
Contributor

commented Mar 25, 2018

@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?

@randolf

This comment has been minimized.

Copy link
Contributor

commented Mar 25, 2018

@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?

@MarcoFalke MarcoFalke requested a review from theuni Mar 25, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Mar 25, 2018

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

@Christewart

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2018

@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.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Mar 26, 2018

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

@laanwj

This comment has been minimized.

Copy link
Member

commented Mar 27, 2018

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.)

src/test/gen/crypto_gen.cpp Outdated
@@ -0,0 +1,23 @@
#include "test/gen/crypto_gen.h"

This comment has been minimized.

Copy link
@fanquake

fanquake Apr 19, 2018

Member

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.
src/test/key_properties.cpp Outdated
@@ -0,0 +1,53 @@
// Copyright (c) 2012-2016 The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

This comment has been minimized.

Copy link
@fanquake

fanquake Apr 19, 2018

Member

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.
$(package)_sha256_hash=9640926223c00af45bce4c7df8b756b5458a89b2ba74cfe3e404467f13ce26df

define $(package)_config_cmds
cmake -DCMAKE_POSITION_INDEPENDENT_CODE:BOOL=true .

This comment has been minimized.

Copy link
@fanquake

fanquake Apr 19, 2018

Member

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
@fanquake

This comment has been minimized.

Copy link
Member

commented Apr 19, 2018

Testing 900a441 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
@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Jul 14, 2018

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

@Christewart Christewart force-pushed the Christewart:rapidcheck_build branch 3 times, most recently Jul 14, 2018

@Christewart

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2018

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

@Christewart Christewart force-pushed the Christewart:rapidcheck_build branch 2 times, most recently Jul 15, 2018

@Christewart

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2018

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

@laanwj

This comment has been minimized.

Copy link
Member

commented Aug 27, 2018

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

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

@Christewart Christewart force-pushed the Christewart:rapidcheck_build branch to b2f49bd Aug 27, 2018

@Christewart

This comment has been minimized.

Copy link
Contributor Author

commented Aug 27, 2018

@laanwj Rebased

RAPIDCHECK_LIBS=-lrapidcheck
fi
AC_SUBST(RAPIDCHECK_LIBS)
AM_CONDITIONAL([ENABLE_PROPERTY_TESTS], [test x$enable_property_tests = xyes])

This comment has been minimized.

Copy link
@laanwj

laanwj Aug 31, 2018

Member

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

define $(package)_build_cmds
$(MAKE) && \
mkdir -p $($(package)_staging_dir)$(host_prefix)/include && \

This comment has been minimized.

Copy link
@laanwj

laanwj Aug 31, 2018

Member

indeed, this is necessary because rapidcheck has no make install

This comment has been minimized.

Copy link
@Christewart

Christewart Aug 31, 2018

Author Contributor

This is an open PR on the repo

emil-e/rapidcheck#152

This comment has been minimized.

Copy link
@Empact

Empact Oct 9, 2018

Member

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

@laanwj

This comment has been minimized.

Copy link
Member

commented Aug 31, 2018

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

@laanwj

This comment has been minimized.

Copy link
Member

commented Aug 31, 2018

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

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

@laanwj

This comment has been minimized.

Copy link
Member

commented Aug 31, 2018

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
@Christewart

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2018

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 :-).

@fanquake

This comment has been minimized.

Copy link
Member

commented Aug 31, 2018

@laanwj

This comment has been minimized.

Copy link
Member

commented Aug 31, 2018

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."

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Aug 31, 2018

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

#include <rapidcheck/gen/Container.h>

/** Generates 1 to 20 keys for OP_CHECKMULTISIG */
rc::Gen<std::vector<CKey>> MultisigKeys()

This comment has been minimized.

Copy link
@practicalswift

practicalswift Sep 6, 2018

Member

MultisigKeys() seems unused? Is that intentional?

This comment has been minimized.

Copy link
@Christewart

Christewart Sep 6, 2018

Author Contributor

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

This comment has been minimized.

Copy link
@Empact

Empact Oct 9, 2018

Member

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.

@bitcoin bitcoin deleted a comment from pawan17kumar Sep 6, 2018

pull bot pushed a commit to jaschadub/bitcoin that referenced this pull request Sep 8, 2018

Merge bitcoin#12775: Integration of property based testing into Bitco…
…in Core

b2f49bd Integration of property based testing into Bitcoin Core (Chris Stewart)

Pull request description:

  This PR is a subset of the changes in bitcoin#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}`.

Tree-SHA512: 895c9d9273dcd29f696b1de8dfe1ee843095831bf1f68472844181278850bec36b20f0ba7e51e796112c5cc75cd24759f9f1771906503bbf3af16f627e18c6c9

@laanwj laanwj merged commit b2f49bd into bitcoin:master Sep 8, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@@ -8,6 +8,7 @@ TEST_SRCDIR = test
TEST_BINARY=test/test_bitcoin$(EXEEXT)

JSON_TEST_FILES = \
test/data/script_tests.json \

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Sep 9, 2018

Member

Why is this change required?

cp -a include/* $($(package)_staging_dir)$(host_prefix)/include/ && \
cp -a extras/boost_test/include/rapidcheck/* $($(package)_staging_dir)$(host_prefix)/include/rapidcheck/ && \
mkdir -p $($(package)_staging_dir)$(host_prefix)/lib && \
cp -a librapidcheck.a $($(package)_staging_dir)$(host_prefix)/lib/

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Sep 9, 2018

Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.