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

fix(libtest): Deprecate '--logfile' #134283

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

epage
Copy link
Contributor

@epage epage commented 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 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 (5cc050b, #2127) and the history gives the
impression this more of slipped through rather than being an intended feature (see also
#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 #3455, this is still subject to final approval from T-libs-api.

Closes rust-lang/testing-devex-team#9

@epage epage added the A-libtest Area: `#[test]` / the `test` library label Dec 13, 2024
@rustbot
Copy link
Collaborator

rustbot commented Dec 13, 2024

r? @Noratrieb

rustbot has assigned @Noratrieb.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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 Dec 13, 2024
@rust-log-analyzer

This comment has been minimized.

@Noratrieb
Copy link
Member

r? libs-api

@rustbot rustbot added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Dec 13, 2024
@rustbot rustbot assigned Amanieu and unassigned Noratrieb Dec 13, 2024
@epage epage added the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Dec 17, 2024
library/test/src/options.rs Outdated Show resolved Hide resolved
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 force-pushed the logfile branch 2 times, most recently from eedd0a3 to 450aaa4 Compare January 7, 2025 16:57
@rustbot
Copy link
Collaborator

rustbot commented Jan 7, 2025

Some changes occurred in src/tools/cargo

cc @ehuss

@traviscross
Copy link
Contributor

This was discussed in the libs-api call today. People on the call were a bit unclear about the context, the motivation, and the broader plan, so it would help if those things might be clarified. In particular:

  • The context: What is it that --log-file does now (setting aside the proposed change in behavior in Export machine-readable test results to a file testing-devex-team#9), when was it added, why was it added in the first place, why do people use it, how widely is it used, etc.?
  • The motivation: Why specifically are we unhappy with the current behavior or existence of --log-file, what do we gain by working toward removing it, and what is the cost of not doing so?
  • The plan: Would this deprecation be working toward a full removal? If so, what's the proposal for how and when that would be done, how the impact of breaking stability here could be mitigated, etc.? Also, is this part of e.g. some larger plan to e.g. simply the arguments in general and deprecate and remove other things?

Thanks for making the proposal here and for filling in the other details that might help the team to build the context needed to make a decision on this.

@epage
Copy link
Contributor Author

epage commented Jan 7, 2025

Unsure if it was a formatting / presentation issue but most answers to these questions were in the PR description. Not all answers were included, in part because its hard to predict what all may be relevant and in part because this is the first libtest relevant work from T-testing-devex, making this the test case for how our shared responsibility for it will work.

This post re-formats the PR description around the questions and fills in some of the missing context.


What is it that --log-file does now

It will tee test output to a file using an undocumented, custom textual format

Example of a test run

ok error::rich_formats_validation_error
ok error::suggest_trailing
failed error::app_error
ok error::suggest_trailing_last
ok error::trailing_already_in_use

Example when passing --list

test action::append
test action::count
test action::count_with_default_value_if_present
test action::count_with_default_value_if_value

when was it added, why was it added in the first place

This was added Pre-1.0, see #2127

For the motivation, quoting from #2127

This is a short term fix for #2075.

When ehuss was writing end-user documentation for libtest (#82350), they said at #82350 (comment)

I was reluctant to document the specific format, since I don't know if it is intended to be stable or relied upon. It was added without much detail in #2127, and I'm not sure anyone actually uses it (I haven't seen it used before).

(while the format wasn't specified in that PR, the existence of the flag was)


why do people use it, how widely is it used, etc.?

Back at the time of #82350, ehuss was not aware of people using it. T-testing-devex is also not aware of people using it todqay.

It is difficult to evaluate how widely it is used as logfile is a very generic term to search github-wide. With more narrow searches

Looking over those, I'm not seeing any situation where the person is successful with it, usually from #105424, meaning those posts likely do not represent someone actively using it.

The only desired use cases were

For this teeing use case, the Cargo team has rejected it for now in rust-lang/cargo#14555.


The motivation: Why specifically are we unhappy with the current behavior or existence of --log-file, what do we gain by working toward removing it, and what is the cost of not doing so?

The current format is undocumented, bespoke format with limited data. There are challenges with evolving this format or switching to an alternative. These could be worked through with enough effort but the value seems low while this puts more of a burden on custom test harnesses to support and is a responsibility that would generally belong in a test runner (e.g. cargo test). For context, in past discussions with T-libs-api, the emphasis was on minimizing the scope of libtest and instead putting the focus on new features in first-class custom test harness or in test runners.

By deprecating this,

  • We feature-freeze the behavior in libtest and make the intent clear to users
  • Remove the need for custom test harnesses to implement this behavior

The plan: Would this deprecation be working toward a full removal? If so, what's the proposal for how and when that would be done, how the impact of breaking stability here could be mitigated, etc.? Also, is this part of e.g. some larger plan to e.g. simply the arguments in general and deprecate and remove other things?

A full removal was not broached as that would be a breaking change. We had not considered a mitigation strategy but I'm unsure if it is worth coming up with one vs freezing the code.

The rest was covered in the previous answer but I'll repeat for ease of discover

The larger plan is to shift the focus of feature development in libtest to Cargo and custom test harness. In doing this, we may want to deprecate stable functionality that is redundant with Cargo. For unstable features, we may want to remove those in favor of them being implemented by Cargo or custom test harnesses. Finer details have not been worked out yet as right now our focus is more on json output to support shifting responsibilities to Cargo. As part of that, we are evaluating the needs for json output through writing a custom test harness which is letting us explore what other features will be needed as we work towards first-class custom test harnesses.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-libtest Area: `#[test]` / the `test` library I-libs-api-nominated Nominated for discussion during a libs-api team meeting. 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. 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.

Export machine-readable test results to a file
7 participants