test: Use consistent assert functions #26356
Conversation
NorrinRadd
changed the title
NorrinRadd
changed the title
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
shaavan
left a comment
shaavan
left a comment
There was a problem hiding this comment.
Concept ACK on using the consistent assert function.
However, I am skeptical of this statement.
Other variations of assert will be removed.
There are many variations of assert in test/test_framework/util.py, and all serve different purposes. For example, assert_is_hex_string will be used in the case of verifying hex strings.
If we eliminate all variants other than assert_equal and assert_not_equal, it will increase the verbosity of the assert code on the usage side, making maintaining and debugging code a challenge.
IMO, I would rather have a bunch of variations of assert statements based on a different case, maintained in the same file, and used consistently throughout the codebase than have only a minimal amount of functions implemented and use them to write custom code on the usage side.
|
That is an important distinction. assert and assert_equal will be used where possible.
Yes, that's an important point. I'll update the summary accordingly and also revert some of the removals of assert_greater_than, as I can see in previous PRs that function was acceptable. |
NorrinRadd
force-pushed
the
issue/23119/part-1
branch
from
ce71207 to
3630f3a
Compare
Updated a subset of the test cases to get approach feedback. assert_equal is preferred over assert where possible. A matching assert_not_equal function has been added also. Where assert_equal is not possible, the built-in assert function will be used.
NorrinRadd
force-pushed
the
issue/23119/part-1
branch
from
3630f3a to
6534222
Compare
|
Any more issues? Is this good to merge? Once accepted I can make these changes to the other TC files and hopefully close out #23119 |
Co-authored-by: Shashwat Vangani <[email protected]>
aureleoules
left a comment
aureleoules
left a comment
There was a problem hiding this comment.
LGTM. I think you should squash the commits.
| assert_greater_than_or_equal(tmpl['sizelimit'], 3999577) # actual maximum size is lower due to minimum mandatory non-witness data | ||
| assert_greater_than_or_equal(tmpl['sizelimit'], 3999577) # actual maximum size is lower due to minimum mandatory non-witness data |
There was a problem hiding this comment.
this can be removed (useless change)
|
🐙 This pull request conflicts with the target branch and needs rebase. |
|
Moved to draft for now. Seems like there could be agreement on making this change, but it needs rebasing (2 months), changes need squashing, needs review comments addressing. |
|
Closing for now. Looks like the user has also disspeared off GitHub. |
7 participants
ref #23119: Updated a subset of the test cases to get approach feedback before making a large amount of changes that may need to change again.
Approach:
Rationale:
This has been discussed in a few PRs / Issue. The most recent consensus is that a minimal approach is best.