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

integration tests: test that lndk forwards onion message #76

Merged
merged 4 commits into from
Nov 14, 2023

Conversation

orbitalturtle
Copy link
Collaborator

@orbitalturtle orbitalturtle commented Oct 3, 2023

This PR:

  • Puts the lndk portion of our integration tests into place, showing that lndk is able to properly forwards onion messages.
  • In order to allow us to save lndk's logs to a file for the integration tests, we also swap simple logger with log4rs.

@orbitalturtle orbitalturtle changed the base branch from tests-makefile to itests-lnd-setup October 3, 2023 04:47
@orbitalturtle orbitalturtle changed the title Test that lndk forwards onion message integration tests: test that lndk forwards onion message Oct 3, 2023
@orbitalturtle orbitalturtle requested a review from carlaKC October 3, 2023 04:52
Copy link
Collaborator

@carlaKC carlaKC left a comment

Choose a reason for hiding this comment

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

Just read through this - looks good! Will wait on #77 to take a proper look.

If you'd like to pull out the refactor + log4rs commits out, we can put those in separately to itest changes.

@orbitalturtle
Copy link
Collaborator Author

@carlaKC, sounds good, so pull those commits into a separate PR?

@carlaKC
Copy link
Collaborator

carlaKC commented Oct 13, 2023

@carlaKC, sounds good, so pull those commits into a separate PR?

Yeah if you want to get them in sooner? But happy either way.

@orbitalturtle orbitalturtle force-pushed the itests-lnd-setup branch 3 times, most recently from 9dd2290 to edee3f2 Compare October 16, 2023 21:08
@orbitalturtle orbitalturtle force-pushed the lndk-itest-part-three branch 2 times, most recently from 399624a to e88f601 Compare November 8, 2023 19:01
@orbitalturtle orbitalturtle changed the base branch from itests-lnd-setup to master November 8, 2023 19:02
@orbitalturtle
Copy link
Collaborator Author

Yeah if you want to get them in sooner? But happy either way.

Cool no rush so I'll just leave it in this one :) Just rebased this on master btw.

Copy link

codecov bot commented Nov 8, 2023

Codecov Report

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

Comparison is base (42eacec) 58.69% compared to head (a46a901) 0.00%.
Report is 14 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #76       +/-   ##
==========================================
- Coverage   58.69%   0.00%   -58.70%     
==========================================
  Files           5       1        -4     
  Lines         983      12      -971     
==========================================
- Hits          577       0      -577     
+ Misses        406      12      -394     
Files Coverage Δ
src/main.rs 0.00% <0.00%> (ø)

... and 4 files with indirect coverage changes

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

let (_bitcoind, mut lnd, ldk1, ldk2, lndk_dir) =
common::setup_test_infrastructure(test_name).await;

// Here we'll produce a little path of two channels. Both ldk nodes are connected to lnd like so:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

reminder for self: this should say "connections" not "channels"

Copy link
Collaborator

@carlaKC carlaKC left a comment

Choose a reason for hiding this comment

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

tACK - everything is working as expected and locally ran the itests. Nice stuff!

Added some nits for it we follow this up, but don't need to be addressed now.

src/lib.rs Outdated
use std::str::FromStr;
use tonic_lnd::lnrpc::GetInfoRequest;

pub async fn run(args: LndCfg) -> Result<(), ()> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

(Pre-existing) I think that we need to do a more general errors followup, so not for this PR but I think that this should return a defined error.

@@ -1,2 +1,2 @@
/target
lndk.conf
lndk.conf*
Copy link
Collaborator

Choose a reason for hiding this comment

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

What do we need this for? Wouldn't *.conf be more helpful?

@@ -204,6 +208,9 @@ impl LndNode {
"--bitcoind.rpchost={:?}",
bitcoind_connect_params.rpc_socket
),
format!("--protocol.custom-message=513"),
format!("--protocol.custom-nodeann=39"),
format!("--protocol.custom-init=39"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: can be " txt ".to_owned() when we hard set values.

@carlaKC
Copy link
Collaborator

carlaKC commented Nov 14, 2023

Note to self that we should add some leniency to codecov, sometimes coverage is going to go down and that's ok.

@carlaKC carlaKC merged commit 3cd3c7a into lndk-org:master Nov 14, 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.

2 participants