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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Concept ACK | shaavan |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
<!--174a7506f384e20aa4161008e828411d-->
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.
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.
55 | @@ -56,6 +56,7 @@ src/qt/bitcoin-qt.creator.user 56 | src/qt/bitcoin-qt.files 57 | src/qt/bitcoin-qt.includes 58 | 59 | +tags
Reason for this?
This is the tags indexing file that IDEs use for quickly navigating to symbols within the project. It updates as changes happen so it is also not static.
You can add it to your personal git config then. It's not specific to the project.
That is an important distinction. assert and assert_equal will be used where possible.
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_stringwill be used in the case of verifying hex strings. If we eliminate all variants other thanassert_equalandassert_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.
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.
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.
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
63 | +def assert_not_equal(thing1, thing2, *args, err_msg=None): 64 | + if thing1 == thing2 or any(thing1 == arg for arg in args): 65 | + msg = "%s" % " == ".join(str(arg) for arg in (thing1, thing2) + args) 66 | + if err_msg is not None: 67 | + msg = err_msg 68 | + raise AssertionError(msg)
nit: I think you can update code to make it consistent with the assert_equal function.
if err_msg is None:
err_msg = "%s" % " == ".join(str(arg) for arg in (thing1, thing2) + args)
raise AssertionError(err_msg)
This makes sense. More efficient.
Co-authored-by: Shashwat Vangani <shaavan.github@gmail.com>
256 | @@ -256,7 +257,7 @@ def run_test(self): 257 | txid = self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), 1) 258 | raw_tx = self.nodes[0].getrawtransaction(txid, True) 259 | tmpl = self.nodes[0].getblocktemplate({'rules': ['segwit']}) 260 | - assert_greater_than_or_equal(tmpl['sizelimit'], 3999577) # actual maximum size is lower due to minimum mandatory non-witness data 261 | + assert_greater_than_or_equal(tmpl['sizelimit'], 3999577) # actual maximum size is lower due to minimum mandatory non-witness data
this can be removed (useless change)
LGTM. I think you should squash the commits.
<!--cf906140f33d8803c4a75a2196329ecb-->
🐙 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.