[tests] Functional test naming convention

ajtowns commented at 9:39 am on November 30, 2017: member

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.

[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] README.md nit fixes 7250b4e563

ajtowns force-pushed on Nov 30, 2017

ajtowns commented at 10:25 am on November 30, 2017: member

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.

fanquake added the label Tests on Nov 30, 2017

in test/functional/test_runner.py:160 in 7bdabc9d49 outdated

156@@ -157,6 +157,21 @@
157 # Place EXTENDED_SCRIPTS first since it has the 3 longest running tests
158 ALL_SCRIPTS = EXTENDED_SCRIPTS + BASE_SCRIPTS
159 
160+def check_script_prefixes():

jnewbery commented at 3:02 pm on November 30, 2017:

Consider adding a doc string for this function:

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

ajtowns commented at 8:22 am on December 5, 2017:

doc string added

in test/functional/test_runner.py:165 in 7bdabc9d49 outdated

156@@ -157,6 +157,21 @@
157 # Place EXTENDED_SCRIPTS first since it has the 3 longest running tests
158 ALL_SCRIPTS = EXTENDED_SCRIPTS + BASE_SCRIPTS
159 
160+def check_script_prefixes():
161+    EXPECTED_VIOLATION_COUNT = 76
162+    LEEWAY = 10
163+
164+    ok = {"feature", "interface", "mempool", "mining", "p2p", "rpc", "wallet"}
165+    found = set(x.split("_",1)[0] for x in ALL_SCRIPTS if x != "example_test.py")

jnewbery commented at 3:04 pm on November 30, 2017:

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 commented at 8:22 am on December 5, 2017:

fixed

jnewbery approved

jnewbery commented at 3:07 pm on November 30, 2017: member

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.

ajtowns force-pushed on Dec 5, 2017

ajtowns commented at 9:31 am on December 5, 2017: member

Fixed bug and nit in check_script_prefixes pointed out by @jnewbery

in test/functional/test_runner.py:180 in 9882ce9ef5 outdated

175+        print("{}HURRAY!{} Number of functional tests violating naming convention reduced!".format(BOLD[1], BOLD[0]))
176+        print("Consider reducing EXPECTED_VIOLATION_COUNT from %d to %d" % (EXPECTED_VIOLATION_COUNT, len(excess)))
177+
178+    assert len(excess) <= EXPECTED_VIOLATION_COUNT + LEEWAY, "Too many tests not following naming convention! (%d found, expected: <= %d)" % (len(excess), EXPECTED_VIOLATION_COUNT)
179+
180+    if 0 < len(excess) <= LEEWAY:

jnewbery commented at 3:10 pm on December 5, 2017:

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

jnewbery commented at 3:14 pm on December 5, 2017: member

:( 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.

[tests] Check tests conform to naming convention

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

9b20bb40fb

ajtowns force-pushed on Dec 7, 2017

jnewbery commented at 9:19 pm on December 8, 2017: member

tested ACK 9b20bb40fbd59c8fd24a7c82e87600ea3c5c7039

ajtowns commented at 8:37 am on December 11, 2017: member

Added patch to comply with unused python imports check

jnewbery commented at 3:29 pm on December 11, 2017: member

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? 🙂

[tests] Remove redundant import in blocktools.py test 5fecd842a6

ajtowns force-pushed on Jan 3, 2018

ajtowns commented at 7:51 am on January 3, 2018: member

Updated to remove the duplicate key_to_p2sh_p2wpkh import

jnewbery commented at 10:08 am on January 3, 2018: member

ACK changes in 5fecd842a6ff3d094c21f84b81b6cef09787c3b7

laanwj commented at 9:35 am on January 15, 2018: member

utACK 5fecd84

laanwj merged this on Jan 15, 2018

laanwj closed this on Jan 15, 2018

laanwj referenced this in commit 4db16ec827 on Jan 15, 2018

PastaPastaPasta referenced this in commit a8951f0183 on Apr 13, 2020

PastaPastaPasta referenced this in commit 3e00841782 on Apr 13, 2020

deadalnix referenced this in commit d474b8dedd on Apr 17, 2020

PastaPastaPasta referenced this in commit 17b61da1d5 on Jul 16, 2020

UdjinM6 referenced this in commit b07a7b810c on Jul 16, 2020

ftrader referenced this in commit fc95a88b01 on Aug 17, 2020

DrahtBot locked this on Sep 8, 2021

[tests] Functional test naming convention #11796