-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
harden cli tests #34844
harden cli tests #34844
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #34844 +/- ##
=========================================
- Coverage 81.8% 81.7% -0.1%
=========================================
Files 825 825
Lines 223269 223269
=========================================
- Hits 182635 182627 -8
- Misses 40634 40642 +8 |
@@ -355,7 +356,7 @@ fn test_transfer_multisession_signing() { | |||
&rpc_client, | |||
&CliConfig::recent_for_tests(), | |||
&offline_fee_payer_signer.pubkey(), | |||
sol_to_lamports(1.0) + 2 * fee, | |||
sol_to_lamports(1.0) + 2 * fee_two_sig, |
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.
is this correct? it says 2 * fee_two_sig
which would be 4?!
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.
it's original test logic :) I only rename the fee
to fee_two_sig
for consistency. It checks 2 * fee_two_sig
at ln 368 below, feels intended.
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.
read the test, it sign a two-signature transaction twice that transfers 42.0
sol. offline_fee_payer_signer
is one of the signer (eg payer) that is initiated with 1 + 2 * fee_two_sig
. After submitted transaction, it's balance should be original_balance - fee_for_two_sig_transaction = (1 + 2 * fee_two_sig) - fee_two_sig = 1 + fee_two_sig
. Which it asserts at Ln#470:
check_balance!(
sol_to_lamports(1.0) + fee_two_sig,
&rpc_client,
&offline_fee_payer_signer.pubkey(),
);
So you are right that offline_fee_payer_signer
does not need to be initialized with 2 *
. But the test is still valid, even a bit of confusing. Happy to straight it out separately.
Problem
TestValidator can be created with custom specified target_lamportds_per_signature
Cli tests uses
with_custom_fees()
but withtarget_lamports_per_signature == 1
. This works accidentally becausecalculate_fee()
currently uses fee_structure's defaultlamports_per_signature
so long fee_rate_governor.lamport_per_signature <> 0.Calculate_fee()
is going to change (eg. be corrected). But in any case, cli tests should use correct value in setting up TestValidator.Summary of Changes
Fixes #