test: dedup file hashing using `sha256sum_file` helper #27572

pull theStack wants to merge 1 commits into bitcoin:master from theStack:test-refactor-use_sha256sum_file_helper changing 2 files +15 −19

theStack commented at 12:22 PM on May 4, 2023: contributor

Rather than doing the open/read/hash-steps manually in the affected functional tests, we can just use the sha256sum_file helper from the utils module instead.

Note that for the tool_wallet.py test, the used hash is changed from sha1 to sha256, but as the only purpose is to detect file content changes, this doesn't matter. Also, the optimization using memoryview is overkill here, as the opened file has only a size of 24KiB and determining the hash via the helper doesn't take longer than a few hundred micro-seconds on my machine.

test: dedup file hashing using `sha256sum_file` helper

Rather than doing the open/read/hash-steps manually in the affected
functional tests, we can just use the `sha256sum_file` helper from the
utils module instead.

Note that for the tool_wallet.py test, the used hash is changed from
sha1 to sha256, but as the only purpose is to detect file content
changes, this doesn't matter. Also, the optimization using `memoryview`
is overkill here, as the opened file has only a size of 24KiB and
determining the hash doesn't take longer than a few hundred
micro-seconds on my machine.

2c0c6f4477

DrahtBot commented at 12:22 PM on May 4, 2023: contributor



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



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK kristapsk

Concept ACK fanquake

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.



Conflicts

No conflicts as of last run.
DrahtBot added the label Tests on May 4, 2023
DrahtBot added the label CI failed on May 4, 2023
DrahtBot removed the label CI failed on May 4, 2023
in test/functional/tool_wallet.py:56 in 2c0c6f4477
```
  53 | @@ -54,12 +54,7 @@ def assert_tool_output(self, output, *args):
  54 |          assert_equal(p.poll(), 0)
  55 |  
  56 |      def wallet_shasum(self):
```
kevkevinpal commented at 3:24 AM on June 16, 2023:

not sure if we'd want to change the name of the test aswell to wallet_sha256sum to be more accurate

theStack commented at 9:55 AM on August 2, 2023:

Thanks for reviewing! Decided to keep the name as it is, as sha256 falls into the SHA message digests category (see e.g. https://linux.die.net/man/1/shasum) and for the sake of the test concrete type of hash used doesn't matter anyway. All we care is detecting if the wallet file contents have been changed.
fanquake commented at 9:16 AM on August 2, 2023: member

Concept ACK. Did you want to address the review comment, or just mark as resolved.
kristapsk approved
kristapsk commented at 10:21 AM on August 2, 2023: contributor

ACK 2c0c6f44770403899bd8514ad7343356853bf38c
fanquake merged this on Aug 2, 2023
fanquake closed this on Aug 2, 2023
theStack deleted the branch on Aug 2, 2023
sidhujag referenced this in commit f413961186 on Aug 9, 2023
bitcoin locked this on Aug 1, 2024

Type	Reviewers
ACK	kristapsk
Concept ACK	fanquake

Contributors

Labels

test: dedup file hashing using `sha256sum_file` helper #27572

Reviews

Conflicts