Test logging interface #463
Conversation
- `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.
enigbe
force-pushed
the
2025-02-test-logging-interface
branch
from
da2edcc to
d85c5c5
Compare
| 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.
| @@ -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?
| @@ -237,7 +239,7 @@ pub(crate) fn random_config(anchor_channels: bool) -> Config { | |||
| println!("Setting random LDK node alias: {:?}", alias); | |||
| config.node_alias = alias; | |||
|
|
|||
| config | |||
| TestConfig { node_config: config, log_writer: TestLogWriter::default() } | |||
There was a problem hiding this comment.
Consider using this type of struct construction:
TestConfig { node_config, ..Default::default() }| @@ -215,7 +217,7 @@ pub(crate) fn random_node_alias() -> Option<NodeAlias> { | |||
| Some(NodeAlias(bytes)) | |||
| } | |||
|
|
|||
| pub(crate) fn random_config(anchor_channels: bool) -> Config { | |||
| pub(crate) fn random_config(anchor_channels: bool) -> TestConfig { | |||
| let mut config = Config::default(); | |||
There was a problem hiding this comment.
nit: if we're already renaming the field, let's name this node_config for consistency.
| @@ -39,12 +40,13 @@ use bitcoincore_rpc::RpcApi; | |||
| use electrsd::{bitcoind, bitcoind::BitcoinD, ElectrsD}; | |||
| use electrum_client::ElectrumApi; | |||
|
|
|||
| use log::{Level, LevelFilter, Log, Record}; | |||
There was a problem hiding this comment.
This is going to be confusing. Can we possibly move these mock logger to a dedicated tests/common/logging.rs or similar? Also let's rename via .. as .. all of these imports to make it clear which version of Level etc. they are referring to (as we have at least 3 or 4 of them floating around now). I.e.
use log::Level as LogFacadeLevel;
..| @@ -1207,3 +1209,90 @@ impl KVStore for TestSyncStore { | |||
| self.do_list(primary_namespace, secondary_namespace) | |||
| } | |||
| } | |||
|
|
|||
| pub(crate) struct MockLogger { | |||
There was a problem hiding this comment.
Let's name this MockLogFacadeLogger for now.
| } | ||
|
|
||
| struct MockLogRecord<'a>(LogRecord<'a>); | ||
| struct MockLogLevel(LogLevel); |
There was a problem hiding this comment.
Do we really need these newtypes?
| 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?
|
|
||
| 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?
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