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

tests: move test lnd logs to a more permanent location #65

Closed
wants to merge 3 commits into from

Conversation

orbitalturtle
Copy link
Collaborator

@orbitalturtle orbitalturtle commented Jun 5, 2023

Here's a first stab at drafting a solution for storing logs, as discussed with @carlaKC in #58 . The code changes are pretty small, but a couple notes below on implementation, including a change to LDK that we need to do to get this fully working.

A couple of notes on implementation:

  • I went with storing the LND logs in a custom /tmp directory for each test. The motivation being that this keeps the logs around after the test so developers can use it for debugging, but since the logs likely won't be needed for long, it's nice to have them stored in the /tmp file so that they're automatically deleted by the OS after a certain amount of time/when the device restarts.
  • It's possible to specify a custom path for LND logs. But this is not supported by LDK quite yet. Should be easy to add support, I'm pinging Elias from LDK about doing a quick PR for this, if that sounds good. Then we can finish up the LDK side of things.

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.

Seems reasonable! We're also going to want these logs uploaded as an artifacts in GH actions so that we can see what's failing (generally slower CI machines tend to flake a lot more so local copies often aren't enough). Can we look into that here as well?

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

@carlaKC ok I added the github action for uploading the logs :) I also now included the ldk logs since this PR for setting the ldk logs elsewhere is now merged lightningdevkit/ldk-node#129

Copy link
Collaborator

@dunxen dunxen left a comment

Choose a reason for hiding this comment

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

This looks good.

The comment is optional if you'd like some other ldk-node fixes in but don't they actually affect us.

electrsd = { version = "0.22.0", features = ["legacy", "esplora_a33e97e1", "bitcoind_23_0"] }
flate2 = "1.0.25"
ldk-node = "0.1.0"
ldk-node = { git = "https://github.com/lightningdevkit/ldk-node", rev = "3a31209" }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's quickly get in some of the other bug fixes :)

Suggested change
ldk-node = { git = "https://github.com/lightningdevkit/ldk-node", rev = "3a31209" }
ldk-node = { git = "https://github.com/lightningdevkit/ldk-node", rev = "77acd3b" }

@carlaKC
Copy link
Collaborator

carlaKC commented Sep 20, 2023

Replaced by #70

@carlaKC carlaKC closed this Sep 20, 2023
@orbitalturtle orbitalturtle deleted the int-test-logs branch October 3, 2023 04:38
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.

3 participants