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

LND portion of integration tests #74

Closed

Conversation

orbitalturtle
Copy link
Collaborator

@orbitalturtle orbitalturtle commented Sep 20, 2023

All righty, here's a PR that sets up the LND portion of our integration tests :-]

This PR:

  • Builds LND in a Makefile. Right now it's pretty simple, wanted to at least get the POC running. But we can probably add some things... like feeding the Makefile more granular options to pass to the integration tests, which we can borrow some of LND's approach for
  • Fixes up the tests file structure, so the user can find all relevant files in /tmp/ldk-tests/
  • Adds an LND struct that we can use to run the LND node and interact with the lightning node via the grpc client.
  • Tests that LND + LDK properly connect

Next PR(s):

  • Will add the necessary Github actions for: 1) uploading logs, 2) run the integration tests properly with the new Makefile setup.
  • Will finally add lndk to the mix and add a test showing it's forwarding onion messages properly :)

Note that I borrowed some of the approach here for the Makefile and associated github action tests from LND, so credit to them there :)

@orbitalturtle
Copy link
Collaborator Author

Oh I forgot that changing up the test commands would break the current github actions build/cargo test process. So I'll add an update to the github actions to this PR soon.

@carlaKC
Copy link
Collaborator

carlaKC commented Sep 22, 2023

So I'll add an update to the github actions to this PR soon.

Does this need review in the meantime or should we wait for this?

@orbitalturtle
Copy link
Collaborator Author

@carlaKC It's ready for a review! The build process is working now but I just need to fix the codecov portion now.

@codecov
Copy link

codecov bot commented Sep 22, 2023

Codecov Report

All modified lines are covered by tests ✅

❗ No coverage uploaded for pull request base (ldk-test-setup@3b9539a). Click here to learn what that means.

Additional details and impacted files
@@                Coverage Diff                @@
##             ldk-test-setup      #74   +/-   ##
=================================================
  Coverage                  ?   58.69%           
=================================================
  Files                     ?        5           
  Lines                     ?      983           
  Branches                  ?        0           
=================================================
  Hits                      ?      577           
  Misses                    ?      406           
  Partials                  ?        0           

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

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.

Nice work!

Running into some path issues:

running 2 tests
test test_ldk_lnd_connect ... thread 'test_ldk_lnd_connect' panicked at 'Failed to execute lnd command: Os { code: 2, kind: NotFound, message: "No such file or directory" }', tests/common/mod.rs:206:14
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
FAILED
test test_ldk_send_onion_message ... thread 'test_ldk_send_onion_message' panicked at 'Failed to execute lnd command: Os { code: 2, kind: NotFound, message: "No such file or directory" }', tests/common/mod.rs:206:14
FAILED

failures:

failures:
    test_ldk_lnd_connect
    test_ldk_send_onion_message

test result: FAILED. 0 passed; 2 failed; 0 ignored; 0 measured; 0 filtered out; finished in 2.25s

error: test failed, to rerun pass `--test integration_tests`

go.mod Outdated Show resolved Hide resolved
@@ -0,0 +1,10 @@
# This Makefile is used to compile an LND binary for the integration tests.

GO_BUILD := go build
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will fail if golang isn't installed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

True, should we just add instructions explaining that Go is a dependency?

@@ -48,7 +62,7 @@ jobs:
- name: Install cargo-llvm-cov
uses: taiki-e/install-action@cargo-llvm-cov
- name: Generate code coverage
run: cargo llvm-cov --all-features --workspace --lcov --output-path lcov.info
run: cargo llvm-cov --bin lndk --workspace --lcov --output-path lcov.info
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you explain what this does?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

With the --all-features flag codecov includes the integration tests in the coverage, but I think we only want to be looking at the unit tests. So --bin lndk specifies that we only want to be looking at the tests in /src for the lndk binary. Would it be helpful to add a note about this in the commit?

.github/workflows/main.yml Show resolved Hide resolved
Makefile Outdated
itest:
@$(call print, "Building lnd for itests.")
$(GO_BUILD) -tags="peersrpc signrpc dev" -o /tmp/lndk-tests/bin/lnd-itest$(EXEC_SUFFIX) $(LND_PKG)/cmd/lnd
$(CARGO_TEST) -- --test-threads=1 --nocapture
Copy link
Collaborator

Choose a reason for hiding this comment

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

Am I correct in thinking that this will also run the unit tests? That doesn't really make sense to me.

Can we use cargo test --test {itest module} to only run the itests? Likewise, we should have a way to only run unit tests.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point, yes I'll update this

Copy link
Collaborator Author

@orbitalturtle orbitalturtle Sep 28, 2023

Choose a reason for hiding this comment

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

Btw, I wrote up some instructions in CONTRIBUTING to specify how to run integration tests vs. unit tests

tests/integration_tests.rs Outdated Show resolved Hide resolved
tests/common/mod.rs Outdated Show resolved Hide resolved
@orbitalturtle
Copy link
Collaborator Author

orbitalturtle commented Sep 28, 2023

@carlaKC Thanks for the review! For the file not existing error, is this when running make itest?

@carlaKC
Copy link
Collaborator

carlaKC commented Sep 29, 2023

@carlaKC Thanks for the review! For the file not existing error, is this when running make itest?

Yeah, on make itest :(

@orbitalturtle
Copy link
Collaborator Author

@carlaKC got it! Just pushed up some changes. Let me know if the addition to the Makefile helps with the path error... since I don't have Mac I'm unable to test but it looks like the problem was that on MacOS the path in the Makefile was not aligning with what temp_dir() returns in the code https://doc.rust-lang.org/nightly/std/env/fn.temp_dir.html

@orbitalturtle
Copy link
Collaborator Author

D'oh, I messed something up when trying to rebase integration tests part 3 on top of this PR. But we can continue our conversation in #77

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