Skip to content
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

Refactor channelmanager tests into more appropriate submodule tests #2977

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

srikanth-iyengar
Copy link

@srikanth-iyengar srikanth-iyengar commented Mar 29, 2024

Fix #2958

Previously channelmanager had a single test module withint channelmanager.rs
This changes introduces moving refactoring 23 tests of channelmanager into following test groups

Group 1: Key Send and Payment Verification

  1. test_keysend_dup_hash_partial_mpp
  2. test_keysend_dup_payment_hash
  3. test_keysend_hash_mismatch
  4. test_keysend_msg_with_secret_err
  5. bad_inbound_payment_hash
  6. reject_excessively_underpaying_htlcs
  7. test_final_incorrect_cltv
  8. test_payment_display
  9. test_malformed_forward_htlcs_ser

Group 2: Channel Management and Limits

  1. test_notify_limits
  2. test_drop_disconnected_peers_when_removing_channels
  3. test_outpoint_to_peer_coverage
  4. test_api_calls_with_unkown_counterparty_node
  5. test_api_calls_with_unavailable_channel
  6. test_connection_limiting
  7. test_outbound_chans_unlimited
  8. test_0conf_limiting
  9. test_trigger_lnd_force_close
  10. test_multi_hop_missing_secret

Group 3: Anchor Channel and Configuration Tests

  1. test_inbound_anchors_manual_acceptance
  2. test_anchors_zero_fee_htlc_tx_fallback
  3. test_update_channel_config

@codecov-commenter
Copy link

codecov-commenter commented Mar 29, 2024

Codecov Report

Attention: Patch coverage is 97.89207% with 25 lines in your changes are missing coverage. Please review.

Project coverage is 89.09%. Comparing base (3a9fe20) to head (77fc367).
Report is 89 commits behind head on main.

Files Patch % Lines
lightning/src/ln/channelmanager_limits_tests.rs 97.64% 9 Missing and 4 partials ⚠️
...tning/src/ln/anchor_channel_configuration_tests.rs 93.49% 8 Missing ⚠️
lightning/src/ln/keysend_payments_tests.rs 99.21% 4 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2977      +/-   ##
==========================================
- Coverage   89.33%   89.09%   -0.24%     
==========================================
  Files         117      121       +4     
  Lines       95618    97509    +1891     
  Branches    95618    97509    +1891     
==========================================
+ Hits        85417    86879    +1462     
- Misses       7952     8383     +431     
+ Partials     2249     2247       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that you'll probably have to rebase this after #2731 adds a new test.

lightning/src/ln/anchor_channel_configuration_tests.rs Outdated Show resolved Hide resolved
@srikanth-iyengar
Copy link
Author

Note that you'll probably have to rebase this after #2731 adds a new test.

Will take care of that for sure 👍

@srikanth-iyengar
Copy link
Author

@TheBlueMatt updated changes from main after the merge of #2731 Took care of added test

* Few tests were dependent on channelmanagers internal state, refactored
  that them to publicly visible apis
Comment on lines +382 to +402
#[test]
fn bad_inbound_payment_hash() {
// Add coverage for checking that a user-provided payment hash matches the payment secret.
let chanmon_cfgs = create_chanmon_cfgs(2);
let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
node_cfgs[0].keys_manager.get_inbound_payment_key_material();

let highest_seen_timestamp = bitcoin::blockdata::constants::genesis_block(bitcoin::Network::Testnet).header.time;
let node_signer = node_cfgs[0].keys_manager;
let inbound_pmt_key_material = node_signer.get_inbound_payment_key_material();
let expanded_inbound_key = inbound_payment::ExpandedKey::new(&inbound_pmt_key_material);

let (_, payment_hash, payment_secret) = get_payment_preimage_hash!(&nodes[0]);
let payment_data = msgs::FinalOnionHopData {
payment_secret,
total_msat: 100_000,
};


Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TheBlueMatt I have refactored this test, can you go through this whether this behaves as expected ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically I tried to untie the dependency between the channelmanagers internal state, let me know if there is better way

Comment on lines +678 to +682
// Should we drop this check ?
// Assert that ChannelUpdate message has been added to node[0] pending broadcast messages
// let pending_broadcast_messages = nodes[0].node.get_and_clear_pending_msg_events();
// assert_eq!(pending_broadcast_messages.len(), 2);
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TheBlueMatt I had to remove this check, because this effects the at the 703-704 and fails
I am not able to figure out the reason why this is happening

@tnull
Copy link
Contributor

tnull commented May 14, 2024

This might need a rebase by now - sorry!

@srikanth-iyengar
Copy link
Author

srikanth-iyengar commented May 14, 2024

This might need a rebase by now - sorry!

Thanks, will rebase with main.
do we have some new tests in channelmanager ? Asking so that I don't accidentally remove the tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move tests in channelmanager.rs into more appropriate test files
4 participants