test: Use assert_greater_than_or_equal helper method to aid debugging. #17794

jbampton commented at 7:04 PM on December 23, 2019: contributor

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).

fanquake added the label Tests on Dec 23, 2019

jbampton renamed this:
~~Use best practice for Python chained comparisions.~~
tests: Use best practice for Python chained comparisions.
on Dec 23, 2019

jonatack commented at 7:22 PM on December 23, 2019: contributor

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.

fanquake requested review from MarcoFalke on Dec 28, 2019

MarcoFalke commented at 10:59 AM on December 29, 2019: member

Tests should use the assert_* helper methods instead of a plain assert to aid debugging in case of failure. E.g. assert_greater_than

Use assert_greater_than_or_equal helper method instead of a plain assert 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).

b6f17a3854

jbampton force-pushed on Dec 29, 2019

jbampton renamed this:
~~tests: Use best practice for Python chained comparisions.~~
test: Use assert_greater_than_or_equal helper method to aid debugging.
on Dec 29, 2019

jimmysong commented at 3:50 PM on January 1, 2020: contributor

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 assert <= which should also be converted if this is the best practice.

jonatack commented at 6:31 PM on January 2, 2020: contributor

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).

achow101 commented at 8:02 PM on June 12, 2020: member

Also -0 on this. This seems like something that can just be added the next time a assert >= is needed in a test.

MarcoFalke commented at 8:45 PM on June 12, 2020: member

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)

fanquake commented at 6:25 AM on April 8, 2021: member

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?

DrahtBot commented at 12:45 AM on October 23, 2021: contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#23127 (tests: Use test framework utils where possible by vincenzopalazzo)

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.

fanquake added the label Up for grabs on Oct 3, 2022

fanquake closed this on Oct 3, 2022

MarcoFalke commented at 12:28 PM on October 3, 2022: member

Not sure why you marked this "up for grabs". There is nothing to be picked up, as the changes are correct as-is.

fanquake commented at 12:31 PM on October 3, 2022: member

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.

MarcoFalke commented at 12:35 PM on October 3, 2022: member

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 -0 comments.

fanquake commented at 12:43 PM on October 3, 2022: member

because it has no ACK and several -0 comments.

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.

MarcoFalke removed the label Up for grabs on Oct 3, 2022

MarcoFalke commented at 12:49 PM on October 3, 2022: member

Then I don't see how someone picking this up/reopening this pull does help in any way

fanquake commented at 12:51 PM on October 3, 2022: member

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.

bitcoin locked this on Oct 3, 2023