Skip to content

Add e2e config startup tests#142

Open
benthecarman wants to merge 1 commit into
lightningdevkit:mainfrom
benthecarman:config-tests
Open

Add e2e config startup tests#142
benthecarman wants to merge 1 commit into
lightningdevkit:mainfrom
benthecarman:config-tests

Conversation

@benthecarman

@benthecarman benthecarman commented Mar 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Continuation of #128 / #113

Add config test suite that verifies server startup with various config combinations (to prevent things like #129) and validates errors for invalid configs.

@ldk-reviews-bot

ldk-reviews-bot commented Mar 3, 2026 •
edited
Loading

Copy link
Copy Markdown

👋 Thanks for assigning @joostjager as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@ldk-reviews-bot ldk-reviews-bot requested a review from jkczyz March 3, 2026 23:25
@benthecarman benthecarman self-assigned this Mar 5, 2026
@ldk-reviews-bot

ldk-reviews-bot commented Mar 5, 2026

Copy link
Copy Markdown

🔔 1st Reminder

Hey @jkczyz! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot

ldk-reviews-bot commented Mar 9, 2026

Copy link
Copy Markdown

🔔 2nd Reminder

Hey @jkczyz! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot

ldk-reviews-bot commented Mar 11, 2026

Copy link
Copy Markdown

🔔 3rd Reminder

Hey @jkczyz! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot

ldk-reviews-bot commented Mar 14, 2026

Copy link
Copy Markdown

🔔 4th Reminder

Hey @jkczyz! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@benthecarman benthecarman force-pushed the config-tests branch 2 times, most recently from 1859bb4 to 957057f Compare March 15, 2026 03:56
@ldk-reviews-bot

ldk-reviews-bot commented Mar 16, 2026

Copy link
Copy Markdown

🔔 5th Reminder

Hey @jkczyz! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

This was referenced Mar 17, 2026
Merged
Open
joostjager
joostjager reviewed Mar 17, 2026
View reviewed changes
Comment thread e2e-tests/src/lib.rs Outdated
std::fs::write(&config_path, &config_content).unwrap();
impl LdkServerHandle {
/// Starts a new ldk-server instance against the given bitcoind.
/// Randomly picks between bitcoind RPC, electrum, and esplora as the chain source.

@joostjager joostjager Mar 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I really don't think we should be doing this to increase test coverage. Rather see a nightly job that runs a more extensive test suite.

@benthecarman benthecarman Mar 17, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

For this in particular was just trying to copy what ldk-node does

@joostjager joostjager Mar 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Imo it is an anti-pattern that emerged in ldk and ldk-node too indeed.

@ldk-reviews-bot

ldk-reviews-bot commented Mar 18, 2026

Copy link
Copy Markdown

🔔 6th Reminder

Hey @jkczyz! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@benthecarman benthecarman force-pushed the config-tests branch 2 times, most recently from 143c748 to 0d33256 Compare March 20, 2026 15:51
@ldk-reviews-bot

ldk-reviews-bot commented Mar 21, 2026

Copy link
Copy Markdown

🔔 7th Reminder

Hey @jkczyz! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot

ldk-reviews-bot commented Mar 23, 2026

Copy link
Copy Markdown

🔔 8th Reminder

Hey @jkczyz! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot

ldk-reviews-bot commented Mar 25, 2026

Copy link
Copy Markdown

🔔 9th Reminder

Hey @jkczyz! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@benthecarman benthecarman force-pushed the config-tests branch 3 times, most recently from 69b3d64 to 81f6bf3 Compare March 26, 2026 21:49
@ldk-reviews-bot

ldk-reviews-bot commented Mar 28, 2026

Copy link
Copy Markdown

🔔 10th Reminder

Hey @jkczyz! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot

ldk-reviews-bot commented Mar 30, 2026

Copy link
Copy Markdown

🔔 11th Reminder

Hey @jkczyz! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot

ldk-reviews-bot commented Apr 1, 2026

Copy link
Copy Markdown

🔔 12th Reminder

Hey @jkczyz! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot

ldk-reviews-bot commented Apr 4, 2026

Copy link
Copy Markdown

🔔 13th Reminder

Hey @jkczyz! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot

ldk-reviews-bot commented Apr 6, 2026

Copy link
Copy Markdown

🔔 14th Reminder

Hey @jkczyz! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot

ldk-reviews-bot commented Apr 8, 2026

Copy link
Copy Markdown

🔔 15th Reminder

Hey @jkczyz! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

Anyitechs
Anyitechs approved these changes Apr 8, 2026
View reviewed changes

@Anyitechs Anyitechs left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the cleanup in config.rs, and the robust test!

The logic looks great to me. Just needs a rebase to clear the conflicts.

@jkczyz jkczyz removed their request for review April 8, 2026 14:32
@Camillarhi

Camillarhi commented Jun 2, 2026

Copy link
Copy Markdown

Thanks for this. Building this branch with cargo build emits dead_code warnings on rabbitmq_connection_string, rabbitmq_exchange_name, and lsps2_service_config in ldk-server/src/util/config.rs. Looks like the #[allow(dead_code)] guard on those fields was dropped when they became Option.

@benthecarman
Add e2e config startup tests
b5fd87b 
Resolve the e2e harness conflicts around gRPC config startup and add
coverage for supported configuration variants and startup failures.

AI-assisted-by: OpenAI Codex
@benthecarman benthecarman requested a review from joostjager June 17, 2026 23:36
@benthecarman

benthecarman commented Jun 17, 2026

Copy link
Copy Markdown
Collaborator Author

Cleaned up to be a lot simpler than previous iteration

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joostjager joostjager Awaiting requested review from joostjager

1 more reviewer

@Anyitechs Anyitechs Anyitechs approved these changes

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

@benthecarman benthecarman

Labels

None yet

Projects

Weekly Goals
Status: No status

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@benthecarman @ldk-reviews-bot @Camillarhi @joostjager @Anyitechs