Skip to content

Improve test coverage of #2575 router fixes #2625

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

Merged
merged 3 commits into from
Oct 15, 2023

Conversation

tnull
Copy link
Contributor

@tnull tnull commented Sep 29, 2023

In #2575 we added a few fixes to the router but didn't add much coverage for them.

Here, we improve on that by

  • Asserting that full RouteParameters included in Route are equal to the query (which is of course the way to go, duh).
  • Adding a test that we now really prefer first hops over route hints for the same channel
  • Adding a test asserting that we consider route hints even if we ourselves are the entry point

Previously we only asserted the `final_value_msat` matches. Looking at
it again we can _of course_ assert the full equality of looked-for and
included route params after all (duh, not sure what I was thinking...).

This cleans up the prior misunderstanding and fixes a bunch of tests
that would now fail otherwise.
We previously added logic that would avoid adding superflous candidates
for route hints if we detect that we have a first hop for this channel.

Here we add test coverage that we actually prefer the first hop over the
route hint, but still consider the remaining hints.
Previously, we would only consider route hints if the entry point was
in our first hops or in the network graph. We fixed this by also
considering hints if our own node ID was the first src.

Here, we add test coverage for this behavior.
@codecov-commenter
Copy link

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (6016101) 89.03% compared to head (3a8bf89) 89.25%.
Report is 11 commits behind head on main.

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2625      +/-   ##
==========================================
+ Coverage   89.03%   89.25%   +0.22%     
==========================================
  Files         112      112              
  Lines       86351    88081    +1730     
  Branches    86351    88081    +1730     
==========================================
+ Hits        76879    78619    +1740     
+ Misses       7235     7226       -9     
+ Partials     2237     2236       -1     
Files Coverage Δ
lightning/src/ln/payment_tests.rs 98.20% <100.00%> (+<0.01%) ⬆️
lightning/src/routing/router.rs 94.10% <100.00%> (+0.17%) ⬆️
lightning/src/util/test_utils.rs 76.96% <100.00%> (ø)
lightning/src/ln/outbound_payment.rs 88.67% <45.45%> (+0.10%) ⬆️

... and 11 files with indirect coverage changes

☔ 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.

Thanks.

@TheBlueMatt TheBlueMatt merged commit c86cacd into lightningdevkit:main Oct 15, 2023
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.

4 participants