test: Use assert_greater_than_or_equal helper method to aid debugging. #17794
Conversation
There was a problem hiding this comment.
The PR description appears to be copied from the third paragraph here: https://docs.python.org/3/reference/expressions.html#comparisons. It states that it can be done but afaik makes no reference to being a best practice. I don't see it mentioned in https://www.python.org/dev/peps/pep-0008/.
I'm -0 on the need for this style change and -0.9 on its readability.
|
Tests should use the |
…ert to aid debugging in case of failure. Comparisons can be chained arbitrarily, e.g., x < y <= z is equivalent to x < y and y <= z, except that y is evaluated only once (but in both cases z is not evaluated at all when x < y is found to be false).
44678a6 to
b6f17a3
Compare
|
Would doing the same in feature_block.py, address.py, test_framework.py, util.py, wallet_create_tx.py also make sense? They all have some form of |
|
I'm -0.1 on this change. Perhaps if done as a scripted diff that converts the rest of the test suite like @jimmysong rightly mentions, and even then... refactoring existing test assertions costs the project in limited review time, the possibility of introducing regressions, code churn, and obscuring the git blame / git history (for little gain). |
|
Also -0 on this. This seems like something that can just be added the next time a |
|
Concept ACK. I think it makes sense to have more verbose error messages. This will help debug issues in failed test runs, where the datadir and debug log has been discarded (e.g. travis) |
|
What is the status of this change? @MarcoFalke you've concept ACK'd. Is there anything more that needs to be done prior to merge? Do the changes need to be more extensive across our test code? Do we need to update some developer docs etc? |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. 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. |
|
Not sure why you marked this "up for grabs". There is nothing to be picked up, as the changes are correct as-is. |
|
It's up for grabs in the sense that, if someone wants to pick this up, and try move this forward, by making a better argument for this/a similar change, they can do. If this is correct, and should be done, we should just reopen this now and merge it immediately. |
|
I don't think anyone can come up with a better argument than the one I gave. (It aids debugging in case of failure) I didn't merge it because it has no ACK and several |
That's why after being open for nearly 3 years, I've decided to close it. I think we need to draw some sort of line in the sand in regards to how long PRs just "sit around", or do some sort of cleanup. Otherwise I think we're going to infinitely accumulate PRs like this; straightforward (code-wise) changes, which we can't get (conceptual) contributor agreement, or review on. This problem is somewhat self-reinforcing in that as the backlog grows, it becomes even harder to find (active/maintained/reviewable) changes. |
|
Then I don't see how someone picking this up/reopening this pull does help in any way |
|
At a minimum, it would help by moving it back to the top of the PR "stack", and all currently active contributors would see it. |
Comparisons can be chained arbitrarily, e.g., x < y <= z is equivalent to x < y and y <= z, except that y is evaluated only once (but in both cases z is not evaluated at all when x < y is found to be false).