Test logging interface #463

enigbe · 2025-02-10T10:52:26Z

What this PR does

This is another follow-up PR to #407 where we test the log facade and custom logging interfaces.

It follows an earlier conversation to address testing without explicitly configuring the logger in all test cases.

cc @tnull

tnull

Thanks!

tnull · 2025-02-11T08:26:14Z

tests/common/mod.rs

+	pub log_writer: TestLogWriter,
+}
+
+impl Default for TestConfig {


No need for this impl, just add Default to the derive above.

tnull · 2025-02-11T08:29:17Z

tests/common/mod.rs

@@ -251,6 +253,34 @@ pub(crate) enum TestChainSource<'a> {
 	BitcoindRpc(&'a BitcoinD),
 }

+#[derive(Clone)]
+pub(crate) enum TestLogWriter {


While TestConfig could have some general utility going forward, I'm not quite convinced we need all this boilerplate just to switch the logger in a one-off test. We could at least simplify this enum, no?

Simplified it a bit to this. We don't really need the fields in the FileWriter variant as we are not really testing this, but the facade and custom variants are left as we test both of them.

#[derive(Clone)] pub(crate) enum TestLogWriter { FileWriter, LogFacade(LogLevel), Custom(Arc<dyn LogWriter>), }

tests/common/mod.rs

tnull · 2025-02-11T08:40:25Z

tests/common/mod.rs

+pub(crate) fn init_log_logger(level: LevelFilter) -> Arc<MockLogger> {
+	let logger = Arc::new(MockLogger::new());
+	log::set_boxed_logger(Box::new(logger.clone())).unwrap();
+	log::set_max_level(level);


Hmm, if this is part of the log API anyways, should we drop our max_log_level value in the log facade related builder methods and LogWriter variants? Do you have an opinion here? Do we gain much by also limiting on our end?

@enigbe Gentle ping here, as we're looking to ship v0.5 in the coming days. Do you think it makes sense to drop the max_log_level for the log facade logger, too, or do we need it?

Hmm, if this is part of the log API anyways, should we drop our max_log_level value in the log facade related builder methods and LogWriter variants? Do you have an opinion here? Do we gain much by also limiting on our end

If we keep the max_log_level, we'd have to nudge users to align the facade level with the LevelFilter given that they can be compared directly. However, we won't have any control over this because setting the max level should be done by the active log implementation.

Do you think it makes sense to drop the max_log_level for the log facade logger, too, or do we need it?
It'd be cleaner to remove max_log_level.

If we are going with the latter, would it be okay to add a commit for it to this PR?

Also, apologies for the late changes. I am ramping up into LDK and got side-tracked.

If we keep the max_log_level, we'd have to nudge users to align the facade level with the LevelFilter given that they can be compared directly. However, we won't have any control over this because setting the max level should be done by the active log implementation.

Right then it might be better to drop it, at least for now.

If we are going with the latter, would it be okay to add a commit for it to this PR?

Yeah, feel free to append another commit here for it.

Also, apologies for the late changes. I am ramping up into LDK and got side-tracked.

No worries!

tnull · 2025-02-11T08:42:50Z

tests/integration_tests_rust.rs

+
+	let logger = init_log_logger(LevelFilter::Trace);
+	let mut config = random_config(false);
+	config.log_writer = TestLogWriter::LogFacade { max_log_level: LogLevel::Gossip };


See above, a bit awkward that we have an additional filter on the level.

tests/integration_tests_rust.rs

enigbe · 2025-02-19T08:00:14Z

@tnull.
Thank for taking a look at this. I have address most of the comments and just have a question about adding a commit that removes max_log_level here or in a new PR.

tnull · 2025-02-21T09:50:39Z

tests/common/mod.rs

+		TestLogWriter::FileWriter => {
+			let file_path = format!("{}/{}", DEFAULT_STORAGE_DIR_PATH, DEFAULT_LOG_FILENAME);
+			let max_log_level = DEFAULT_LOG_LEVEL;
+			builder.set_filesystem_logger(Some(file_path), Some(max_log_level));


If we're using defaults anyways, why are we overriding them here? Shouldn't we just use Nones instead?

tnull · 2025-03-04T10:15:27Z

This is in need of a rebase now.

enigbe · 2025-03-04T13:10:01Z

This is in need of a rebase now.

Rebased.

tnull

Excuse the delay here, I wasn't realizing you previously addressed the comments.

Basically LGTM, please feel free to squash the fixups.

tests/common/mod.rs

tests/integration_tests_rust.rs

General improvements: - Add TestConfig wrapper around Config to allow per-test log field overrides while preserving the general test setup. - Test facade logger. - Add logging module and validate log entries. - Improve UniFFI logging adding UniFFI flag for relevant log objects. - Remove facade level, because the concrete `Log` impl controls max log level.

enigbe · 2025-03-10T17:41:27Z

Excuse the delay here, I wasn't realizing you previously addressed the comments.

Basically LGTM, please feel free to squash the fixups.

Thanks once again for your reviews @tnull. Squashed all fixes.

enigbe force-pushed the 2025-02-test-logging-interface branch from da2edcc to d85c5c5 Compare February 10, 2025 14:27

tnull reviewed Feb 11, 2025

View reviewed changes

tnull mentioned this pull request Feb 13, 2025

Use log facade and log4rs for logging. lightningdevkit/ldk-server#44

Open

enigbe force-pushed the 2025-02-test-logging-interface branch from d85c5c5 to bff5778 Compare February 19, 2025 07:30

enigbe force-pushed the 2025-02-test-logging-interface branch from bff5778 to 30bee82 Compare February 20, 2025 08:54

tnull reviewed Feb 21, 2025

View reviewed changes

enigbe force-pushed the 2025-02-test-logging-interface branch from 30bee82 to 4e59771 Compare February 23, 2025 22:51

enigbe force-pushed the 2025-02-test-logging-interface branch from 4e59771 to 7e27939 Compare March 4, 2025 12:06

tnull reviewed Mar 10, 2025

View reviewed changes

tests/common/mod.rs Show resolved Hide resolved

tests/integration_tests_rust.rs Outdated Show resolved Hide resolved

enigbe force-pushed the 2025-02-test-logging-interface branch from 7e27939 to 56d5dc9 Compare March 10, 2025 16:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test logging interface #463

Test logging interface #463

enigbe commented Feb 10, 2025

tnull left a comment

tnull Feb 11, 2025

tnull Feb 11, 2025

enigbe Feb 19, 2025

tnull Feb 11, 2025

tnull Feb 18, 2025

enigbe Feb 19, 2025

enigbe Feb 19, 2025

tnull Feb 21, 2025 •

edited

Loading

tnull Feb 11, 2025

enigbe commented Feb 19, 2025

tnull Feb 21, 2025

tnull commented Mar 4, 2025

enigbe commented Mar 4, 2025

tnull left a comment

enigbe commented Mar 10, 2025

Test logging interface #463

Are you sure you want to change the base?

Test logging interface #463

Conversation

enigbe commented Feb 10, 2025

What this PR does

tnull left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull Feb 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enigbe commented Feb 19, 2025

Choose a reason for hiding this comment

tnull commented Mar 4, 2025

enigbe commented Mar 4, 2025

tnull left a comment

Choose a reason for hiding this comment

enigbe commented Mar 10, 2025

tnull Feb 21, 2025 •

edited

Loading