Skip to content

test: dedup file hashing using sha256sum_file helper#27572

Merged
fanquake merged 1 commit intobitcoin:masterfrom
theStack:test-refactor-use_sha256sum_file_helper
Aug 2, 2023
Merged

test: dedup file hashing using sha256sum_file helper#27572
fanquake merged 1 commit intobitcoin:masterfrom
theStack:test-refactor-use_sha256sum_file_helper

Conversation

@theStack
Copy link
Copy Markdown
Contributor

@theStack theStack commented May 4, 2023

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.

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.
@DrahtBot
Copy link
Copy Markdown
Contributor

DrahtBot commented May 4, 2023

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.

@fanquake
Copy link
Copy Markdown
Member

fanquake commented Aug 2, 2023

Concept ACK. Did you want to address the review comment, or just mark as resolved.

Copy link
Copy Markdown
Contributor

@kristapsk kristapsk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 2c0c6f4

@fanquake fanquake merged commit 2dea6c5 into bitcoin:master Aug 2, 2023
@theStack theStack deleted the test-refactor-use_sha256sum_file_helper branch August 2, 2023 10:55
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 9, 2023
@bitcoin bitcoin locked and limited conversation to collaborators Aug 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants