Add e2e config startup tests#142
Conversation
|
👋 Thanks for assigning @joostjager as a reviewer! |
benthecarman
force-pushed
the
config-tests
branch
from
aa4e94a to
8a9bf3a
Compare
|
🔔 1st Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 4th Reminder Hey @jkczyz! This PR has been waiting for your review. |
benthecarman
force-pushed
the
config-tests
branch
2 times, most recently
from
1859bb4 to
957057f
Compare
|
🔔 5th Reminder Hey @jkczyz! This PR has been waiting for your review. |
benthecarman
force-pushed
the
config-tests
branch
from
957057f to
0db68fb
Compare
|
🔔 6th Reminder Hey @jkczyz! This PR has been waiting for your review. |
benthecarman
force-pushed
the
config-tests
branch
2 times, most recently
from
143c748 to
0d33256
Compare
|
🔔 7th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 8th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 9th Reminder Hey @jkczyz! This PR has been waiting for your review. |
benthecarman
force-pushed
the
config-tests
branch
3 times, most recently
from
69b3d64 to
81f6bf3
Compare
|
🔔 10th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 11th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 12th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 13th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 14th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 15th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
Thanks for this. Building this branch with |
benthecarman
force-pushed
the
config-tests
branch
from
81f6bf3 to
b5fd87b
Compare
|
Cleaned up to be a lot simpler than previous iteration |
joostjager
left a comment
joostjager
left a comment
There was a problem hiding this comment.
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.
benthecarman
force-pushed
the
config-tests
branch
from
b5fd87b to
3d8273e
Compare
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
left a comment
joostjager
left a comment
There was a problem hiding this comment.
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.
| 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 { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
made this a lot cleaner
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
3d8273e to
d86df20
Compare
joostjager
left a comment
joostjager
left a comment
There was a problem hiding this comment.
Nicer indeed with the builder
| pub enum ChainSource { | ||
| Bitcoind { rpc_address: String, rpc_user: String, rpc_password: String }, | ||
| Electrum { server_url: String }, | ||
| Esplora { server_url: String }, |
There was a problem hiding this comment.
Electrum and Esplora seem to be unused. Would remove it if it is dead currently.
There was a problem hiding this comment.
I am just gonna leave for now, we may implement it eventually.
5 participants
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.