Lint to prevent redundant `test_` prefix in test names. #12861

Caylub · 2024-05-28T13:00:15Z

Adds redundant_test_prefix lint, which checks test names for the test_ prefix, and warns if found. Closes #8931

changelog: New Lint: [redundant_test_prefix] #12861

rustbot · 2024-05-28T13:00:21Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @xFrednet (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 · 2024-05-28T13:00:24Z

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git pull --rebase https://github.com/rust-lang/rust-clippy.git master
$ git push --force-with-lease

The following commits are merge commits:

eea4c36

Closes rust-lang#8931.

J-ZhengLi

looks like there are conflicting opinions in the related issue, about whether this should only lint the functions that are directly within a cfg(test) modules. So it might be a good idea to add a configuration allowing users to turn it on or off, something like warn-test-prefix-in-test-module-only, but don't count me on naming stuffs, I'm so bad at it lol 😄

J-ZhengLi · 2024-05-29T08:48:28Z

clippy_lints/src/redundant_test_prefix.rs

+            && is_in_test_function(cx.tcx, body.id().hir_id)
+            && is_in_cfg_test(cx.tcx, body.id().hir_id)
+        {
+            span_lint_and_help(


Would be nicer if this lint could be auto-fixed, so, maybe use span_lint_and_sugg instead?

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 Applicability::MaybeIncorrect or something better... suggest changing all those names as well.

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.

Was in a family trip few days ago so I didn't see this message, sorry~

...and I was wondering how to plumb up fixing this automatically...

Well, what I meant to say was instead of using span_lint_and_help, try using span_lint_and_sugg. Therefore you can offer a suggestion with the lint, in this case, a new function name without the test_ prefix, as well as an applicablity tell it's machine applicable, then people can run clippy with --fix to automatically apply the suggestion.

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 ); }

But then it'll make things a bit more complicated when such test function is called in another test function

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:

the function's DefId (key)

Spans where it was called

a suggested name without prefix

a bool indicates whether this function is a test function or not

for example

FxHashMap<DefId, (Vec<Span>, String, bool)>

Then you can check_fn as usual but don't call span_lint* yet, just updating the bool value (or insert new entry to) the map you had and continue.

Finally, having a check_crate_post function, you can go over that map you have, picking only the ones marked as test function and call emit lint, if the Vec<Span> is not empty, you can then use multipart_suggestion.

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 😅 .

J-ZhengLi · 2024-05-29T09:07:11Z

tests/ui/redundant_test_prefix.rs

+    fn test_redundant_name() {
+        //
+    }
+}


Could you add some test cases where the test functions are not in #[cfg(test)], because it seems like this should only lint when the test is_in_cfg_test, but there are changes against it... well, got me a bit confused 🤔

it seems like this should only lint when the test is_in_cfg_test
Not quite, because test can also be defined outside of a a `#[cfg(test)] annotation (and typical "test" module). IIRC, there's at least one example of that in the diff from this PR.

andrewbanchich · 2024-05-30T06:49:26Z

I'm not sure if there are conflicting opinions about this being constrained to modules or if that comment just thought it was more about not repeating the module name in the test name.

Personally, I can't think of any good reason even if the test fn isn't in a test mod. It's clear it's a test since we can see the macro annotation directly above it. Same with looking at the stdout from running cargo test.

y21 · 2024-05-30T10:39:49Z

Sometimes you can't remove the test_* prefix because it's testing a function with the exact same name (1 2), so you'd end up with two functions of the same name, generating an error. You can work around it by adding a "_works" suffix like cargo's default-generated lib crate, but that feels like it's just working around the lint unnecessarily.
I don't know how common this is but I expect quite a few FP reports from that.

(To be clear, this is an argument for at the very least making linting outside of test modules configurable. In test modules this seems like a non issue)

y21 · 2024-05-30T10:52:36Z

clippy_lints/src/redundant_test_prefix.rs

+    /// ### Example
+    /// ```no_run
+    /// #[test]
+    /// fn my_test_case() {


Suggested change

/// fn my_test_case() {

/// fn test_my_test_case() {

I assume you're meaning to show a 'bad' example here that should trigger the lint.

Ah, right, I need to update this.

andrewbanchich · 2024-05-30T11:15:16Z

@y21 True, but the intention of this is to avoid a common Java-ism where people prefix test_ even when the test already is named something clear, like test_foo_fails_on_nonutf8_input. It's a common pattern we (and evidently others) have seen, and for those cases the assertion is already super clear.

I agree, though, that it's hard to think of a name for happy path tests, since it's either test_foo or foo_works. But then I've never seen foo_fails_on_nonutf8_input_works before, since that would make no sense.

So based on what I've seen:

The test_ prefix is commonly used for both happy and unhappy path testing.
It seems reasonable for happy path testing since the assertion is basically "yeah, it works".
Never seems reasonable for unhappy path testing since you should have clear assertions.
_works seems better since it means "happy path", so smaller chance for abuse since if you have a clear assertion it wouldn't make sense.

That being said, people can still write a test like foo_works and put negative assertions in it, but seems better than nothing 😅.

andrewbanchich · 2024-05-30T11:24:14Z

That being said, I wonder why module_name_repetitions doesn't already catch these for functions in test modules. Maybe would be good to have it do that too, since it seems like we're talking about two different, but related, issues?

xFrednet

The implementation mostly looks good to me. I left some comments, mostly regarding the tests. The comments from @J-ZhengLi are also excellent and should be addressed.

You're welcome to ask if you need any clarification or help :)

xFrednet · 2024-06-04T12:45:25Z

clippy_lints/src/redundant_test_prefix.rs

+    /// ```
+    #[clippy::version = "1.79.0"]
+    pub REDUNDANT_TEST_PREFIX,
+    pedantic,


I feel like this shouldn't be in pedantic but in restriction instead. The usage of the test_ prefix doesn't seem like an antipattern to me. There are good arguments against it, but the number of changes required in existing changes shows that the usage is still quite common.

I'll include this decision in the group discussion before we merge this PR :)

There are arguments to be made for several categories: style, pedantic, restriction, etc.

The usage of test_ seems to be pedantic to me, because it's clearly unnecessary in almost every case, including some of the ones mentioned in the threads in this PR. For example, test names that have test_ + method_name, should just be super::method_name IMO, following the same like of reasoning for as everywhere else.

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.

xFrednet · 2024-06-04T12:50:23Z

tests/ui/redundant_test_prefix.rs

@@ -0,0 +1,11 @@
+#![warn(clippy::redundant_test_prefix)]


Could you add more tests? Also negative examples, where the test_ prefix should be kept. For example, in normal functions and methods.

The normal functions and methods example makes sense, but can you (or anyone else here) think of other examples?

I think those would be enough. One specific example I can think of would be:

mod no_attr { pub mod tests { pub fn test_nothing() {} } }

This looks very close to unit tests, but is missing the attributes. This should not be linted.

xFrednet · 2024-06-04T12:54:45Z

tests/ui/redundant_test_prefix.rs

+    use super::*;
+
+    #[test]
+    fn test_redundant_name() {


Our UI test support error annotations. Could you add them?

These should work:

Suggested change

fn test_redundant_name() {

fn test_redundant_name() {

//~^ ERROR: this test is prefixed redundantly with `test`

//~| HELP: consider removing the redundant `test` prefix from the test name

//~| NOTE: `-D clippy::redundant-test-prefix` implied by `-D warnings`

//~| HELP: to override `-D warnings` add `#[allow(clippy::redundant_test_prefix)]`

I'll add these when I'm on later tonight.

xFrednet · 2024-06-04T12:56:53Z

@rustbot author

bors · 2024-06-11T23:22:23Z

☔ The latest upstream changes (presumably #12849) made this pull request unmergeable. Please resolve the merge conflicts.

xFrednet · 2024-06-30T20:09:30Z

Hey @Caylub, this is a ping from triage, since there hasn't been any activity in some time. Are you still planning to continue this implementation?

If you have any questions, you're always welcome to ask them in this PR or on Zulip.

@rustbot author

xFrednet · 2024-07-23T06:43:09Z

Hey this is triage, I'm closing this due to inactivity. If you want to continue this implementation, you're welcome to create a new PR. Thank you for the time, you already put into this!

Interested parties are welcome to pick this implementation up as well :)

@rustbot label +S-inactive-closed -S-waiting-on-author -S-waiting-on-review

farazdagi · 2024-11-19T21:30:07Z

The followup PR: #13710

This PR has started as an effort to proceed from the feedback in #12861. - Checks test functions (functions marked with `#[test]` annotation) for redundant "test_" prefix. - Auto-fix is supported (and handles collisions gracefully, see below). - If removing "test_" prefix from, say, `test_foo()` results in a name collision (either because function `foo()` is already defined within the current scope, or because the `foo()` call exists within function -- thus creating an unwanted recursion), lint suggests function rename, warning the user that a simple trimming of `test_` prefix will result in a name collision. - If removing "test_" prefix results in invalid identifier (consider `test_const`, `test_`, `test_42`), then again no auto-fix is suggested, user is asked to rename function, with a note that a simple prefix trimming will result in an invalid function name. (`Applicability::HasPlaceholders` is used and user is suggested to: drop `test_` prefix + add `_works` suffix, i.e. `test_foo` becomes `foo_works` -- but again, user has to apply those changes manually). - If trimmed version of the function name is a valid identifier, doesn't result in name collision or unwanted recursion, then user is able to run auto-fix. fixes #8931 changelog: new lint: [`redundant_test_prefix`]

rustbot assigned xFrednet May 28, 2024

feat: adds redundant_test_prefix clippy lint

0d1441f

Closes rust-lang#8931.

Caylub force-pushed the master branch from eea4c36 to 0d1441f Compare May 28, 2024 13:09

rustbot removed has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels May 28, 2024

J-ZhengLi reviewed May 29, 2024

View reviewed changes

y21 reviewed May 30, 2024

View reviewed changes

xFrednet reviewed Jun 4, 2024

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Jun 4, 2024

xFrednet closed this Jul 23, 2024

rustbot added S-inactive-closed Status: Closed due to inactivity and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Jul 23, 2024

farazdagi mentioned this pull request Nov 19, 2024

New lint: redundant_test_prefix #13710

Merged

xFrednet removed the S-inactive-closed Status: Closed due to inactivity label Nov 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lint to prevent redundant `test_` prefix in test names. #12861

Lint to prevent redundant `test_` prefix in test names. #12861

Caylub commented May 28, 2024 •

edited by xFrednet

Loading

rustbot commented May 28, 2024

rustbot commented May 28, 2024

J-ZhengLi left a comment •

edited

Loading

J-ZhengLi May 29, 2024

Caylub Jun 7, 2024

J-ZhengLi Jun 11, 2024

J-ZhengLi May 29, 2024

Caylub Jun 7, 2024

andrewbanchich commented May 30, 2024

y21 commented May 30, 2024 •

edited

Loading

y21 May 30, 2024

Caylub Jun 7, 2024

andrewbanchich commented May 30, 2024

andrewbanchich commented May 30, 2024 •

edited

Loading

xFrednet left a comment

xFrednet Jun 4, 2024

Caylub Jun 7, 2024

xFrednet Jun 11, 2024

xFrednet Jun 4, 2024

Caylub Jun 7, 2024

xFrednet Jun 11, 2024

xFrednet Jun 4, 2024

Caylub Jun 7, 2024

xFrednet commented Jun 4, 2024

bors commented Jun 11, 2024

xFrednet commented Jun 30, 2024

xFrednet commented Jul 23, 2024

farazdagi commented Nov 19, 2024

-    fn test_redundant_name() {
+    fn test_redundant_name() {
+        //~^ ERROR: this test is prefixed redundantly with `test`
+        //~| HELP: consider removing the redundant `test` prefix from the test name
+        //~| NOTE: `-D clippy::redundant-test-prefix` implied by `-D warnings`
+        //~| HELP: to override `-D warnings` add `#[allow(clippy::redundant_test_prefix)]`

Lint to prevent redundant test_ prefix in test names. #12861

Lint to prevent redundant test_ prefix in test names. #12861

Conversation

Caylub commented May 28, 2024 • edited by xFrednet Loading

rustbot commented May 28, 2024

rustbot commented May 28, 2024

J-ZhengLi left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewbanchich commented May 30, 2024

y21 commented May 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewbanchich commented May 30, 2024

andrewbanchich commented May 30, 2024 • edited Loading

xFrednet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xFrednet commented Jun 4, 2024

bors commented Jun 11, 2024

xFrednet commented Jun 30, 2024

xFrednet commented Jul 23, 2024

farazdagi commented Nov 19, 2024

Lint to prevent redundant `test_` prefix in test names. #12861

Lint to prevent redundant `test_` prefix in test names. #12861

Caylub commented May 28, 2024 •

edited by xFrednet

Loading

J-ZhengLi left a comment •

edited

Loading

y21 commented May 30, 2024 •

edited

Loading

andrewbanchich commented May 30, 2024 •

edited

Loading