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
qa: Add multiwallet prefix test #11743
Conversation
Tested ACK fa61c6f |
Could rename |
? |
fa61c6f qa: Add multiwallet prefix test (MarcoFalke) Pull request description: Fixes #10849 (comment) Tree-SHA512: 7967be04e76d935398b3bea60c864ffc9e38dbb4cfb55890bb146a6f16c28d81ca5d89736275e2d0b03642806f6f7093beeea979f5257c464f437c4e5a9684f1
Come on, I think I made a valid point, and if it wasn't clear at least give a chance to explain. Beside it only had one ACK. Test code should follow the same review rules as the other right? My point is that renaming w3 to w would be enough, or am I wrong? It is slower in the sense that at least more RPC calls are (unnecessarily) made. |
Yes, you are right that renaming w3 to w achieves the goal in a different way. However, I don't think we should be counting the number of rpc calls, when optimizing the functional test suite for speed. Especially, since some calls like I'd understand your concern in case, let's say, we called Re: Review on test pulls. |
@MarcoFalke really appreciate the reply. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what this is testing. What exactly is a "length extension bug" in this context? Either of the changes discussed seems fine but I think should definitely be accompanied by a comment explaining precisely what bug (or type of bug) the "w" test case is intended to catch so coverage is not lost in the future.
Note that #11687 also makes significant changes to this test and renames some of the wallets.
It tests that a RPC to wallet w doesn't hit wallet w1 for instance. |
@ryanofsky As I understand, the potential bug is that a call to "w" matches any of "w1", "w2", ... , because the one wallets' name is a prefix of another wallet. Alternatively, a call to wallet "w3" will instead call to "w", because or logic might only compare up to the length of the shortest wallet file name. Overall this should be testing our "endpoint-to-wallet" method. |
Oops, didn't realize this was already merged. Guess I will add the comment myself in #11687. |
@ryanofsky Jup, thanks for doing that. I should have done it in this pull. |
fa61c6f qa: Add multiwallet prefix test (MarcoFalke) Pull request description: Fixes bitcoin#10849 (comment) Tree-SHA512: 7967be04e76d935398b3bea60c864ffc9e38dbb4cfb55890bb146a6f16c28d81ca5d89736275e2d0b03642806f6f7093beeea979f5257c464f437c4e5a9684f1
fa61c6f qa: Add multiwallet prefix test (MarcoFalke) Pull request description: Fixes bitcoin#10849 (comment) Tree-SHA512: 7967be04e76d935398b3bea60c864ffc9e38dbb4cfb55890bb146a6f16c28d81ca5d89736275e2d0b03642806f6f7093beeea979f5257c464f437c4e5a9684f1
fa61c6f qa: Add multiwallet prefix test (MarcoFalke) Pull request description: Fixes bitcoin#10849 (comment) Tree-SHA512: 7967be04e76d935398b3bea60c864ffc9e38dbb4cfb55890bb146a6f16c28d81ca5d89736275e2d0b03642806f6f7093beeea979f5257c464f437c4e5a9684f1
fa61c6f qa: Add multiwallet prefix test (MarcoFalke) Pull request description: Fixes bitcoin#10849 (comment) Tree-SHA512: 7967be04e76d935398b3bea60c864ffc9e38dbb4cfb55890bb146a6f16c28d81ca5d89736275e2d0b03642806f6f7093beeea979f5257c464f437c4e5a9684f1
fa61c6f qa: Add multiwallet prefix test (MarcoFalke) Pull request description: Fixes bitcoin#10849 (comment) Tree-SHA512: 7967be04e76d935398b3bea60c864ffc9e38dbb4cfb55890bb146a6f16c28d81ca5d89736275e2d0b03642806f6f7093beeea979f5257c464f437c4e5a9684f1
fa61c6f qa: Add multiwallet prefix test (MarcoFalke) Pull request description: Fixes bitcoin#10849 (comment) Tree-SHA512: 7967be04e76d935398b3bea60c864ffc9e38dbb4cfb55890bb146a6f16c28d81ca5d89736275e2d0b03642806f6f7093beeea979f5257c464f437c4e5a9684f1
fa61c6f qa: Add multiwallet prefix test (MarcoFalke) Pull request description: Fixes bitcoin#10849 (comment) Tree-SHA512: 7967be04e76d935398b3bea60c864ffc9e38dbb4cfb55890bb146a6f16c28d81ca5d89736275e2d0b03642806f6f7093beeea979f5257c464f437c4e5a9684f1
Fixes #10849 (comment)