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

avoid eq_op in test code #7811

Merged
merged 1 commit into from
Oct 19, 2021
Merged

avoid eq_op in test code #7811

merged 1 commit into from
Oct 19, 2021

Conversation

llogiq
Copy link
Contributor

@llogiq llogiq commented Oct 12, 2021

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

@rust-highfive
Copy link

r? @xFrednet

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 12, 2021
@giraffate
Copy link
Contributor

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.

@llogiq
Copy link
Contributor Author

llogiq commented Oct 13, 2021

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

Copy link
Member

@xFrednet xFrednet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, just a small NIT, the rest LGTM 🙃

clippy_lints/src/eq_op.rs Outdated Show resolved Hide resolved
tests/ui/eq_op.rs Outdated Show resolved Hide resolved
@llogiq
Copy link
Contributor Author

llogiq commented Oct 13, 2021

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)?

@llogiq
Copy link
Contributor Author

llogiq commented Oct 14, 2021

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.

@llogiq
Copy link
Contributor Author

llogiq commented Oct 14, 2021

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!

@llogiq
Copy link
Contributor Author

llogiq commented Oct 14, 2021

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

@llogiq
Copy link
Contributor Author

llogiq commented Oct 16, 2021

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.

@llogiq
Copy link
Contributor Author

llogiq commented Oct 16, 2021

Now all tests pass this should be ready for review.

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
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@llogiq
Copy link
Contributor Author

llogiq commented Oct 19, 2021

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

@llogiq
Copy link
Contributor Author

llogiq commented Oct 19, 2021

@xFrednet r?

@xFrednet
Copy link
Member

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

@llogiq
Copy link
Contributor Author

llogiq commented Oct 19, 2021

@xFrednet Done!

tests/ui/eq_op_macros.rs Outdated Show resolved Hide resolved
@llogiq
Copy link
Contributor Author

llogiq commented Oct 19, 2021

@xFrednet now r?

Copy link
Member

@xFrednet xFrednet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 👍

@xFrednet
Copy link
Member

Thank you for the changes! 🙃

@bors r+

@bors
Copy link
Collaborator

bors commented Oct 19, 2021

📌 Commit e88c956 has been approved by xFrednet

@bors
Copy link
Collaborator

bors commented Oct 19, 2021

⌛ Testing commit e88c956 with merge c1e7a07...

@bors
Copy link
Collaborator

bors commented Oct 19, 2021

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: xFrednet
Pushing c1e7a07 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants