-
Notifications
You must be signed in to change notification settings - Fork 667
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
feat: allow pretty print logging in tests #5354
base: develop
Are you sure you want to change the base?
Conversation
stacks-common/src/util/log.rs
Outdated
#[cfg(not(any(test, feature = "testing")))] | ||
let decorator = slog_term::PlainSyncDecorator::new(std::io::stderr()); | ||
#[cfg(any(test, feature = "testing"))] | ||
let decorator = slog_term::PlainSyncDecorator::new(slog_term::TestStdoutWriter); |
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 the canonical way to do this is a little cleaner:
let decorator = {
#[cfg(not(any(test, feature = "testing")))]
{
slog_term::PlainSyncDecorator::new(std::io::stderr())
}
#[cfg(any(test, feature = "testing"))]
{
slog_term::PlainSyncDecorator::new(slog_term::TestStdoutWriter)
}
};
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.
Even better would be to put the entire declaration block into its own function, and have one function for testing and one for prod.
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.
Good call, I pulled it out into its own function 👍🏼
This allows
STACKS_LOG_PP=1
to be used in tests.We previously had two
make_logger
functions - one for tests and one for not tests. The only differences were:slog_term::TestStdoutWriter
This PR makes it so that we only have one
make_logger
function. Inside of that function, I've added a flag to useTestStdoutWriter
in tests.