-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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 test_without_fail_case
lint to check if a test actually has a way to fail or not
#13579
base: master
Are you sure you want to change the base?
Add test_without_fail_case
lint to check if a test actually has a way to fail or not
#13579
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @blyxyas (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 (
|
test_without_fail_case
lint to check if a test actually has a way to fail or not
@rustbot author |
I am adding configuration capabilities to this PR and will also add more tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good starting point, but I'm worried for performance and memory usage. I'm trying to think of a way to check if a function links to the panic
handler DefId.
book/src/lint_configuration.md
Outdated
Whether to consider indexing as a fallible operation while assesing if a test can fail. | ||
Indexing is fallible, and thus the a test that is doing that can fail but it is likely | ||
that tests that fail this way were not intended. | ||
|
||
If set true, the lint will consider indexing into a slice a failable case | ||
and won't lint tests that has some sort of indexing. This analysis still done | ||
in a interprocedural manner. Meaning that any indexing opeartion done inside of | ||
a function that the test calls will still result the test getting marked fallible. | ||
|
||
By default this is set to `false`. That is because from a usability perspective, | ||
indexing an array is not the intended way to fail a test. So setting this `true` | ||
reduces false positives but makes the analysis more focused on possible byproducts | ||
of a test. That is the set of operations to get the point we assert something rather | ||
than the existance of asserting that thing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be excellent to have a way to, instead of adding a third configuration option, be able to use std::ops::Index::index
or similar in the other two configuration options.
Anyway, this is way too complex.
Whether to consider indexing as a fallible operation while assesing if a test can fail. | |
Indexing is fallible, and thus the a test that is doing that can fail but it is likely | |
that tests that fail this way were not intended. | |
If set true, the lint will consider indexing into a slice a failable case | |
and won't lint tests that has some sort of indexing. This analysis still done | |
in a interprocedural manner. Meaning that any indexing opeartion done inside of | |
a function that the test calls will still result the test getting marked fallible. | |
By default this is set to `false`. That is because from a usability perspective, | |
indexing an array is not the intended way to fail a test. So setting this `true` | |
reduces false positives but makes the analysis more focused on possible byproducts | |
of a test. That is the set of operations to get the point we assert something rather | |
than the existance of asserting that thing. | |
Whether to consider indexing (`a[b]`) as a fallible operation while checking if a test can fail. | |
Indexing is fallible, and thus it can panic, but this panic is likely not intended to be tested. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be excellent to have a way to, instead of adding a third configuration option, be able to use std::ops::Index::index or similar in the other two configuration options.
I agree, I'll think about it
indexing_fallible: conf.test_without_fail_case_include_indexing_as_fallible, | ||
fallible_paths: conf.test_without_fail_case_fallible_paths.iter().cloned().collect(), | ||
non_fallible_paths: conf.test_without_fail_case_non_fallible_paths.iter().cloned().collect(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason that these are FxHashSet
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly out of habit of using FxHashSet
in the compiler for performance reasons. But given that this is only the configuration I'll change it into HashSet
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant: Is there a reason is it not a simple Vec
? We prefer FxHashSet
to HashSet
, but I just see no use for this to be a hashed collection. (Unless there's a very performance improvement I'm not aware of)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it is a small lookup table, as the list coming from configuration file likely to be limited, can't see a reason why it couldn't be a Vec
for sure. Can change it.
type NestedFilter = nested_filter::OnlyBodies; | ||
|
||
fn visit_expr(&mut self, expr: &'tcx Expr<'_>) { | ||
if self.fail_found { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you use ControlFlow<()>
instead of self.fail_found
?
The Visitor trait accepts it :)
|
||
impl<'tcx> Visitor<'tcx> for SearchFailIntraFunction<'_, 'tcx> { | ||
type NestedFilter = nested_filter::OnlyBodies; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type Result = ControlFlow<()>; |
d2c1c4b
to
1ab9ae6
Compare
Co-authored-by: Alejandra González <[email protected]>
If there is way to see if a function links to a panic handler without visiting them explicitly that would be great! Otherwise to access the performance characteristics it might be wise to take a look at set of benchmarks. I am curious to see how many interleaved levels of functions calls are average/likely to be upper limit. |
[`test-without-fail-case-fallible-paths`]: https://doc.rust-lang.org/clippy/lint_configuration.html#test-without-fail-case-fallible-paths | ||
[`test-without-fail-case-include-indexing-as-fallible`]: https://doc.rust-lang.org/clippy/lint_configuration.html#test-without-fail-case-include-indexing-as-fallible | ||
[`test-without-fail-case-non-fallible-paths`]: https://doc.rust-lang.org/clippy/lint_configuration.html#test-without-fail-case-non-fallible-paths |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find 3 configuration values for this lint a bit much. Also those are a mouth full.
I think there should only one configuration option at most: A list of items/functions that should prevent triggering the lint.
fn implicit_panic() { | ||
panic!("this is an implicit panic"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect that most of those functions, that move the assertion or panic out of the #[test]
function is defined inside the same module as the tested code. Pruning the functions this lint checks with this heuristic, whether they panic, might prevent big performance impacts this lint might have.
fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'tcx>) { | ||
// Only interested in functions that are annotated with `#[test]`. | ||
if let ItemKind::Fn(_, _, body_id) = item.kind | ||
&& is_in_test_function(cx.tcx, item.hir_id()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This lint only runs on tests. This also means, that the lint can only detect something, if Clippy is run with cargo clippy --all-targets
or cargo clippy --tests
. This should be mentioned in the documentation of the lint.
I'm not sure, if lintcheck
is run with --all-targets
. It at least didn't trigger once for all the crates we test on.
related to #12484.
changelog: Adds a
test_without_fail_case
lint which checks if a test can fail or not. If failure is not possible, the test is linted. This is done to prevent any silently failing tests that actually was not asserting anything, improving test suite quality.