Add e2e config startup tests by benthecarman · Pull Request #142 · lightningdevkit/ldk-server

benthecarman · 2026-03-03T23:15:31Z

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 · 2026-03-03T23:15:34Z

👋 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 · 2026-03-05T23:26:11Z

🔔 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 · 2026-03-09T00:00:10Z

🔔 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 · 2026-03-11T00:00:18Z

🔔 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 · 2026-03-14T00:01:11Z

🔔 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.

ldk-reviews-bot · 2026-03-16T00:01:30Z

🔔 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.

joostjager · 2026-03-17T09:20:32Z

-		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.


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.

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

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

seems dropped from this pr now, so no discussion point anymore

ldk-reviews-bot · 2026-03-18T00:01:54Z

🔔 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.

ldk-reviews-bot · 2026-03-21T00:01:04Z

🔔 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 · 2026-03-23T00:01:54Z

🔔 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 · 2026-03-25T00:02:12Z

🔔 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.

ldk-reviews-bot · 2026-03-28T00:00:18Z

🔔 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 · 2026-03-30T00:01:02Z

🔔 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 · 2026-04-01T00:01:51Z

🔔 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 · 2026-04-04T00:00:21Z

🔔 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 · 2026-04-06T00:01:22Z

🔔 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 · 2026-04-08T00:02:05Z

🔔 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

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.

Camillarhi · 2026-06-02T23:55:53Z

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 · 2026-06-17T23:36:25Z

Cleaned up to be a lot simpler than previous iteration

joostjager

E2E tests seem to be pretty heavy for config validation. Can't that be done just as well on the unit test level?

Perhaps a smoke test is useful that tests one invalid config, see how that traverses the full stack.

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 · 2026-06-18T17:00:49Z

E2E tests seem to be pretty heavy for config validation. Can't that be done just as well on the unit test level?

Perhaps a smoke test is useful that tests one invalid config, see how that traverses the full stack.

Yeah we already have config unit tests, the goal of these is to make it so we test things that our config can't really validate and also make sure we are properly handling a valid config. Removed some of the tests that were basically recreations of the config unit tests and added more that actually target where we could and should throw errors.

joostjager

The latest shape is better after trimming config tests.

I do still get a slight feeling of overlap with ldk-node coverage. Going forward, I think we should be pretty strict about keeping that duplication to a minimum. Just test the things in ldk-server that also mainly live there.

Left a few non-blocking remarks.

joostjager · 2026-06-19T06:58:04Z

+	})
+	.await;
+	let info = server.client().get_node_info(GetNodeInfoRequest {}).await.unwrap();
+	assert!(info.current_best_block.is_some());


Here and above, perhaps assert a bit more, in particular the addresses?

These tests do look quite similar to what's already in ldk-node, IIRC. Not sure if it adds much.

joostjager · 2026-06-19T07:03:35Z

+	pub async fn start_with_options(bitcoind: &TestBitcoind, config: LdkServerConfig) -> Self {
+		Self::start_with_config(bitcoind, |params| {
+			let chain_source = format!(
+				"[bitcoind]\nrpc_address = \"{}\"\nrpc_user = \"{}\"\nrpc_password = \"{}\"",


This is duplicated in a few places, perhaps it can be a helper.

joostjager · 2026-06-19T07:05:21Z

+	config.lines().filter(|line| !line.trim_start().starts_with(key)).collect::<Vec<_>>().join("\n")
+}
+
+fn replace_config_line(config: &str, key: &str, new_line: &str) -> String {


I am not sure about this mix of having a config builder, but then also surgically modifying it again. Might be a bit brittle too. Any way to make this more consistent/robust?

ldk-reviews-bot requested a review from jkczyz March 3, 2026 23:25

benthecarman force-pushed the config-tests branch from aa4e94a to 8a9bf3a Compare March 4, 2026 17:07

benthecarman added this to Weekly Goals Mar 5, 2026

benthecarman self-assigned this Mar 5, 2026

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

benthecarman force-pushed the config-tests branch from 957057f to 0db68fb Compare March 16, 2026 20:47

This was referenced Mar 17, 2026

Add Dockerfile #154

Merged

Beta Release #121

Open

joostjager reviewed Mar 17, 2026

View reviewed changes

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

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

Anyitechs approved these changes Apr 8, 2026

View reviewed changes

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

benthecarman force-pushed the config-tests branch from 81f6bf3 to b5fd87b Compare June 17, 2026 23:36

benthecarman requested a review from joostjager June 17, 2026 23:36

joostjager reviewed Jun 18, 2026

View reviewed changes

Add e2e config startup tests

3d8273e

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 force-pushed the config-tests branch from b5fd87b to 3d8273e Compare June 18, 2026 16:59

joostjager approved these changes Jun 19, 2026

View reviewed changes

Conversation

benthecarman commented Mar 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldk-reviews-bot commented Mar 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldk-reviews-bot commented Mar 5, 2026

Uh oh!

ldk-reviews-bot commented Mar 9, 2026

Uh oh!

ldk-reviews-bot commented Mar 11, 2026

Uh oh!

ldk-reviews-bot commented Mar 14, 2026

Uh oh!

ldk-reviews-bot commented Mar 16, 2026

Uh oh!

joostjager Mar 17, 2026

Choose a reason for hiding this comment

Uh oh!

benthecarman Mar 17, 2026

Choose a reason for hiding this comment

Uh oh!

joostjager Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

joostjager Jun 18, 2026

Choose a reason for hiding this comment

Uh oh!

ldk-reviews-bot commented Mar 18, 2026

Uh oh!

ldk-reviews-bot commented Mar 21, 2026

Uh oh!

ldk-reviews-bot commented Mar 23, 2026

Uh oh!

ldk-reviews-bot commented Mar 25, 2026

Uh oh!

ldk-reviews-bot commented Mar 28, 2026

Uh oh!

ldk-reviews-bot commented Mar 30, 2026

Uh oh!

ldk-reviews-bot commented Apr 1, 2026

Uh oh!

ldk-reviews-bot commented Apr 4, 2026

Uh oh!

ldk-reviews-bot commented Apr 6, 2026

Uh oh!

ldk-reviews-bot commented Apr 8, 2026

Uh oh!

Anyitechs left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Camillarhi commented Jun 2, 2026

Uh oh!

benthecarman commented Jun 17, 2026

Uh oh!

joostjager left a comment

Choose a reason for hiding this comment

Uh oh!

benthecarman commented Jun 18, 2026

Uh oh!

joostjager left a comment

Choose a reason for hiding this comment

Uh oh!

joostjager Jun 19, 2026

Choose a reason for hiding this comment

Uh oh!

joostjager Jun 19, 2026

Choose a reason for hiding this comment

Uh oh!

joostjager Jun 19, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

benthecarman commented Mar 3, 2026 •

edited

Loading

ldk-reviews-bot commented Mar 3, 2026 •

edited

Loading

Anyitechs left a comment •

edited

Loading