Backport 11796 + 11774 (#3612)

* Merge #11796: [tests] Functional test naming convention

5fecd84 [tests] Remove redundant import in blocktools.py test (Anthony Towns)
9b20bb4 [tests] Check tests conform to naming convention (Anthony Towns)
7250b4e [tests] README.md nit fixes (Anthony Towns)
82b2712 [tests] move witness util functions to blocktools.py (John Newbery)
1e10854 [tests] [docs] update README for new test naming scheme (John Newbery)

Pull request description:

  Splitting #11774 into two parts -- this part updates the README with the proposed naming convention, and adds some checks to test_runner.py that the number of tests violating the naming convention doesn't increase too much. Idea is this part of the change should not introduce merge conflicts or require much rebasing, so reviews of the complicated bits won't become invalidated too often; while the second part will just be file renames, which will require regular rebasing and will introduce merge conflicts with pending PRs, but can be merged later, and should also be much easier to review, since it will only include relatively trivial changes.

Tree-SHA512: b96557d41714addbbfe2aed62fb5a48639eaeb1eb3aba30ac1b3a86bb3cb8d796c6247f9c414c4695c4bf54c0ec9968ac88e2f88fb62483bc1a2f89368f7fc80

* update violation count

Signed-off-by: pasta <pasta@dashboost.org>

* Merge #11774: [tests] Rename functional tests

6f881cc880 [tests] Remove EXPECTED_VIOLATION_COUNT (Anthony Towns)
3150b3fea7 [tests] Rename misc functional tests. (Anthony Towns)
81b79f2c39 [tests] Rename rpc_* functional tests. (Anthony Towns)
61b8f7f273 [tests] Rename p2p_* functional tests. (Anthony Towns)
90600bc7db [tests] Rename wallet_* functional tests. (Anthony Towns)
ca6523d0c8 [tests] Rename feature_* functional tests. (Anthony Towns)

Pull request description:

  This PR changes the functional tests to have a consistent naming scheme:

      tests for individual RPC methods are named rpc_...
      tests for interfaces (REST, ZMQ, RPC features) are named interface_...
      tests that explicitly test the p2p interface are named p2p_...
      tests for wallet features are named wallet_...
      tests for mining features are named mining_...
      tests for mempool behaviour are named mempool_...
      tests for full features that aren't wallet/mining/mempool are named feature_...

  Rationale: it's sometimes difficult for new contributors to know what's already covered by existing tests and where new tests should be added. Naming in a consistent fashion makes it easier to see what's already covered at a glance.

Tree-SHA512: 4246790552d42bbd95f6d5bdf67702b81b3b2c583ce7eaf1fe6d8e254721279b47315973c6e9ae82dad6e4c747f12188160764bf2624c0f8f3b4d39330ec8b16

* rename tests and edit associated strings to align test-suite with test name standards

Signed-off-by: pasta <pasta@dashboost.org>

* fix grammar in test/functional/test_runner.py

Co-authored-by: dustinface <35775977+xdustinface@users.noreply.github.com>

* ci: Fix excluded test names

* rename feature_privatesend.py to rpc_privatesend.py

Signed-off-by: pasta <pasta@dashboost.org>

Co-authored-by: Wladimir J. van der Laan <laanwj@gmail.com>
Co-authored-by: MarcoFalke <falke.marco@gmail.com>
Co-authored-by: dustinface <35775977+xdustinface@users.noreply.github.com>
Co-authored-by: xdustinface <xdustinfacex@gmail.com>

.gitlab-ci.yml

@@ -113,7 +113,7 @@ builder-image:
   stage: test
   extends: .base-template
   variables:
-    INTEGRATION_TESTS_ARGS: "--extended --exclude pruning,dbcrash"
+    INTEGRATION_TESTS_ARGS: "--extended --exclude feature_pruning,feature_dbcrash"
   script:
     - echo "INTEGRATION_TESTS_ARGS=${INTEGRATION_TESTS_ARGS}"
     - ./ci/test_integrationtests.sh $INTEGRATION_TESTS_ARGS

.travis.yml

@@ -47,7 +47,7 @@ runtests: &runtests
     - $DOCKER_RUN_IN_BUILDER ./ci/build_depends.sh
     - $DOCKER_RUN_IN_BUILDER ./ci/build_src.sh
     - $DOCKER_RUN_IN_BUILDER ./ci/test_unittests.sh
-    - if [ "$TRAVIS_EVENT_TYPE" = "cron" ]; then extended="--extended --exclude pruning,dbcrash"; fi
+    - if [ "$TRAVIS_EVENT_TYPE" = "cron" ]; then extended="--extended --exclude feature_pruning,feature_dbcrash"; fi
     - $DOCKER_RUN_IN_BUILDER ./ci/test_integrationtests.sh --quiet --jobs=3 ${extended}
 
 builddocker: &builddocker

test/functional/README.md

@@ -27,6 +27,20 @@ don't have test cases for.
   `set_test_params()`, `add_options()` and `setup_xxxx()` methods at the top of
   the subclass, then locally-defined helper methods, then the `run_test()` method.
 
+#### Naming guidelines
+
+- Name the test `<area>_test.py`, where area can be one of the following:
+    - `feature` for tests for full features that aren't wallet/mining/mempool, eg `feature_rbf.py`
+    - `interface` for tests for other interfaces (REST, ZMQ, etc), eg `interface_rest.py`
+    - `mempool` for tests for mempool behaviour, eg `mempool_reorg.py`
+    - `mining` for tests for mining features, eg `mining_prioritisetransaction.py`
+    - `p2p` for tests that explicitly test the p2p interface, eg `p2p_disconnect_ban.py`
+    - `rpc` for tests for individual RPC methods or features, eg `rpc_listtransactions.py`
+    - `wallet` for tests for wallet features, eg `wallet_keypool.py`
+- use an underscore to separate words
+    - exception: for tests for specific RPCs or command line options which don't include underscores, name the test after the exact RPC or argument name, eg `rpc_decodescript.py`, not `rpc_decode_script.py`
+- Don't use the redundant word `test` in the name, eg `interface_zmq.py`, not `interface_zmq_test.py`
+
 #### General test-writing advice
 
 - Set `self.num_nodes` to the minimum number of nodes necessary for the test.
@@ -73,7 +87,7 @@ start the networking thread.  (Continue with the test logic in your existing
 thread.)
 
 - Can be used to write tests where specific P2P protocol behavior is tested.
-Examples tests are `p2p-accept-block.py`, `p2p-compactblocks.py`.
+Examples tests are `p2p_unrequested_blocks.py`, `p2p_compactblocks.py`.
 
 ### test-framework modules
 

test/functional/feature_dip4_coinbasemerkleroots.py

@@ -7,7 +7,7 @@ from test_framework.test_framework import DashTestFramework
 from test_framework.util import assert_equal
 
 '''
-dip4-coinbasemerkleroots.py
+feature_dip4_coinbasemerkleroots.py
 
 Checks DIP4 merkle roots in coinbases
 

test/functional/feature_llmq_chainlocks.py

@@ -10,7 +10,7 @@ from test_framework.test_framework import DashTestFramework
 from test_framework.util import *
 
 '''
-llmq-chainlocks.py
+feature_llmq_chainlocks.py
 
 Checks LLMQs based ChainLocks
 

test/functional/feature_llmq_connections.py

@@ -7,7 +7,7 @@ from test_framework.test_framework import DashTestFramework
 from test_framework.util import *
 
 '''
-llmq-connections.py
+feature_llmq_connections.py
 
 Checks intra quorum connections
 

test/functional/feature_llmq_dkgerrors.py

@@ -6,7 +6,7 @@
 from test_framework.test_framework import DashTestFramework
 
 '''
-llmq-dkgerrors.py
+feature_llmq_dkgerrors.py
 
 Simulate and check DKG errors
 

test/functional/feature_llmq_is_cl_conflicts.py

@@ -11,7 +11,7 @@ from test_framework.test_framework import DashTestFramework
 from test_framework.util import assert_raises_rpc_error
 
 '''
-llmq-is-cl-conflicts.py
+feature_llmq_is_cl_conflicts.py
 
 Checks conflict handling between ChainLocks and InstantSend
 

test/functional/feature_llmq_is_retroactive.py

@@ -8,7 +8,7 @@ from test_framework.test_framework import DashTestFramework
 from test_framework.util import set_node_times, isolate_node, reconnect_isolated_node
 
 '''
-llmq-is-retroactive.py
+feature_llmq_is_retroactive.py
 
 Tests retroactive signing
 

test/functional/feature_llmq_signing.py

@@ -8,7 +8,7 @@ from test_framework.test_framework import DashTestFramework
 from test_framework.util import *
 
 '''
-llmq-signing.py
+feature_llmq_signing.py
 
 Checks LLMQs signing sessions
 

test/functional/feature_llmq_simplepose.py

@@ -9,7 +9,7 @@ from test_framework.test_framework import DashTestFramework
 from test_framework.util import *
 
 '''
-llmq-simplepose.py
+feature_llmq_simplepose.py
 
 Checks simple PoSe system based on LLMQ commitments
 

test/functional/feature_multikeysporks.py

@@ -8,7 +8,7 @@ from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import connect_nodes, wait_until
 
 '''
-multikeysporks.py
+feature_multikeysporks.py
 
 Test logic for several signer keys usage for spork broadcast.
 

test/functional/p2p_instantsend.py

@@ -9,7 +9,7 @@ from test_framework.util import isolate_node, reconnect_isolated_node, assert_eq
     assert_raises_rpc_error
 
 '''
-p2p-instantsend.py
+p2p_instantsend.py
 
 Tests InstantSend functionality (prevent doublespend for unconfirmed transactions)
 '''

test/functional/rpc_privatesend.py

@@ -7,7 +7,7 @@ from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import assert_equal
 
 '''
-privatesend.py
+rpc_privatesend.py
 
 Tests PrivateSend basic RPC
 '''

test/functional/test_runner.py

@@ -58,103 +58,104 @@ TRAVIS_TIMEOUT_DURATION = 20 * 60
 BASE_SCRIPTS= [
     # Scripts that are run by the travis build process.
     # Longest test should go first, to favor running tests in parallel
-    'dip3-deterministicmns.py', # NOTE: needs dash_hash to pass
+    'feature_dip3_deterministicmns.py', # NOTE: needs dash_hash to pass
-    'wallet-hd.py',
+    'wallet_hd.py',
-    'walletbackup.py',
+    'wallet_backup.py',
     # vv Tests less than 5m vv
-    'p2p-fullblocktest.py', # NOTE: needs dash_hash to pass
+    'feature_block.py', # NOTE: needs dash_hash to pass
-    'fundrawtransaction.py',
+    'rpc_fundrawtransaction.py',
-    'fundrawtransaction-hd.py',
+    'rpc_fundrawtransaction_hd.py',
     # vv Tests less than 2m vv
-    'p2p-instantsend.py',
+    'p2p_instantsend.py',
-    'wallet.py',
+    'wallet_basic.py',
-    'wallet-accounts.py',
+    'wallet_accounts.py',
-    'wallet-dump.py',
+    'wallet_dump.py',
-    'listtransactions.py',
+    'rpc_listtransactions.py',
-    'multikeysporks.py',
+    'feature_multikeysporks.py',
-    'llmq-signing.py', # NOTE: needs dash_hash to pass
+    'feature_llmq_signing.py', # NOTE: needs dash_hash to pass
-    'llmq-signing.py --spork21', # NOTE: needs dash_hash to pass
+    'feature_llmq_signing.py --spork21', # NOTE: needs dash_hash to pass
-    'llmq-chainlocks.py', # NOTE: needs dash_hash to pass
+    'feature_llmq_chainlocks.py', # NOTE: needs dash_hash to pass
-    'llmq-connections.py', # NOTE: needs dash_hash to pass
+    'feature_llmq_connections.py', # NOTE: needs dash_hash to pass
-    'llmq-simplepose.py', # NOTE: needs dash_hash to pass
+    'feature_llmq_simplepose.py', # NOTE: needs dash_hash to pass
-    'llmq-is-cl-conflicts.py', # NOTE: needs dash_hash to pass
+    'feature_llmq_is_cl_conflicts.py', # NOTE: needs dash_hash to pass
-    'llmq-is-retroactive.py', # NOTE: needs dash_hash to pass
+    'feature_llmq_is_retroactive.py', # NOTE: needs dash_hash to pass
-    'llmq-dkgerrors.py', # NOTE: needs dash_hash to pass
+    'feature_llmq_dkgerrors.py', # NOTE: needs dash_hash to pass
-    'dip4-coinbasemerkleroots.py', # NOTE: needs dash_hash to pass
+    'feature_dip4_coinbasemerkleroots.py', # NOTE: needs dash_hash to pass
     # vv Tests less than 60s vv
-    'sendheaders.py', # NOTE: needs dash_hash to pass
+    'p2p_sendheaders.py', # NOTE: needs dash_hash to pass
-    'zapwallettxes.py',
+    'wallet_zapwallettxes.py',
-    'importmulti.py',
+    'wallet_importmulti.py',
     'mempool_limit.py',
-    'merkle_blocks.py',
+    'rpc_txoutproof.py',
-    'receivedby.py',
+    'wallet_listreceivedby.py',
-    'abandonconflict.py',
+    'wallet_abandonconflict.py',
-    'bip68-112-113-p2p.py',
+    'feature_csv_activation.py',
-    'rawtransactions.py',
+    'rpc_rawtransaction.py',
-    'reindex.py',
+    'feature_reindex.py',
     # vv Tests less than 30s vv
-    'keypool-topup.py',
+    'wallet_keypool_topup.py',
-    'zmq_test.py',
+    'interface_zmq.py',
-    'bitcoin_cli.py',
+    'interface_bitcoin_cli.py',
-    'mempool_resurrect_test.py',
+    'mempool_resurrect.py',
-    'txn_doublespend.py --mineblock',
+    'wallet_txn_doublespend.py --mineblock',
-    'txn_clone.py',
+    'wallet_txn_clone.py',
-    'getchaintips.py',
+    'rpc_getchaintips.py',
-    'rest.py',
+    'interface_rest.py',
-    'mempool_spendcoinbase.py',
+    'mempool_spend_coinbase.py',
     'mempool_reorg.py',
     'mempool_persist.py',
-    'multiwallet.py',
+    'wallet_multiwallet.py',
-    'multiwallet.py --usecli',
+    'wallet_multiwallet.py --usecli',
-    'httpbasics.py',
+    'interface_http.py',
-    'multi_rpc.py',
+    'rpc_users.py',
-    'proxy_test.py',
+    'feature_proxy.py',
-    'signrawtransactions.py',
+    'rpc_signrawtransaction.py',
-    'disconnect_ban.py',
+    'p2p_disconnect_ban.py',
-    'addressindex.py',
+    'feature_addressindex.py',
-    'timestampindex.py',
+    'feature_timestampindex.py',
-    'spentindex.py',
+    'feature_spentindex.py',
-    'decodescript.py',
+    'rpc_decodescript.py',
-    'blockchain.py',
+    'rpc_blockchain.py',
-    'deprecated_rpc.py',
+    'rpc_deprecated.py',
-    'disablewallet.py',
+    'wallet_disable.py',
-    'net.py',
+    'rpc_net.py',
-    'keypool.py',
+    'wallet_keypool.py',
-    'keypool-hd.py',
+    'wallet_keypool_hd.py',
-    'p2p-mempool.py',
+    'p2p_mempool.py',
-    'prioritise_transaction.py',
+    'mining_prioritisetransaction.py',
-    'invalidblockrequest.py', # NOTE: needs dash_hash to pass
+    'p2p_invalid_block.py',
-    'invalidtxrequest.py', # NOTE: needs dash_hash to pass
+    'p2p_invalid_tx.py',
-    'p2p-versionbits-warning.py',
+    'feature_versionbits_warning.py',
-    'preciousblock.py',
+    'rpc_preciousblock.py',
-    'importprunedfunds.py',
+    'wallet_importprunedfunds.py',
-    'signmessages.py',
+    'rpc_signmessage.py',
-    'nulldummy.py',
+    'feature_nulldummy.py',
-    'import-rescan.py',
+    'wallet_import_rescan.py',
-    'rpcbind_test.py --ipv4',
+    'rpc_bind.py --ipv4',
-    'rpcbind_test.py --ipv6',
+    'rpc_bind.py --ipv6',
-    'rpcbind_test.py --nonloopback',
+    'rpc_bind.py --nonloopback',
-    'mining.py',
+    'mining_basic.py',
-    'rpcnamedargs.py',
+    'rpc_named_arguments.py',
-    'listsinceblock.py',
+    'wallet_listsinceblock.py',
-    'p2p-leaktests.py',
+    'p2p_leak.py',
-    'p2p-compactblocks.py',
+    'p2p_compactblocks.py',
-    'sporks.py',
+    'feature_sporks.py',
     'rpc_getblockstats.py',
-    'p2p-fingerprint.py',
+    'wallet_encryption.py',
-    'wallet-encryption.py',
+    'feature_dersig.py',
-    'bipdersig-p2p.py',
+    'feature_cltv.py',
-    'bip65-cltv-p2p.py',
+    'rpc_uptime.py',
-    'uptime.py',
+    'wallet_resendwallettransactions.py',
-    'resendwallettransactions.py',
+    'feature_minchainwork.py',
-    'minchainwork.py',
+    'p2p_unrequested_blocks.py', # NOTE: needs dash_hash to pass
-    'p2p-acceptblock.py', # NOTE: needs dash_hash to pass
     'feature_shutdown.py',
-    'privatesend.py',
+    'rpc_privatesend.py',
-    'uacomment.py',
+    'p2p_fingerprint.py',
+    'feature_uacomment.py',
+    'p2p_unrequested_blocks.py',
     'feature_logging.py',
-    'node_network_limited.py',
+    'p2p_node_network_limited.py',
-    'conf_args.py',
+    'feature_config_args.py',
     'feature_help.py',
     # Don't append tests at the end to avoid merge conflicts
     # Put them in a random line within the section that fits their approximate run-time
@@ -163,26 +164,26 @@ BASE_SCRIPTS= [
 EXTENDED_SCRIPTS = [
     # These tests are not run by the travis build process.
     # Longest test should go first, to favor running tests in parallel
-    'pruning.py', # NOTE: Prune mode is incompatible with -txindex, should work governance validation disabled though.
+    'feature_pruning.py', # NOTE: Prune mode is incompatible with -txindex, should work with governance validation disabled though.
     # vv Tests less than 20m vv
-    'smartfees.py',
+    'feature_fee_estimation.py',
     # vv Tests less than 5m vv
-    'maxuploadtarget.py',
+    'feature_maxuploadtarget.py',
     'mempool_packages.py',
-    'dbcrash.py',
+    'feature_dbcrash.py',
     # vv Tests less than 2m vv
-    'bip68-sequence.py',
+    'feature_bip68_sequence.py',
-    'getblocktemplate_longpoll.py',  # FIXME: "socket.error: [Errno 54] Connection reset by peer" on my Mac, same as  https://github.com/bitcoin/bitcoin/issues/6651
+    'mining_getblocktemplate_longpoll.py',  # FIXME: "socket.error: [Errno 54] Connection reset by peer" on my Mac, same as  https://github.com/bitcoin/bitcoin/issues/6651
-    'p2p-timeouts.py',
+    'p2p_timeouts.py',
     # vv Tests less than 60s vv
     # vv Tests less than 30s vv
-    'assumevalid.py',
+    'feature_assumevalid.py',
     'example_test.py',
-    'txn_doublespend.py',
+    'wallet_txn_doublespend.py',
-    'txn_clone.py --mineblock',
+    'wallet_txn_clone.py --mineblock',
-    'txindex.py',
+    'feature_txindex.py',
-    'notifications.py',
+    'feature_notifications.py',
-    'invalidateblock.py',
+    'rpc_invalidateblock.py',
 ]
 
 # Place EXTENDED_SCRIPTS first since it has the 3 longest running tests
@@ -292,6 +293,7 @@ def main():
         sys.exit(0)
 
     check_script_list(src_dir=config["environment"]["SRCDIR"], fail_on_warn=args.ci)
+    check_script_prefixes()
 
     if not args.keepcache:
         shutil.rmtree("%s/test/cache" % config["environment"]["BUILDDIR"], ignore_errors=True)
@@ -532,6 +534,24 @@ class TestResult():
         return self.status != "Failed"
 
 
+def check_script_prefixes():
+    """Check that at most a handful of the
+       test scripts don't start with one of the allowed name prefixes."""
+
+    # LEEWAY is provided as a transition measure, so that pull-requests
+    # that introduce new tests that don't conform with the naming
+    # convention don't immediately cause the tests to fail.
+    LEEWAY = 10
+
+    good_prefixes_re = re.compile("(example|feature|interface|mempool|mining|p2p|rpc|wallet)_")
+    bad_script_names = [script for script in ALL_SCRIPTS if good_prefixes_re.match(script) is None]
+
+    if len(bad_script_names) > 0:
+        print("INFO: %d tests not meeting naming conventions:" % (len(bad_script_names)))
+        print("  %s" % ("\n  ".join(sorted(bad_script_names))))
+    assert len(bad_script_names) <= LEEWAY, "Too many tests not following naming convention! (%d found, maximum: %d)" % (len(bad_script_names), LEEWAY)
+
+
 def check_script_list(*, src_dir, fail_on_warn):
     """Check scripts directory.