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

Move compile-pass tests to check-pass or build-pass #62277

Open
cramertj opened this issue Jul 1, 2019 · 7 comments
Open

Move compile-pass tests to check-pass or build-pass #62277

cramertj opened this issue Jul 1, 2019 · 7 comments
Labels
A-compiletest Area: The compiletest test runner A-testsuite Area: The testsuite used to check the correctness of rustc C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@cramertj
Copy link
Member

cramertj commented Jul 1, 2019

compile-pass was the old way to assert that UI tests were able to successfully build. However, it would do a full build of the code, including codegen and linking. Many of our tests don't need this, however, and should instead use the new check-pass, introduced in #61778. For tests that exercise codegen and linking, build-pass can be used instead.

As a first step, it would be good to remove compile-pass to prevent users adding new tests that unconsciously take a dependency on codegen/linking, and push them to use check/build-pass to make the distinction explicit.

To do this, we'd like to start with a mass migration of compile-pass tests to build-pass (rather than check-pass because we don't want to accidentally stop testing codegen/linking functionality). However, we don't want to lose the distinction between tests which are intentionally build-pass and those that have been automatically migrated, so automatically-migrated tests should include a note like // build-pass (FIXME(this-issue-#): could be check-pass?) or similar.

Once that's done, we can remove compile-pass and work on moving over the FIXME-tagged tests to either check-pass or remove the FIXME.

@cramertj cramertj added C-cleanup Category: PRs that clean code up or issues documenting cleanup. A-testsuite Area: The testsuite used to check the correctness of rustc labels Jul 1, 2019
@Centril Centril added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 1, 2019
@Centril
Copy link
Contributor

Centril commented Jul 1, 2019

@Centril Centril added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. labels Jul 1, 2019
@Centril
Copy link
Contributor

Centril commented Jul 1, 2019

cc @petrochenkov

Centril added a commit to Centril/rust that referenced this issue Jul 3, 2019
… r=Centril

Migrate `compile-pass` annotations to `build-pass`

This is a part of rust-lang#62277.

As a first step, the `compile-pass` tests are migrated to `build-pass`.

r? @cramertj
cc @Centril
Centril added a commit to Centril/rust that referenced this issue Jul 3, 2019
… r=Centril

Migrate `compile-pass` annotations to `build-pass`

This is a part of rust-lang#62277.

As a first step, the `compile-pass` tests are migrated to `build-pass`.

r? @cramertj
cc @Centril
Centril added a commit to Centril/rust that referenced this issue Jul 3, 2019
… r=Centril

Migrate `compile-pass` annotations to `build-pass`

This is a part of rust-lang#62277.

As a first step, the `compile-pass` tests are migrated to `build-pass`.

r? @cramertj
cc @Centril
Centril added a commit to Centril/rust that referenced this issue Jul 3, 2019
… r=Centril

Migrate `compile-pass` annotations to `build-pass`

This is a part of rust-lang#62277.

As a first step, the `compile-pass` tests are migrated to `build-pass`.

r? @cramertj
cc @Centril
Centril added a commit to Centril/rust that referenced this issue Jul 5, 2019
…trochenkov

Remove `compile-pass` from compiletest

This is a part of rust-lang#62277.
Removes `compile-pass` from compiletest (and modify some tests' annotations).

r? @Centril
Centril added a commit to Centril/rust that referenced this issue Jul 5, 2019
…trochenkov

Remove `compile-pass` from compiletest

This is a part of rust-lang#62277.
Removes `compile-pass` from compiletest (and modify some tests' annotations).

r? @Centril
Centril added a commit to Centril/rust that referenced this issue Jul 5, 2019
…trochenkov

Remove `compile-pass` from compiletest

This is a part of rust-lang#62277.
Removes `compile-pass` from compiletest (and modify some tests' annotations).

r? @Centril
@monoflo
Copy link

monoflo commented Aug 4, 2019

Hi! I'm willing to take this on, but I'd like to confirm what needs to be done.

With a quick ripgrep, I've seen a ton of references with the following line:

// build-pass (FIXME(62277): could be check-pass?)

Is the goal here to go through each and every one and see what ought to be check-pass as opposed to build-pass in order to speed up testing / CI?

Per #61778 (comment):

// check-pass will compile the test skipping codegen (which is expensive and isn't supposed to fail in most cases).
// build-pass will compile and link the test without running it.
// run-pass will compile, link and run the test.

This seems like a fairly manual job that will require getting pretty familiar with the test codebase.

Where might be the best place to start chipping away at this?

@Centril
Copy link
Contributor

Centril commented Aug 6, 2019

@monoflo Basically yeah. The idea is to check whether a particular // build-pass test is just about type checking or whether codegen is relevant. I would just grep for the tests and do a few at a time.

Centril added a commit to Centril/rust that referenced this issue Nov 7, 2019
Update some build-pass ui tests to use check-pass where applicable

Helps with issue rust-lang#62277.
JohnTitor added a commit to JohnTitor/rust that referenced this issue Jan 24, 2020
Use check-pass mode for lint tests and nll tests

Helps with issue rust-lang#62277.
Centril added a commit to Centril/rust that referenced this issue Mar 12, 2020
…r=RalfJung

Move some `build-pass` tests to `check-pass`

Helps with rust-lang#62277.

r? @cramertj cc @Centril
Centril added a commit to Centril/rust that referenced this issue Mar 13, 2020
…r=RalfJung

Move some `build-pass` tests to `check-pass`

Helps with rust-lang#62277.

r? @cramertj cc @Centril
JohnTitor added a commit to JohnTitor/rust that referenced this issue Mar 13, 2020
…r=RalfJung

Move some `build-pass` tests to `check-pass`

Helps with rust-lang#62277.

r? @cramertj cc @Centril
@jyn514
Copy link
Member

jyn514 commented Nov 4, 2020

For contributors: when opening PRs, could you explain why these only need to be check-pass and not build-pass? It's easy enough to do a simple find/replace, the hard part is telling whether it needs to be build-pass or not.

  • If the test is a regression test from an issue, you could either
    • test with an old version of nightly to confirm that the error it fixes was present with only cargo check (or cargo rustc -- --profile check if it's an old enough version)
    • look at the backtrace for the issue to make sure it happens before codegen
  • If the test is not a regression test, but was added along with a feature, ask the original author why it was added and whether it could be check and still test everything it should.
    • If it's a trivial test that shouldn't touch codegen or linking at all, it's fine to say that without asking the author.

@Mark-Simulacrum Mark-Simulacrum added E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. and removed E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. labels Nov 6, 2020
m-ou-se added a commit to m-ou-se/rust that referenced this issue Nov 7, 2020
…-size, r=jyn514

Remove FIXME comment in print_type_sizes ui test suite

## Overview
Helps with rust-lang#62277

> The type sizes are likely only printed when the actual layout is computed. For generic types, this only happens during codegen.

ref: https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/Codegen.20process.20question/near/215836807

Some tests like `multiple_types.rs` are passed even if using `check-pass`. But tests should be agnostic to when the actual layout is computed. The `build-pass` is intentionally used for them. I remove FIXME comments.
m-ou-se added a commit to m-ou-se/rust that referenced this issue Nov 7, 2020
…-size, r=jyn514

Remove FIXME comment in print_type_sizes ui test suite

## Overview
Helps with rust-lang#62277

> The type sizes are likely only printed when the actual layout is computed. For generic types, this only happens during codegen.

ref: https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/Codegen.20process.20question/near/215836807

Some tests like `multiple_types.rs` are passed even if using `check-pass`. But tests should be agnostic to when the actual layout is computed. The `build-pass` is intentionally used for them. I remove FIXME comments.
m-ou-se added a commit to m-ou-se/rust that referenced this issue Nov 7, 2020
…-size, r=jyn514

Remove FIXME comment in print_type_sizes ui test suite

## Overview
Helps with rust-lang#62277

> The type sizes are likely only printed when the actual layout is computed. For generic types, this only happens during codegen.

ref: https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/Codegen.20process.20question/near/215836807

Some tests like `multiple_types.rs` are passed even if using `check-pass`. But tests should be agnostic to when the actual layout is computed. The `build-pass` is intentionally used for them. I remove FIXME comments.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Nov 8, 2020
…-size, r=jyn514

Remove FIXME comment in print_type_sizes ui test suite

## Overview
Helps with rust-lang#62277

> The type sizes are likely only printed when the actual layout is computed. For generic types, this only happens during codegen.

ref: https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/Codegen.20process.20question/near/215836807

Some tests like `multiple_types.rs` are passed even if using `check-pass`. But tests should be agnostic to when the actual layout is computed. The `build-pass` is intentionally used for them. I remove FIXME comments.
m-ou-se added a commit to m-ou-se/rust that referenced this issue Nov 8, 2020
…-size, r=jyn514

Remove FIXME comment in print_type_sizes ui test suite

## Overview
Helps with rust-lang#62277

> The type sizes are likely only printed when the actual layout is computed. For generic types, this only happens during codegen.

ref: https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/Codegen.20process.20question/near/215836807

Some tests like `multiple_types.rs` are passed even if using `check-pass`. But tests should be agnostic to when the actual layout is computed. The `build-pass` is intentionally used for them. I remove FIXME comments.
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Nov 22, 2020
…l, r=jyn514

Remove FIXME comment in some incremental test suite

Helps with rust-lang#62277

I removed FIXME comment in some incremental tests with [rustc_partition_codegened](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_incremental/assert_module_sources/index.html). This seems using codegen process. So it uses intentionally `build-pass`
JohnTitor added a commit to JohnTitor/rust that referenced this issue Jun 22, 2021
…ts-check-pass, r=JohnTitor

Mark some edition tests as check-pass

## Overview
This helps with rust-lang#62277. In short, there are some tests that were marked as `build-pass` when it was unclear whether `check-pass` might be more appropriate. This PR marks some of those tests as `compile-pass`, in addition to making some incidental formatting improvements.

## A brief explanation of why this is correct
These tests fall into a few buckets.

`src/test/ui/dyn-keyword/dyn-2015-edition-keyword-ident-lint.rs`
`src/test/ui/dyn-keyword/dyn-2015-idents-in-decl-macros-unlinted.rs`
`src/test/ui/dyn-keyword/dyn-2015-idents-in-macros-unlinted.rs`
`src/test/ui/dyn-keyword/dyn-2015-no-warnings-without-lints.rs`
`src/test/ui/dyn-keyword/issue-56327-dyn-trait-in-macro-is-okay.rs`

These test a lint for a keyword added in a new edition and the corresponding changes in keyword rules.

`src/test/ui/editions/edition-feature-ok.rs`
This checks that a feature related to an edition transition is valid.

`src/test/ui/editions/edition-imports-virtual-2015-ambiguity.rs`
This checks that imports between editions work correctly.

`src/test/ui/editions/edition-keywords-2015-2015-expansion.rs`
`src/test/ui/editions/edition-keywords-2018-2015-expansion.rs`
This checks the interaction between a change in keyword status over editions and macros.

All of the things being tested come before linking and codegen, so it is safe to use `check-pass` for them.
JohnTitor added a commit to JohnTitor/rust that referenced this issue Jun 22, 2021
…ts-check-pass, r=JohnTitor

Mark some edition tests as check-pass

## Overview
This helps with rust-lang#62277. In short, there are some tests that were marked as `build-pass` when it was unclear whether `check-pass` might be more appropriate. This PR marks some of those tests as `compile-pass`, in addition to making some incidental formatting improvements.

## A brief explanation of why this is correct
These tests fall into a few buckets.

`src/test/ui/dyn-keyword/dyn-2015-edition-keyword-ident-lint.rs`
`src/test/ui/dyn-keyword/dyn-2015-idents-in-decl-macros-unlinted.rs`
`src/test/ui/dyn-keyword/dyn-2015-idents-in-macros-unlinted.rs`
`src/test/ui/dyn-keyword/dyn-2015-no-warnings-without-lints.rs`
`src/test/ui/dyn-keyword/issue-56327-dyn-trait-in-macro-is-okay.rs`

These test a lint for a keyword added in a new edition and the corresponding changes in keyword rules.

`src/test/ui/editions/edition-feature-ok.rs`
This checks that a feature related to an edition transition is valid.

`src/test/ui/editions/edition-imports-virtual-2015-ambiguity.rs`
This checks that imports between editions work correctly.

`src/test/ui/editions/edition-keywords-2015-2015-expansion.rs`
`src/test/ui/editions/edition-keywords-2018-2015-expansion.rs`
This checks the interaction between a change in keyword status over editions and macros.

All of the things being tested come before linking and codegen, so it is safe to use `check-pass` for them.
workingjubilee added a commit to workingjubilee/rustc that referenced this issue Jul 27, 2023
…henkov

tests/ui/hello_world/main.rs: Remove FIXME (rust-lang#62277)

The purpose of the test is to make sure that compiling hello world produces no compiler output. To properly test that, we need to run the entire compiler pipeline. We don't want the test to pass if codegen accidentally starts writing to stdout. So keep it as build-pass.

Part of rust-lang#62277
bors added a commit to rust-lang-ci/rust that referenced this issue Jul 27, 2023
…kingjubilee

Rollup of 4 pull requests

Successful merges:

 - rust-lang#97571 (Add documentation on v0 symbol mangling.)
 - rust-lang#114122 (tests/ui/hello_world/main.rs: Remove FIXME (rust-lang#62277))
 - rust-lang#114133 (Revert "add tidy check that forbids issue ui test filenames")
 - rust-lang#114139 (Make `--print` with path unstable)

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jul 28, 2023
tests/ui/proc-macro/*: Migrate FIXMEs to check-pass

proc-macros are processed early in the compiler pipeline. There is no need to involve codegen. So change to check-pass.

I have also looked through each changed test and to me it is sufficiently clear that codegen is not needed for the purpose of the test.

I skipped changing `tests/ui/proc-macro/no-missing-docs.rs` in this commit because it was not clear to me that it can be changed to check-pass.

Part of rust-lang#62277
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jul 29, 2023
tests/ui/proc-macro/*: Migrate FIXMEs to check-pass

proc-macros are processed early in the compiler pipeline. There is no need to involve codegen. So change to check-pass.

I have also looked through each changed test and to me it is sufficiently clear that codegen is not needed for the purpose of the test.

I skipped changing `tests/ui/proc-macro/no-missing-docs.rs` in this commit because it was not clear to me that it can be changed to check-pass.

Part of rust-lang#62277
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Dec 12, 2023
tests: CGU tests require build-pass, not check-pass (remove FIXME)

CGU tests require CGU code to be exercised. We can't merely do "cargo check" on these tests.

Part of rust-lang#62277
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Dec 12, 2023
Rollup merge of rust-lang#118877 - Enselic:remove-cgu-fixme, r=Nilstrieb

tests: CGU tests require build-pass, not check-pass (remove FIXME)

CGU tests require CGU code to be exercised. We can't merely do "cargo check" on these tests.

Part of rust-lang#62277
@jieyouxu jieyouxu added the A-compiletest Area: The compiletest test runner label May 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-compiletest Area: The compiletest test runner A-testsuite Area: The testsuite used to check the correctness of rustc C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants