Skip to content

Remove backticks from ShouldPanic::YesWithMessage's TrFailedMsg #136160

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.

Sign up for GitHub

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

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

Remove backticks from ShouldPanic::YesWithMessage's TrFailedMsg #136160

wants to merge 1 commit into from
+10 −11

Conversation

ShE3py
Copy link
Contributor

@ShE3py ShE3py commented Jan 27, 2025 •
edited
Loading

More legible imo

#[test]
#[should_panic = "love"]
fn foo() {
    assert!(1 == 2);
}

Before:

note: panic did not contain expected string
      panic message: `"assertion failed: 1 == 2"`,
 expected substring: `"love"`

After:

note: panic did not contain expected string
      panic message: "assertion failed: 1 == 2"
 expected substring: "love"

Also removed the comma as assert_eq! / assert_ne! don't use one.

@rustbot label +A-libtest

@rustbot
Copy link
Collaborator

rustbot commented Jan 27, 2025

r? @thomcc

rustbot has assigned @thomcc.
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. A-libtest Area: `#[test]` / the `test` library labels Jan 27, 2025
@ShE3py ShE3py force-pushed the should-panic-backticks branch from b227014 to afb78f0 Compare January 27, 2025 23:12
@thomcc thomcc added the T-testing-devex Relevant to the testing devex team (testing DX), which will review and decide on the PR/issue. label Jan 28, 2025
@thomcc thomcc removed the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Mar 16, 2025
@thomcc
Copy link
Member

thomcc commented Mar 16, 2025

I doubt anybody feels strongly about this, but I guess it is superfluous.

@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented Mar 16, 2025 •
edited by calebcartwright
Loading

Team member @thomcc has proposed to merge 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 rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Mar 16, 2025
@epage
Copy link
Contributor

epage commented Mar 17, 2025

My comment from [zulip](#t-testing-devex > Backticks around should_panic output)

I feel like I can go either way. I understand why backticks are used (literal code) and why its superfluous (its just strings).

If none of us feel strongly about something, how do we break the indifference-tie?

@ShE3py
Copy link
Contributor Author

ShE3py commented Mar 17, 2025

I don't mind it that much, it's just that I find `"foo"` hard to read. It will also matches assert_eq!, which doesn't use backticks:

assertion `left == right` failed
  left: "a"
 right: "b"

@epage
Copy link
Contributor

epage commented Mar 17, 2025

Thanks for calling out the inconsistency. That is reason enough to change one and assert_eq is the one people have seen more and likely put more attention into, so I'm fine matching it.

@weihanglo
Copy link
Member

@rfcbot reviewed

I am okay with either way.

To check whether Cargo's test suite need to change any output snapshot, I am going to do a try build.

@bors try

@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Mar 18, 2025
@rfcbot
Copy link
Collaborator

rfcbot commented Mar 18, 2025

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

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Mar 18, 2025
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 18, 2025
@bors
Auto merge of rust-lang#136160 - ShE3py:should-panic-backticks, r=<try>
f95fba5 
Remove backticks from `ShouldPanic::YesWithMessage`'s `TrFailedMsg`

More legible imo
```rs
#[test]
#[should_panic = "love"]
fn foo() {
    assert!(1 == 2);
}
```
Before:
```
note: panic did not contain expected string
      panic message: `"assertion failed: 1 == 2"`,
 expected substring: `"love"`
```
After:
```
note: panic did not contain expected string
      panic message: "assertion failed: 1 == 2"
 expected substring: "love"
```
Also removed the comma as `assert_eq!` / `assert_ne!` don't use one.

`@rustbot` label +A-libtest
@bors
Copy link
Collaborator

bors commented Mar 18, 2025

⌛ Trying commit afb78f0 with merge f95fba5...

@bors
Copy link
Collaborator

bors commented Mar 18, 2025

☀️ Try build successful - checks-actions
Build commit: f95fba5 (f95fba5bf499cd1aca21c54617540bd025a84653)

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Mar 28, 2025
@rfcbot
Copy link
Collaborator

rfcbot commented Mar 28, 2025

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.

This will be merged soon.

@rustbot rustbot mentioned this pull request Mar 28, 2025
Open
@thomcc
Copy link
Member

thomcc commented Apr 27, 2025

@bors r+

@bors
Copy link
Collaborator

bors commented Apr 27, 2025

📌 Commit afb78f0 has been approved by thomcc

It is now in the queue for this repository.

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 27, 2025
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Apr 27, 2025
ChrisDenton added a commit to ChrisDenton/rust that referenced this pull request Apr 28, 2025
@ChrisDenton
Rollup merge of rust-lang#136160 - ShE3py:should-panic-backticks, r=t…
de05c64 
…homcc

Remove backticks from `ShouldPanic::YesWithMessage`'s `TrFailedMsg`

More legible imo
```rs
#[test]
#[should_panic = "love"]
fn foo() {
    assert!(1 == 2);
}
```
Before:
```
note: panic did not contain expected string
      panic message: `"assertion failed: 1 == 2"`,
 expected substring: `"love"`
```
After:
```
note: panic did not contain expected string
      panic message: "assertion failed: 1 == 2"
 expected substring: "love"
```
Also removed the comma as `assert_eq!` / `assert_ne!` don't use one.

`@rustbot` label +A-libtest
@ChrisDenton ChrisDenton mentioned this pull request Apr 28, 2025
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 28, 2025
@bors
Auto merge of rust-lang#140376 - ChrisDenton:rollup-use71pz, r=ChrisD…
4f1494c 
…enton

Rollup of 10 pull requests

Successful merges:

 - rust-lang#136160 (Remove backticks from `ShouldPanic::YesWithMessage`'s `TrFailedMsg`)
 - rust-lang#138395 (Download GCC from CI on test builders)
 - rust-lang#138737 (uefi: Update r-efi)
 - rust-lang#138939 (Add `Arc::is_unique`)
 - rust-lang#139224 (fix(test): Expose '--no-capture' in favor of `--nocapture`)
 - rust-lang#139546 (std(docs): clarify how std::fs::set_permisions works with symlinks)
 - rust-lang#139883 (crashes: more tests)
 - rust-lang#140345 (Avoid re-interning in `LateContext::get_def_path`)
 - rust-lang#140351 (docs: fix incorrect stability markers on `std::{todo, matches}`)
 - rust-lang#140359 (specify explicit safety guidance for from_utf8_unchecked)

r? `@ghost`
`@rustbot` modify labels: rollup
@ChrisDenton
Copy link
Member

@bors r-

failed in #140376 (comment)

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 28, 2025
@ShE3py ShE3py force-pushed the should-panic-backticks branch from afb78f0 to 55a419f Compare April 28, 2025 19:43
ShE3py
ShE3py commented Apr 28, 2025
View reviewed changes
tests/ui/test-attrs/test-should-panic-failed-show-span.run.stdout
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Blessed this file (from PR #138603.)
@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@thomcc thomcc

Labels
A-libtest Area: `#[test]` / the `test` library 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. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-testing-devex Relevant to the testing devex team (testing DX), which will review and decide on the PR/issue. to-announce Announce this issue on triage meeting
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@ShE3py @rustbot @thomcc @rfcbot @epage @weihanglo @bors @ChrisDenton