From 3e0084178248814c851f37ff6e6bc05d50aa8c6c Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Mon, 15 Jan 2018 10:33:43 +0100 Subject: [PATCH] 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 --- test/functional/README.md | 16 +++++++++++++- test/functional/test_framework/blocktools.py | 18 ++++++++++++++- test/functional/test_runner.py | 23 ++++++++++++++++++++ 3 files changed, 55 insertions(+), 2 deletions(-) diff --git a/test/functional/README.md b/test/functional/README.md index 1e7e0108ca0081..5840ca7e36952e 100644 --- a/test/functional/README.md +++ b/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 `_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-acceptblock.py`, `p2p-compactblocks.py`. ### test-framework modules diff --git a/test/functional/test_framework/blocktools.py b/test/functional/test_framework/blocktools.py index 67a632824135f8..c0baace74e4f2a 100644 --- a/test/functional/test_framework/blocktools.py +++ b/test/functional/test_framework/blocktools.py @@ -4,8 +4,24 @@ # file COPYING or http://www.opensource.org/licenses/mit-license.php. """Utilities for manipulating blocks and transactions.""" +from .address import ( + key_to_p2sh_p2wpkh, + key_to_p2wpkh, + script_to_p2sh_p2wsh, + script_to_p2wsh, +) from .mininode import * -from .script import CScript, OP_TRUE, OP_CHECKSIG +from .script import ( + CScript, + OP_0, + OP_1, + OP_CHECKMULTISIG, + OP_CHECKSIG, + OP_RETURN, + OP_TRUE, + hash160, +) +from .util import assert_equal # Create a block (with regtest difficulty) def create_block(hashprev, coinbase, nTime=None): diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index 2af3779f8c3396..af3509242802c8 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -287,6 +287,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) @@ -517,6 +518,28 @@ def was_successful(self): return self.status != "Failed" +def check_script_prefixes(): + """Check that no more than `EXPECTED_VIOLATION_COUNT` of the + test scripts don't start with one of the allowed name prefixes.""" + EXPECTED_VIOLATION_COUNT = 77 + + # 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) < EXPECTED_VIOLATION_COUNT: + print("{}HURRAY!{} Number of functional tests violating naming convention reduced!".format(BOLD[1], BOLD[0])) + print("Consider reducing EXPECTED_VIOLATION_COUNT from %d to %d" % (EXPECTED_VIOLATION_COUNT, len(bad_script_names))) + elif len(bad_script_names) > EXPECTED_VIOLATION_COUNT: + print("INFO: %d tests not meeting naming conventions (expected %d):" % (len(bad_script_names), EXPECTED_VIOLATION_COUNT)) + print(" %s" % ("\n ".join(sorted(bad_script_names)))) + assert len(bad_script_names) <= EXPECTED_VIOLATION_COUNT + LEEWAY, "Too many tests not following naming convention! (%d found, expected: <= %d)" % (len(bad_script_names), EXPECTED_VIOLATION_COUNT) + + def check_script_list(*, src_dir, fail_on_warn): """Check scripts directory.