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

x.py check should check tests by default #87846

Closed
kornelski opened this issue Aug 7, 2021 · 4 comments · Fixed by #88011
Closed

x.py check should check tests by default #87846

kornelski opened this issue Aug 7, 2021 · 4 comments · Fixed by #88011
Labels
C-bug Category: This is a bug.

Comments

@kornelski
Copy link
Contributor

It's possible to have errors in files such as: library/std/src/collections/hash/map/tests.rs and library/alloc/tests/lib.rs, and have ./x.py check pass.

I would expect x.py check to be equivalent of cargo check --all and check every file that CI checks. Otherwise, I run x.py check, push code to a PR, and only find later that I have a typo or such.

@kornelski kornelski added the C-bug Category: This is a bug. label Aug 7, 2021
@Mark-Simulacrum
Copy link
Member

We don't check tests by default, but you can pass --all-targets (x.py check --all-targets) to check tests. I believe this is consistent with cargo check behavior?

One reason we don't check tests by default is that causes duplicate error messages for the 'base' crate typically which are annoying (and most changes don't actually need them). If Cargo handled that and didn't show duplicate messages, I imagine we could change the default as well.

@kornelski
Copy link
Contributor Author

Oh, you're right. So my mistake was assuming that cargo check is more thorough than it is (my text editor invokes it for me, even for tests, so I didn't realize the command doesn't do everything itself).

@ehuss
Copy link
Contributor

ehuss commented Aug 7, 2021

If Cargo handled that and didn't show duplicate messages

Just FYI, 1.55 now deduplicates messages.

@Mark-Simulacrum
Copy link
Member

Oh, great news @ehuss -- in that case I'm going to go ahead and reopen this as it seems likely to make sense to check tests (by default, and likely without an option to disable that). Should be a pretty simple adjustment to bootstrap.

@jyn514 jyn514 changed the title x.py check does not check tests x.py check should check tests by default Aug 13, 2021
m-ou-se added a commit to m-ou-se/rust that referenced this issue Aug 16, 2021
…mulacrum

Enable `--all-targets` for `x.py check` unconditionally

Now that Cargo deduplicates diagnostics from different targets, this doesn't flood the console with
duplicate errors.

Note that this doesn't add `--all-targets` in `Builder::cargo` directly because `impl Step for Std`
actually wants to omit `--all-targets` the first time while it's still building libtest.

When passed `--all-targets`, this warns that the option isn't needed, but still continues to compile.

Fixes rust-lang#87846.
r? `@Mark-Simulacrum`
@bors bors closed this as completed in fe71be7 Aug 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants