Test logging interface #463
Conversation
enigbe
force-pushed
the
2025-02-test-logging-interface
branch
from
da2edcc to
d85c5c5
Compare
tests/common/mod.rs
Outdated
| pub log_writer: TestLogWriter, | ||
| } | ||
|
|
||
| impl Default for TestConfig { |
There was a problem hiding this comment.
No need for this impl, just add Default to the derive above.
tests/common/mod.rs
Outdated
| @@ -251,6 +253,34 @@ pub(crate) enum TestChainSource<'a> { | |||
| BitcoindRpc(&'a BitcoinD), | |||
| } | |||
|
|
|||
| #[derive(Clone)] | |||
| pub(crate) enum TestLogWriter { | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
Outdated
| } | ||
|
|
||
| struct MockLogRecord<'a>(LogRecord<'a>); | ||
| struct MockLogLevel(LogLevel); |
There was a problem hiding this comment.
Do we really need these newtypes?
There was a problem hiding this comment.
Yes we do. We can't implement external traits (log::Log) on external types (LogLevel and LogRecord belong to ldk_node crate) so we work around this by wrapping the external types.
tests/common/mod.rs
Outdated
| 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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
Hmm, if this is part of the
logAPI anyways, should we drop ourmax_log_levelvalue in the log facade related builder methods andLogWritervariants? 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 removemax_log_level.
If we are going with the latter, would it be okay to add a commit for it to this PR?
There was a problem hiding this comment.
Also, apologies for the late changes. I am ramping up into LDK and got side-tracked.
There was a problem hiding this comment.
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!
tests/integration_tests_rust.rs
Outdated
|
|
||
| let logger = init_log_logger(LevelFilter::Trace); | ||
| let mut config = random_config(false); | ||
| config.log_writer = TestLogWriter::LogFacade { max_log_level: LogLevel::Gossip }; |
There was a problem hiding this comment.
See above, a bit awkward that we have an additional filter on the level.
| let _node = setup_node(&chain_source, config, None); | ||
| println!("== Facade logging end =="); | ||
|
|
||
| assert!(!logger.retrieve_logs().is_empty()); |
There was a problem hiding this comment.
Should we do more checks to ensure the messages contain something valid (or even check the structure, certain fields, etc?), rather than doing all this work just to check the Vec isn't empty?
There was a problem hiding this comment.
Yes we should. I thought to use regular expressions but that would lead to adding another dependency just for testing. Instead, I have added a simple function to validate the log entries.
enigbe
force-pushed
the
2025-02-test-logging-interface
branch
from
d85c5c5 to
bff5778
Compare
|
@tnull. |
enigbe
force-pushed
the
2025-02-test-logging-interface
branch
from
bff5778 to
30bee82
Compare
tests/common/mod.rs
Outdated
| 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)); |
There was a problem hiding this comment.
If we're using defaults anyways, why are we overriding them here? Shouldn't we just use Nones instead?
- `TestConfig` wraps `Config` and other fields, specifically a log-related field that can be over-written on a per-test basis. - With `TestConfig`, we can maintain the general testing setup/APIs as is and only modify fields based on specific features we want to test.
Additionally addresses minor issues related to: - Default impl - Verbose TestLogWriter enum - MockLogFacadeLogger and node_config rename
- use uniffi flag for appropriate log objects - use default filewriter arguments - remove test for custom logger and the logger initialization code
This is now considered unnecessary because the concrete implementation of the facade has control over the maximum log level that can be configured.
enigbe
force-pushed
the
2025-02-test-logging-interface
branch
from
30bee82 to
4e59771
Compare
What this PR does
This is another follow-up PR to #407 where we test the
logfacade and custom logging interfaces.It follows an earlier conversation to address testing without explicitly configuring the logger in all test cases.
cc @tnull