Reorganise qa directory #9956

jnewbery commented at 11:03 PM on March 8, 2017: member

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 commented at 7:47 AM on March 9, 2017: contributor

Concept ACK. What about using ...

|--test
   |---rpc-tests

instead of

|--test
   |---qa

?

jonasschnelli added the label Tests on Mar 9, 2017

sipa commented at 7:57 AM on March 9, 2017: member

@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 commented at 8:52 AM on March 9, 2017: member

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 commented at 10:21 AM on March 9, 2017: contributor

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 force-pushed on Mar 9, 2017

jnewbery commented at 3:18 PM on March 9, 2017: member

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.

laanwj assigned theuni on Mar 9, 2017

jnewbery force-pushed on Mar 9, 2017

jnewbery commented at 10:41 PM on March 9, 2017: member

rebased

fanquake commented at 3:48 AM on March 10, 2017: member

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 commented at 6:30 AM on March 12, 2017: member

Needs rebase.

theuni commented at 9:38 PM on March 13, 2017: member

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

jnewbery force-pushed on Mar 15, 2017

jnewbery commented at 3:20 PM on March 15, 2017: member

@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 commented at 1:33 PM on March 16, 2017: contributor

Needs rebase.

MarcoFalke commented at 1:57 PM on March 16, 2017: member

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

On Thu, Mar 16, 2017 at 2:33 PM, Jonas Schnelli notifications@github.com wrote:

Needs rebase.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/9956#issuecomment-287057726, or mute the thread https://github.com/notifications/unsubscribe-auth/AGGmvyo8dHQFuV1SI6ibAgsXiBQCj7_Iks5rmToygaJpZM4MXbTb .

in test/functional/test_runner.py:None in 9747bf5200 outdated

  85 | @@ -87,7 +86,7 @@
  86 |  
  87 |  ZMQ_SCRIPTS = [
  88 |      # ZMQ test can only be run if bitcoin was built with zmq-enabled.
  89 | -    # call rpc_tests.py with -nozmq to explicitly exclude these tests.
  90 | +    # call runner.py with -nozmq to explicitly exclude these tests.

theuni commented at 5:21 PM on March 16, 2017:

runner -> test_runner from here down?

in configure.ac:None in 3d42c4e508 outdated

1130 | @@ -1131,10 +1131,13 @@ AC_SUBST(EVENT_PTHREADS_LIBS)
1131 |  AC_SUBST(ZMQ_LIBS)
1132 |  AC_SUBST(PROTOBUF_LIBS)
1133 |  AC_SUBST(QR_LIBS)
1134 | -AC_CONFIG_FILES([Makefile src/Makefile doc/man/Makefile share/setup.nsi share/qt/Info.plist src/test/buildenv.py])
1135 | -AC_CONFIG_FILES([qa/pull-tester/tests_config.ini],[chmod +x qa/pull-tester/tests_config.ini])
1136 | +AC_CONFIG_FILES([Makefile src/Makefile doc/man/Makefile share/setup.nsi share/qt/Info.plist])
1137 | +AC_CONFIG_FILES([test/functional/config.ini],[chmod +x test/functional/config.ini])

theuni commented at 6:47 PM on March 16, 2017:

Do these really need +x?

in configure.ac:None in 3d42c4e508 outdated

 413 | @@ -414,8 +414,8 @@ if test x$use_pkgconfig = xyes; then
 414 |    ])
 415 |  fi
 416 |  
 417 | -if test x$use_extended_rpc_tests != xno; then
 418 | -  AC_SUBST(EXTENDED_RPC_TESTS, -extended)
 419 | +if test x$use_extended_functional_tests != xno; then
 420 | +  AC_SUBST(EXTENDED_FUNCTIONAL_TESTS, -extended)

theuni commented at 6:55 PM on March 16, 2017:

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.

MarcoFalke commented at 10:18 AM on March 20, 2017:

This should be --extended with two dashes?

theuni commented at 6:57 PM on March 16, 2017: member

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 commented at 8:16 PM on March 16, 2017: contributor

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 commented at 8:52 PM on March 16, 2017: member

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 force-pushed on Mar 16, 2017

jnewbery commented at 8:56 PM on March 16, 2017: member

@theuni's comments addressed, and rebased.

Old tip: https://github.com/jnewbery/bitcoin/tree/pr9956.2 / https://github.com/jnewbery/bitcoin/commit/3d42c4e508ae0fa73eef5ae6584be3a624d5d129 new tip: https://github.com/jnewbery/bitcoin/tree/pr9956.3 / https://github.com/jnewbery/bitcoin/commit/f1653606a6c5803c4711ddaa4fea8242c78393e3

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

Ready for additional review.

jtimon commented at 11:29 PM on March 17, 2017: contributor

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

fanquake commented at 9:58 AM on March 19, 2017: member

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

in contrib/rpm/bitcoin.spec:315 in f1653606a6 outdated

 310 | @@ -311,10 +311,8 @@ rm -f %{buildroot}%{_bindir}/test_*
 311 |  
 312 |  %check
 313 |  make check
 314 | -pushd src
 315 | -srcdir=. test/bitcoin-util-test.py
 316 | -popd
 317 | -qa/pull-tester/rpc-tests.py -extended
 318 | +srcdir=src test/bitcoin-util-test.py
 319 | +test/functional/test_runner.py -extended

MarcoFalke commented at 10:24 AM on March 20, 2017:

Also double dash?

in test/functional/config.ini.in:6 in f1653606a6 outdated

   2 | @@ -3,7 +3,7 @@
   3 |  # file COPYING or http://www.opensource.org/licenses/mit-license.php.
   4 |  
   5 |  # These environment variables are set by the build process and read by
   6 | -# rpc-tests.py
   7 | +# test/runner/runner.py

MarcoFalke commented at 10:31 AM on March 20, 2017:

'test/runner/runner.py': No such file or directory

in test/README.md:1 in f1653606a6 outdated

   0 | @@ -1,5 +1,5 @@
   1 | -The [pull-tester](/qa/pull-tester/) folder contains a script to call

MarcoFalke commented at 10:36 AM on March 20, 2017:

Can you mention test/util/bitcoin-util-test.py in this file as well?

MarcoFalke commented at 10:37 AM on March 20, 2017: member

utACK f1653606a6c5803c4711ddaa4fea8242c78393e3, but needs feedback addressed

laanwj commented at 11:05 AM on March 20, 2017: member

As soon as there is agreement on this we should probably merge this. Otherwise it will continue to be outdated by every single change in the tests.

MarcoFalke commented at 11:10 AM on March 20, 2017: member

I plan to merge this after the next rebase, so if someone disagrees with anything, please raise your voice.

Rename qa directory to test 00902c48cd

Rename rpc-tests directory to functional c28ee91db0

Rename test/pull-tester/rpc-tests.py to test/functional/test_runner.py a9bd622a65

Rename --enable-extended-rpc-tests to --enable-extended-functional-tests 5b0bff4581

Move src/test/bitcoin-util-test.py to test/util/bitcoin-util-test.py 63d66ba20a

jnewbery force-pushed on Mar 20, 2017

jnewbery commented at 2:43 PM on March 20, 2017: member

I've addressed @MarcoFalke's comments and rebased. New tip: https://github.com/jnewbery/bitcoin/tree/pr9956.4 / https://github.com/jnewbery/bitcoin/commit/63d66ba20a634b54f6d5e8b051fb4a106f2cef6c @jtimon - re: extended tests in a subdirectory: Interesting idea. I'm not convinced that it's necessary, but it's orthognal to this move, so let's not hold this PR up on that.

fanquake commented at 3:34 PM on March 20, 2017: member

Merged and ran test/functional/test_runner.py with no arguments:

TEST                           | PASSED | DURATION

fundrawtransaction.py          | True   | 192 s
walletbackup.py                | True   | 311 s
p2p-compactblocks.py           | True   | 166 s
segwit.py                      | True   | 83 s
wallet-accounts.py             | True   | 65 s
wallet.py                      | True   | 149 s
wallet-dump.py                 | True   | 50 s
wallet-hd.py                   | True   | 574 s
p2p-segwit.py                  | True   | 116 s
zapwallettxes.py               | True   | 29 s
listtransactions.py            | True   | 53 s
p2p-fullblocktest.py           | True   | 613 s
sendheaders.py                 | True   | 49 s
mempool_limit.py               | True   | 16 s
merkle_blocks.py               | True   | 25 s
importmulti.py                 | True   | 40 s
abandonconflict.py             | True   | 23 s
receivedby.py                  | True   | 31 s
mempool_resurrect_test.py      | True   | 4 s
bip68-112-113-p2p.py           | True   | 29 s
reindex.py                     | True   | 19 s
txn_doublespend.py --mineblock | True   | 18 s
rawtransactions.py             | True   | 34 s
mempool_spendcoinbase.py       | True   | 4 s
txn_clone.py                   | True   | 19 s
mempool_reorg.py               | True   | 11 s
httpbasics.py                  | True   | 11 s
rest.py                        | True   | 21 s
multi_rpc.py                   | True   | 4 s
signrawtransactions.py         | True   | 5 s
decodescript.py                | True   | 4 s
getchaintips.py                | True   | 43 s
proxy_test.py                  | True   | 15 s
blockchain.py                  | True   | 7 s
disablewallet.py               | True   | 3 s
p2p-mempool.py                 | True   | 4 s
keypool.py                     | True   | 11 s
invalidblockrequest.py         | True   | 6 s
prioritise_transaction.py      | True   | 11 s
nodehandling.py                | True   | 26 s
invalidtxrequest.py            | True   | 6 s
signmessages.py                | True   | 5 s
importprunedfunds.py           | True   | 13 s
nulldummy.py                   | True   | 6 s
preciousblock.py               | True   | 16 s
p2p-versionbits-warning.py     | True   | 19 s
rpcnamedargs.py                | True   | 4 s
p2p-leaktests.py               | True   | 10 s
zmq_test.py                    | True   | 19 s
bumpfee.py                     | True   | 41 s
listsinceblock.py              | True   | 49 s
import-rescan.py               | True   | 59 s

ALL                            | True   | 3141 s (accumulated)

Runtime: 803 s

Running through the extended tests and passing some options now.

MarcoFalke commented at 4:41 PM on March 20, 2017: member

utACK 63d66ba

and thanks @fanquake for testing!

On Mon, Mar 20, 2017 at 4:35 PM, Michael notifications@github.com wrote:

Merged and ran test/functional/test_runner.py with no arguments:

TEST | PASSED | DURATION

fundrawtransaction.py | True | 192 s walletbackup.py | True | 311 s p2p-compactblocks.py | True | 166 s segwit.py | True | 83 s wallet-accounts.py | True | 65 s wallet.py | True | 149 s wallet-dump.py | True | 50 s wallet-hd.py | True | 574 s p2p-segwit.py | True | 116 s zapwallettxes.py | True | 29 s listtransactions.py | True | 53 s p2p-fullblocktest.py | True | 613 s sendheaders.py | True | 49 s mempool_limit.py | True | 16 s merkle_blocks.py | True | 25 s importmulti.py | True | 40 s abandonconflict.py | True | 23 s receivedby.py | True | 31 s mempool_resurrect_test.py | True | 4 s bip68-112-113-p2p.py | True | 29 s reindex.py | True | 19 s txn_doublespend.py --mineblock | True | 18 s rawtransactions.py | True | 34 s mempool_spendcoinbase.py | True | 4 s txn_clone.py | True | 19 s mempool_reorg.py | True | 11 s httpbasics.py | True | 11 s rest.py | True | 21 s multi_rpc.py | True | 4 s signrawtransactions.py | True | 5 s decodescript.py | True | 4 s getchaintips.py | True | 43 s proxy_test.py | True | 15 s blockchain.py | True | 7 s disablewallet.py | True | 3 s p2p-mempool.py | True | 4 s keypool.py | True | 11 s invalidblockrequest.py | True | 6 s prioritise_transaction.py | True | 11 s nodehandling.py | True | 26 s invalidtxrequest.py | True | 6 s signmessages.py | True | 5 s importprunedfunds.py | True | 13 s nulldummy.py | True | 6 s preciousblock.py | True | 16 s p2p-versionbits-warning.py | True | 19 s rpcnamedargs.py | True | 4 s p2p-leaktests.py | True | 10 s zmq_test.py | True | 19 s bumpfee.py | True | 41 s listsinceblock.py | True | 49 s import-rescan.py | True | 59 s

ALL | True | 3141 s (accumulated)

Runtime: 803 s

Running through the extended tests and passing some options now.

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

theuni commented at 8:57 PM on March 20, 2017: member

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

MarcoFalke merged this on Mar 20, 2017

MarcoFalke closed this on Mar 20, 2017

MarcoFalke referenced this in commit 3192e5278a on Mar 20, 2017

PastaPastaPasta referenced this in commit c4cb13ff86 on Mar 10, 2019

PastaPastaPasta referenced this in commit 747e0a938a on Mar 11, 2019

PastaPastaPasta referenced this in commit 4de6223e75 on Mar 11, 2019

PastaPastaPasta referenced this in commit da4ad06773 on Mar 12, 2019

PastaPastaPasta referenced this in commit 789a5676f2 on Mar 13, 2019

UdjinM6 referenced this in commit a666dd1878 on Mar 13, 2019

PastaPastaPasta referenced this in commit 0e200df979 on Mar 13, 2019

PastaPastaPasta referenced this in commit f29633fc18 on Mar 14, 2019

PastaPastaPasta referenced this in commit d703bf9a50 on Mar 14, 2019

PastaPastaPasta referenced this in commit 4891b0ccf4 on Mar 15, 2019

PastaPastaPasta referenced this in commit 6fc884b832 on Mar 16, 2019

PastaPastaPasta referenced this in commit cb61f4e5be on Apr 3, 2019

PastaPastaPasta referenced this in commit 6fe6ebf254 on Apr 3, 2019

PastaPastaPasta referenced this in commit f06e2c76b4 on May 10, 2019

PastaPastaPasta referenced this in commit 39c5d2f126 on May 15, 2019

PastaPastaPasta referenced this in commit cc3915822d on May 15, 2019

PastaPastaPasta referenced this in commit ec0772f8f3 on May 17, 2019

UdjinM6 referenced this in commit 6edbc9cebd on May 19, 2019

PastaPastaPasta referenced this in commit 814804d60d on May 20, 2019

barrystyle referenced this in commit c2255bd60b on Jan 22, 2020

DrahtBot locked this on Sep 8, 2021