Conditions had virtually no effect, lets activate them.
Activating them implies either:
assert <condition>, which is terse but skipped in optimized Python executions.assert_equal(True, <condition>), which is overly verbose.Instead we introduce assert_true(<condition>), inspired by Google Test (discussed in #30529 (review)).
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31874.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Concept NACK | maflcko |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, 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.
96@@ -96,9 +97,9 @@ def run_test(self):
97 self.log.info("Check that every participant's multisig generates the same addresses...")
98 for _ in range(10): # we check that the first 10 generated addresses are the same for all participant's multisigs
99 receive_addresses = [multisig.getnewaddress() for multisig in participants["multisigs"]]
100- all(address == receive_addresses[0] for address in receive_addresses)
101+ assert_true(all(address == receive_addresses[0] for address in receive_addresses))
102 change_addresses = [multisig.getrawchangeaddress() for multisig in participants["multisigs"]]
103- all(address == change_addresses[0] for address in change_addresses)
104+ assert_true(all(address == change_addresses[0] for address in change_addresses))
69@@ -69,7 +70,7 @@ def participants_create_multisigs(self, external_xpubs, internal_xpubs):
70 "timestamp": "now",
71 },
72 ])
73- assert all(r["success"] for r in result)
74+ assert_true(all(r["success"] for r in result))
assert all() constructs as well, e.g. https://github.com/bitcoin/bitcoin/blob/master/test/functional/wallet_conflicts.py#L152 ?
Good find, two observations/requests:
*) Can we use assert_equal(all(), True) for consistency?
*) Can we convert the remaining assert all() constructs to whatever we will end up using here?
Edit: a full example: https://github.com/bitcoin/bitcoin/blob/master/test/functional/wallet_encryption.py#L162
Thanks for having a look @l0rinc!
Can we use
assert_equal(all(), True)for consistency?
I prefer assert_true(all()), since the condition may span the full line, so having what we are testing against at the beginning of the line is more readable. One could go Yoda-style and assert_equal(True, all()) to solve that issue, but why not bite the bullet and support assert_true(all())? Other frameworks like GoogleTest have ASSERT_TRUE().
*) Can we convert the remaining assert all() constructs to whatever we will end up using here?
My first preference is to just have assert_true() roll out organically. But I am open to authoring scripted diffs if there is enough support.
git grep "assert_equal.*, True)$" |wc -l => 228
This seems like a reasonable candidate for conversion.
git grep "^ *assert\b" *.py |wc -l => 1592
I’m a bit more reluctant to change all built-in assert usages in a scripted diff as some may be intentionally intended to be internal checks for non-optimized runs. See discussion in other PR: #30529 (review). Maybe I could do a scripted diff for git grep -E "^ *assert (all|any)" *.py |wc -l => 30.
Want to point out that I don’t want to spend too much of our time bikeshedding, I’m okay with switching back to mis-using assert for now and kick this can down the road. In practice it’s very rare that anyone attempts to run tests in optimized mode, evidenced by several tests failing if run that way.
why not bite the bullet and support
I don’t really see what it adds, but won’t block if others are in favor.
Can we separate the two problems?
I mean adding back the assertions in one PR (though I’d vote for fixing all remaining assert (all|any in the same PR that fixes the unasserted ones), and proposing the new assert_true together with a scripted diff that applies it, to see if reviewers (or at least the author) thinks that assert_true is indeed better than assert_equals(True,.
I don’t really see what it adds, but won’t block if others are in favor.
Can we separate the two problems?
Lets see what others think. I think the PR content & size is okay as-is, both fixing a minor issue and demonstrating a slight use-case for assert_true(), but am open to expand it with scripted diffs if others agree. Not too keen on both expanding and splitting it at the same time though.
This follows the style of ASSERT_TRUE() in GoogleTest: https://google.github.io/googletest/reference/assertions.html
The two ones in run_test() were basically no-ops.
68@@ -69,6 +69,14 @@ def summarise_dict_differences(thing1, thing2):
69 d2[k] = thing2[k]
70 return d1, d2
71
72+
73+def assert_true(condition):
74+ """Separate from the `assert` keyword in that it should not be optimized out
75+ when environment var `PYTHONOPTIMIZE=1`, or Python is run with `-O`."""
76+ if not condition:
77+ raise AssertionError
Not sure about adding a new function, because assert may be optimized out. Is there any report of anyone ever setting this? I’d say it is more common to not run the tests at all.
If you think this should be addressed, it would be better to check it at startup and fail early, instead of (starting to ) apply this replacement to all code.
If there was a reason to apply this to all code, it should be enforced in one way or another.
assert to check test conditions (several tests fail already when attempting it).
@ryanofsky expressed seeing value in reserving assert for internal sanity checks in the tests (see linked thread), which inspired me to author this PR in the way I did.
expressed seeing value in reserving
assertfor internal sanity checks in the tests
Is there an example of an internal sanity check? How would that be different from the functional framework unit tests? Also, why would there be any risk in treating an internal test error as a “normal” test error?
I understand the distinction of internal errors in user-facing programs, but for dev-only test-only scripts, the meaning and the tradeoffs become less clear.
Another review note, it seems like assert_true("abc") will pass. This seems confusing and I am also not sure if it is clearer than assert "abc". (Same for integral values like 3, …)
And on a general note, the attempt here seems similar or related to the discussion (and changes) stemming from #23119. However that issue is stale for more than 3 years, with no meaningful progress, so I wonder how important it is and whether it can be closed.
Thanks, added type annotations to assert_true() in latest push. mypy --check-untyped-defs catches type mismatches for it, but not for built-in assert. It also triggers a deluge of other errors though.
The point of #23119 is for assertion failures to output the values that made them fail. While it also entails replacing a subset of instances of built-in assert with our own, the purpose is different.
assert_true(1) will still pass the test.
@hodlinator, could we separate fixing dead code from introducing another assertion helper?
I’d say #29500 is more related to #23119 than this PR.
Btw, type hints in Python are just hints, adding assert_true(1) will still pass the test.
Yes, that’s why I commented about the linter situation above.
@hodlinator, could we separate fixing dead code from introducing another assertion helper?
Opinion noted (again). I won’t repeat myself.
PYTHONOPTIMIZE, due to the outstanding issues mentioned in #31874 (review)