Improve test coverage of #2575 router fixes by tnull · Pull Request #2625 · lightningdevkit/rust-lightning

tnull · 2023-09-29T18:26:24Z

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 · 2023-09-29T19:03:38Z

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.

TheBlueMatt

Thanks.

tnull · 2023-09-29T18:26:24Z

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 · 2023-09-29T19:03:38Z

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.

TheBlueMatt

Thanks.

tnull · 2023-09-29T18:26:24Z

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 · 2023-09-29T19:03:38Z

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.

TheBlueMatt

Thanks.

tnull · 2023-09-29T18:26:24Z

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 · 2023-09-29T19:03:38Z

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.

TheBlueMatt

Thanks.

tnull · 2023-09-29T18:26:24Z

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 · 2023-09-29T19:03:38Z

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.

TheBlueMatt

Thanks.

tnull · 2023-09-29T18:26:24Z

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 · 2023-09-29T19:03:38Z

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.

TheBlueMatt

Thanks.

tnull · 2023-09-29T18:26:24Z

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 · 2023-09-29T19:03:38Z

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.

TheBlueMatt

Thanks.

tnull · 2023-09-29T18:26:24Z

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 · 2023-09-29T19:03:38Z

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.

TheBlueMatt

Thanks.

tnull added 3 commits September 29, 2023 16:49

TheBlueMatt approved these changes Oct 15, 2023
View reviewed changes

valentinewallace approved these changes Oct 15, 2023
View reviewed changes

TheBlueMatt merged commit c86cacd into lightningdevkit:mainOct 15, 2023

tnull added 3 commits September 29, 2023 16:49

TheBlueMatt approved these changes Oct 15, 2023
View reviewed changes

valentinewallace approved these changes Oct 15, 2023
View reviewed changes

TheBlueMatt merged commit c86cacd into lightningdevkit:mainOct 15, 2023

tnull added 3 commits September 29, 2023 16:49

TheBlueMatt approved these changes Oct 15, 2023
View reviewed changes

valentinewallace approved these changes Oct 15, 2023
View reviewed changes

TheBlueMatt merged commit c86cacd into lightningdevkit:mainOct 15, 2023

tnull added 3 commits September 29, 2023 16:49

TheBlueMatt approved these changes Oct 15, 2023
View reviewed changes

valentinewallace approved these changes Oct 15, 2023
View reviewed changes

TheBlueMatt merged commit c86cacd into lightningdevkit:mainOct 15, 2023

tnull added 3 commits September 29, 2023 16:49

TheBlueMatt approved these changes Oct 15, 2023
View reviewed changes

valentinewallace approved these changes Oct 15, 2023
View reviewed changes

TheBlueMatt merged commit c86cacd into lightningdevkit:mainOct 15, 2023

tnull added 3 commits September 29, 2023 16:49

TheBlueMatt approved these changes Oct 15, 2023
View reviewed changes

valentinewallace approved these changes Oct 15, 2023
View reviewed changes

TheBlueMatt merged commit c86cacd into lightningdevkit:mainOct 15, 2023

tnull added 3 commits September 29, 2023 16:49

TheBlueMatt approved these changes Oct 15, 2023
View reviewed changes

valentinewallace approved these changes Oct 15, 2023
View reviewed changes

TheBlueMatt merged commit c86cacd into lightningdevkit:mainOct 15, 2023

tnull added 3 commits September 29, 2023 16:49

TheBlueMatt approved these changes Oct 15, 2023
View reviewed changes

valentinewallace approved these changes Oct 15, 2023
View reviewed changes

TheBlueMatt merged commit c86cacd into lightningdevkit:mainOct 15, 2023

Uh oh!

Conversation

tnull commented Sep 29, 2023

Uh oh!

codecov-commenter commented Sep 29, 2023

Codecov Report

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

tnull commented Sep 29, 2023

Uh oh!

codecov-commenter commented Sep 29, 2023

Codecov Report

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

tnull commented Sep 29, 2023

Uh oh!

codecov-commenter commented Sep 29, 2023

Codecov Report

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

tnull commented Sep 29, 2023

Uh oh!

codecov-commenter commented Sep 29, 2023

Codecov Report

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

tnull commented Sep 29, 2023

Uh oh!

codecov-commenter commented Sep 29, 2023

Codecov Report

Uh oh!

TheBlueMatt left a comment