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

Move tests in channelmanager.rs into more appropriate test files #2958

Open
dunxen opened this issue Mar 21, 2024 · 7 comments · May be fixed by #2977
Open

Move tests in channelmanager.rs into more appropriate test files #2958

dunxen opened this issue Mar 21, 2024 · 7 comments · May be fixed by #2977
Labels
good first issue Good for newcomers

Comments

@dunxen
Copy link
Contributor

dunxen commented Mar 21, 2024

The channelmanager.rs file is huuuge³ at the moment and we shouldn't be adding more tests to it. We should move them into more appropriate test files (which may involve creating new _test.rs files too.

This is unlikely to create a ton of git noise as it should just be pure moves of tests so it could probably be done in one PR.

@srikanth-iyengar
Copy link

I would like to work on this issue, is this up for grabs?

@dunxen
Copy link
Contributor Author

dunxen commented Mar 23, 2024

I would like to work on this issue, is this up for grabs?

Hey, thanks! It sure is! Let us know on this issue if you have any other questions while working on this. I'm not sure everything would move into existing test files so you may need to create some new ones that are more relevant but please feel free to ask!

@srikanth-iyengar
Copy link

Yeah sure, will get back here if I face any issue

@srikanth-iyengar
Copy link

srikanth-iyengar commented Mar 26, 2024

Hey @dunxen I have analyzed the channelmanager tests and prepared the following test groups,
lemme know of your thoughts

There are total 22 tests

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

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

@dunxen
Copy link
Contributor Author

dunxen commented Mar 27, 2024

Hey @srikanth-iyengar, those groupings seem to make sense to me from first glance! I think we can get some more comments on the moves int he PR to see it in context.

@srikanth-iyengar
Copy link

srikanth-iyengar commented Mar 28, 2024

I started working on the issue, Currently I found that some of the fields/functions in the channel manager are private , should I make them public like pub(crate)?

@srikanth-iyengar
Copy link

srikanth-iyengar commented Apr 1, 2024

Hey @dunxen I have raised a PR for this issue, Can you take a look ?

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

Successfully merging a pull request may close this issue.

2 participants