-
Notifications
You must be signed in to change notification settings - Fork 35.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test: various USDT functional test cleanups (27831 follow-ups) #27944
Conversation
Since we already store all the blocks in `events`, keeping an additional counter is redundant.
Makes it easier to recognize this variable represents a flag.
Carve out the comparison logic into a helper function to avoid code duplication.
By storing the events instead of doing the comparison inside the handle_utxocache_* functions, we simplify the overall logic and potentially making debugging easier, by allowing pdb to access the events. Mostly a refactor, but changes logging behaviour slightly by not raising and not calling self.log.exception("Assertion failed")
Since we're only mutating, and not reassigning, we don't need to declare `events` as `nonlocal`.
Even though we expect these functions to only produce one event, we still keep a counter to check if that's true. By simply storing all the events, we can remove the counters and make debugging easier, by allowing pdb to access the events.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
Thanks for picking this up! Concept ACK. I'll have a closer look. |
cc @virtu |
Concept ACK. Will take a closer look and test next week. |
ACK 9f55773. Reviewed the code and ran the USDT interface tests. I stepped through the commits and think all changes are reasonable. |
…31 follow-ups) 9f55773 test: refactor: usdt_mempool: store all events (stickies-v) bc43270 test: refactor: remove unnecessary nonlocal (stickies-v) 326db63 test: log sanity check assertion failures (stickies-v) f5525ad test: store utxocache events (stickies-v) f1b99ac test: refactor: deduplicate handle_utxocache_* logic (stickies-v) ad90ba3 test: refactor: rename inbound to is_inbound (stickies-v) afc0224 test: refactor: remove unnecessary blocks_checked counter (stickies-v) Pull request description: Various cleanups to the USDT functional tests, largely (but not exclusively) follow-ups to bitcoin#27831 (review). Except for slightly different logging behaviour in "test: store utxocache events" and "test: log sanity check assertion failures", this is a refactor PR, removing unnecessary code and (imo) making it more readable and maintainable. The rationale for each change is in the corresponding commit message. Note: except for "test: store utxocache events" (which relies on its parent, and I separated into two commits because we may want the parent but not the child), all commits are stand-alone and I'm okay with dropping one/multiple commits if they turn out to be controversial or undesired. ACKs for top commit: 0xB10C: ACK 9f55773. Reviewed the code and ran the USDT interface tests. I stepped through the commits and think all changes are reasonable. Tree-SHA512: 6c37a0265b6c26d4f9552a056a690b8f86f7304bd33b4419febd8b17369cf6af799cb87c16df35d0c2a1b839ad31de24661d4384eafa88816c2051c522fd3bf5
Various cleanups to the USDT functional tests, largely (but not exclusively) follow-ups to #27831 (review). Except for slightly different logging behaviour in "test: store utxocache events" and "test: log sanity check assertion failures", this is a refactor PR, removing unnecessary code and (imo) making it more readable and maintainable.
The rationale for each change is in the corresponding commit message.
Note: except for "test: store utxocache events" (which relies on its parent, and I separated into two commits because we may want the parent but not the child), all commits are stand-alone and I'm okay with dropping one/multiple commits if they turn out to be controversial or undesired.