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

pull jnewbery wants to merge 7 commits into bitcoin:master from jnewbery:2020-06-addr-unit-tests changing 4 files +57 −51

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: contributor

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: contributor



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



Conflicts

No conflicts as of last run.
DrahtBot cross-referenced this on Jul 1, 2020 from issue Add hash_type MUHASH for gettxoutsetinfo by fjahr
DrahtBot cross-referenced this on Jul 1, 2020 from issue Add Muhash3072 implementation in Python by fjahr
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: contributor

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: contributor

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.
fjahr cross-referenced this on Sep 13, 2020 from issue "Good First Review" label by fjahr
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
bitcoin locked this on Feb 15, 2022

Contributors

Labels

Linked (view graph)

#19239 tests: move generate_wif_key to wallet_util.py #19941 "Good First Review" label

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

Conflicts