New issue

avoid `eq_op` in test code #7811

Merged

bors merged 1 commit into

master

from

eq-op-testless

9 days ago

Conversation

Copy link

Collaborator

llogiq commented 16 days ago

Add a check to eq_op that will avoid linting in functions annotated with #[test]

Please write a short comment explaining your change (or "none" for internal only changes)

changelog: avoid eq_op in test functions

Copy link

Collaborator

rust-highfive commented 16 days ago

r? @xFrednet

(rust-highfive has picked a reviewer for you, use r? to override)

Copy link

Collaborator

giraffate commented 15 days ago

I did Re-run jobs on CI some times, but for some reason it's canceled in the middle of installing toolchain and CI failed.

Copy link

Collaborator

Author

llogiq commented 15 days ago

Yeah, that's really strange. Tests work locally, so I'm not too worried.

Copy link

Member

xFrednet left a comment

Hey, just a small NIT, the rest LGTM

clippy_lints/src/eq_op.rs

Outdated Show resolved

Copy link

Collaborator

Author

llogiq commented 15 days ago •

edited

I now have extended our compile-test harness to compile tests in test/ui_test. However, it appears my test function detection is ineffective. Does anyone have an idea on how to reliably detect test code in HIR (which is already expanded to $deity knows what)?

Copy link

Collaborator

Author

llogiq commented 14 days ago •

edited

The dev guide has something to say about how tests are compiled. So basically all we get is a pub and a reexport. I guess the only way to get at this is to look into the span's ExpnData and see if we can detect something test-alike.

Copy link

Collaborator

Author

llogiq commented 14 days ago

Zulip user DevinR528 found a solution. As looking into the expansion data doesn't seem to help, I'm going to incorporate that for now. Thanks, Devin!

Copy link

Collaborator

Author

llogiq commented 14 days ago

The error seems unrelated but persistent even on re-running the jobs.

Copy link

Collaborator

Author

llogiq commented 12 days ago

Ah, I see! I change the compiletest parameters without cloning the Config, and as a result, everything afterwards gets compiled with --test!

Luckily that's easy to fix.

Copy link

Collaborator

Author

llogiq commented 12 days ago

Now all tests pass this should be ready for review.

clippy_utils/src/lib.rs

let parent_mod = tcx.parent_module(id);

let mut vis = VisitConstTestStruct { names, found: false };

tcx.hir().visit_item_likes_in_module(parent_mod, &mut vis);

vis.found

Copy link

Collaborator

camsteffen 10 days ago

You could use tcx.item_children instead of a visitor here. I would just check item.name and #[rustc_test_marker] and then you don't need to go into item.kind.

Copy link

Collaborator

camsteffen 10 days ago

Also I think you can't do tcx.parent_module since you can have

fn f() {
    #[test]
    fn t() {} // the test marker is in a fn not a module
}

so maybe move this logic inside the parent iteration where you find ItemKind::Fn.

Copy link

Collaborator

Author

llogiq 10 days ago

I'd consider nested test methods a degenerate case and if those who do this get a false positive, they likely deserve it .

Copy link

Collaborator

Author

llogiq commented 9 days ago

@camsteffen I've added the #[rustc_test_marker] check, which works, but I couldn't get the item_children query to work. So I kept the visitor for now.

Copy link

Collaborator

Author

llogiq commented 9 days ago

@xFrednet r?

Copy link

Member

xFrednet commented 9 days ago

Jup, I'll try to review this today. The code I already saw was looking good . One NIT I already found, could you document that lints using is_test_module_or_function and is_test_function have to add a ui test to tests/ui_test/ to make sure that the tests are included in the compilation? You can just add that in the doc comment of the function.