[Test] Add node-level tests with open+payment+close, with many checks #2814
Conversation
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2814 +/- ##
==========================================
+ Coverage 88.49% 90.44% +1.94%
==========================================
Files 114 120 +6
Lines 91935 104556 +12621
Branches 91935 104556 +12621
==========================================
+ Hits 81359 94562 +13203
+ Misses 8097 7598 -499
+ Partials 2479 2396 -83 ☔ View full report in Codecov by Sentry. |
|
Nice. These looks like well-written tests, but its not clear to me exactly what they're testing that isn't already covered. There's a ton of noise in the codecov stats but scrolling through it (https://app.codecov.io/gh/lightningdevkit/rust-lightning/pull/2814/indirect-changes) I don't see any material new coverage at the line-level. |
I don't think this adds any new coverage, this is a very typical use case, and all parts of it should be covered by some tests. |
WalkthroughThe update introduces significant enhancements to the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- lightning/src/ln/functional_tests.rs (3 hunks)
Additional comments: 7
lightning/src/ln/functional_tests.rs (7)
- 38-38: The addition of
ChannelHandshakeConfigto the import list is appropriate for the new functionality being tested. This change aligns with the PR's objective to enhance testing around channel operations.- 52-52: The introduction of the
hexcrate dependency is justified by the need to work with hexadecimal representations in the new test functions, particularly for verifying scripts and transactions. This change supports the PR's goal of comprehensive testing.- 68-98: The functions
create_multisig_redeem_script,create_multisig_output_script, andverify_multisig_output_scriptare well-implemented, focusing on creating and verifying multisig scripts, which are crucial for funding transactions in Lightning Network channels. These additions are in line with the PR's objectives to simulate and verify channel operations comprehensively.- 100-107: The function
get_funding_keycorrectly retrieves the funding key from a node for a given channel, which is essential for verifying funding transactions. This utility function enhances the test suite's ability to validate channel setups accurately.- 111-129: The function
verify_funding_txeffectively locates and verifies the funding transaction output based on the expected value and funding keys. This function is crucial for ensuring the correctness of funding transactions in the test suite, aligning with the PR's goals.- 135-258: The test
test_channel_open_and_closecomprehensively simulates the process of opening and closing a channel, including the creation and verification of funding transactions. This test aligns with the PR's objectives by providing a detailed illustration of channel lifecycle operations. However, ensure that the hardcoded values, such as expected channel IDs and transaction lengths, are derived from a predictable setup to avoid flakiness in tests.- 263-431: The test
test_channel_open_and_paymenteffectively simulates opening a channel, making a payment, and then closing the channel. This test provides a clear, step-by-step demonstration of a common use case in the Lightning Network, aligning with the PR's objectives to enhance the test suite's clarity and comprehensibility. Similar to the previous test, ensure that the hardcoded values and assumptions are based on a stable setup.
|
This isn't a particularly representative example of how tests should be written, however. We have a large pile of utilities which implement the various steps involved here, which we strongly prefer to use where possible as it avoids having to update every test whenever any small thing changes, and allows us to instead simply update the utilities. Thus, I don't particularly think we should be upstreaming more fully-written tests like this unless it increases coverage. |
|
Retracting |
Add some new integration tests, for the common scenarios:
Test features:
ChannelManager)I suggest to include even if there is some level of duplication with other tests. If very similar tests exists, feel free to close.