-
Notifications
You must be signed in to change notification settings - Fork 104
Have the mock log facade not always print to stdout
#510
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
tests/common/logging.rs
Outdated
@@ -48,6 +48,7 @@ impl LogFacadeLog for MockLogFacadeLogger { | |||
record.line().unwrap(), | |||
record.args() | |||
); | |||
#[cfg(ldk_node_test_log_print)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't the real problem that we have trace level logs as default for tests?
Cant we work with debug level logs for tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we could also consider reducing it, but more generally there is no value in having the MockLogFacadeLogger
spamming stdout
, as we're really only using it to test our logging interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can reduce the log level and cfg
-gate it so it does not clutter stdout
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed offline. It seems printing is never a good idea because it is not compatible with parallel test running, so probably better to remove completely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whatever you decide regarding the log level change is fine by me. The gating LGTM.
f3d9dce
to
e5057f5
Compare
Rebased to resolve minor conflict. |
In lightningdevkit#463 we introduced a mock logger that implements the `log` facade in tests. Previously, it defaulted to always print `TRACE`-level logs to `stdout`. This however can be very spammy (especially since it includes TRACE-level logs of `electrum_client` now). Here, we simply disable printing to `stdout`
e5057f5
to
c5a014f
Compare
Pushed fixup that simply drops the |
In #463 we introduced a mock logger that implements the
log
facade in tests. Previously, it defaulted to always printTRACE
-level logs tostdout
. This however can be very spammy (especially since it includes TRACE-level logs ofelectrum_client
now).Here, we make printing the log lines optional by
cfg
-gating it.(cc @enigbe)