[tests] [build] Run functional tests in `make check`

jnewbery commented at 6:31 pm on March 21, 2017: member

EDITED 2017/03/23. This PR is now ready for wider review

This PR adds the following make targets:

make unit_tests: runs the unit tests
make functional_tests: runs a small number of functional tests. Currently those tests are:
- nodehandling - ‘Test node handling’
- httpbasics - ‘Test the RPC HTTP basics’
- invalidblockrequest - ‘Test node responses to invalid blocks’
- blockchain - ‘Test RPCs related to blockchain state’
- rpcnamedargs - ‘Test using named arguments for RPCs’ (but any feedback is welcome on whether we should change that list)
make all_functional_tests: runs all the functional tests, including the extended tests
make util_tests: runs the bitcoin-tx tests

make check is updated to run the unit_tests, functional_tests and util_tests targets.

All make test targets can be run from the root directory or the src directory.

In this implementation I’ve moved the functional and util targets to their own /test/Makefile.include file to separate them from the unit tests.

~This is a WIP PR and shouldn’t be merged yet. I’ve opened this to solicit feedback, particularly from @theuni , @MarcoFalke and @jtimon.~

~This PR does the following:~

~move the running of the ‘integration’ tests (currently the python functional tests and bitcoin-tx tests) from make check in /src/Makefile.test.include to make check in a new /test/Makefile.include file. Functionally, there is no difference for users running make check from the base dir, but it makes more sense to contain the integration tests in their own makefile rather then executing them from /src~
~runs a small number of functional tests as part of make check. I picked the following since they cover basic functionality and are fairly quick to run (20s when run in parallel on my PC), but I’m happy to change the list if people think we should add/remove particular tests:~
- ~nodehandling - ‘Test node handling’~
- ~httpbasics - ‘Test the RPC HTTP basics’~
- ~invalidblockrequest - ‘Test node responses to invalid blocks’~
- ~blockchain - ‘Test RPCs related to blockchain state’~
- ~rpcnamedargs - ‘Test using named arguments for RPCs’~

~@jtimon - you mentioned wanting to keep separate make targets for just running the unit tests or just running the functional tests. I haven’t implemented that yet, but I plan to before this PR gets merged. Are you happy with the target names make unittests and make functionaltests?~

fanquake added the label Tests on Mar 21, 2017

in test/Makefile.include:5 in ef6ace22ca outdated

0@@ -0,0 +1,16 @@
1+# Copyright (c) 2013-2017 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.
4+
5+EXTRA_DIST += test/functional test/util/bctest.py test/util/bitcoin-util-test.py test/util/data

theuni commented at 7:21 pm on March 22, 2017:

Please enumerate the files here as before so that it remains deterministic.

jnewbery commented at 4:15 pm on March 23, 2017:

done

in test/Makefile.include:11 in ef6ace22ca outdated

 6+
 7+check-local:
 8+	@echo "Running test/util/bitcoin-util-test.py..."
 9+	$(PYTHON) $(top_builddir)/test/util/bitcoin-util-test.py
10+	@echo "Running test/functional/test_runner.py..."
11+	$(top_builddir)/test/functional/test_runner.py nodehandling httpbasics invalidblockrequest blockchain rpcnamedargs.py

theuni commented at 7:27 pm on March 22, 2017:

I like this! But please make a var for it so that we can have obvious diffs when we add tests. Something like:

0FUNCTIONAL_TESTS =
1FUNCTIONAL_TESTS += nodehandling
2FUNCTIONAL_TESTS += httpbasics
3...
4$(top_builddir)/test/functional/test_runner.py $(FUNCTIONAL_TESTS)

Going even further, as a later step, we may be able to use test_runner.py as an automake test launcher, so that each test gets parallelized as its own make job.

jnewbery commented at 4:15 pm on March 23, 2017:

done

in test/Makefile.include:9 in ef6ace22ca outdated

0@@ -0,0 +1,16 @@
1+# Copyright (c) 2013-2017 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.
4+
5+EXTRA_DIST += test/functional test/util/bctest.py test/util/bitcoin-util-test.py test/util/data
6+
7+check-local:
8+	@echo "Running test/util/bitcoin-util-test.py..."
9+	$(PYTHON) $(top_builddir)/test/util/bitcoin-util-test.py

theuni commented at 7:41 pm on March 22, 2017:

I think all of these can just be (builddir), since this file is included rather than invoked recursively.

jnewbery commented at 4:15 pm on March 23, 2017:

done

in src/Makefile.test.include:149 in ef6ace22ca outdated

145@@ -146,8 +146,6 @@ bitcoin_test_clean : FORCE
146 	rm -f $(CLEAN_BITCOIN_TEST) $(test_test_bitcoin_OBJECTS) $(TEST_BINARY)
147 
148 check-local:
149-	@echo "Running test/util/bitcoin-util-test.py..."

theuni commented at 7:48 pm on March 22, 2017:

As you mentioned in the OP, the python tests will only be run now when running ‘make check’ from the root dir.

I was going to suggest adding a target “check-full” or so, to run the python checks from src/, but that would effectively give us 3 levels of checking, confusion, and lots more bikeshedding.

So I think it would be wise to not differentiate what “make check” does based on your pwd. Let’s call the other tests from here as well.

jnewbery commented at 4:15 pm on March 23, 2017:

done

theuni changes_requested

theuni commented at 7:49 pm on March 22, 2017: member

Concept ACK.

jnewbery force-pushed on Mar 23, 2017

jnewbery renamed this:
~~[WIP] Run functional tests in `make check`~~
Run functional tests in `make check`
on Mar 23, 2017

jtimon commented at 5:22 pm on March 23, 2017: contributor

Concept ACK, this includes all the targets we discussed and more.

jnewbery force-pushed on Mar 24, 2017

sipa commented at 8:22 pm on March 24, 2017: member

@theuni Can you update your “Changes requested” review if they no longer apply?

jnewbery commented at 8:55 pm on March 24, 2017: member

This builds locally and on travis. I’m still happy to receive input on the following:

are people happy with the new target names make unit_tests, make functional_tests, make all_functional_tests, make util_tests and the new behaviour of make check?
are people happy with the list of functional tests I’ve chosen?
this currently calls into the test_runner with multiple test cases as arguments, which runs multiple tests in parallel. I don’t know whether it’s better to get make to run each test as a separate job. That could be done as a later step outside this PR, but feedback here would be welcomed.

theuni approved

MarcoFalke commented at 1:01 pm on March 25, 2017: member

Shouldn’t make check -j 2 pass down the number of jobs?

jnewbery commented at 1:47 pm on March 25, 2017: member

@MarcoFalke I haven’t tested. By default test_runner.py will run 4 tests in parallel, but I don’t know how that compares with make starting 4 different instances of test_runner.py, each running a single test.

MarcoFalke commented at 1:50 pm on March 25, 2017: member

No, running several instances of test_runner is not supported. What I meant is that make check -j 2 will call ./test_runner.py --jobs 2 instead of ./test_rnner.py (--jobs 4), which is the default. (I choose 4 as the default because it was the best performing option on travis at that time)

JeremyRubin commented at 6:27 pm on March 28, 2017: contributor

utack, I reviewed the changeset and it seems reasonable (but I don’t know Make well enough to fully Ack). @MarcoFalke -j is for the number of jobs allocated across the Make DAG traversal build process, not for sub-processes; that should be handled somehow else.

TheBlueMatt commented at 7:39 pm on March 28, 2017: member

Concept ACK. I have no ability to judge whether the makefile changes are sane, but do wonder about removing the various caches in test, eg test/pycache and test/tmp, as clean should usually restore the state of the directory to what it was pre-make, afaiu.

jnewbery commented at 8:37 pm on March 28, 2017: member

@TheBlueMatt good catch. I’d added these to the CLEANFILES variable, but that only cleans files, not directories! I’ve restored the previous clean-local: target now.

fanquake commented at 10:47 am on April 2, 2017: member

Started testing this on top of fbf36cae3a4613e289cec6c5eaeb1395d362a037 . Just ran make initially, then: make unit_tests

 0bash-3.2$ make unit_tests
 1/Applications/Xcode.app/Contents/Developer/usr/bin/make -C src unit_tests
 2  CC       src/tests-tests.o
 3  CCLD     tests
 4  CC       src/exhaustive_tests-tests_exhaustive.o
 5  CCLD     exhaustive_tests
 6/Applications/Xcode.app/Contents/Developer/usr/bin/make  check-TESTS
 7PASS: tests
 8PASS: exhaustive_tests
 9============================================================================
10Testsuite summary for libsecp256k1 0.1
11============================================================================
12# TOTAL: 2
13# PASS:  2
14# SKIP:  0
15# XFAIL: 0
16# FAIL:  0
17# XPASS: 0
18# ERROR: 0
19============================================================================
20  CXX      test/test_unitester-unitester.o
21  CXXLD    test/unitester
22/Applications/Xcode.app/Contents/Developer/usr/bin/make  check-TESTS
23PASS: test/unitester
24============================================================================
25Testsuite summary for univalue 1.0.2
26============================================================================
27# TOTAL: 1
28# PASS:  1
29# SKIP:  0
30# XFAIL: 0
31# FAIL:  0
32# XPASS: 0
33# ERROR: 0
34============================================================================

make util_tests

0bash-3.2$ make util_tests
1Running test/util/bitcoin-util-test.py...
2/usr/local/bin/python3.6 ./test/util/bitcoin-util-test.py

make functional_tests

 0bash-3.2$ make functional_tests
 1Running test/functional/test_runner.py...
 2./test/functional/test_runner.py nodehandling httpbasics invalidblockrequest blockchain rpcnamedargs
 3..........
 4invalidblockrequest.py passed, Duration: 5 s
 5......
 6blockchain.py passed, Duration: 9 s
 7
 8rpcnamedargs.py passed, Duration: 4 s
 9......
10httpbasics.py passed, Duration: 13 s
11...........................
12nodehandling.py passed, Duration: 27 s
13
14TEST                   | STATUS  | DURATION
15
16invalidblockrequest.py | Passed  | 5 s
17blockchain.py          | Passed  | 9 s
18rpcnamedargs.py        | Passed  | 4 s
19httpbasics.py          | Passed  | 13 s
20nodehandling.py        | Passed  | 27 s
21
22ALL                    | True    | 58 s (accumulated)
23
24Runtime: 27 s

make all_functional_tests

 0bash-3.2$ make all_functional_tests
 1Running all functional tests
 2./test/functional/test_runner.py --extended
 3.........................................................................................................................................................................................................................................................................................................................................................
 4fundrawtransaction.py passed, Duration: 173 s
 5.....................................................................................................................................................................................................................
 6p2p-compactblocks.py passed, Duration: 107 s
 7.....................................................................................................
 8walletbackup.py passed, Duration: 332 s
 9.........................................................................
10segwit.py passed, Duration: 88 s
11..................................................................................................
12wallet-accounts.py passed, Duration: 50 s
13.............................................................................................
14wallet.py passed, Duration: 134 s
15....................................................................................
16wallet-hd.py passed, Duration: 510 s
17................................
18p2p-segwit.py passed, Duration: 107 s
19
20... snip ...

in Makefile.am:238 in 8f52a73b27 outdated

291 
292 clean-local:
293-	rm -rf coverage_percent.txt test_bitcoin.coverage/ total.coverage/ test/tmp/ cache/ $(OSX_APP)
294-	rm -rf test/functional/__pycache__
295+	rm -rf coverage_percent.txt test_bitcoin.coverage/ total.coverage/ cache/ $(OSX_APP)
296+	rm -rf test/cache test/functional/__pycache__ test/tmp

fanquake commented at 10:48 am on April 2, 2017:

Should functional/test_framework/__pycache__ and util/__pycache__ be included here?

jnewbery commented at 1:08 pm on April 3, 2017:

Yes, I think you’re right. Thanks.

jnewbery force-pushed on Apr 3, 2017

jnewbery commented at 1:08 pm on April 3, 2017: member

nits addressed and commits squashed

fanquake commented at 2:51 am on April 4, 2017: member

Both linux builds failed with:

 0Traceback (most recent call last):
 1  File "test/functional/test_runner.py", line 435, in <module>
 2    main()
 3  File "test/functional/test_runner.py", line 231, in main
 4    run_tests(test_list, config["environment"]["SRCDIR"], config["environment"]["BUILDDIR"], config["environment"]["EXEEXT"], args.jobs, args.coverage, passon_args)
 5  File "test/functional/test_runner.py", line 270, in run_tests
 6    (name, stdout, stderr, status, duration) = job_queue.get_next()
 7  File "test/functional/test_runner.py", line 329, in get_next
 8    stderr=log_stderr),
 9  File "/usr/lib/python3.4/subprocess.py", line 859, in __init__
10    restore_signals, start_new_session)
11  File "/usr/lib/python3.4/subprocess.py", line 1457, in _execute_child
12    raise child_exception_type(errno_num, err_msg)
13FileNotFoundError: [Errno 2] No such file or directory: '/home/travis/build/bitcoin/bitcoin/build/bitcoin-i686-pc-linux-gnu/test/functional/net.py'

ryanofsky commented at 12:48 pm on April 4, 2017: member

There are a lot of targets to keep track of here. It would be nice at some point to add a make help rule that would describe common targets you expect people will want to run.

jnewbery force-pushed on Apr 4, 2017

Run functional tests in `make check` 7761435f80

jnewbery force-pushed on Apr 4, 2017

jnewbery commented at 6:23 pm on April 4, 2017: member

thanks @fanquake . This was a bad merge that github wasn’t warning about. Hopefully should work now that I’ve rebased.

laanwj added this to the "Blockers" column in a project

theuni commented at 10:24 pm on April 13, 2017: member

As travis runs “make check” as well as the functional tests, I believe it will be running some of these twice now? If so, I’m afraid we’ll need yet another target that means either check+all_functional_tests, or check - functional_tests.

jnewbery commented at 1:56 pm on April 17, 2017: member

@theuni I really don’t want to add yet another make target. As @ryanofsky has pointed out there are already too many to reasonably keep track of. Ideally the functional tests run by make check should be so fast that it doesn’t matter that travis is running them twice.

Thinking about this a little bit more, I think it might make sense to have a new basic_tests.py functional test script specifically for this purpose. There are a few requirements:

it should be fast. Ideally < 1 second for the script and < 5 seconds including set up and tear down.
it shouldn’t start more than one instance of bitcoind, so there are no issues running it on a low-powered machine
it should cover mainline basic functionality (RPCs, P2P, tx/block validation & propogation)

I chose invalidblockrequest.py, blockchain.py, rpcnamedargs.py, httpbasics.py and nodehandling.py initially since they provided a fairly good cover of basic functionality. If instead of those, we ran a more targeted basic_tests script, we could probably have better test coverage with a lower run time.

Thoughts?

MarcoFalke commented at 2:36 pm on April 17, 2017: member

So the basic test is a single test script with excerpts taken from the other test scripts, and is exclusively run by make check?

An alternative would be to unfiddle the make check on travis into make unit_tests && make util_tests.

jnewbery commented at 2:57 pm on April 17, 2017: member

So the basic test is a single test script with excerpts taken from the other test scripts, and is exclusively run by make check?

That’s basically the idea, yes. Not exactly copy-and-pasted from those other tests, but modified slightly to ensure good functional coverage, fast execution, and low resource demands.

in test/Makefile.include:161 in 7761435f80

156+    httpbasics \
157+    invalidblockrequest \
158+    blockchain \
159+    rpcnamedargs
160+
161+.PHONY: all_functional_tests functional_tests util_tests

jtimon commented at 6:20 pm on April 18, 2017:

why these targets need to be here in the new file instead of alongside unit_tests and check-local? It seems nice for EXTRA_DIST and FUNCTIONAL_TESTS, but that’s it IMO.

jtimon commented at 6:27 pm on April 18, 2017: contributor

As travis runs “make check” as well as the functional tests, I believe it will be running some of these twice now? If so, I’m afraid we’ll need yet another target that means either check+all_functional_tests, or check - functional_tests.

What about just excluding the tests in FUNCTIONAL_TESTS for travis? We have the exclude option now, it seems we just need a file that both the makefile and travis can read to put FUNCTIONAL_TESTS in. Wouldn’t test/Makefile.include do it directly if some of the things were left out?

sipa commented at 8:14 pm on May 5, 2017: member

What is needed here to fix Travis?

jnewbery commented at 8:33 pm on May 5, 2017: member

This PR breaks whenever functional tests are added/renamed. In this instance nodehandling.py was renamed disconnect_ban.py. I haven’t fixed it because there didn’t seem to be much general interest in reviewing or providing concept feedback so I’ve been focusing on other things.

TheBlueMatt commented at 3:57 pm on May 9, 2017: member

I think we should be happy to let make check go for 10/30 minutes, we wouldnt be the only project to do so, and its good to get test coverage during package building (which, really, is the largest make check user). My vote would be for moving all the test running into make, then we also dont have the double-file-listing of alll the tests in the makefile and in the test_runner.

jnewbery commented at 8:13 pm on May 9, 2017: member

@TheBlueMatt - I’m not sure that we should run all the test in make. Some of them have quite high memory/CPU/disk requirements - for example pruning.py runs 6 nodes concurrently and uses > 4GiB of disk space. We don’t want make check to fail for users with lower powered systems.

I also don’t think this resolves the double-file-listing issue. The functional tests need to be listed in EXTRA_DIST in the make file to be included in a distdir.

TheBlueMatt commented at 9:53 pm on May 9, 2017: member

@jnewbery indeed, I meant that we should avoid the travis issue by putting everything that normally runs in travis in make check, essentially. if we then had a second make target (extended-tests), we could drop test_runner.py and then we dont need to double-list anything :).

jnewbery commented at 10:04 pm on May 9, 2017: member

test_runner is doing more than you think. For one, we can’t run multiple tests in parallel without providing a different port seed to each (otherwise they’ll fight over ports). There are various other checks and nice things (such as logging) that it does. I don’t think we can remove it and rely on make.

sipa commented at 10:30 pm on May 9, 2017: member

Why not just invoke test_runner.py in Travis/make check, then? test_runner already distinguishes between normal and extended tests.

jnewbery commented at 3:06 pm on May 16, 2017: member

Why not just invoke test_runner.py in Travis/make check, then? test_runner already distinguishes between normal and extended tests.

Because I don’t want make check to become impossible or unusably slow for people running on constrained hardware. There are tests in the normal tests which fire up multiple versions of bitcoind or create long blockchains. Systems with limited memory could easily end up paging heavily or running out of memory. My understanding is that anyone building bitcoind locally, even on their raspberry pi, should be able to run make check to verify their build.

test_runner.py also controls how many tests are run in parallel. If we change the default from 4 down to 1, it may make it easier to run on more limited systems, at the cost of making make check take even longer.

sipa removed this from the "Blockers" column in a project

jnewbery renamed this:
~~Run functional tests in `make check`~~
[tests] [build] Run functional tests in `make check`
on Jun 30, 2017

jnewbery commented at 3:09 pm on September 14, 2017: member

Closing for now. I’ll pick this up again at some point

jnewbery closed this on Sep 14, 2017

DrahtBot locked this on Sep 8, 2021

[tests] [build] Run functional tests in make check #10044

[tests] [build] Run functional tests in `make check` #10044