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

itest: add integration tests for integrated and remote modes #294

Merged
merged 9 commits into from
Feb 11, 2022

Conversation

guggero
Copy link
Member

@guggero guggero commented Dec 14, 2021

This PR adds a full integration test suite for testing the different proxy and authentication modes of the built-in proxy.

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

Great set of initial test coverage!

This'll enable us to more easily test the final bundle to make sure we haven't introduced any breaking changes across the set of daemons we package, in both the remote and integrated mode.

Did an initial pass, main question is w.r.t the first few commits: is there anything fundamental preventing us from just using the lnd/lntest package as is and not copying over all the structs? I ask as there're a few updates to the lntest package itself queued up as lnd PRs, and ideally we can just update our commit hash here vs needing to manually cherry-pick the set of changes.

itest/litd_node.go Outdated Show resolved Hide resolved
itest/litd_node.go Outdated Show resolved Hide resolved
itest/litd_node.go Show resolved Hide resolved
itest/litd_mode_integrated_test.go Outdated Show resolved Hide resolved
itest/litd_node.go Show resolved Hide resolved
itest/server_harness.go Show resolved Hide resolved
itest/litd_mode_integrated_test.go Outdated Show resolved Hide resolved
itest/litd_mode_integrated_test.go Show resolved Hide resolved
subserver_permissions.go Show resolved Hide resolved
itest/litd_node.go Show resolved Hide resolved
Copy link
Member

@ellemouton ellemouton left a comment

Choose a reason for hiding this comment

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

tAck 🚀 Awesome auth coverage!

One thing i noticed was that make itest-only only works if make build-itest was run previously (ie the btcd-itest, litd-itest & lnd-itest binaries have been built already). So maybe building these binaries should be extracted into their own make file func so that both those methods can call it?

@guggero
Copy link
Member Author

guggero commented Dec 16, 2021

Thanks for the review and feedback. I'm going to look into back-porting some of the custom stuff we need to make a harness for litd. Not sure if making the lnd harness more generic is worth the effort vs. having a copy of some of the code.

@Roasbeef
Copy link
Member

Roasbeef commented Jan 8, 2022

Is this ready for another round of review? Want to make sure we're able to have proper itest coverage for the set of pending PRs that modify authentication.

@guggero
Copy link
Member Author

guggero commented Jan 11, 2022

Yes, it's ready for another round. But merging is now blocked by the release of lnd v0.14.2-beta because we need to include lightningnetwork/lnd#6139 and don't want to bump lnd to a non-tagged version.

@lightninglabs-deploy
Copy link

@guggero, remember to re-request review from reviewers when ready

@guggero
Copy link
Member Author

guggero commented Feb 4, 2022

I refactored this PR to re-use some of the structs from lntest. The rest is too lnd specific to be re-used without a huuuge refactor in the lnd repo.

@guggero guggero force-pushed the itest-harness branch 2 times, most recently from 473ed75 to e5d5555 Compare February 4, 2022 12:43
Copy link
Member

@ellemouton ellemouton left a comment

Choose a reason for hiding this comment

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

tACK 🚀 although , my comment from before remain. Cant run the tests with make itest-only without first having run make itest which does the whole app build first. This is because make itest doesnt build the required btcd & lnd binaries by itself. Maybe it should?

@guggero
Copy link
Member Author

guggero commented Feb 7, 2022

tACK rocket although , my comment from before remain. Cant run the tests with make itest-only without first having run make itest which does the whole app build first. This is because make itest doesnt build the required btcd & lnd binaries by itself. Maybe it should?

The itest-only goal is only meant as a shortcut if you changed something in the tests only and don't want to re-compile the whole UI and dependencies. You need to run make itest initially, the same way we do it in lnd.

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 🐡

@Roasbeef Roasbeef merged commit 5cf26cb into master Feb 11, 2022
@guggero guggero deleted the itest-harness branch February 12, 2022 18:39
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