-
Notifications
You must be signed in to change notification settings - Fork 17
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 select_coin equals knapsack #42
base: main
Are you sure you want to change the base?
Test select_coin equals knapsack #42
Conversation
src/lib.rs
Outdated
let target_value = 5000; | ||
|
||
let options = CoinSelectionOpt { | ||
target_value, |
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.
because the input values are static, it doesn't serve any purpose to declare a separate variable target_value outside the struct.
@Musab1258 Please consider the comment above. Also perform fmt and clippy before committing. Otherwise the test looks good. |
@jkciw , I did fmt and clippy when I made my first commit and I also did it now but it is still failing. I think the problem lies with the Github Actions. |
You must also assert that the result from coin_select() is not the same as results from fifo(), bnb(), srd() |
let options = CoinSelectionOpt { | ||
target_value, | ||
target_value: 5000, | ||
target_feerate: 1.0, | ||
min_absolute_fee: 0, | ||
base_weight: 1, |
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.
select_coin() implements a copy trait. It is not necessary to clone 'options'. Passing the variable will copy it. This will clear your clippy error.
src/lib.rs
Outdated
}; | ||
|
||
let selection_result = select_coin(&inputs, options).unwrap(); | ||
let knapsack_result = select_coin_knapsack(&inputs, options).unwrap(); |
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.
Don't call the knapsack fn.
You have to deterministically choose a result with justification in comments
src/lib.rs
Outdated
let bnb_result = select_coin_bnb(&inputs, options).unwrap(); | ||
let srd_result = select_coin_srd(&inputs, options).unwrap(); | ||
|
||
// Ensure that knapsack result is not the same as other algorithms |
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.
No need for these asserts.
They might fail
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #42 +/- ##
=======================================
Coverage ? 93.42%
=======================================
Files ? 2
Lines ? 669
Branches ? 0
=======================================
Hits ? 625
Misses ? 44
Partials ? 0 ☔ View full report in Codecov by Sentry. |
Hi @delcin-raj, I have pulled from the main branch to resolve the clippy issues. |
I have solved case 3 of the issue #34