test: refactor: use consistent bytes <-> hex-string conversion in functional test framework #22619

pull Zero-1729 wants to merge 1 commits into bitcoin:master from Zero-1729:test-remove-binascii-in-test-framework changing 5 files +10 −14
  1. Zero-1729 commented at 8:11 pm on August 3, 2021: contributor

    This PR continues the work started in PR #22593, regarding using the bytes built-in module. In this PR specifically, instances of binascii’s methods hexlify, unhexlify, and a2b_hex have been replaced with the build-in bytes module’s hex and fromhex methods where appropriate to make bytes <-> hex-string conversions consistent across the functional test files and test_framework.

    Additionally, certain changes made are based on the following assumption:

    0bytes.hex(data) == binascii.hexlify(data).decode()
1bytes.hex(data).encode() == binascii.hexlify(data)

    Ran the functional tests to ensure behaviour is still consistent and changes didn’t break existing tests.

    closes #22605

  2. DrahtBot added the label Tests on Aug 3, 2021
  3. in test/functional/interface_rest.py:237 in 91b5350119 outdated 
    233@@ -235,13 +234,13 @@ def run_test(self):
234         response_hex = self.test_rest_request("/block/{}".format(bb_hash), req_type=ReqType.HEX, ret_type=RetType.OBJ)
235         assert_greater_than(int(response_hex.getheader('content-length')), BLOCK_HEADER_SIZE*2)
236         response_hex_bytes = response_hex.read().strip(b'\n')
237-        assert_equal(binascii.hexlify(response_bytes), response_hex_bytes)
238+        assert_equal(bytes.hex(response_bytes).encode(), response_hex_bytes)
    MarcoFalke commented at 5:40 am on August 4, 2021: 
    0        assert_equal(response_bytes.hex().encode(), response_hex_bytes)

    Is it possible to use the member method? (Didn’t check myself)

    Zero-1729 commented at 7:34 am on August 4, 2021:

    Yes, just tested on my end to verify, it has the same effect. Nice catch!

    Makes sense, as bytes.hex(bytes_obj) == bytes_obj.hex() and by extension bytes_obj.hex().encode() == binascii.hexlify(bytes_obj).

  4. MarcoFalke commented at 5:41 am on August 4, 2021: member
    Concept ACK, but I am wondering if the member method can be used
  5. Zero-1729 commented at 7:48 am on August 4, 2021: contributor

    @MarcoFalke Yeah, it works. It also applies to the change made in test_framework/bdb.py.

    I could just switch them to use bytes_obj.hex().encode() as suggested. It shouldn’t really affect the consistency of the bytes<->hex conversion as we are just accessing the built-in bytes and then str methods. Plus, it looks cleaner.

  6. Zero-1729 force-pushed on Aug 4, 2021
  7. practicalswift commented at 8:46 am on August 4, 2021: contributor
    Concept ACK
  8. Saviour1001 commented at 9:34 am on August 4, 2021: none
    Tested the changes on my end and works correctly.
  9. in test/functional/rpc_createmultisig.py:70 in 306dad77ad outdated 
    64@@ -66,9 +65,9 @@ def run_test(self):
65 
66         # decompress pk2
67         pk_obj = ECPubKey()
68-        pk_obj.set(binascii.unhexlify(pk2))
69+        pk_obj.set(bytes.fromhex(pk2))
70         pk_obj.compressed = False
71-        pk2 = binascii.hexlify(pk_obj.get_bytes()).decode()
72+        pk2 = bytes.hex(pk_obj.get_bytes())
    theStack commented at 11:09 am on August 4, 2021: 
    0        pk2 = pk_obj.get_bytes().hex()
    Zero-1729 commented at 7:05 pm on August 4, 2021:
    Oops! fixed.
  10. in test/functional/interface_rest.py:237 in 306dad77ad outdated 
    233@@ -235,13 +234,13 @@ def run_test(self):
234         response_hex = self.test_rest_request("/block/{}".format(bb_hash), req_type=ReqType.HEX, ret_type=RetType.OBJ)
235         assert_greater_than(int(response_hex.getheader('content-length')), BLOCK_HEADER_SIZE*2)
236         response_hex_bytes = response_hex.read().strip(b'\n')
237-        assert_equal(binascii.hexlify(response_bytes), response_hex_bytes)
238+        assert_equal(response_bytes.hex().encode(), response_hex_bytes)
    theStack commented at 11:13 am on August 4, 2021:

    nit: Possible alternative, though I think both variants are fine:

    0        assert_equal(response_bytes.hex(), response_hex_bytes.decode())
    Zero-1729 commented at 6:52 pm on August 4, 2021:

    Yeah, I didn’t want to disrupt the flow; it looked like the asserts in the file were of the kind assert_equal(actual, desired) (for the most part), i.e. the left arg being compared to the right.

    If that doesn’t matter much I’ll just change it.

  11. theStack commented at 11:19 am on August 4, 2021: member

    Concept ACK There is one more instance where the hex() member method can be used, see below.

    binascii is still used in Python files outside of the test framework, e.g. in contrib and test/util (I didn’t think about those in the issue, my bad…). If you want, you can also tackle those in this PR, if not, a follow-up is also okay I guess.

  12. test: refactor: remove binascii from test_framework 5a1bef60a0
  13. Zero-1729 commented at 7:03 pm on August 4, 2021: contributor

    binascii is still used in Python files outside of the test framework, e.g. in contrib and test/util (I didn’t think about those in the issue, my bad…). If you want, you can also tackle those in this PR, if not, a follow-up is also okay I guess.

    Yeah, initially after running git grep binascii I noticed those instances outside of the test/functional and test_framework. I think it would probably be best to handle those in a follow-up. This PR’s scope is already explicitly set as changes to the test framework, so a follow-up cleaning the instances outside it might be better.

  14. Zero-1729 force-pushed on Aug 4, 2021
  15. theStack approved
  16. theStack commented at 9:25 am on August 5, 2021: member

    Code-review ACK 5a1bef60a03b57de708a1a751bd90b8245fd8b83 🔢

    Checked that all binascii instances are removed and properly replaced within the test framework, also ran the changed tests locally 👌

  17. MarcoFalke referenced this in commit f4328ebef5 on Aug 5, 2021
  18. DrahtBot added the label Needs rebase on Aug 5, 2021
  19. DrahtBot commented at 10:32 am on August 5, 2021: member

    🐙 This pull request conflicts with the target branch and needs rebase.

    Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

  20. MarcoFalke commented at 10:49 am on August 5, 2021: member
    No idea what is going on with GitHub, this was merged 35 minutes ago: f4328ebef52f51a5ee45cc42815c91ccd28b0669
  21. MarcoFalke closed this on Aug 5, 2021
  22. sidhujag referenced this in commit 0ef6e23bcd on Aug 5, 2021
  23. MarcoFalke referenced this in commit f5a406f003 on Aug 21, 2021
  24. Fabcien referenced this in commit 6d4745579c on Mar 7, 2022
  25. DrahtBot locked this on Aug 16, 2022


Zero-1729 MarcoFalke practicalswift Saviour1001 theStack DrahtBot

Labels
Tests Needs rebase

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-09-29 01:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me