test: Add SyncWithValidationInterfaceQueue to mockscheduler RPC#28118
Conversation
This makes existing tests less brittle, see https://github.com/bitcoin/bitcoin/pull/28108/files#r1268966663
node_context is never null, but if it was, it would lead to a nullptr dereference in node_context->scheduler. Just use EnsureAnyNodeContext everywhere for more robust, consistent, and correct code.
|
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. |
furszy
left a comment
There was a problem hiding this comment.
Little topic, mainly to not mix RPC commands responsibilities.
Why not implementing this on the test framework instead?
Could add a proxy function mockscheduler to test_node.py that calls to the real mockscheduler and, subsequently to syncwithvalidationinterfacequeue.
That just seems brittle, and bloated, for no reason? It is brittle because one may miss to implement a proxy, as they'll have to be done for the bitcoin-cli utility and the python rpc wrapper, and devs can still accidentally sidestep it. It is bloated because it will be more than one line of code. |
I'm not really strong here but It just seems a bit error-prone to always call to the About the tests bloat, it should just be a: def mockscheduler(self, time):
self.__getattr__('mockscheduler')(time)
self.__getattr__('syncwithvalidationinterfacequeue')()But it is just a gut feeling. All good either way. |
No, they shouldn't be affected by this change. The scheduler thread is already free to behave at any time as-if This will only reduce intermittent test failure rates where the test assumed that the scheduler thread behaved as-if |
…ckscheduler RPC fabef12 refactor: Use EnsureAnyNodeContext (MarcoFalke) fa16406 test: Add SyncWithValidationInterfaceQueue to mockscheduler RPC (MarcoFalke) Pull request description: There should be no risk or downside in adding a call to `SyncWithValidationInterfaceQueue` here. In fact, it will make tests less brittle. For example, * If one sets the timeouts in `test/functional/feature_fee_estimation.py` to `0`, on `master` the test will fail and here it will pass. * It may avoid a rare (theoretic) intermittent issue in https://github.com/bitcoin/bitcoin/pull/28108/files#r1268966663 ACKs for top commit: TheCharlatan: ACK fabef12 furszy: Code review ACK fabef12. Convinced by checking all current tests usages. Tree-SHA512: c9e9a536a8721d1b3f267a66b40578b34948892301affdcad121ef8e02bf17037305d0dd53aa94b1b064753e66f9cfb31823b916b707a9d812627f502b818003
There should be no risk or downside in adding a call to
SyncWithValidationInterfaceQueuehere. In fact, it will make tests less brittle. For example,test/functional/feature_fee_estimation.pyto0, onmasterthe test will fail and here it will pass.