test: Add algo assert to bnb_search_test #29206
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. 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. ConflictsReviewers, 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. |
bd8ddfa to
914f908
Compare
|
hmm it looks like now it is testing |
|
Ah wait I see what happened. If the check for fee_rate is removed from |
|
Thanks @yancyribbens for your report. There are a number of issues with these tests specifically and the Either way, you are looking at hopefully soon obsoleted tests. In conjunction with that bugfix I started working on completely rewriting the |
Ok. The test reads that it expects [9, 1] as a result AFAICT and since the fee_rate is set to be high (fee_rate > long_term_feerate) it should be 10. I verified that the BnB algo is actually doing the correct thing, although it's good to know there are some fixes coming for the tests. |
|
In the test case the |
There was a problem hiding this comment.
Concept NACK
Tests should ensure that the expected outcome is achieved. The expected outcome here is that the coin selection process returns the input set that has the best waste score.
I don’t think it’s useful to require that the best input set was produced by BnB. While in our current implementation the best input set is guaranteed to be found by BnB, it is possible that future coin selection improvements would enable another algorithm to produce the same result. As such, requiring that BnB produced the input set is a layer violation that tests an implementation detail rather than the outcome.
That's fine, however the tests in question are part of the |
|
The presence of BnB ensures that these expected input set will be found. The presence of BnB does neither preclude another algorithm from finding the same solution, nor cause the BnB solution to be necessarily picked among two equivalent solutions produced by two different algorithms. Hence it does not make sense to require that the solution was produced by BnB, only that the best solution will be found. |
I noted earlier that my objection is that this test should go elsewhere then if it's not testing BnB. Also, as I demonstrated in a separate PR, I can remove a valuable part of the waste metric calculation and no test fails. Therefore this PR increases the test coverage by adding that code section to the test suite. |
|
This PR does not seem to have conceptual support. |
Adds algo assertion to bnb_search tests. This also highlights a test bug here. The case where fees are high which should cause bnb to select fewer inputs isn't actually being tested here since the result is making assertion on what's returned by
knapsack, notbnb.