assert_greater_than(_or_equal) to assert_gt/assert_ge
#34395
The functional test framework’s assert_greater_than and assert_greater_than_or_equal helper names are verbose, discouraging their use despite providing better error messages for debugging intermittent failures.
They’re also using a different nomenclature for comparison than what we’re used to on C++ side.
Rename them to assert_gt and assert_ge via a scripted diff to align with the C++ BOOST_CHECK_GT/BOOST_CHECK_GE naming.
Discussed in #34328 (review)
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34395.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Concept NACK | dergoegge |
If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.
Reviewers, this pull request conflicts with the following ones:
CBlockPolicyEstimator return sub 1 sat/vb fee rate estimates by ismaelsadeeq)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.
85@@ -86,12 +86,12 @@ def assert_not_equal(thing1, thing2, *, error_message=""):
86
87 def assert_greater_than(thing1, thing2):
88 if thing1 <= thing2:
89- raise AssertionError("%s <= %s" % (str(thing1), str(thing2)))
90+ raise AssertionError(f"{thing1!r} <= {thing2!r}")
please do not tag commits with “refactor”, which change behavior. This is annoying to review (https://github.com/bitcoin/bitcoin/pull/34132#discussion_r2685955268).
Also, it would be good to mention what effect this will have in practise. I guess none, because for raw numerical values there is no difference here and this function isn’t yet used on more involved objects.
This is annoying to review (https://github.com/bitcoin/bitcoin/pull/34132#discussion_r2685955268)
That was an interactive rebase where I added to a pure rename afterwards - which was still arguably a refactor since it didn’t change current behavior. But I have split it into a scripted diff.
please do not tag commits with “refactor”, which change behavior.
Repr is the standard way to debug the values, what makes you say it’s not a refactor? We’re comparing numbers, as you also mentioned, didn’t realize it needed explanation.
But I can also just revert that if it bothers you, I don’t mind.
Repr is the standard way to debug the values, what makes you say it’s not a refactor? We’re comparing numbers, as you also mentioned, didn’t realize it needed explanation.
str() is different from repr() for more involved objects, like datetime.
I just think we should try to follow CONTRIBUTING.md and clearly label, illustrate and explain commits.
Decimals are indeed shown differently, e.g.
0 assert_equal(f"{thing1!r} <= {thing2!r}", ("%s <= %s" % (str(thing1), str(thing2))))
fails with
0 File "/Users/lorinc/CLionProjects/bitcoin/test/functional/test_framework/util.py", line 80, in assert_equal
1 raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
2AssertionError: not(1 <= Decimal('0.9756888059122014') == 1 <= 0.9756888059122014)
i.e. Decimal('0.9756888059122014') vs 0.9756888059122014. I have reverted this change.
NACK
Sorry for the push back, but I think the subjective benefit here does not outweigh the cost on the project
the subjective benefit here does not outweigh the cost on the project
what’s the cost on the project? I’m unifying python testing nomenclature with the C++ one.
The functional test framework's `assert_greater_than*` helpers are verbose and noisy at call sites.
Rename them to `assert_gt` and `assert_ge` to align with the C++ `BOOST_CHECK_GT`/`BOOST_CHECK_GE` naming.
The comparison semantics are unchanged.
-BEGIN VERIFY SCRIPT-
git grep -qE "\b(assert_gt|assert_ge)\b" -- test/functional && echo "Error: Target names already exist" >&2 && exit 1
perl -pi -e "s/\bassert_greater_than_or_equal\b/assert_ge/g" $(git grep -l "\bassert_greater_than_or_equal\b" -- test/functional)
perl -pi -e "s/\bassert_greater_than\b/assert_gt/g" $(git grep -l "\bassert_greater_than\b" -- test/functional)
-END VERIFY SCRIPT-
what’s the cost on the project? I’m unifying python testing nomenclature with the C++ one.
The cost of reviewing this PR (e.g. #34395 (review)), and the rebasing this would cause post merge (7 conflicts atm).
The cost of reviewing this PR
It’s fine if you don’t want to invest time in doing or reviewing refactorings, but not sure why you want to stop others from doing so - most of our work is refactoring and cleanup…