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

subscriber: Impl TestWriter for correct capturing of logs by cargo test #938

Merged
merged 6 commits into from
Aug 19, 2020
Merged

subscriber: Impl TestWriter for correct capturing of logs by cargo test #938

merged 6 commits into from
Aug 19, 2020

Conversation

samrg472
Copy link
Contributor

@samrg472 samrg472 commented Aug 18, 2020 •
edited
Loading

Motivation

Using cargo test doesn't correctly capture stdout. This is due to the difference between the print! macros and writing directly to the output stream. The result is messy output during testing.

Solution

A TestWriter was created that implements the MakeWriter and std::io::Write traits to wrap around the print! macro.

Closes #638

@samrg472 samrg472 requested review from hawkw and a team as code owners August 18, 2020 02:57
@davidbarsky davidbarsky self-requested a review August 18, 2020 13:22
davidbarsky
davidbarsky requested changes Aug 18, 2020
View reviewed changes
Copy link
Member

@davidbarsky davidbarsky left a comment

Choose a reason for hiding this comment

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

Thanks for opening this PR! Just a few requested changes.

tracing-subscriber/src/fmt/writer.rs Outdated Show resolved Hide resolved
tracing-subscriber/src/fmt/mod.rs Outdated Show resolved Hide resolved
tracing-subscriber/src/fmt/mod.rs Outdated Show resolved Hide resolved
@hawkw
Copy link
Member

hawkw commented Aug 18, 2020

I believe that this implements the functionality requested in issue #638. Mind adding "Closes #638" to the PR description?

hawkw
hawkw reviewed Aug 18, 2020
View reviewed changes
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

Overall, this looks correct, thank you for working on it!

I had some small API and documentation suggestions. Hopefully they're helpful.

tracing-subscriber/src/fmt/writer.rs Outdated Show resolved Hide resolved
tracing-subscriber/src/fmt/writer.rs Outdated Show resolved Hide resolved
tracing-subscriber/src/fmt/mod.rs Outdated Show resolved Hide resolved
@samrg472
Update docs on TestWriter
9bad1ac
@samrg472
Impl with_test_writer on Layer
2645a67
@samrg472
Use buf.len() in the TestWriter impl
6d3f277 
In the event that any non-UTF8 bytes are passed into the write stream, the
output may be incorrect if the out_str length is less than the buffer
length.
@samrg472
Prevent TestWriter from being directly constructed
0f41dca
samrg472
samrg472 commented Aug 19, 2020
View reviewed changes
Copy link
Contributor Author

@samrg472 samrg472 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Updated.

Edit: I should've marked this as reviewed sooner

samrg472
samrg472 commented Aug 19, 2020
View reviewed changes
Copy link
Contributor Author

@samrg472 samrg472 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Updated. Seems like GitHub stays in the "Changes Requested" state when the changes were made and is ready for another review.

hawkw
hawkw approved these changes Aug 19, 2020
View reviewed changes
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks for working on this! I had one last thought, but not a blocker.

tracing-subscriber/src/fmt/writer.rs Show resolved Hide resolved
@davidbarsky davidbarsky merged commit ea633f5 into tokio-rs:master Aug 19, 2020
@acfoltzer
Copy link

Yay, thank you!

hawkw added a commit that referenced this pull request Sep 8, 2020
@hawkw
subscriber: prepare to release 0.1.12
fbff9d4 
Fixed

- **env-filter**: Fixed a regression where `Option<Level>` lost its
  `Into<LevelFilter>` impl ([#966])
- **env-filter**: Fixed `EnvFilter` enabling spans that should not be
  enabled when multiple subscribers are in use ([#927])

Changed

- **json**: `format::Json` now outputs fields in a more readable order
  ([#892])
- Updated `tracing-core` dependency to 0.1.16

Added

- **fmt**: Add `BoxMakeWriter` for erasing the type of a `MakeWriter`
  implementation ([#958])
- **fmt**: Add `TestWriter` `MakeWriter` implementation to support
  libtest output capturing ([#938])
- **layer**: Add `Layer` impl for `Option<T> where T: Layer` ([#910])
- **env-filter**: Add `From<Level>` impl for `Directive` ([#918])
- Multiple documentation fixes and improvements

Thanks to @Pothulapati, @samrg472, @bryanburgers, @keetonian, and
@SriRamanujam for contributing to this release!

[#927]: #927
[#966]: #966
[#958]: #958
[#892]: #892
[#938]: #938
[#910]: #910
[#918]: #918
@hawkw hawkw mentioned this pull request Sep 8, 2020
Merged
hawkw added a commit that referenced this pull request Sep 9, 2020
@hawkw
subscriber: prepare to release 0.2.12 (#970)
3dd5c03 
# 0.2.12 (September 8, 2020)

### Fixed

- **env-filter**: Fixed a regression where `Option<Level>` lost its
  `Into<LevelFilter>` impl ([#966])
- **env-filter**: Fixed `EnvFilter` enabling spans that should not be enabled
  when multiple subscribers are in use ([#927])
  
### Changed

- **json**: `format::Json` now outputs fields in a more readable order ([#892])
- Updated `tracing-core` dependency to 0.1.16

### Added

- **fmt**: Add `BoxMakeWriter` for erasing the type of a `MakeWriter`
  implementation ([#958])
- **fmt**: Add `TestWriter` `MakeWriter` implementation to support libtest
  output capturing ([#938])
- **layer**: Add `Layer` impl for `Option<T> where T: Layer` ([#910])
- **env-filter**: Add `From<Level>` impl for `Directive` ([#918])
- Multiple documentation fixes and improvements

Thanks to @Pothulapati, @samrg472, @bryanburgers, @keetonian, 
and @SriRamanujam for contributing to this release!

[#927]: #927
[#966]: #966
[#958]: #958
[#892]: #892
[#938]: #938
[#910]: #910
[#918]: #918
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MikailBag MikailBag MikailBag left review comments

@davidbarsky davidbarsky davidbarsky approved these changes

@hawkw hawkw hawkw approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

fmt::Subscribers don't want their output to be captured
5 participants
@samrg472 @hawkw @acfoltzer @davidbarsky @MikailBag