Reorganise qa directory #9956

jnewbery · 2017-03-08T23:03:52Z

Initially proposed by @jonasschnelli here: https://botbot.me/freenode/bitcoin-core-dev/2017-02-16/?msg=81095853&page=5

This re-organises the qa test directory to meet the following requirements:

qa directory should be renamed to come after /src alpabetically, so that git diffs and github PRs show changes to qa tests beneath changes to product code.
rpc-tests should be renamed to something, since they test more than the RPC interface
pull-tester should be renamed to something, since it's run locally as well as on PRs
bitcoin-util-test should be moved to the qa test directory, since it's a functional test rather than a unit test.

This PR has the following commits:

commit 1 renames qa to test
commit 2 renames the rpc-tests directory to qa
commit 3 renames test/pull-tester/rpc-tests.py to test/runner/runner.py
commit 4 renames the --enable-extended-rpc-tests configure option to --enable-extended-qa-tests
commit 5 moves the bitcoin-util-test.py test into test/util

The new directory structure is [EDITED]:

|--src
|   |--<source files etc>
|   |--test
|       |---<unit tests>
|
|--test
   |---functional
   |   |---<tests (were previously in /qa/rpc-tests)>
   |   |---test_runner.py (was previously qa/pull-tester/rpc-tests.py)
   |   |---test_config.ini
   |  
   |---README.md
   |
   |---util
   |   |---bctest.py (was previously in src/test)
   |   |---bitcoin-util-test.py (was previously in src/test)
   |   |---buildenv.py.in (was previously in src/test)
   |   |---data (was previously in src/test)
   |   |   |---<data files>

The actual structure within the new directories is unchanged to minimize changes.

I've tested this at each commit by rebuilding and running the test cases. Reviewers note that you'll need to ./autogen.sh and ./configure to test this.

Any feedback on names or structure welcomed.

jonasschnelli · 2017-03-09T07:47:20Z

Concept ACK.
What about using ...

|--test
   |---rpc-tests

instead of

|--test
   |---qa

?

sipa · 2017-03-09T07:57:04Z

@jonasschnelli It doesn't contain purely RPC tests anymore (also P2P and REST).

@JNewberry Why is runner separate? My #1 complaint about the current structure is that the runner is in a separate directory.

laanwj · 2017-03-09T08:52:30Z

Concept ACK. New structure looks good to me.

Agree that there's no need for the runner to be separate. It can go with the other Python code.

@jonasschnelli It doesn't contain purely RPC tests anymore (also P2P and REST).

Indeed. These are "functional tests" or "integration tests" in contrast to the "unit tests" which are part of the C++ source code. They make use of RPC, but that doesn't define them, ideally they should test every interface.

jonasschnelli · 2017-03-09T10:21:13Z

Indeed. These are "functional tests" or "integration tests" in contrast to the "unit tests" which are part of the C++ source code. They make use of RPC, but that doesn't define them, ideally they should test every interface.

Okay. That makes sense.

@jnewbery: I think you need to update .travis.yml.
Tested a bit and it seems to work well.

jnewbery · 2017-03-09T15:18:10Z

Thanks everyone for the feedback. I've made the following changes:

commit 1 renames qa to test
commit 2 renames the rpc-tests directory to qa functional
commit 3 renames test/pull-tester/rpc-tests.py to ~~test/runner/runner.py~~ test/functional/test_runner.py and renames test/pull-tester/tests_config.ini.in to test/functional/config.ini.in. It also updates .travis.yml so travis knows where to find test_runner.py
commit 4 renames the --enable-extended-rpc-tests configure option to ~~--enable-extended-qa-tests~~ --enable-extended-functional-tests
commit 5 moves the bitcoin-util-test.py test into test/util

also rebased.

jnewbery · 2017-03-09T22:41:47Z

rebased

fanquake · 2017-03-10T03:48:55Z

Needs .gitignore updated to include test/functional/config.ini and test/util/buildenv.py. You can probably drop the old qa/pull-tester/tests_config.py entry now as well.

Currently failing with:

==============================================
   Bitcoin Core 0.14.99: src/test-suite.log
==============================================

# TOTAL: 2
# PASS:  1
# SKIP:  0
# XFAIL: 0
# FAIL:  1
# XPASS: 0
# ERROR: 0

.. contents:: :depth: 2

FAIL: test/test_bitcoin
=======================

Assertion failed: lock cs_wallet not held in wallet/wallet.cpp:163; locks held:
Running 232 test cases...
unknown location:0: fatal error: in "wallet_tests/rescan": signal: SIGABRT (application abort requested)
wallet/test/wallet_tests.cpp:362: last checkpoint: "rescan" entry.
Assertion failed: (secp256k1_context_sign == NULL), function ECC_Start, file key.cpp, line 300.
unknown location:0: fatal error: in "wallet_tests/coin_mark_dirty_immature_credit": signal: SIGABRT (application abort requested)
wallet/test/wallet_tests.cpp:435: last checkpoint: "coin_mark_dirty_immature_credit" fixture entry.
Assertion failed: (secp256k1_context_sign == NULL), function ECC_Start, file key.cpp, line 300.
unknown location:0: fatal error: in "wallet_tests/ComputeTimeSmart": signal: SIGABRT (application abort requested)
wallet/test/wallet_tests.cpp:479: last checkpoint: "ComputeTimeSmart" fixture entry.
Assertion failed: (secp256k1_context_sign == NULL), function ECC_Start, file key.cpp, line 300.
unknown location:0: fatal error: in "wallet_crypto/passphrase": signal: SIGABRT (application abort requested)
wallet/test/crypto_tests.cpp:186: last checkpoint: "passphrase" fixture entry.
Assertion failed: (secp256k1_context_sign == NULL), function ECC_Start, file key.cpp, line 300.
unknown location:0: fatal error: in "wallet_crypto/encrypt": signal: SIGABRT (application abort requested)
wallet/test/crypto_tests.cpp:202: last checkpoint: "encrypt" fixture entry.
Assertion failed: (secp256k1_context_sign == NULL), function ECC_Start, file key.cpp, line 300.
unknown location:0: fatal error: in "wallet_crypto/decrypt": signal: SIGABRT (application abort requested)
wallet/test/crypto_tests.cpp:217: last checkpoint: "decrypt" fixture entry.

*** 6 failures are detected in the test module "Bitcoin Test Suite"
Assertion failed: (!pthread_mutex_destroy(&m)), function ~recursive_mutex, file /usr/local/include/boost/thread/pthread/recursive_mutex.hpp, line 104.
FAIL test/test_bitcoin (exit status: 134)

sipa · 2017-03-12T06:30:21Z

Needs rebase.

theuni · 2017-03-13T21:38:14Z

@jnewbery Other than the rebase, is this now ready for review? Travis is happy, at least.

jnewbery · 2017-03-15T15:20:48Z

@theuni thanks for your help on this. I've rebased and squashed, so assuming travis passes, this is now ready for review. The two parts which I'm a little unsure of and I'd especially like reviewers to look at are:

changes to /contrib/rpm/bitcoin.spec. I actually think we can remove the line:
srcdir=. test/bitcoin-util-test.py
entirely, since bitcoin-util-test.py is run as part of make check, but I've left it in for now. Let me know if you think we can remove it.
changes to Makefile.am, configure.ac and src/test/Makefile.test.include in the final commit (Move src/test/bitcoin-util-test.py to test/util/bitcoin-util-test.py)

jonasschnelli · 2017-03-16T13:33:23Z

Needs rebase.

maflcko · 2017-03-16T13:57:00Z

No need to rebase this until the merge is scheduled. Needs review first.

…

On Thu, Mar 16, 2017 at 2:33 PM, Jonas Schnelli ***@***.***> wrote: Needs rebase. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#9956 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGGmvyo8dHQFuV1SI6ibAgsXiBQCj7_Iks5rmToygaJpZM4MXbTb> .

theuni · 2017-03-16T17:21:58Z

test/functional/test_runner.py

 ZMQ_SCRIPTS = [
    # ZMQ test can only be run if bitcoin was built with zmq-enabled.
-    # call rpc_tests.py with -nozmq to explicitly exclude these tests.
+    # call runner.py with -nozmq to explicitly exclude these tests.


runner -> test_runner from here down?

theuni · 2017-03-16T18:47:52Z

configure.ac

-AC_CONFIG_FILES([Makefile src/Makefile doc/man/Makefile share/setup.nsi share/qt/Info.plist src/test/buildenv.py])
-AC_CONFIG_FILES([qa/pull-tester/tests_config.ini],[chmod +x qa/pull-tester/tests_config.ini])
+AC_CONFIG_FILES([Makefile src/Makefile doc/man/Makefile share/setup.nsi share/qt/Info.plist])
+AC_CONFIG_FILES([test/functional/config.ini],[chmod +x test/functional/config.ini])


Do these really need +x?

theuni · 2017-03-16T18:55:28Z

configure.ac

-if test x$use_extended_rpc_tests != xno; then
-  AC_SUBST(EXTENDED_RPC_TESTS, -extended)
+if test x$use_extended_functional_tests != xno; then
+  AC_SUBST(EXTENDED_FUNCTIONAL_TESTS, -extended)


Please update Makefile.am to match. I'm pretty sure the cov stuff is busted atm, but at least this part won't be busted too.

Actually, it'd make sense to use this in test/functional/config.ini.in directly if you'd prefer to skip to that.

theuni · 2017-03-16T18:57:27Z

Looks good to me other than a few little nits. Passes Travis' out-of-tree-distdir gauntlet.

I'd like to move the related stuff to a new Makefile.include, but as a next step.

I'm unsure about the bitcoin.spec, but we probably can't assume they're running it from inside our tree?

jtimon · 2017-03-16T20:16:02Z

Could you update the struct on the top instead of only the comments? It still says tests/runner and test/qa.
Concept ACK. Needs rebase.

I think "qa" instead of "functional" was just fine, but I guess "functional", or "integration" or "tests" or "py" would be fine as well. Maybe I just prefer qa because it's shorter. Anyway, bikeshedding.

About the runner and the config, what about just leaving them in the top level test wallet instead of putting them with the rest of the tests in functional?

jnewbery · 2017-03-16T20:52:02Z

Could you update the struct on the top instead of only the comments? It still says tests/runner and test/qa.

Apologies. Now updated.

I think "qa" instead of "functional" was just fine, but I guess "functional", or "integration" or "tests" or "py" would be fine as well. Maybe I just prefer qa because it's shorter. Anyway, bikeshedding.

I think functional or integration are best because they are strongly differentiated from unit tests. qa is kind of meaningless in this context.

About the runner and the config, what about just leaving them in the top level test wallet instead of putting them with the rest of the tests in functional?

I aim to eventually merge test/functional/config.ini.in and test/util/buildenv.py into a single config file which will live in the base /test directory. I think it makes sense to have test_runner.py live in the /test/functional directory since it only runs the functional tests. Having it in the base /test may confuse people into thinking it also runs the util tests.

jnewbery · 2017-03-16T20:56:11Z

@theuni's comments addressed, and rebased.

Old tip: https://github.com/jnewbery/bitcoin/tree/pr9956.2 / jnewbery@3d42c4e
new tip: https://github.com/jnewbery/bitcoin/tree/pr9956.3 / jnewbery@f165360

I've built from distdir locally, so hopefully this will pass Travis!

Ready for additional review.

jtimon · 2017-03-17T23:29:35Z

Random thought, maybe create a subdirectory for the extended tests? That would make it more clear I think.

fanquake · 2017-03-19T09:58:28Z

I've been testing this, looks good so far 👍 . However, needs another rebase.

maflcko · 2017-03-20T10:18:01Z

configure.ac

-if test x$use_extended_rpc_tests != xno; then
-  AC_SUBST(EXTENDED_RPC_TESTS, -extended)
+if test x$use_extended_functional_tests != xno; then
+  AC_SUBST(EXTENDED_FUNCTIONAL_TESTS, -extended)


This should be --extended with two dashes?

maflcko · 2017-03-20T16:41:56Z

utACK 63d66ba and thanks @fanquake for testing!

…

theuni · 2017-03-20T20:57:16Z

utACK 63d66ba since it works, any cleanups can happen as a next step.

63d66ba Move src/test/bitcoin-util-test.py to test/util/bitcoin-util-test.py (John Newbery) 5b0bff4 Rename --enable-extended-rpc-tests to --enable-extended-functional-tests (John Newbery) a9bd622 Rename test/pull-tester/rpc-tests.py to test/functional/test_runner.py (John Newbery) c28ee91 Rename rpc-tests directory to functional (John Newbery) 00902c4 Rename qa directory to test (John Newbery) Tree-SHA512: ee7125c0c647d81590177beef2c8852c4ef76fdcf888096d9d4d360562a01d8d3b453345c3040487b2a043935bd1e7e80018f34462d6e02262bedbe23edcc576

63d66ba Move src/test/bitcoin-util-test.py to test/util/bitcoin-util-test.py (John Newbery) 5b0bff4 Rename --enable-extended-rpc-tests to --enable-extended-functional-tests (John Newbery) a9bd622 Rename test/pull-tester/rpc-tests.py to test/functional/test_runner.py (John Newbery) c28ee91 Rename rpc-tests directory to functional (John Newbery) 00902c4 Rename qa directory to test (John Newbery) Tree-SHA512: ee7125c0c647d81590177beef2c8852c4ef76fdcf888096d9d4d360562a01d8d3b453345c3040487b2a043935bd1e7e80018f34462d6e02262bedbe23edcc576 resolve build errors Signed-off-by: Pasta <Pasta@dash.org> update test_runner.py in testintegrations.sh Signed-off-by: Pasta <Pasta@dash.org>

* Merge bitcoin#9956: Reorganise qa directory 63d66ba Move src/test/bitcoin-util-test.py to test/util/bitcoin-util-test.py (John Newbery) 5b0bff4 Rename --enable-extended-rpc-tests to --enable-extended-functional-tests (John Newbery) a9bd622 Rename test/pull-tester/rpc-tests.py to test/functional/test_runner.py (John Newbery) c28ee91 Rename rpc-tests directory to functional (John Newbery) 00902c4 Rename qa directory to test (John Newbery) Tree-SHA512: ee7125c0c647d81590177beef2c8852c4ef76fdcf888096d9d4d360562a01d8d3b453345c3040487b2a043935bd1e7e80018f34462d6e02262bedbe23edcc576 resolve build errors Signed-off-by: Pasta <Pasta@dash.org> update test_runner.py in testintegrations.sh Signed-off-by: Pasta <Pasta@dash.org> * moved dash specific tests Signed-off-by: Pasta <Pasta@dash.org> * dashify README.md Signed-off-by: Pasta <Pasta@dash.org> * removed autoix*.py Signed-off-by: Pasta <Pasta@dash.org> * change back file perms * dedashify Signed-off-by: Pasta <Pasta@dash.org>

jonasschnelli added the Tests label Mar 9, 2017

jnewbery force-pushed the reorganise_qa branch from d145278 to bc549cf Compare March 9, 2017 15:14

laanwj assigned theuni Mar 9, 2017

jnewbery force-pushed the reorganise_qa branch 2 times, most recently from fb0f1ae to 6906ba2 Compare March 9, 2017 22:41

jnewbery force-pushed the reorganise_qa branch 2 times, most recently from c2bab0a to 3d42c4e Compare March 15, 2017 15:13

theuni reviewed Mar 16, 2017

View reviewed changes

jnewbery force-pushed the reorganise_qa branch from 3d42c4e to f165360 Compare March 16, 2017 20:53

maflcko reviewed Mar 20, 2017

View reviewed changes

maflcko merged commit 63d66ba into bitcoin:master Mar 20, 2017

laanwj mentioned this pull request Mar 21, 2017

combine_logs.py - aggregates log files from multiple bitcoinds during functional tests. #10017

Merged

ryanofsky mentioned this pull request Mar 21, 2017

Make bumpfee tests less fragile #9701

Merged

voanhcuoc mentioned this pull request May 2, 2017

RPM build fail: Wrongs in specfile Source fields, testscript p2p-fullblocktest.py fail #10312

Closed

UdjinM6 mentioned this pull request Nov 5, 2018

[rpc] getblockchaininfo: add size_on_disk, prune_target_size, automat… dashpay/dash#2416

Closed

PastaPastaPasta mentioned this pull request Apr 5, 2019

Backports 0.15 pr8 dashpay/dash#2763

Merged

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

Reorganise qa directory #9956

Reorganise qa directory #9956

Uh oh!

Conversation

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

Uh oh!

Uh oh!

jonasschnelli commented Mar 9, 2017

Uh oh!

sipa commented Mar 9, 2017

Uh oh!

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

Uh oh!

Uh oh!

jonasschnelli commented Mar 9, 2017

Uh oh!

jnewbery commented Mar 9, 2017

Uh oh!

jnewbery commented Mar 9, 2017

Uh oh!

fanquake commented Mar 10, 2017

Uh oh!

sipa commented Mar 12, 2017

Uh oh!

theuni commented Mar 13, 2017

Uh oh!

jnewbery commented Mar 15, 2017

Uh oh!

jonasschnelli commented Mar 16, 2017

Uh oh!

maflcko commented Mar 16, 2017 via email

Uh oh!

theuni Mar 16, 2017

Choose a reason for hiding this comment

Uh oh!

theuni Mar 16, 2017

Choose a reason for hiding this comment

Uh oh!

theuni Mar 16, 2017

Choose a reason for hiding this comment

Uh oh!

theuni commented Mar 16, 2017

Uh oh!

jtimon commented Mar 16, 2017

Uh oh!

jnewbery commented Mar 16, 2017

Uh oh!

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

Uh oh!

Uh oh!

jtimon commented Mar 17, 2017

Uh oh!

fanquake commented Mar 19, 2017

Uh oh!

maflcko Mar 20, 2017

Choose a reason for hiding this comment

Uh oh!

maflcko commented Mar 20, 2017 via email

Uh oh!

theuni commented Mar 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

jnewbery commented Mar 8, 2017 •

edited

Loading

laanwj commented Mar 9, 2017 •

edited

Loading

jnewbery commented Mar 16, 2017 •

edited

Loading