[tests] rename functional tests

jnewbery commented at 8:04 pm on August 14, 2017: member

This may be a non-starter because of the churn caused, but I thought I’d put it out there and see what people think. Not that git is usually smart enough to figure out filename only changes, so rebasing shouldn’t be an issue.

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.

ryanofsky commented at 8:41 pm on August 14, 2017: member

I like this because there have been times I haven’t known how to name a new test based on the existing names. Two suggestions:

Could more consistently abbreviate transaction as either tx or txn
Maybe rename to category/test.py instead of category_test.py to make it easier to distinguish between test utitilities (like combine_logs.py and test_runner.py) and tests, and to make the utilities more discoverable.

promag commented at 10:18 pm on August 14, 2017: member

Maybe rename to category/test.py instead of category_test.py.

:+1:

MarcoFalke commented at 10:29 pm on August 14, 2017: member

Concept ACK. At this point we have so many test scripts that some sort of naming scheme or grouping really makes sense.

fanquake added the label Tests on Aug 14, 2017

jonasschnelli commented at 6:40 pm on August 15, 2017: contributor

I would have preferred folders but I guess that makes things complicated. utACK deda19ea50b49cebe7e62caf3770989e46af505f

MarcoFalke commented at 6:55 pm on August 15, 2017: member

Would you mind adding the naming rules from the OP to the QA developer notes?

On Tue, Aug 15, 2017 at 8:41 PM, Jonas Schnelli notifications@github.com wrote:

I would have preferred folder but I guess that makes things complicated. utACK deda19e https://github.com/bitcoin/bitcoin/commit/deda19ea50b49cebe7e62caf3770989e46af505f

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

jnewbery commented at 6:55 pm on August 15, 2017: member

@jonasschnelli folders is possible. This is just a concept at the moment. @ryanofsky also prefers folders. I’m happy to take feedback on what scheme people prefer.

laanwj commented at 8:38 am on August 21, 2017: member

I would have preferred folders but I guess that makes things complicated.

I’d dread the resulting python mess when using folders as it conflicts with modules. Also having them in one directory makes it easier to find all the tests. So I slightly prefer this naming scheme.

meshcollider commented at 9:57 am on August 21, 2017: contributor

I also prefer the prefix style over the folders

mess110 commented at 4:27 pm on August 23, 2017: contributor

For now, I think prefix solves the problem of grouping and provides some extra clarity.

jnewbery force-pushed on Aug 29, 2017

jnewbery commented at 6:13 pm on August 29, 2017: member

Updated with more consistent naming. test_runner.py and README.md have also been updated.

jnewbery force-pushed on Aug 29, 2017

jnewbery force-pushed on Sep 14, 2017

jnewbery commented at 5:09 pm on September 14, 2017: member

reimplemented as a scripted-diff, since this is going to need rebase for every change in /test/functional.

jnewbery force-pushed on Sep 14, 2017

jnewbery force-pushed on Sep 29, 2017

jnewbery force-pushed on Oct 2, 2017

MarcoFalke commented at 7:04 pm on October 2, 2017: member

Can you remove the scripted diff tags? It takes longer to review the script than to look at the git diff. Though, feel free to keep the script in the commit body if that is helpful to you.

You might want to ask if anyone objects those changes on Thursday. If not, we can merge on Friday.

[tests] [docs] update README for new test naming scheme 1e10854038

[tests] move witness util functions to blocktools.py

This avoids importing from segwit.py to bumpfee.py

82b2712a66

[tests] Rename functional tests.

Not implemented as a scripted-diff since the script is more difficult
to review than the diff. The script used was:

for i in \
disablewallet.py,wallet_disable.py \
wallet-hd.py,wallet_hd.py \
walletbackup.py,wallet_backup.py \
p2p-fullblocktest.py,p2p_block.py \
fundrawtransaction.py,rpc_fundrawtransaction.py \
p2p-compactblocks.py,p2p_compactblocks.py \
segwit.py,p2p_segwit_activation.py \
multiwallet.py,wallet_multiwallet.py \
wallet.py,wallet_basic.py \
wallet-accounts.py,wallet_accounts.py \
p2p-segwit.py,feature_segwit.py \
wallet-dump.py,wallet_dump.py \
listtransactions.py,rpc_listtransactions.py \
sendheaders.py,p2p_sendheaders.py \
zapwallettxes.py,wallet_zapwallettxes.py \
importmulti.py,wallet_importmulti.py \
merkle_blocks.py,rpc_txoutproof.py \
receivedby.py,wallet_listreceivedby.py \
abandonconflict.py,wallet_abandonconflict.py \
bip68-112-113-p2p.py,p2p_csv_activation.py \
reindex.py,feature_reindex.py \
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_clone.py,wallet_txn_clone.py \
getchaintips.py,rpc_getchaintips.py \
rest.py,interface_rest.py \
mempool_spendcoinbase.py,mempool_spend_coinbase.py \
httpbasics.py,interface_http.py \
multi_rpc.py,rpc_users.py \
proxy_test.py,feature_proxy.py \
signrawtransactions.py,rpc_signrawtransaction.py \
rawtransactions.py,rpc_rawtransaction.py \
disconnect_ban.py,p2p_disconnect_ban.py \
decodescript.py,rpc_decodescript.py \
blockchain.py,rpc_blockchain.py \
net.py,rpc_net.py \
keypool.py,wallet_keypool.py \
p2p-mempool.py,p2p_mempool.py \
prioritise_transaction.py,mining_prioritisetransaction.py \
invalidblockrequest.py,p2p_invalid_block.py \
invalidtxrequest.py,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 \
mining.py,mining_basic.py \
bumpfee.py,wallet_bumpfee.py \
rpcnamedargs.py,rpc_named_arguments.py \
listsinceblock.py,wallet_listsinceblock.py \
p2p-leaktests.py,p2p_leak.py \
wallet-encryption.py,wallet_encryption.py \
bipdersig-p2p.py,feature_dersig.py \
bip65-cltv-p2p.py,feature_cltv.py \
uptime.py,rpc_uptime.py \
resendwallettransactions.py,wallet_resendwallettransactions.py \
pruning.py,feature_pruning.py \
smartfees.py,feature_fee_estimation.py \
maxuploadtarget.py,feature_maxuploadtarget.py \
dbcrash.py,feature_dbcrash.py \
bip68-sequence.py,feature_csv.py \
getblocktemplate_longpoll.py,mining_getblocktemplate_longpoll.py \
p2p-timeouts.py,p2p_timeouts.py \
bip9-softforks.py,feature_bip9_softforks.py \
p2p-feefilter.py,p2p_feefilter.py \
rpcbind_test.py,rpc_bind.py \
assumevalid.py,feature_assumevalid.py \
txn_doublespend.py,wallet_txn_doublespend.py \
forknotify.py,feature_forknotify.py \
invalidateblock.py,rpc_invalidateblock.py \
p2p-acceptblock.py,p2p_unrequested_blocks.py \
replace-by-fee.py,feature_rbf.py \
minchainwork.py,feature_minchainwork.py \
deprecated_rpc.py,rpc_deprecated.py;
do IFS=","; set -- $i; mv test/functional/$1 test/functional/$2; sed -i -e "s/'$1/'$2/g" test/functional/test_runner.py test/functional/README.md; done

8ad38c68df

jnewbery force-pushed on Oct 10, 2017

jnewbery commented at 1:14 pm on October 10, 2017: member

Rebased with the final commit not implemented as a scripted-diff.

in test/functional/README.md:42 in 1e10854038 outdated

37+    - `p2p` for tests that explicitly test the p2p interface, eg `p2p_disconnect_ban.py`
38+    - `rpc` for tests for individual RPC methods or features, eg `rpc_listtransactions.py`
39+    - `wallet` for tests for wallet features, eg `wallet_keypool.py`
40+- use an underscore to separate words
41+    - 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`
42+- Don't use the redundant work `test` in the name, eg `interface_zmq.py`, not `interface_zmq_test.py`

ajtowns commented at 2:08 am on October 11, 2017:

Nit: “redundant word” not work

in test/functional/test_runner.py:67 in 8ad38c68df

68+    'p2p_compactblocks.py',
69+    'p2p_segwit_activation.py',
70     # vv Tests less than 2m vv
71-    'wallet.py',
72-    'wallet-accounts.py',
73-    'p2p-segwit.py',

ajtowns commented at 2:23 am on October 11, 2017:

This seems backwards: segwit.py becomes p2p_segwit_activation, but p2p-segwit.py becomes feature_segwit?

in test/functional/test_runner.py:78 in 8ad38c68df

93-    'rawtransactions.py',
94-    'reindex.py',
95+    'rpc_txoutproof.py',
96+    'wallet_listreceivedby.py',
97+    'wallet_abandonconflict.py',
98+    'p2p_csv_activation.py',

ajtowns commented at 2:39 am on October 11, 2017:

This doesn’t seem to test p2p protocol aspects, so maybe should be feature_csv_activation? (Or feature_bip68_112_113_activation?)

in test/functional/test_runner.py:60 in 8ad38c68df

61     # vv Tests less than 5m vv
62-    'p2p-fullblocktest.py',
63-    'fundrawtransaction.py',
64-    'p2p-compactblocks.py',
65-    'segwit.py',
66+    'p2p_block.py',

ajtowns commented at 2:43 am on October 11, 2017:

p2p_block tests block acceptance and reorg behaviour rather than the p2p protocol so should probably be in the feature_ namespace

ajtowns commented at 2:45 am on October 11, 2017: member

ACK with a few nits (see other comments).

MarcoFalke commented at 6:42 pm on November 10, 2017: member

Will be closing in a couple of weeks, if this sits still.

jnewbery commented at 5:37 pm on November 27, 2017: member

Closing in favour of #11774. Thanks @ajtowns!

jnewbery closed this on Nov 27, 2017

jnewbery deleted the branch on Nov 27, 2017

MarcoFalke locked this on Sep 8, 2021

[tests] rename functional tests #11047