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

Show test type during prints #84863

Merged
merged 6 commits into from
Jun 6, 2021
Merged

Show test type during prints #84863

merged 6 commits into from
Jun 6, 2021

Conversation

ABouttefeux
Copy link
Contributor

@ABouttefeux ABouttefeux commented May 3, 2021

Test output can sometimes be confusing. For example doctest with the no_run argument are displayed the same way than test that are run.

During #83857 I got the feedback that test output can be confusing.

For the moment test output is

test $DIR/test-type.rs - f (line 12) ... ignored
test $DIR/test-type.rs - f (line 15) ... ok
test $DIR/test-type.rs - f (line 21) ... ok
test $DIR/test-type.rs - f (line 6) ... ok

I propose to change output by indicating the test type as

test $DIR/test-type.rs - f (line 12) ... ignored
test $DIR/test-type.rs - f (line 15) - compile ... ok
test $DIR/test-type.rs - f (line 21) - compile fail ... ok
test $DIR/test-type.rs - f (line 6) ... ok

by indicating the test type after the test name (and in the case of doctest after the function name and line) and before the "...".


Note: this is a proof of concept, the implementation is probably not optimal as the properties added in TestDesc are only use in the display and does not represent actual change of behavior, maybe TestType::DocTest could have fields

@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 3, 2021
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

Copy link
Member

@m-ou-se m-ou-se left a comment

Choose a reason for hiding this comment

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

I like this change. But for regular #[test]s the output looks a bit messy, since there's nothing that visually separates the test name from the test type:

running 4 tests
test hello ignore ... ignored
test hey should panic ... ok
test hi run ... ok
test hi2 run ... ok

Maybe it should be in parenthesis? Or preceded by - or : ?

(E.g.

running 4 tests
test hello - ignore ... ignored
test hey - should panic ... ok
test hi - run ... ok
test hi2 - run ... ok

)

What do you think?

library/test/src/types.rs Outdated Show resolved Hide resolved
library/test/src/types.rs Outdated Show resolved Hide resolved
@m-ou-se m-ou-se added needs-fcp This change is insta-stable, so needs a completed FCP to proceed. A-libtest Area: #[test] related T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels May 3, 2021
@bjorn3
Copy link
Member

bjorn3 commented May 3, 2021

test $DIR/test-type.rs - f (line 12) ignore ... ignored
test $DIR/test-type.rs - f (line 15) compile ... ok
test $DIR/test-type.rs - f (line 21) compile fail ... ok
test $DIR/test-type.rs - f (line 6) run ... ok

I think ignore and run should be omitted as they are redundant cq the most common type.

@ABouttefeux
Copy link
Contributor Author

ABouttefeux commented May 3, 2021

Concerning the failing CI, I am not sure where it came from. Correct me if I am wrong but I think the problem is that it tries to build the library at stage 0 (so with the beta compiler) but it does not have the change of the #[test] macro so fail to build test function.
Is there anything I can do ?

@rust-log-analyzer

This comment has been minimized.

@m-ou-se
Copy link
Member

m-ou-se commented May 4, 2021

Concerning the failing CI, I am not sure where it came from. Correct me if I am wrong but I think the problem is that it tries to build the library at stage 0 (so with the beta compiler) but it does not have the change of the #[test] macro so fail to build test function.
Is there anything I can do ?

Indeed. Everything library/std, library/test, etc. are all used with both the beta compiler and the newly built compiler. So the beta compiler will expand #[test] to something that does not include the new fields, while using the test crate that already has those.

You can use cfg(bootstrap) to enable/disable code in this first stage. So you could apply #[cfg(not(bootstrap)] to the new compile_fail and no_run fields. (Those cfgs will be removed when upgrading the stage 0 compiler.) Alternatively, you could try to implement this feature in such a way that the code that #[test] currently expands to in beta still works, but that's probably more work.

@m-ou-se m-ou-se assigned m-ou-se and unassigned kennytm May 4, 2021
@m-ou-se m-ou-se added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 5, 2021
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@ABouttefeux ABouttefeux marked this pull request as ready for review May 9, 2021 15:00
@ABouttefeux
Copy link
Contributor Author

I remove the redundant print for run and ignore. Let me know if I need to change something else 😄.

@rust-log-analyzer

This comment has been minimized.

@rfcbot
Copy link

rfcbot commented May 19, 2021

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

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels May 19, 2021
@pickfire
Copy link
Contributor

I would prefer to keep it short. Just wondering what happens if the test name is too long and overflow an 80 characters terminal? Wouldn't that be ugly? Or is there some truncation mechanism?

@ABouttefeux
Copy link
Contributor Author

I would prefer to keep it short. Just wondering what happens if the test name is too long and overflow an 80 characters terminal? Wouldn't that be ugly? Or is there some truncation mechanism?

I don't think there is a truncation mechanism.

@pickfire
Copy link
Contributor

I think once this issue is merged we need to add a truncation mechanism, because with the addition of - compile fail, there are too many characters and now the smaller terminal will have a worse experience, maybe we have truncation mechanism to disable this part first.

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels May 29, 2021
@rfcbot
Copy link

rfcbot commented May 29, 2021

The final comment period, with a disposition to merge, 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.

The RFC will be merged soon.

@rfcbot rfcbot added the to-announce Announce this issue on triage meeting label May 29, 2021
@m-ou-se
Copy link
Member

m-ou-se commented Jun 5, 2021

Thanks again!

@bors r+

@bors
Copy link
Contributor

bors commented Jun 5, 2021

📌 Commit 6de13c3 has been approved by m-ou-se

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 5, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jun 5, 2021
Show test type during prints

Test output can sometimes be confusing. For example doctest with the no_run argument are displayed the same way than test that are run.

During rust-lang#83857 I got the feedback that test output can be confusing.

For the moment test output is
```
test $DIR/test-type.rs - f (line 12) ... ignored
test $DIR/test-type.rs - f (line 15) ... ok
test $DIR/test-type.rs - f (line 21) ... ok
test $DIR/test-type.rs - f (line 6) ... ok
```

I propose to change output by indicating the test type as
```
test $DIR/test-type.rs - f (line 12) ... ignored
test $DIR/test-type.rs - f (line 15) - compile ... ok
test $DIR/test-type.rs - f (line 21) - compile fail ... ok
test $DIR/test-type.rs - f (line 6) ... ok
```
by indicating the test type after the test name (and in the case of doctest after the function name and line) and before the "...".

------------

Note: this is a proof of concept, the implementation is probably not optimal as the properties added in `TestDesc` are only use in the display and does not represent actual change of behavior, maybe `TestType::DocTest` could have fields
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jun 5, 2021
Show test type during prints

Test output can sometimes be confusing. For example doctest with the no_run argument are displayed the same way than test that are run.

During rust-lang#83857 I got the feedback that test output can be confusing.

For the moment test output is
```
test $DIR/test-type.rs - f (line 12) ... ignored
test $DIR/test-type.rs - f (line 15) ... ok
test $DIR/test-type.rs - f (line 21) ... ok
test $DIR/test-type.rs - f (line 6) ... ok
```

I propose to change output by indicating the test type as
```
test $DIR/test-type.rs - f (line 12) ... ignored
test $DIR/test-type.rs - f (line 15) - compile ... ok
test $DIR/test-type.rs - f (line 21) - compile fail ... ok
test $DIR/test-type.rs - f (line 6) ... ok
```
by indicating the test type after the test name (and in the case of doctest after the function name and line) and before the "...".

------------

Note: this is a proof of concept, the implementation is probably not optimal as the properties added in `TestDesc` are only use in the display and does not represent actual change of behavior, maybe `TestType::DocTest` could have fields
@bors
Copy link
Contributor

bors commented Jun 6, 2021

⌛ Testing commit 6de13c3 with merge 3740ba2...

@bors
Copy link
Contributor

bors commented Jun 6, 2021

☀️ Test successful - checks-actions
Approved by: m-ou-se
Pushing 3740ba2 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 6, 2021
@bors bors merged commit 3740ba2 into rust-lang:master Jun 6, 2021
@rustbot rustbot added this to the 1.54.0 milestone Jun 6, 2021
@ghost ghost mentioned this pull request Jun 7, 2021
bors added a commit to rust-lang/miri that referenced this pull request Jun 7, 2021
Update `cargo-miri` tests

The test output has been changed recently (likely by rust-lang/rust#84863) and caused a test failure: https://github.com/rust-lang/miri/runs/2761591139#step:8:1228
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Jun 10, 2021
cormacrelf added a commit to cormacrelf/datatest that referenced this pull request Aug 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-libtest Area: #[test] related disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. merged-by-bors This PR was explicitly merged by bors. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.