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.

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

Reusable handle to thread-local replaceable stdout/err #50457

Closed
wants to merge 1 commit into from

Conversation

CAD97
Copy link
Contributor

@CAD97 CAD97 commented May 5, 2018

Lays the groundwork for mitigating:

When running tests with logging, logs directed to io::stdout() are not captured by the test runner, and intermingle with the test runner's output. This does not aim to directly solve that problem, but rather provides the infrastructure towards allowing users to opt-in to the same behavior that println! and friends have in regards to std(out|err) for any type generic over Write.

By providing LocalStderr/LocalStdout, when building logging for tests, the logger can be constructed to use io::LocalStdout rather than io::stdout(), which will redirect the output with the exact same mechanism that println! output is captured in the test runner.

The hidden fn _print and _eprint have not been removed to avoid necessitating inlining the panicking code for if stdio/stdout error. However, they have been adjusted to write to LocalStdout/LocalStderr. print_to has been replaced with with_write, allowing the Write impls to defer to the backing Write implementation for all methods.

Is this approach amenable? Is this approach towards allowing well-behaved logs to be captured by test runners one that could in the future potentially be stabilized? Would that require a full RFC? (I can write one up.) (I'm not exactly certain how to add a new feature gate, so for now they're gated by set_stdio, which isn't wrong.) Any bikeshed on naming?

This is mostly a proof-of-concept that this approach would (hopefully) work. Alternately, this switching behavior can be added directly onto io::Stdin/Stdout, in which case the local versions would not be needed, and test output capturing would be closer to avoiding leakage, but spawning a new thread would still avoid the capture, as the approach used to change stdout in tests is inherently thread-local.

(My local ./x.py test also stalled out due to what I assume is an environment error, so I'm hoping Travis will indicate if I messed something up horribly.)

WARNING: this touches the output routine used by print!/eprint!/the default panic handler.
HANDLE WITH CAUTION

@rust-highfive
Copy link
Collaborator

r? @kennytm

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 5, 2018
@CAD97
Copy link
Contributor Author

CAD97 commented May 5, 2018

As an example usage, I currently have the following helper function for tests in a workspace using slog for logging:

    fn make_logger() -> ::slog::Logger {
        use slog::{Drain, Logger};
        use std::{io, sync::Mutex};
        Logger::root(
            Mutex::new(
                slog_term::CompactFormat::new(
                    slog_term::PlainSyncDecorator::new(io::stdout())
                ).build().fuse()
            ).fuse(),
            o!(),
        )
    }

With the approach taken in this PR, I should be able to replace io::stdout() directly with io::LocalStdout, and the logging done will go from escaping into the test logger's output to being captured as would any other simple println! call.

The alternate solution here would be to create a new logger type that uses println! rather than writing to a Write sink, but that scales linearly with the number of logger frameworks, instead of this approach's constant amount of work :P

@kennytm kennytm added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label May 5, 2018
@kennytm
Copy link
Member

kennytm commented May 8, 2018

r? @alexcrichton

@rust-highfive rust-highfive assigned alexcrichton and unassigned kennytm May 8, 2018
@alexcrichton
Copy link
Member

Thanks for the PR! This I think is pretty highly related to the work being done on custom test frameworks and so I'm somewhat hesitatnt to land without weighing in with those folks.

In terms of this particular solution I've never been too fond of the ability to override stdout/stderr in libstd and it was basically always just a necessary evil of thet test harness itself. I'd prefer to pursue different avenues to avoid libtest needing to capture entirely first before we add more api surface area to libstd itself.

@CAD97
Copy link
Contributor Author

CAD97 commented May 9, 2018

Seems reasonable to me! (I'm fine with closing this PR for the time being to reduce clutter if that's desired.)

The stdout/stderr overriding is only necessary as I understand it because stdout/stderr are by-process and the test harness works by spawning threads in-process to run the individual tests.

If you point me in the direction where the test harness discussion is happening, I'd be happy to participate, I'm just not exactly certain where that is, though I've been excited by what bits of it I've stumbled across.

@alexcrichton
Copy link
Member

I'm not sure myself exactly where the test framework discussion is happening, but I suspect that @killercup might!

@pietroalbini
Copy link
Member

Ping from triage @alexcrichton! If I understood correctly this is blocked on the discussion about custom test frameworks?

@killercup
Copy link
Member

@CAD97 the custom test frameworks eRFC was merged two weeks ago, but there is no implementation work ongoing yet. Here's the tracking issue: #50297

@alexcrichton
Copy link
Member

Ok I'm gonna close this in favor of that issue, @CAD97 mind writing up your thoughts though on the issue about the problem here with this possible solution?

@CAD97 CAD97 deleted the local-stdout branch May 14, 2018 22:06
@CAD97
Copy link
Contributor Author

CAD97 commented May 14, 2018

It has been done. See #50297 (comment) for my current thoughts. TL;DR: To have sane capture behavior for stdout and stderr for print-family macros, panic, and io::{stdout, stderr}, running each test in its own (child) process is probably required. I'd love to help out wherever I can, but lack confidence.

@alexcrichton
Copy link
Member

@CAD97 ok, thanks for the comment!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants