Tests: tidy up address.py and segwit

jnewbery commented at 6:06 pm on June 11, 2020: member

Lots of small fixes:

moving unit tests to test_framework implementation files
renaming functions to be clearer
removing multiple imports
removing unreadable byte literals from the code
fixing pep8 violations
correcting out-of-date docstring

DrahtBot added the label Tests on Jun 11, 2020

decentclock commented at 11:05 pm on June 21, 2020: none

I ran test_runner.py on mac os 10.15.5 and all the tests changed in this PR passed!

I did fail wallet_keypool_topup.py --descriptors See the logs below. I am not sure if this related to the changes in this PR. test38failure.txt

I reviewed all the changes, and it all looks good to me, except for a small detail, please see question inline.

in test/functional/test_framework/segwit_addr.py:121 in b450389f31 outdated

115+            self.assertEqual(hrp, "bcrt")
116+            (witver, witprog) = decode_segwit_address(hrp, addr)
117+            self.assertEqual(encode_segwit_address(hrp, witver, witprog), addr)
118+
119+        test_python_bech32('bcrt1qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqq3xueyj')
120+        test_python_bech32('bcrt1qft5p2uhsdcdc3l2ua4ap5qqfg4pjaqlp250x7us7a8qqhrxrxfsqseac85')

decentclock commented at 11:10 pm on June 21, 2020:

I’m wondering if it would be better to use the ADDRESS_BCRT1_UNSPENDABLE and ADDRESS_BCRT1_P2WSH_OP_TRUE constants from address.py here.

jonatack commented at 1:55 am on July 1, 2020:

I’m wondering if it would be better to use the ADDRESS_BCRT1_UNSPENDABLE and ADDRESS_BCRT1_P2WSH_OP_TRUE constants from address.py here.

Good idea!

jnewbery commented at 3:45 pm on July 1, 2020:

I would do this if the ADDRESS_BCRT_* constants were in segwit_addr.py. address.py imports from segwit_addr.py, so adding importing these constants from address.py would be a circular dependency. Other options:

move the constants to segwit_addr.py. Doesn’t seem worth it since a bunch of test files would need to be updated (grep for ADDRESS_BCRT to see which test files import these constants from address.py)
merge address.py and segwit_addr.py into a single file. I think this makes sense as a follow-up (and previously suggested it here: #19239 (comment))

MarcoFalke commented at 2:51 pm on September 3, 2020:

Previously this was testing p2wsh and p2wpkh, now it is only testing p2wsh

jnewbery commented at 3:48 pm on September 3, 2020:

Added a P2WPKH test

in test/functional/test_runner.py:71 in b450389f31 outdated

67@@ -68,6 +68,7 @@
68 
69 TEST_FRAMEWORK_MODULES = [
70     "address",
71+    "segwit_addr",

jonatack commented at 1:49 am on July 1, 2020:

nit: sort

jnewbery commented at 3:48 pm on July 1, 2020:

done

jonatack commented at 1:57 am on July 1, 2020: member

ACK modulo #19253 (review)

The pep8, docstring and imports tidy-ups can probably be one clean-up commit.

jnewbery force-pushed on Jul 1, 2020

jnewbery commented at 3:49 pm on July 1, 2020: member

I did fail wallet_keypool_topup.py –descriptors See the logs below. I am not sure if this related to the changes in this PR.

I don’t think it’s related. I’m able to run that test successfully on this branch.

DrahtBot commented at 6:49 pm on July 1, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

DrahtBot added the label Needs rebase on Sep 1, 2020

jnewbery commented at 10:36 am on September 3, 2020: member

rebased

jnewbery force-pushed on Sep 3, 2020

DrahtBot removed the label Needs rebase on Sep 3, 2020

jonatack commented at 11:22 am on September 3, 2020: member

utACK 7edcdcd72a7886ec2aacf7de8f8b9d6be99f1e32

[tests] Move bech32 unit tests to test framework e4557133f5

[tests] Rename segwit encode and decode functions

These functions can be exported to other modules,
so be explicit that they're encoding and decoding
segwit addresses

011e784f74

[tests] Remove unused optional verify_checksum parameter

This optional parameter is never used, so remove it.

7f639df0b8

[tests] Tidy up imports in address.py

No need to import twice from util.py

ea70e6a2ca

[tests] Correct docstring for address.py b230f8b3f3

[tests] Fix pep8 style violations in address.py 64eca45100

[tests] Replace bytes literals with hex literals

It's almost impossible to read bytes literals in code, so replace them
with the hex string literal and then convert them to a bytes object
using bytes.fromhex().

825fcae484

jnewbery force-pushed on Sep 3, 2020

jonatack commented at 4:48 pm on September 3, 2020: member

re-ACK 825fcae484f31182041dfacbf820e818d759b130 per git range-diff a0a422c 7edcdcd 825fcae and verified wallet_address_types.py and wallet_basic.py --descriptors (the failure on one travis job) are green locally.

Good catch, Marco, on the p2wpkh address test.

fanquake commented at 6:58 am on October 1, 2020: member

ACK 825fcae484f31182041dfacbf820e818d759b130 - looks ok to me.

MarcoFalke commented at 7:42 am on October 1, 2020: member

ACK 825fcae484f31182041dfacbf820e818d759b130

MarcoFalke merged this on Oct 1, 2020

MarcoFalke closed this on Oct 1, 2020

jnewbery deleted the branch on Oct 1, 2020

sidhujag referenced this in commit e4b5357789 on Oct 4, 2020

Fabcien referenced this in commit e875608a68 on Nov 3, 2021

Fabcien referenced this in commit 2149648052 on Nov 3, 2021

DrahtBot locked this on Feb 15, 2022

Tests: tidy up address.py and segwit_addr.py #19253

Conflicts