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.

Sign up for GitHub

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

Jump to bottom

Suggest adding a #[cfg(test)] to to a test module #91770

Merged
merged 1 commit into from
Dec 21, 2021
Merged

Suggest adding a #[cfg(test)] to to a test module #91770

merged 1 commit into from
Dec 21, 2021

Conversation

TaKO8Ki
Copy link
Member

@TaKO8Ki TaKO8Ki commented Dec 11, 2021

closes #88138

@rust-highfive
Copy link
Collaborator

r? @matthewjasper

(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 Dec 11, 2021
@TaKO8Ki TaKO8Ki changed the title Suggest adding #[cfg(test)] to to a test module Suggest adding a #[cfg(test)] to to a test module Dec 11, 2021
camelid
camelid reviewed Dec 12, 2021
View reviewed changes
src/test/ui/imports/unused-imports-in-test-module.stderr Outdated
Comment on lines 19 to 23
help: consider adding a `#[cfg(test)]` to the parent module
--> $DIR/unused-imports-in-test-module.rs:8:1
|
LL | mod test {
| ^^^^^^^^
Copy link
Member

Choose a reason for hiding this comment

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

Seems like it'd be good to add tests for out-of-line modules too (i.e,, mod test;). But I'm not sure how feasible it is to test that.

@camelid camelid added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 13, 2021
@joshtriplett
Copy link
Member

This looks great to me. I posted one comment about the proposed diagnostic message; with that changed, r=me.

@TaKO8Ki TaKO8Ki force-pushed the suggest-adding-cfg-test branch from e343965 to 4103248 Compare December 15, 2021 14:49
@camelid
Copy link
Member

camelid commented Dec 15, 2021

Also, please squash your commits if you can :)

@TaKO8Ki
suggest adding a #[cfg(test)] to test modules
6f8ad6d 
remove a empty line

import `module_to_string`

use `contains("test")`

show a suggestion in case module starts_with/ends_with "test"

replace `parent` with `containing`
@TaKO8Ki TaKO8Ki force-pushed the suggest-adding-cfg-test branch from 4103248 to 6f8ad6d Compare December 16, 2021 02:49
@jackh726
Copy link
Member

@bors r=joshtriplett

@bors
Copy link
Contributor

bors commented Dec 20, 2021

📌 Commit 6f8ad6d has been approved by joshtriplett

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 20, 2021
@bors
Copy link
Contributor

bors commented Dec 20, 2021

⌛ Testing commit 6f8ad6d with merge e6c58674ebb4b3aeded006ee47526f6ec77d97e0...

@bors
Copy link
Contributor

bors commented Dec 20, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 20, 2021
@matthiaskrgr
Copy link
Member

@bors retry docker.io 503

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 20, 2021
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot) 
Attempting with retry: docker build --rm -t rust-ci -f /gha/_work/rust/rust/src/ci/docker/host-aarch64/aarch64-gnu/Dockerfile /gha/_work/rust/rust/src/ci/docker
Sending build context to Docker daemon  506.4kB

Step 1/6 : FROM ubuntu:20.04
Head https://registry-1.docker.io/v2/library/ubuntu/manifests/20.04: received unexpected HTTP status: 503 Service Unavailable
Sending build context to Docker daemon  506.4kB

Step 1/6 : FROM ubuntu:20.04
Step 1/6 : FROM ubuntu:20.04
Head https://registry-1.docker.io/v2/library/ubuntu/manifests/20.04: received unexpected HTTP status: 503 Service Unavailable
Sending build context to Docker daemon  506.4kB

Step 1/6 : FROM ubuntu:20.04
Step 1/6 : FROM ubuntu:20.04
Head https://registry-1.docker.io/v2/library/ubuntu/manifests/20.04: received unexpected HTTP status: 503 Service Unavailable
Sending build context to Docker daemon  506.4kB

Step 1/6 : FROM ubuntu:20.04
Step 1/6 : FROM ubuntu:20.04
Head https://registry-1.docker.io/v2/library/ubuntu/manifests/20.04: received unexpected HTTP status: 503 Service Unavailable
Sending build context to Docker daemon  506.4kB

Step 1/6 : FROM ubuntu:20.04
Step 1/6 : FROM ubuntu:20.04
Head https://registry-1.docker.io/v2/library/ubuntu/manifests/20.04: received unexpected HTTP status: 503 Service Unavailable
##[error]Process completed with exit code 1.
Post job cleanup.

@matthiaskrgr matthiaskrgr mentioned this pull request Dec 21, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 21, 2021
@bors
Auto merge of rust-lang#92152 - matthiaskrgr:rollup-nmskpw6, r=matthi…
3d57c61 
…askrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#90345 (Stabilise entry_insert)
 - rust-lang#91412 (Improve suggestions for importing out-of-scope traits reexported as `_`)
 - rust-lang#91770 (Suggest adding a `#[cfg(test)]` to to a test module)
 - rust-lang#91823 (Make `PTR::as_ref` and similar methods `const`.)
 - rust-lang#92127 (Move duplicates removal when generating results instead of when displaying them)
 - rust-lang#92129 (JoinHandle docs: add missing 'the')
 - rust-lang#92131 (Sync rustc_codegen_cranelift)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 790950a into rust-lang:master Dec 21, 2021
@rustbot rustbot added this to the 1.59.0 milestone Dec 21, 2021
@TaKO8Ki TaKO8Ki deleted the suggest-adding-cfg-test branch December 22, 2021 04:53
@ken-matsui ken-matsui mentioned this pull request May 2, 2022
Merged
JohnTitor added a commit to JohnTitor/rust that referenced this pull request May 17, 2022
@JohnTitor
Rollup merge of rust-lang#96651 - ken-matsui:omit-unnecessary-help-to…
78c709c 
…-add-cfg-test, r=cjgillot

Omit unnecessary help to add `#[cfg(test)]` when already annotated

Closes: rust-lang#96611

The related PR is: rust-lang#91770
@TaKO8Ki TaKO8Ki mentioned this pull request Jul 8, 2022
Merged
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jul 17, 2022
@JohnTitor
Rollup merge of rust-lang#99036 - TaKO8Ki:fix-test-for-88138, r=compi…
0748638 
…ler-errors

Add `#[test]` to functions in test modules

I implemented a suggestion in rust-lang#91770, but the ui test I created was inadequate and I have fixed that.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joshtriplett joshtriplett Awaiting requested review from joshtriplett

@camelid camelid Awaiting requested review from camelid

Assignees

@matthewjasper matthewjasper

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.59.0
Development

Successfully merging this pull request may close these issues.

Unused import warning in a test could suggest #[cfg(test)]
10 participants
@TaKO8Ki @rust-highfive @joshtriplett @camelid @jackh726 @bors @matthiaskrgr @rust-log-analyzer @matthewjasper @rustbot