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

Export machine-readable test results to a file #9

Open
aspotashev opened this issue Jul 3, 2024 · 15 comments · May be fixed by rust-lang/rust#134283
Open

Export machine-readable test results to a file #9

aspotashev opened this issue Jul 3, 2024 · 15 comments · May be fixed by rust-lang/rust#134283

Comments

@aspotashev
Copy link

aspotashev commented Jul 3, 2024

As a follow up to the testing-devex's feedback for rust-lang/rust#123365 (draft), let me describe on a high level our use case for capturing JUnit output when running Rust test via Bazel.

In our setup, Bazel (build system) runs Rust test binaries. We use a slightly patched version of https://github.com/bazelbuild/rules_rust. Bazel is a mediator between the terminal UI, IDEs, code review websites, or remote build/test services, and the executed test. Bazel runs the test binary directly, i.e. doesn't use cargo test. Bazel uses JUnit XML files for communicating machine-parsable language-agnostic test results (already supported by GoogleTest, JUnit, and other languages). Right now, there is no mechanism to communicate Rust test results through Bazel to users (again, terminal, IDE plugins...). See also Bazel Rust rules FR.


One straightforward candidate solution consists of these 2 steps:

  1. Have CLI flag(s) defined in Rust libtest to control export of machine-readable test results (e.g. JUnit or JSON) from the Rust test processes. For context, these PRs were attempts to add/fix such CLI flag(s):
  2. Modify Bazel Rust rules to pass the relevant CLI flags.

We also considered using a wrapper binary (Bazel runs wrapper, wrapper runs Rust test binary and captures stdout), however it has downsides which are hard-to-impossible to resolve:

  • Capturing an output stream (e.g. stdout, stderr) isn't reliable because a Rust test may also write directly to stdout and that will garble XML/JSON test results output from libtest.
  • Without a significant rework of Bazel, the systems that sit on top of it (e.g. IDEs) will treat the wrapper as a main application, therefore an naive attempt to debug a Rust test would start a debugger for the wrapper instead.

I'm looking forward to hearing your suggestions for problem resolutions that may be acceptable from the perspective of T-testing-devex. If we find a solution that doesn't require a significant rework of libtest / Rust testing framework, that would be great, and I would then be interested in contributing the relevant code.

@heisen-li
Copy link

What is the purpose of exporting test results to a file?My guess is to better manage the testing process and analyze test results.Perhaps consider generating a more comprehensive test report?

In addition to keeping the logfile file consistent with stdout, perhaps consider displaying some other information in the file.For example:

  • show the time spent on each test function: --report-time;
  • even include cpu usage efficiency and more precise memory allocation: refer to golang?
  • Is it necessary to keep a consistent order of output in multithreaded cases?

@epage
Copy link

epage commented Jul 5, 2024

Wanted to highlight some things needed for moving this forward

we need more information about current logfile usage patterns so that we can provide a recommendation on next steps.

Could you share some additional background about the issue you're facing with the current format discrepancy

Particularly

  • Why does this need to baked in, rather than something built on top of json output?

identify use cases leveraging the logfile in its current format?

@epage
Copy link

epage commented Jul 5, 2024

In particular, the unofficial (atm) goal is to narrowly focus the official libtest's functionality

  • Reduce the minimal burden on alternative implementations
  • Reduce the API (including CLI) burden on T-libs-api and maintenance burden on T-libs.

We generally see new feature development happening in either

  • Third party test harnesses (i.e. alternatives to libtest)
  • Tooling layers that sit above the test harness (e.g. cargo test or bazel)

@aspotashev
Copy link
Author

aspotashev commented Oct 15, 2024

Sorry for the huge delay. Let me address Ed's questions and propose next steps:

we need more information about current logfile usage patterns so that we can provide a recommendation on next steps.

Unfortunately, I don't have data on this and not sure how to collect it. If behavior change in --logfile is considered a big concern because people may rely on the one-line-per-test output, we can modify aspotashev/rust#1 (comment) to only have effect when --format=json or --format=junit is also passed and therefore still solve rust-lang/rust#57147 (more details in the proposal below). IMO it's reasonable to assume that if someone depends on the current --logfile output format, they wouldn't additionally pass --format=json or --format=junit because --format wouldn't influence the --logfile output format (in its today's implementation).

Could you share some additional background about the issue you're facing with the current format discrepancy
Why does this need to baked in, rather than something built on top of json output?

Our use case would be satisfied if rust-lang/rust#57147 was fixed in one way or another. Having at least one machine-readable output format that writes into a file would be sufficient for us. If it would be JSON,

  • Bazel users can configure their CI to convert JSON files into XML files after bazel test finishes. Although it may not be perfect for integrating with IDEs, the major downsides mentioned in Supporting junit XML report in rust_test bazelbuild/rules_rust#1303 would be addressed.
  • In our fairly customized setup, there's a different workaround that allows us to postprocess the JSON files and convert them to XML of the appropriate format.

The reason why I also attempted to address discrepancy in the same PR was the general willingness to avoid code repetition. Having 2 code paths carries the risk of them going out of sync.

In particular, the unofficial (atm) goal is to narrowly focus the official libtest's functionality

If reworked JSON output (https://github.com/rust-lang/libtest-next/milestone/1) would land in a file, we will similarly be able to build postprocessing on top of it. Only having JSON output on stdout would be problematic because

  1. It's harder to integrate with, at least in Bazel
  2. The output JSON may be garbled by other stdout output produced by code under test (IIUC, std::io would be captured, but e.g. C dependency libraries may still access stdout/stderr directly).

Although it may be fine to just wait for libtest-next to land, it would be better to also have a shorter term solution.

Would a solution similar to rust-lang/rust#123365 yet preserving the current behavior of --logfile & no --format be reasonable? What do you think?

More specifically:

  • Refactor the current logfile output logic into a new formatter compact.
  • When you pass --logfile, the format defaults to compact rather than pretty (unless --format=... overrides it).
  • Both stdout and the log file receive the same content.

To visualize, here's how --format and --logfile would interact. Each table cell at an intersection described the resulting format(s). "both: ..." means both stdout and logfile receive output in the same format.

today today linked PR linked PR proposal proposal
--format no --logfile --logfile no --logfile --logfile no --logfile --logfile
(unset) pretty stdout: pretty, logfile: compact pretty both: pretty pretty both: compact
json json stdout: json, logfile: compact json both: json json both: json
junit junit stdout: junit, logfile: compact junit both: junit junit both: junit
pretty pretty stdout: pretty, logfile: compact pretty both: pretty pretty both: pretty
terse terse stdout: terse, logfile: compact terse both: terse terse both: terse
compact not supported not supported not supported not supported compact both: compact

@aspotashev
Copy link
Author

What is the purpose of exporting test results to a file?My guess is to better manage the testing process and analyze test results.Perhaps consider generating a more comprehensive test report?

@heisen-li , exporting test results to a file makes it possible to read those results without having to capture stdout/stderr of the Rust test binary. I mentioned more specific reasons in my previous comment ("Only having JSON output on stdout would be problematic ...")

In addition to keeping the logfile file consistent with stdout, perhaps consider displaying some other information in the file.

This may be doable, however is orthogonal to the problems I'm trying to address in the current issue: Whatever decision we make around machine-readable output to a file, it will not make enriching the output with more information neither harder nor easier. If you feel strongly about report-time or CPU usage reporting, maybe it deserves a separate feature request at https://github.com/rust-lang/rust/issues?

@epage
Copy link

epage commented Oct 30, 2024

While I still need to catch up on your comments, I wanted to note that the Cargo team has recently rejected the idea of teeing separate programmatic / human output modes, see rust-lang/cargo#14555 and that is for a higher level command. rustc and libtest are generally lower level.

@epage
Copy link

epage commented Nov 5, 2024

More personal notes so I don't forget

As --logfile is stableand--formatis stable, changing the output of logfile based on--format` would be a breaking change.

This also calls for stabilizing a new --format value, compact, and the impression I have of libs-api is they aren't too keen on stabilizing additional surface area in test.

@epage epage added the T-testing-devex Team: Testing DevEx label Nov 6, 2024
@epage
Copy link

epage commented Nov 6, 2024

We talked about this in yesterday's T-testing-devex meeting. We did not have everyone so I'm going to attempt an FCP to get confirmation from the team on our proposal.

@rfcbot fcp close

We propose that, instead of changing --logfile, we deprecate it. We would post a PR that would mark it as deprecated in the help and print out a warning on use (maybe gated by --format pretty or something).

The proposal lists two reasons for changing the behavior of --logfile

(1) Bazel can't programmatically process stdout. This seems like a limitation in Bazel and we recommend focusing on that. If we look at the wider Rust ecosystem, Rustc and Cargo don't support any such mechanism and the Cargo team rejected having one. Expecting this in libtest when its not supported elsewhere seems too specialized.

(2) Tests that leak out non-programmatic output that intermixes with programmatic output. We acknowledge this is a problem to be evaluated but we need to make sure we are stepping back and gathering requirements, rather than assuming --logfile will fit the needs.

Independent of the motive, regarding using or changing --logfile

(1) Most ways to do it would be a breaking change, like if we respect any stable --format. As suggested above, we could specialize this to new --format values but that would be confusing for some values to apply but not others.

(2) Other ways of solving this add new features to libtest when we are instead wanting to limit the feature set it has to minimize the compatibility surface that has to be maintained and the burden it would put on third party harnesses which are a focus area. Examples include --format compact or a --log-format flag

(3) The existence of --logfile dates back quite a ways (rust-lang/rust@5cc050b, rust-lang/rust#2127) and the history gives the impression this more of slipped through rather than being an intended feature (see also rust-lang/rust#82350 (comment)). Deprecation would better match to how it has been treated. Deprecation would also further reduce the burden on third party harnesses

@rfcbot

This comment has been minimized.

@epage

This comment has been minimized.

@rfcbot

This comment has been minimized.

@epage
Copy link

epage commented Nov 6, 2024

@rfcbot fcp close

@rfcbot
Copy link
Collaborator

rfcbot commented Nov 6, 2024

Team member @epage has proposed to close this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot
Copy link
Collaborator

rfcbot commented Nov 19, 2024

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot
Copy link
Collaborator

rfcbot commented Nov 29, 2024

The final comment period, with a disposition to close, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

epage added a commit to epage/rust that referenced this issue Dec 13, 2024
rust-lang/testing-devex-team#9 proposed changing the behavior of `--logfile`.
The given reasons were:

(1) Bazel can't programmatically process stdout.  This seems like a
limitation in Bazel and we recommend focusing on that.  If we look at
the wider Rust ecosystem, Rustc and Cargo don't support any such
mechanism and the Cargo team rejected having one.  Expecting this in
libtest when its not supported elsewhere seems too specialized.

(2) Tests that leak out non-programmatic output that intermixes with
programmatic output.  We acknowledge this is a problem to be evaluated
but we need to make sure we are stepping back and gathering
requirements, rather than assuming `--logfile` will fit the needs.

Independent of the motive, regarding using or changing  `--logfile`

(1) Most ways to do it would be a breaking change, like if we respect
any stable `--format`.  As suggested above, we could specialize this to
new `--format` values but that would be confusing for some values to
apply but not others.

(2) Other ways of solving this add new features to lib`test` when we are
instead wanting to limit the feature set it has to minimize the
compatibility surface that has to be maintained and the burden it would
put on third party harnesses which are a focus area.  Examples include
`--format compact` or a `--log-format` flag

(3) The existence of `--logfile` dates back quite a ways
(rust-lang@5cc050b,
rust-lang#2127) and the history gives the
impression this more of slipped through rather than being an intended
feature (see also
rust-lang#82350 (comment)).
Deprecation would better match to how it has been treated.
By deprecating this, we do not expect custom test harnesses
(rust-lang/testing-devex-team#2) to implement this.

T-testing-devex held an FCP for deprecating in rust-lang/testing-devex-team#9
though according to
[RFC rust-lang#3455](https://rust-lang.github.io/rfcs/3455-t-test.html),
this is still subject to final approval from T-libs-api.
@epage epage linked a pull request Dec 13, 2024 that will close this issue
epage added a commit to epage/rust that referenced this issue Dec 13, 2024
rust-lang/testing-devex-team#9 proposed changing the behavior of `--logfile`.
The given reasons were:

(1) Bazel can't programmatically process stdout.  This seems like a
limitation in Bazel and we recommend focusing on that.  If we look at
the wider Rust ecosystem, Rustc and Cargo don't support any such
mechanism and the Cargo team rejected having one.  Expecting this in
libtest when its not supported elsewhere seems too specialized.

(2) Tests that leak out non-programmatic output that intermixes with
programmatic output.  We acknowledge this is a problem to be evaluated
but we need to make sure we are stepping back and gathering
requirements, rather than assuming `--logfile` will fit the needs.

Independent of the motive, regarding using or changing  `--logfile`

(1) Most ways to do it would be a breaking change, like if we respect
any stable `--format`.  As suggested above, we could specialize this to
new `--format` values but that would be confusing for some values to
apply but not others.

(2) Other ways of solving this add new features to lib`test` when we are
instead wanting to limit the feature set it has to minimize the
compatibility surface that has to be maintained and the burden it would
put on third party harnesses which are a focus area.  Examples include
`--format compact` or a `--log-format` flag

(3) The existence of `--logfile` dates back quite a ways
(rust-lang@5cc050b,
rust-lang#2127) and the history gives the
impression this more of slipped through rather than being an intended
feature (see also
rust-lang#82350 (comment)).
Deprecation would better match to how it has been treated.
By deprecating this, we do not expect custom test harnesses
(rust-lang/testing-devex-team#2) to implement this.

T-testing-devex held an FCP for deprecating in rust-lang/testing-devex-team#9
though according to
[RFC rust-lang#3455](https://rust-lang.github.io/rfcs/3455-t-test.html),
this is still subject to final approval from T-libs-api.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants