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

Add configuration option for ignoring panic!() in tests #12803

Merged
merged 1 commit into from
May 16, 2024

Conversation

wutchzone
Copy link
Contributor

changelog: [`panic`]: Now can be disabled in tests with the `allow-panic-in-tests` option

I often find myself using panic!(…) in tests a lot, where I often do something like:

match enam {
  Enam::A => …,
  Enam::B => …,
  _ => panic!("This should not happen at all."),
}

I think this patch should go nicely with already existing allow-unwrap-in-tests and allow-expect-in-tests.

@rustbot
Copy link
Collaborator

rustbot commented May 15, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @y21 (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 the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label May 15, 2024
@J-ZhengLi
Copy link
Member

It might already be clear that this would work, but do you mind adding some tests to justify it? Similar to tests/ui-toml/unwrap_used/ and tests/ui-toml/expect_used/

@wutchzone
Copy link
Contributor Author

I added the test, I took an inspiration from the tests/ui-toml/expect_used/. Thank you for pointing it out.

@y21
Copy link
Member

y21 commented May 15, 2024

Looks great, thanks!

@bors r+

@bors
Copy link
Collaborator

bors commented May 15, 2024

📌 Commit f0cc170 has been approved by y21

It is now in the queue for this repository.

bors added a commit that referenced this pull request May 15, 2024
Add configuration option for ignoring `panic!()` in tests

```
changelog: [`panic`]: Now can be disabled in tests with the `allow-panic-in-tests` option
```

I often find myself using `panic!(…)` in tests a lot, where I often do something like:

```rust
match enam {
  Enam::A => …,
  Enam::B => …,
  _ => panic!("This should not happen at all."),
}
```
I think this patch should go nicely with already existing `allow-unwrap-in-tests` and `allow-expect-in-tests`.
@bors
Copy link
Collaborator

bors commented May 15, 2024

⌛ Testing commit f0cc170 with merge 5996cc5...

@bors
Copy link
Collaborator

bors commented May 15, 2024

💔 Test failed - checks-action_test

@y21
Copy link
Member

y21 commented May 15, 2024

Needs a cargo collect-metadata run. Looks like the config option isn't correctly set in all places (I suspect it's missing in the changelog). The cargo command should correct this

@wutchzone
Copy link
Contributor Author

Updated 🤞 . You were right, missing in the changelog.

@y21
Copy link
Member

y21 commented May 16, 2024

@bors r+

@bors
Copy link
Collaborator

bors commented May 16, 2024

📌 Commit c342a61 has been approved by y21

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented May 16, 2024

⌛ Testing commit c342a61 with merge 430c885...

bors added a commit that referenced this pull request May 16, 2024
Add configuration option for ignoring `panic!()` in tests

```
changelog: [`panic`]: Now can be disabled in tests with the `allow-panic-in-tests` option
```

I often find myself using `panic!(…)` in tests a lot, where I often do something like:

```rust
match enam {
  Enam::A => …,
  Enam::B => …,
  _ => panic!("This should not happen at all."),
}
```
I think this patch should go nicely with already existing `allow-unwrap-in-tests` and `allow-expect-in-tests`.
@bors
Copy link
Collaborator

bors commented May 16, 2024

💔 Test failed - checks-action_test

@wutchzone
Copy link
Contributor Author

wutchzone commented May 16, 2024

Is it possible that I have somehow managed to break only the windows build with this patch? From the pipeline log it seems to me, that it is unable to initialize the toolchain.

  TOOLCHAIN=$(rustup show active-toolchain | cut -f1 -d' ')
  rustup run $TOOLCHAIN bash .github/driver.sh
  shell: C:\Program Files\Git\bin\bash.EXE --noprofile --norc -e -o pipefail {0}
  env:
    RUST_BACKTRACE: 1
    CARGO_TARGET_DIR: D:\a\rust-clippy\rust-clippy/target
    NO_FMT_TEST: 1
    CARGO_INCREMENTAL: 0
    OS: Windows
++ ./target/debug/clippy-driver --print sysroot
+ sysroot=
Error: Process completed with exit code 127.

@y21
Copy link
Member

y21 commented May 16, 2024

I'm not at home right now so I can't properly take a look at it (Github won't even let me view the logs), but that looks very spurious/unrelated to the change here, so...

@bors retry

@bors
Copy link
Collaborator

bors commented May 16, 2024

⌛ Testing commit c342a61 with merge 4a24494...

bors added a commit that referenced this pull request May 16, 2024
Add configuration option for ignoring `panic!()` in tests

```
changelog: [`panic`]: Now can be disabled in tests with the `allow-panic-in-tests` option
```

I often find myself using `panic!(…)` in tests a lot, where I often do something like:

```rust
match enam {
  Enam::A => …,
  Enam::B => …,
  _ => panic!("This should not happen at all."),
}
```
I think this patch should go nicely with already existing `allow-unwrap-in-tests` and `allow-expect-in-tests`.
@bors
Copy link
Collaborator

bors commented May 16, 2024

💔 Test failed - checks-action_test

@J-ZhengLi
Copy link
Member

Is it possible that I have somehow managed to break only the windows build with this patch?

Nah, look's like it's just the CI is having a stroke, same go here, don't worry

@y21
Copy link
Member

y21 commented May 16, 2024

Let's try this again with #12812 merged. Third time's a charm.

@bors retry

@bors
Copy link
Collaborator

bors commented May 16, 2024

⌛ Testing commit c342a61 with merge 5459429...

@bors
Copy link
Collaborator

bors commented May 16, 2024

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: y21
Pushing 5459429 to master...

@bors bors merged commit 5459429 into rust-lang:master May 16, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants