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

Support test output postprocessing by a child process. #122145

Closed
wants to merge 1 commit into from

Conversation

aspotashev
Copy link

@aspotashev aspotashev commented Mar 7, 2024

Support test results output postprocessing by a child process.

Add the following two optional flags to libtest (rustc's built-in unit-test
framework), in order to support postprocessing of the test results using a
separate executable:

  • --output_postprocess_executable [PATH]
  • --output_postprocess_args [ARGUMENT] (can be repeated.)

If you don't pass --output_postprocess_executable [PATH], the behavior stays
the same as before this commit. That is, the test results are sent to stdout.

If you pass --output_postprocess_executable [PATH], libtest will

  1. Spawn a child process from the executable binary (aka postprocessor) at
    the given path.
  2. Pass the arguments from the --output_postprocess_args [ARGUMENT] flags (if
    any) to the child process. If --output_postprocess_args was used multiple
    times, all listed arguments will be passed in the original order.
  3. Propagate the environment variables to the child process.

The postprocessor executable is expected to wait for the end of input (EOF)
and then terminate.

Usage example #1: Filter lines of the test results

$ LD_LIBRARY_PATH=$(pwd) ./test-05daf44cb501aee6 --output_postprocess_executable=/usr/bin/grep --output_postprocess_args="test result"
test result: ok. 59 passed; 0 failed; 1 ignored; 0 measured; 0 filtered out; finished in 0.31s

Usage example #2: Save test results into a file

$ LD_LIBRARY_PATH=$(pwd) ./test-05daf44cb501aee6 --output_postprocess_executable=/usr/bin/sh --output_postprocess_args=-c --output_postprocess_args="cat > /tmp/output.txt"

Usage example #3: Save test results into a file while keeping the command line
output

$ LD_LIBRARY_PATH=$(pwd) ./test-05daf44cb501aee6 --output_postprocess_executable=/usr/bin/tee --output_postprocess_args="/tmp/output.txt"

running 60 tests
...

Usage example #4: Prepend every line of test results with the value of an
environment variable (to demonstrate environment variable propagation)

$ LOG_PREFIX=">>>" LD_LIBRARY_PATH=$(pwd) ./test-05daf44cb501aee6 --output_postprocess_executable=/usr/bin/sh --output_postprocess_args=-c --output_postprocess_args="sed s/^/\$LOG_PREFIX/"
>>>
>>>running 60 tests
...

Usage example #5: Change format of JSON output (using
https://jqlang.github.io/jq/)

$ LD_LIBRARY_PATH=$(pwd) ./test-05daf44cb501aee6 -Zunstable-options --format=json --output_postprocess_executable=/usr/bin/jq

Usage example #6: Print test execution time in machine-readable format

$ LD_LIBRARY_PATH=$(pwd) ./test-05daf44cb501aee6 -Zunstable-options --format=json --output_postprocess_executable=/usr/bin/jq --output_postprocess_args=".exec_time | numbers"
0.234317996

Rationale for adding this functionality:

Rationale for implementation details:

@rustbot
Copy link
Collaborator

rustbot commented Mar 7, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 7, 2024
@rust-log-analyzer

This comment has been minimized.

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Mar 7, 2024
@ehuss
Copy link
Contributor

ehuss commented Mar 7, 2024

cc @rust-lang/testing-devex

@aspotashev aspotashev marked this pull request as ready for review March 7, 2024 16:43
@rustbot
Copy link
Collaborator

rustbot commented Mar 7, 2024

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

@epage
Copy link
Contributor

epage commented Mar 7, 2024

Thank you for documenting the rationale. However, could you describe what you mean by "support postprocessing" without relying on us having to dig into the code and infer it from the implementation?

@aspotashev
Copy link
Author

I've just updated the PR and commit description. Please let me know if I missed anything.

Add the following two optional flags to `libtest` (rustc's built-in unit-test
framework), in order to support postprocessing of the test results using a
separate executable:

*   `--output_postprocess_executable [PATH]`
*   `--output_postprocess_args [ARGUMENT]` (can be repeated.)

If you don't pass `--output_postprocess_executable [PATH]`, the behavior stays
the same as before this commit. That is, the test results are sent to stdout.

If you pass `--output_postprocess_executable [PATH]`, `libtest` will

1.  Spawn a child process from the executable binary (aka *postprocessor*) at
    the given path.
2.  Pass the arguments from the `--output_postprocess_args [ARGUMENT]` flags (if
    any) to the child process. If `--output_postprocess_args` was used multiple
    times, all listed arguments will be passed in the original order.
3.  Propagate the environment variables to the child process.

The *postprocessor* executable is expected to wait for the end of input (EOF)
and then terminate.

Usage example #1: Filter lines of the test results

```shell
$ LD_LIBRARY_PATH=$(pwd) ./test-05daf44cb501aee6 --output_postprocess_executable=/usr/bin/grep --output_postprocess_args="test result"
test result: ok. 59 passed; 0 failed; 1 ignored; 0 measured; 0 filtered out; finished in 0.31s
```

Usage example rust-lang#2: Save test results into a file

```shell
$ LD_LIBRARY_PATH=$(pwd) ./test-05daf44cb501aee6 --output_postprocess_executable=/usr/bin/sh --output_postprocess_args=-c --output_postprocess_args="cat > /tmp/output.txt"
```

Usage example rust-lang#3: Save test results into a file while keeping the command line
output

```shell
$ LD_LIBRARY_PATH=$(pwd) ./test-05daf44cb501aee6 --output_postprocess_executable=/usr/bin/tee --output_postprocess_args="/tmp/output.txt"

running 60 tests
...
```

Usage example rust-lang#4: Prepend every line of test results with the value of an
environment variable (to demonstrate environment variable propagation)

```shell
$ LOG_PREFIX=">>>" LD_LIBRARY_PATH=$(pwd) ./test-05daf44cb501aee6 --output_postprocess_executable=/usr/bin/sh --output_postprocess_args=-c --output_postprocess_args="sed s/^/\$LOG_PREFIX/"
>>>
>>>running 60 tests
...
```

Usage example rust-lang#5: Change format of JSON output (using
https://jqlang.github.io/jq/)

```shell
$ LD_LIBRARY_PATH=$(pwd) ./test-05daf44cb501aee6 -Zunstable-options --format=json --output_postprocess_executable=/usr/bin/jq
```

Usage example rust-lang#6: Print test execution time in machine-readable format

```shell
$ LD_LIBRARY_PATH=$(pwd) ./test-05daf44cb501aee6 -Zunstable-options --format=json --output_postprocess_executable=/usr/bin/jq --output_postprocess_args=".exec_time | numbers"
0.234317996
```

Rationale for adding this functionality:

*   Bazel (build system) doesn't provide a way to process output from a binary
    (in this case, Rust test binary's output) other using a wrapper binary.
    However, using a wrapper binary potentially breaks debugging, because Bazel
    would suggest to debug the wrapper binary rather than the Rust test itself.
    *   See bazelbuild/rules_rust#1303.
    *   Cargo is not used in Rust Bazel rules.
    *   Although we could wait for rust-lang#96290
        and then modify Rust Bazel rules to pass `--logfile` on the command line
        to provisionally unblock
        bazelbuild/rules_rust#1303, that solution
        still wouldn't allow advanced test results postprocessing such as
        changing JSON/XML schema and injecting extra JUnit properties.
*   Due to limitations of Rust libtest formatters, Rust developers often use a
    separate tool to postprocess the test results output (see comments to
    rust-lang#85563).
    *   Examples of existing postprocessing tools:
        *   https://crates.io/crates/cargo2junit
        *   https://crates.io/crates/gitlab-report
        *   https://crates.io/crates/cargo-suity
    *   For these use cases, it might be helpful to use the new flags
        `--output_postprocess_executable`, `--output_postprocess_args` instead
        of piping the test results explicitly, e.g. to more reliably separate
        test results from other output.

Rationale for implementation details:

*   Use platform-dependent scripts (.sh, .cmd) because it doesn't seem to be
    possible to enable unstable feature `bindeps`
    (https://rust-lang.github.io/rfcs/3028-cargo-binary-dependencies.html) in
    "x.py" by default.
    *   Here's a preexisting test that also uses per-platform specialization:
        `library/std/src/process/tests.rs`.
@epage
Copy link
Contributor

epage commented Mar 8, 2024

Personally, I think this is the wrong direction for libtest to be going.

This should be the responsibility of the calling tool to deal with but is being shifted here due to limitations in the calling tool. In doing this, we are increasing what we need to maintain compatibility on and what other test harness frameworks need to support for interoperability. T-testing-devex is working on shifting responsibilities from test harnesses to test runners (bazel, cargo). The hope is that libtest can remove, deprecate, or simplify as many features as possible. From there, we expect to work to make custom test harnesses first class which will need to maintain the same interface as libtest.

In addition to that...

If the post-processing is expected to be responsible for output, then this would interfere with test runners programmatically processing the output.

If the post-processing is not meant to output to stdout, then this is a logging process sync and the conversation just returns to logfile.

Either way, I feel like this would unintentionally encourage people to be programmatically reacting to non-programmatic output which can change at any time (even one of the included examples does this).

@Mark-Simulacrum
Copy link
Member

r? epage

@rustbot rustbot assigned epage and unassigned Mark-Simulacrum Mar 9, 2024
@bors
Copy link
Contributor

bors commented Mar 12, 2024

☔ The latest upstream changes (presumably #122036) made this pull request unmergeable. Please resolve the merge conflicts.

@aspotashev
Copy link
Author

Sorry for the super-late reply.

T-testing-devex is working on shifting responsibilities from test harnesses to test runners (bazel, cargo).

@epage Thanks for letting me know! This seems to be tracked in rust-lang/testing-devex-team#2 (correct me if I'm wrong), although I'm yet to evaluate how well the new approach may support running tests with Bazel.

In the meantime, I think fixing file output in XML/JSON format(s) would help unblock the specific Rust+Bazel use case that I'm working on. Looking forward to see progress on #123365 or on alternative solutions for --logfile.

@epage
Copy link
Contributor

epage commented May 21, 2024

@epage Thanks for letting me know! This seems to be tracked in rust-lang/testing-devex-team#2 (correct me if I'm wrong), although I'm yet to evaluate how well the new approach may support running tests with Bazel.

That issue is for shifting focus from libtest to custom test harnesses. That is another potential reason to limit what we add to libtest: libtest could be considered a minimum bar of support and we'd be increasing the burden on what custom test harnesses should be supporting.

The work I'm referring to is part of rust-lang/testing-devex-team#1.

@epage
Copy link
Contributor

epage commented May 21, 2024

@aspotashev it is unclear from your message. Are you seeing this PR as no longer relevant, preferring #123365 or other work instead? Should this be closed?

@aspotashev
Copy link
Author

Thanks for the update!

Let me close this one, since it may not fit well in the future plans for libtest as you described.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants