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

Check for possible silently passing invalid tests by making sure some sort of panic! exists in tests #12484

Open
kayagokalp opened this issue Mar 14, 2024 · 4 comments
Assignees
Labels
A-lint Area: New lints

Comments

@kayagokalp
Copy link

kayagokalp commented Mar 14, 2024

What it does

Consider the following code piece:

...
#[test]
fn test_with_silent_fail() {
  let result = ...;
  
  if result.is_err() {
     eprintln!("failed to do something");
  }
}

This is recently discovered for a timout use case, the test had fail cases correctly implemented but also had a outer check for timeouts with tokio runtime. If the user is using eprintln! inside a test, I feel like the test should also fail in that block. Otherwise println! should be used instead.

So in the control graph, the block that contains eprintln should contain a panic as well, or eprintln should be a println in my opinion and clippy can enforce this.

Advantage

It helps preventing cases where a test can silently pass, while the intention was to give an error thus fail.

Drawbacks

It might hurt the expressivity a little, as people may want to use eprintln inside a test for other reasons than printing an error.

Example

#[test]
fn test_with_silent_fail() {
  let result = ...;
  
  if result.is_err() {
     eprintln!("failed to do something");
  }
}

Could be written as:

#[test]
fn test_with_silent_fail() {
  let result = ...;
  
  if result.is_err() {
     panic!("failed to do something");
  }
}

or

#[test]
fn test_with_silent_fail() {
  let result = ...;
  
  if result.is_err() {
     println!("failed to do something");
  }
}
@kayagokalp kayagokalp added the A-lint Area: New lints label Mar 14, 2024
@kayagokalp kayagokalp changed the title Check for possible silently passing invalid tests by making sure some sort of panic exists in branch with eprintln Check for possible silently passing invalid tests by making sure some sort of panic exists in alongside aeprintln Mar 14, 2024
@kayagokalp kayagokalp changed the title Check for possible silently passing invalid tests by making sure some sort of panic exists in alongside aeprintln Check for possible silently passing invalid tests by making sure some sort of panic! exists in alongside a eprintln! Mar 14, 2024
@Centri3
Copy link
Member

Centri3 commented Mar 14, 2024

We could probably generalize this to tests that never actually fail (modulo println! failing or someting).

@kayagokalp
Copy link
Author

We could probably generalize this to tests that never actually fail (modulo println! failing or someting).

Yeah makes sense to me!

@kayagokalp kayagokalp changed the title Check for possible silently passing invalid tests by making sure some sort of panic! exists in alongside a eprintln! Check for possible silently passing invalid tests by making sure some sort of panic! exists in tests Oct 21, 2024
@kayagokalp
Copy link
Author

@rustbot claim

@kayagokalp
Copy link
Author

I got a reference implementation here at: #13579 Basically a generic implementation checking if a test can fail or not. If a test is not possible to panic it is flagged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints
Projects
None yet
Development

No branches or pull requests

2 participants