[tests] Functional test naming convention #11796

ajtowns · 2017-11-30T09:39:30Z

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.

This avoids importing from segwit.py to bumpfee.py

ajtowns · 2017-11-30T10:25:58Z

This PR is at the same height as #11047 was, which may make it easier to re-review for anyone who has looked at the original PR.

jnewbery

Tested ACK 7bdabc9d4921da7e0e33f1871664ff3ef8de5c01

I've left a couple of nits on your check_script_prefixes(), but I don't think you should take them. That functionality should be changed after #11774 is merged (for example we'd want to remove EXPECTED_VIOLATION_COUNT after #11774 ), so it's fine to merge this as it is, and then make an update to it as a final commit in #11774.

jnewbery · 2017-11-30T15:02:17Z

test/functional/test_runner.py

@@ -157,6 +157,21 @@
 # Place EXTENDED_SCRIPTS first since it has the 3 longest running tests
 ALL_SCRIPTS = EXTENDED_SCRIPTS + BASE_SCRIPTS

+def check_script_prefixes():


Consider adding a doc string for this function:

"""Check that no more than `EXPECTED_VIOLATION_COUNT` of the test scripts don't start with one of the allowed name prefixes."""

doc string added

jnewbery · 2017-11-30T15:04:33Z

test/functional/test_runner.py

+    LEEWAY = 10
+
+    ok = {"feature", "interface", "mempool", "mining", "p2p", "rpc", "wallet"}
+    found = set(x.split("_",1)[0] for x in ALL_SCRIPTS if x != "example_test.py")


nit: this is testing the number of prefixes violating the rule rather than the number of test names violating the rule. If I had badprefix_test1.py and badprefix_test2.py, that would only count as one violation.

ajtowns · 2017-12-05T09:31:23Z

Fixed bug and nit in check_script_prefixes pointed out by @jnewbery

jnewbery

:( I was hoping you wouldn't rework this - it did the job and I didn't think the minor bug was worth fixing for the re-review effort.

The extra juggling you're doing with sets in this version is more confusing than it was, and you seem to have introduced a new bug checking against the LEEWAY buffer. I think it's simpler just to do away with LEEWAY entirely, use python's re to do the hard work, and run the checking from within the main() function (rather than on module load).

Can you take a look at https://github.com/jnewbery/bitcoin/tree/pr11796.1 and let me know what you think? Without LEEWAY, the final commit in the subsequent PR will make the check_script_prefixes() very straightforward.

jnewbery · 2017-12-05T15:10:01Z

test/functional/test_runner.py

+
+    assert len(excess) <= EXPECTED_VIOLATION_COUNT + LEEWAY, "Too many tests not following naming convention! (%d found, expected: <= %d)" % (len(excess), EXPECTED_VIOLATION_COUNT)
+
+    if 0 < len(excess) <= LEEWAY:


bug: this should be EXPECTED_VIOLATION_COUNT < len(excess) <= EXPECTED_VIOLATION_COUNT + LEEWAY

Extra-Author: John Newbery <john@johnnewbery.com>

jnewbery · 2017-12-08T21:19:12Z

tested ACK 9b20bb4

ajtowns · 2017-12-11T08:37:21Z

Added patch to comply with unused python imports check

jnewbery · 2017-12-11T15:29:29Z

a179c4f93b502437f166c5738a1bfe26e68a213e looks good.

While you're fixing my crumby code, can you remove the duplicate key_to_p2sh_p2wpkh import at https://github.com/bitcoin/bitcoin/pull/11796/files#diff-9c6fb177b6d4f8f833ca4a00b5d94c3eR8? 🙂

ajtowns · 2018-01-03T07:51:20Z

Updated to remove the duplicate key_to_p2sh_p2wpkh import

jnewbery · 2018-01-03T10:08:22Z

ACK changes in 5fecd84

laanwj · 2018-01-15T09:35:36Z

utACK 5fecd84

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

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 bitcoin#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

Summary: * [tests] [docs] update README for new test naming scheme * [tests] README.md nit fixes * [tests] Check tests conform to naming convention Extra-Author: John Newbery <john@johnnewbery.com> This is a backport of Core [[bitcoin/bitcoin#11796 | PR11796]] Test Plan: ninja check-functional Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D5750

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 bitcoin#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

* Merge bitcoin#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 bitcoin#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 bitcoin#11774: [tests] Rename functional tests 6f881cc [tests] Remove EXPECTED_VIOLATION_COUNT (Anthony Towns) 3150b3f [tests] Rename misc functional tests. (Anthony Towns) 81b79f2 [tests] Rename rpc_* functional tests. (Anthony Towns) 61b8f7f [tests] Rename p2p_* functional tests. (Anthony Towns) 90600bc [tests] Rename wallet_* functional tests. (Anthony Towns) ca6523d [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>

Summary: * [tests] [docs] update README for new test naming scheme * [tests] README.md nit fixes * [tests] Check tests conform to naming convention Extra-Author: John Newbery <john@johnnewbery.com> This is a backport of Core [[bitcoin/bitcoin#11796 | PR11796]] Test Plan: ninja check-functional Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D5750

jnewbery added 2 commits October 10, 2017 09:12

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

1e10854

[tests] move witness util functions to blocktools.py

82b2712

This avoids importing from segwit.py to bumpfee.py

ajtowns mentioned this pull request Nov 30, 2017

[tests] Rename functional tests #11774

Merged

[tests] README.md nit fixes

7250b4e

ajtowns force-pushed the soft_rename_tests branch from d761403 to 7bdabc9 Compare November 30, 2017 10:22

fanquake added the Tests label Nov 30, 2017

jnewbery approved these changes Nov 30, 2017

View reviewed changes

ajtowns mentioned this pull request Dec 1, 2017

Add -debuglogfile option #11781

Merged

ajtowns force-pushed the soft_rename_tests branch from 7bdabc9 to 9882ce9 Compare December 5, 2017 08:21

ajtowns mentioned this pull request Dec 5, 2017

Add per-network config file network.conf #10996

Closed

jnewbery reviewed Dec 5, 2017

View reviewed changes

[tests] Check tests conform to naming convention

9b20bb4

Extra-Author: John Newbery <john@johnnewbery.com>

ajtowns force-pushed the soft_rename_tests branch from 9882ce9 to 9b20bb4 Compare December 7, 2017 16:06

ajtowns mentioned this pull request Dec 10, 2017

Add Travis check for unused Python imports #11835

Merged

[tests] Remove redundant import in blocktools.py test

5fecd84

ajtowns force-pushed the soft_rename_tests branch from a179c4f to 5fecd84 Compare January 3, 2018 06:34

laanwj merged commit 5fecd84 into bitcoin:master Jan 15, 2018

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[tests] Functional test naming convention #11796

[tests] Functional test naming convention #11796

ajtowns commented Nov 30, 2017

ajtowns commented Nov 30, 2017

jnewbery left a comment

jnewbery Nov 30, 2017

ajtowns Dec 5, 2017

jnewbery Nov 30, 2017

ajtowns Dec 5, 2017

ajtowns commented Dec 5, 2017

jnewbery left a comment

jnewbery Dec 5, 2017

jnewbery commented Dec 8, 2017

ajtowns commented Dec 11, 2017

jnewbery commented Dec 11, 2017

ajtowns commented Jan 3, 2018

jnewbery commented Jan 3, 2018

laanwj commented Jan 15, 2018


		assert len(excess) <= EXPECTED_VIOLATION_COUNT + LEEWAY, "Too many tests not following naming convention! (%d found, expected: <= %d)" % (len(excess), EXPECTED_VIOLATION_COUNT)

		if 0 < len(excess) <= LEEWAY:

[tests] Functional test naming convention #11796

[tests] Functional test naming convention #11796

Conversation

ajtowns commented Nov 30, 2017

ajtowns commented Nov 30, 2017

jnewbery left a comment

Choose a reason for hiding this comment

jnewbery Nov 30, 2017

Choose a reason for hiding this comment

ajtowns Dec 5, 2017

Choose a reason for hiding this comment

jnewbery Nov 30, 2017

Choose a reason for hiding this comment

ajtowns Dec 5, 2017

Choose a reason for hiding this comment

ajtowns commented Dec 5, 2017

jnewbery left a comment

Choose a reason for hiding this comment

jnewbery Dec 5, 2017

Choose a reason for hiding this comment

jnewbery commented Dec 8, 2017

ajtowns commented Dec 11, 2017

jnewbery commented Dec 11, 2017

ajtowns commented Jan 3, 2018

jnewbery commented Jan 3, 2018

laanwj commented Jan 15, 2018