Lint to prevent redundant test_ prefix in test names.
#12861
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| use clippy_utils::diagnostics::span_lint_and_help; | ||
| use clippy_utils::{is_in_cfg_test, is_in_test_function}; | ||
| use rustc_hir::intravisit::FnKind; | ||
| use rustc_hir::{Body, FnDecl}; | ||
| use rustc_lint::{LateContext, LateLintPass}; | ||
| use rustc_session::declare_lint_pass; | ||
| use rustc_span::def_id::LocalDefId; | ||
| use rustc_span::Span; | ||
|
|
||
| declare_clippy_lint! { | ||
| /// ### What it does | ||
| /// Triggers when a function annotated with the `#[test]` macro begins with `test_` | ||
| /// ### Why is this bad? | ||
| /// This is redundant, since the annotations already denotes the function a test. | ||
| /// Tests are executed using `cargo test `module::test-name-here::`. | ||
| /// It's clear that a test is a test, without prefixing `test_` to the test name. | ||
| /// ### Example | ||
| /// ```no_run | ||
| /// #[test] | ||
| /// fn my_test_case() { | ||
| /// // test logic here... | ||
| /// } | ||
| /// ``` | ||
| /// Use instead: | ||
| /// ```no_run | ||
| /// #[test] | ||
| /// fn my_test_case() { | ||
| /// // test logic here... | ||
| /// } | ||
| /// ``` | ||
| #[clippy::version = "1.79.0"] | ||
| pub REDUNDANT_TEST_PREFIX, | ||
| pedantic, | ||
|
There was a problem hiding this comment. I feel like this shouldn't be in I'll include this decision in the group discussion before we merge this PR :) There was a problem hiding this comment. There are arguments to be made for several categories: The usage of There was a problem hiding this comment. One argument I always consider is the number of lint triggers that happen in current code. Clippy can be a very pedantic, but the number of lint emissions has to be reasonable. If users get 100+ warnings of this lint with the next Clippy release it's way more likely that users get annoyed and simply allow the lint and that would help nobody. This is not meant as a hard no, but just an explanation of my suggestion. I'm not sure where to place it and would leave this up to a group decision. |
||
| "A test name is prefixed with `test`" | ||
| } | ||
|
|
||
| declare_lint_pass!(RedundantTestPrefix => [REDUNDANT_TEST_PREFIX]); | ||
|
|
||
| impl LateLintPass<'_> for RedundantTestPrefix { | ||
| fn check_fn( | ||
| &mut self, | ||
| cx: &LateContext<'_>, | ||
| fn_kind: FnKind<'_>, | ||
| _: &FnDecl<'_>, | ||
| body: &Body<'_>, | ||
| span: Span, | ||
| _: LocalDefId, | ||
| ) { | ||
| if is_prefixed_with_test_fn(fn_kind) | ||
| && is_in_test_function(cx.tcx, body.id().hir_id) | ||
| && is_in_cfg_test(cx.tcx, body.id().hir_id) | ||
| { | ||
| span_lint_and_help( | ||
|
There was a problem hiding this comment. Would be nicer if this lint could be auto-fixed, so, maybe use But then it'll make things a bit more complicated when such test function is called in another test function, such as: #[cfg(test)]
mod tests {
#[test]
fn test_a() {
// ...
}
#[test]
fn test_b() {
// ...
}
#[test]
fn test_a_and_b() {
test_a();
test_b();
}
}therefore, you'll need to check for the function calls within the test function bodies, then adjust the applicability to There was a problem hiding this comment. I'm interested in learning more about how to get this done, I'm pretty new to the clippy tooling as a contributor, and I was wondering how to plumb up fixing this automatically. Perhaps we could pair on this if you have some time. There was a problem hiding this comment. Was in a family trip few days ago so I didn't see this message, sorry~
Well, what I meant to say was instead of using Something like...: if let FnKind::ItemFn(ident, ..) = fn_kind &&
let Some(non_prefixed) = ident.name.as_str()
{
span_lint_and_sugg(
cx,
REDUNDANT_TEST_PREFIX,
span,
"this test is prefixed redundantly with `test`",
"consider removing the redundant `test` prefix from the test name",
non_prefixed,
Applicability::MachineApplicable
);
}
As for this, it might just me being too paranoid 😅, as I haven't seen any real world example doing that afaik, so you can ignore it for now. But if you are interested, here's a little hint: Expand Me!You can check each `expr` if it `is_in_test_function`, then if it is `Call`ing a function that have `test_` prefixed name, you can have the following info stored in a map:
for example FxHashMap<DefId, (Vec<Span>, String, bool)>Then you can Finally, having a I think this is one way to do it? idk, I'm writing this at almost 2am so most of them might not make sense lol 😅 . |
||
| cx, | ||
| REDUNDANT_TEST_PREFIX, | ||
| span, | ||
| "this test is prefixed redundantly with `test`", | ||
| None, | ||
| "consider removing the redundant `test` prefix from the test name", | ||
| ); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fn is_prefixed_with_test_fn(fn_kind: FnKind<'_>) -> bool { | ||
| match fn_kind { | ||
| FnKind::ItemFn(ident, ..) => { | ||
| // check if `fn` name is prefixed with `test_` | ||
| ident.name.as_str().starts_with("test_") | ||
| }, | ||
| // ignore closures | ||
| _ => false, | ||
| } | ||
| } | ||
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 assume you're meaning to show a 'bad' example here that should trigger the lint.
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.
Ah, right, I need to update this.