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

jnewbery · 2017-03-21T18:31:37Z

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?

theuni

Concept ACK.

theuni · 2017-03-22T19:21:02Z

test/Makefile.include

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

theuni · 2017-03-22T19:27:41Z

test/Makefile.include

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

FUNCTIONAL_TESTS = FUNCTIONAL_TESTS += nodehandling FUNCTIONAL_TESTS += httpbasics ... $(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.

theuni · 2017-03-22T19:41:04Z

test/Makefile.include

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

theuni · 2017-03-22T19:48:10Z

src/Makefile.test.include

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.

jtimon · 2017-03-23T17:22:18Z

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

sipa · 2017-03-24T20:22:09Z

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

jnewbery · 2017-03-24T20:55:29Z

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.

maflcko · 2017-03-25T13:01:52Z

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

jnewbery · 2017-03-25T13:47:17Z

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

maflcko · 2017-03-25T13:50:48Z

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 · 2017-03-28T18:27:23Z

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 · 2017-03-28T19:39:40Z

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 · 2017-03-28T20:37:13Z

@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 · 2017-04-02T10:47:01Z

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

bash-3.2$ make unit_tests
/Applications/Xcode.app/Contents/Developer/usr/bin/make -C src unit_tests
  CC       src/tests-tests.o
  CCLD     tests
  CC       src/exhaustive_tests-tests_exhaustive.o
  CCLD     exhaustive_tests
/Applications/Xcode.app/Contents/Developer/usr/bin/make  check-TESTS
PASS: tests
PASS: exhaustive_tests
============================================================================
Testsuite summary for libsecp256k1 0.1
============================================================================
# TOTAL: 2
# PASS:  2
# SKIP:  0
# XFAIL: 0
# FAIL:  0
# XPASS: 0
# ERROR: 0
============================================================================
  CXX      test/test_unitester-unitester.o
  CXXLD    test/unitester
/Applications/Xcode.app/Contents/Developer/usr/bin/make  check-TESTS
PASS: test/unitester
============================================================================
Testsuite summary for univalue 1.0.2
============================================================================
# TOTAL: 1
# PASS:  1
# SKIP:  0
# XFAIL: 0
# FAIL:  0
# XPASS: 0
# ERROR: 0
============================================================================

make util_tests

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

make functional_tests

bash-3.2$ make functional_tests
Running test/functional/test_runner.py...
./test/functional/test_runner.py nodehandling httpbasics invalidblockrequest blockchain rpcnamedargs
..........
invalidblockrequest.py passed, Duration: 5 s
......
blockchain.py passed, Duration: 9 s

rpcnamedargs.py passed, Duration: 4 s
......
httpbasics.py passed, Duration: 13 s
...........................
nodehandling.py passed, Duration: 27 s

TEST                   | STATUS  | DURATION

invalidblockrequest.py | Passed  | 5 s
blockchain.py          | Passed  | 9 s
rpcnamedargs.py        | Passed  | 4 s
httpbasics.py          | Passed  | 13 s
nodehandling.py        | Passed  | 27 s

ALL                    | True    | 58 s (accumulated)

Runtime: 27 s

make all_functional_tests

bash-3.2$ make all_functional_tests
Running all functional tests
./test/functional/test_runner.py --extended
.........................................................................................................................................................................................................................................................................................................................................................
fundrawtransaction.py passed, Duration: 173 s
.....................................................................................................................................................................................................................
p2p-compactblocks.py passed, Duration: 107 s
.....................................................................................................
walletbackup.py passed, Duration: 332 s
.........................................................................
segwit.py passed, Duration: 88 s
..................................................................................................
wallet-accounts.py passed, Duration: 50 s
.............................................................................................
wallet.py passed, Duration: 134 s
....................................................................................
wallet-hd.py passed, Duration: 510 s
................................
p2p-segwit.py passed, Duration: 107 s

... snip ...

fanquake · 2017-04-02T10:48:13Z

Makefile.am

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

Yes, I think you're right. Thanks.

jnewbery · 2017-04-03T13:08:19Z

nits addressed and commits squashed

fanquake · 2017-04-04T02:51:26Z

Both linux builds failed with:

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

ryanofsky · 2017-04-04T12:48:43Z

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 · 2017-04-04T18:23:10Z

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

theuni · 2017-04-13T22:24:43Z

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 · 2017-04-17T13:56:59Z

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

maflcko · 2017-04-17T14:36:04Z

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 · 2017-04-17T14:57:56Z

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.

jtimon · 2017-04-18T18:20:39Z

test/Makefile.include

+    blockchain \
+    rpcnamedargs
+
+.PHONY: all_functional_tests functional_tests util_tests


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 · 2017-04-18T18:27:09Z

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 · 2017-05-05T20:14:53Z

What is needed here to fix Travis?

jnewbery · 2017-05-05T20:33:03Z

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 · 2017-05-09T15:57:22Z

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 · 2017-05-09T20:13:59Z

@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 · 2017-05-09T21:53:26Z

@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 · 2017-05-09T22:04:53Z

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 · 2017-05-09T22:30:23Z

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

jnewbery · 2017-05-16T15:06:28Z

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.

jnewbery · 2017-09-14T15:09:08Z

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

fanquake added the Tests label Mar 21, 2017

theuni suggested changes Mar 22, 2017

View reviewed changes

jnewbery force-pushed the reorg_makefiles branch from a87a99e to d776f9e Compare March 23, 2017 16:13

jnewbery changed the title ~~[WIP] Run functional tests in make check~~ Run functional tests in make check Mar 23, 2017

jnewbery force-pushed the reorg_makefiles branch 5 times, most recently from 4aca0a7 to a77e25a Compare March 24, 2017 19:45

theuni approved these changes Mar 24, 2017

View reviewed changes

fanquake reviewed Apr 2, 2017

View reviewed changes

jnewbery force-pushed the reorg_makefiles branch from 8f52a73 to 0a8f95c Compare April 3, 2017 13:07

jnewbery force-pushed the reorg_makefiles branch from 0a8f95c to 0f6deb1 Compare April 4, 2017 18:18

Run functional tests in make check

7761435

jnewbery force-pushed the reorg_makefiles branch from 0f6deb1 to 7761435 Compare April 4, 2017 18:21

jtimon reviewed Apr 18, 2017

View reviewed changes

laanwj mentioned this pull request May 23, 2017

Share config between util and functional tests #10331

Merged

jnewbery changed the title ~~Run functional tests in make check~~ [tests] [build] Run functional tests in make check Jun 30, 2017

jnewbery closed this Sep 14, 2017

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

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

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

Uh oh!

Conversation

jnewbery commented Mar 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theuni left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtimon commented Mar 23, 2017

Uh oh!

sipa commented Mar 24, 2017

Uh oh!

jnewbery commented Mar 24, 2017

Uh oh!

maflcko commented Mar 25, 2017

Uh oh!

jnewbery commented Mar 25, 2017

Uh oh!

maflcko commented Mar 25, 2017

Uh oh!

JeremyRubin commented Mar 28, 2017

Uh oh!

TheBlueMatt commented Mar 28, 2017

Uh oh!

jnewbery commented Mar 28, 2017

Uh oh!

fanquake commented Apr 2, 2017

Uh oh!

fanquake Apr 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Apr 3, 2017

Uh oh!

fanquake commented Apr 4, 2017

Uh oh!

ryanofsky commented Apr 4, 2017

Uh oh!

jnewbery commented Apr 4, 2017

Uh oh!

theuni commented Apr 13, 2017

Uh oh!

jnewbery commented Apr 17, 2017

Uh oh!

maflcko commented Apr 17, 2017

Uh oh!

jnewbery commented Apr 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtimon commented Apr 18, 2017

Uh oh!

sipa commented May 5, 2017

Uh oh!

jnewbery commented May 5, 2017

Uh oh!

TheBlueMatt commented May 9, 2017

Uh oh!

jnewbery commented May 9, 2017

Uh oh!

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

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

jnewbery commented Mar 21, 2017 •

edited

Loading

fanquake Apr 2, 2017 •

edited

Loading