Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix pretty formatter thread id double space #1778

Merged
merged 2 commits into from
Dec 22, 2021
Merged

Fix pretty formatter thread id double space #1778

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d42a261
Fix pretty formatter thread id double space
rukai Dec 15, 2021
92d08a8
Update tracing-subscriber/src/fmt/format/pretty.rs
hawkw Dec 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 65 additions & 11 deletions tracing-subscriber/src/fmt/format/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1468,6 +1468,8 @@ pub(super) mod test {
use super::{FmtSpan, TimingDisplay, Writer};
use std::fmt;

use regex::Regex;

pub(crate) struct MockTime;
impl FormatTime for MockTime {
fn format_time(&self, w: &mut Writer<'_>) -> fmt::Result {
Expand All @@ -1488,21 +1490,31 @@ pub(super) mod test {
.with_thread_names(false);
#[cfg(feature = "ansi")]
let subscriber = subscriber.with_ansi(false);
run_test(subscriber, make_writer, "hello\n")
assert_info_hello(subscriber, make_writer, "hello\n")
}

#[cfg(feature = "ansi")]
#[test]
fn with_ansi_true() {
let expected = "\u{1b}[2mfake time\u{1b}[0m \u{1b}[32m INFO\u{1b}[0m \u{1b}[2mtracing_subscriber::fmt::format::test\u{1b}[0m\u{1b}[2m:\u{1b}[0m hello\n";
test_ansi(true, expected);
assert_info_hello_ansi(true, expected);
}

#[cfg(feature = "ansi")]
#[test]
fn with_ansi_false() {
let expected = "fake time INFO tracing_subscriber::fmt::format::test: hello\n";
test_ansi(false, expected);
assert_info_hello_ansi(false, expected);
}

#[cfg(feature = "ansi")]
fn assert_info_hello_ansi(is_ansi: bool, expected: &str) {
let make_writer = MockMakeWriter::default();
let subscriber = crate::fmt::Collector::builder()
.with_writer(make_writer.clone())
.with_ansi(is_ansi)
.with_timer(MockTime);
assert_info_hello(subscriber, make_writer, expected)
}

#[cfg(not(feature = "ansi"))]
Expand All @@ -1513,7 +1525,7 @@ pub(super) mod test {
let subscriber = crate::fmt::Collector::builder()
.with_writer(make_writer)
.with_timer(MockTime);
run_test(subscriber, make_writer, expected);
assert_info_hello(subscriber, make_writer, expected);
}

#[test]
Expand All @@ -1526,23 +1538,65 @@ pub(super) mod test {
.with_timer(MockTime);
let expected = "fake time tracing_subscriber::fmt::format::test: hello\n";

run_test(subscriber, make_writer, expected);
assert_info_hello(subscriber, make_writer, expected);
}

#[cfg(feature = "ansi")]
fn test_ansi(is_ansi: bool, expected: &str) {
#[test]
fn with_thread_ids() {
let make_writer = MockMakeWriter::default();
let subscriber = crate::fmt::Collector::builder()
.with_writer(make_writer.clone())
.with_ansi(is_ansi)
.with_thread_ids(true)
.with_ansi(false)
.with_timer(MockTime);
let expected =
"fake time INFO ThreadId(NUMERIC) tracing_subscriber::fmt::format::test: hello\n";

assert_info_hello_ignore_numeric(subscriber, make_writer, expected);
}

#[test]
fn pretty_default() {
let make_writer = MockMakeWriter::default();
let subscriber = crate::fmt::Collector::builder()
.pretty()
.with_writer(make_writer.clone())
.with_ansi(false)
.with_timer(MockTime);
run_test(subscriber, make_writer, expected)
let expected = format!(
r#" fake time INFO tracing_subscriber::fmt::format::test: hello
at {}:NUMERIC

"#,
file!()
);

assert_info_hello_ignore_numeric(subscriber, make_writer, &expected)
}

fn assert_info_hello(subscriber: impl Into<Dispatch>, buf: MockMakeWriter, expected: &str) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, take it or leave it: the "hello" doesn't really say too much about the test; i'd rather have the previous set of names here.

let _default = set_default(&subscriber.into());
tracing::info!("hello");
let result = buf.get_string();

assert_eq!(expected, result)
}

fn run_test(subscriber: impl Into<Dispatch>, buf: MockMakeWriter, expected: &str) {
// When numeric characters are used they often form a non-deterministic value as they usually represent things like a thread id or line number.
// This assert method should be used when non-deterministic numeric characters are present.
fn assert_info_hello_ignore_numeric(
subscriber: impl Into<Dispatch>,
buf: MockMakeWriter,
expected: &str,
) {
Comment on lines +1585 to +1591
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a non-blocking nit (eliza is extremely welcome to override with an "ignore david, this is fine"), but since this only used in two callsites, i'd rather have the contents of this method be inlined into each test.

let _default = set_default(&subscriber.into());
tracing::info!("hello");
assert_eq!(expected, buf.get_string())

let regex = Regex::new("[0-9]+").unwrap();
let result = buf.get_string();
let result_cleaned = regex.replace_all(&result, "NUMERIC");
Comment on lines +1595 to +1597
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, take it or leave it: we may want to put the regex in a lazy_static! to avoid recompiling the regex in every test:

Suggested change
let regex = Regex::new("[0-9]+").unwrap();
let result = buf.get_string();
let result_cleaned = regex.replace_all(&result, "NUMERIC");
lazy_static::lazy_static! {
static ref REGEX: Regex = Regex::new("[0-9]+").unwrap();
}
let result = buf.get_string();
let result_cleaned = regex.replace_all(&REGEX, "NUMERIC");

in theory, this might improve performance somewhat. in practice, I don't think the regex is compiled enough to have a significant impact on how long it takes to run these tests, so this may not really be worth the effort.


assert_eq!(expected, result_cleaned)
}

#[test]
Expand Down
9 changes: 4 additions & 5 deletions tracing-subscriber/src/fmt/format/pretty.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,13 +174,12 @@ where
if let Some(name) = thread.name() {
write!(writer, "{}", name)?;
if self.display_thread_id {
write!(writer, " ({:?})", thread.id())?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, this changes the formatting slightly because we'll no longer have parentheses around thread IDs when thread names are also enabled. i'm not sure if this is particularly important, though.

Copy link
Contributor Author

@rukai rukai Dec 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point, I didnt notice those extra brackets.
However looking at it I think this is an improvement as the thread id is displayed with the debug display ThreadId(1) so having (ThreadId(1)) is a bit weird.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, i don't have a strong opinion on whether the parentheses are good or not, although they seemed more grammatically correct here. not a big deal either way though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

normally, i'd try to reserve formatting changes for a major version bump, but this seems reasonable to me.

writer.write_char(' ')?;
}
} else if !self.display_thread_id {
write!(writer, " {:?}", thread.id())?;
}
} else if self.display_thread_id {
write!(writer, " {:?}", thread.id())?;
}
if self.display_thread_id {
write!(writer, "{:?}", thread.id())?;
}
writer.write_char('\n')?;
}
Expand Down