[willwang-io]

View all comments

Fixes #17488

Tests without assertions or other potential failure sites can pass without verifying behavior. This adds test_without_assertions, a suspicious lint that detects these tests, including empty test bodies.

changelog: [test_without_assertions]: add a suspicious lint for tests without assertions or other potential failure sites

• Followed lint naming conventions
• Added passing UI tests, including the .stderr file
• cargo test passes locally
• Executed cargo dev update_lints
• Added lint documentation
• Ran cargo dev fmt

[rustbot]

Thanks for the pull request, and welcome!

You should hear from one of our reviewers after this PR gets at least 2 reviews from the community.

Please see the contribution instructions for more information.

[CommanderStorm]

Community review

From the issue:

This can be a more general lint for tests which can never fail or always fail. Empty tests aren't particularly unique other than being the degenerate case.

I think you missed this part.
the lint I guess can start out with only linting for the degenerate case, but it should have the ambition to also lint for the provably true/false cases..

🤔
unless..
Actually I think type tests (where you ensure that for example a types size always is below a cache line for perf) cannot happen.
But that sounds a bit like an edge case
CC @Jarcho if that would be something where a "false positive" (is it one?) is good or bad?

View changes since this review

[CommanderStorm]

it would be very nice to get the #[test] attr into this span, but that sounds like some work, so this is also fine likely.

[willwang-io]

Thanks, I agree that including #[test] in the span would make the diagnostic clearer. I looked into it, but it seems that #[test] has already been expanded by the time this late lint runs. I left the current function span as is for now. Please let me know if there is a simpler approach I have missed.

[CommanderStorm]

and the requested scope widening from the issue?

@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[willwang-io]

@CommanderStorm I checked the existing assertions_on_constants lint, and it already covers things like assert!(true) and assert!(false), and several constant-valued assertion cases.

I am not yet sure which additional always-passing or always-falling test cases this lint should cover without overlapping with it, or flagging useful type/layout regression tests. Could you point me to the intended cases for this PR? I can then extend the lint accordingly.

[CommanderStorm]

I mean a testcase without assertions or other without panic sites would be my idea.
Do note that there are tests that have should panic active, I don't think we should lint on those, except for empty and if non-empty.

[willwang-io]

@CommanderStorm Thanks, that makes sense now. I’ll widen this to cover tests with no assertions or other visible panic sites, while treating ordinary calls as possible panic sites.

For #[should_panic] tests, I’ll skip the broader check since the panic may come from a call. Should an empty #[should_panic] test like this be skipped too?

#[test]
#[should_panic]
fn empty_should_panic() {}

It already fails because it returns normally instead of panicking.

[CommanderStorm]

I think lining on this since empty is fair too. It is suspicious in any case

[willwang-io]

@CommanderStorm It looks like this newly added lint is causing some existing UI and ui-toml tests to fail. I’m not sure that adding#![allow(clippy::test_without_assertions)] to unrelated fixtures is the right approach.

Is that expected for a new lint in this category, or is there a different approach you would prefer?

[CommanderStorm]

Nice progress.
I have some minor comments below, but nothing that is hard to fix

View changes since this review

[CommanderStorm]

I flew over the changes in assertions.
Nothing jumped out at me.
Make sure to use expect(..) instead of assert though ^^
Adding expect(..) to lines in the code that are truely violating this is a an indicator that the lint is working and being useful imo!

Also run an lintcheck run to see if I missed a case that should actually lint in your opinion upon second view.
I am in a plane for the next few hours, so can't do this, sorry 😔

[CommanderStorm]

I have two comments that popped up because of how this is implemented

View changes since this review

[willwang-io]

@rustbot ready

[CommanderStorm]

Community review:
Lgtm. One typo fix ^^
Make sure to squash your changes into one commit.

View changes since this review

[DanielEScherzer]

Community review: looks good to me
Looked into why there wasn't a lintcheck report - its because thee were no new lint hits, which makes sense because code doesn't normally include no-op tests

View changes since this review

[rustbot]

r? @llogiq

rustbot has assigned @llogiq for the project review.
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

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: 9 candidates
• 9 candidates expanded to 9 candidates
• Random selection from Jarcho, Manishearth, blyxyas, dswij, llogiq

[rustbot]

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.