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

compiletest: Deficiencies and pitfalls of //@ normalize-* headers #126372

Open
2 tasks done
Zalathar opened this issue Jun 13, 2024 · 0 comments
Open
2 tasks done

compiletest: Deficiencies and pitfalls of //@ normalize-* headers #126372

Zalathar opened this issue Jun 13, 2024 · 0 comments
Labels
A-compiletest Area: The compiletest test runner A-testsuite Area: The testsuite used to check the correctness of rustc C-bug Category: This is a bug. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

Comments

@Zalathar
Copy link
Contributor

Zalathar commented Jun 13, 2024

These headers allow individual tests to define custom output-normalization rules, so that output snapshots are less sensitive to things like platform differences, line numbers, and timestamps.

Unfortunately, they have a few limitations and pitfalls:

@rustbot label +A-compiletest

@rustbot rustbot added needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. A-compiletest Area: The compiletest test runner labels Jun 13, 2024
@jieyouxu jieyouxu 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 T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Jun 13, 2024
bors added a commit to rust-lang-ci/rust that referenced this issue Jul 10, 2024
Require a colon in `//@ normalize-*:` test headers

The previous parser for `//@ normalize-*` headers (before rust-lang#126370) was so lax that it did not require `:` after the header name. As a result, the test suite contained a mix of with-colon and without-colon normalize headers, both numbering in the hundreds.

This PR updates the without-colon headers to add a colon (matching the style used by other headers), and then updates the parser to make the colon mandatory.

(Because the normalization parser only runs *after* the header system identifies a normalize header, this will detect and issue an error for relevant headers that lack the colon.)

Addresses one of the points of rust-lang#126372.
bors added a commit to rust-lang-ci/rust that referenced this issue Jul 11, 2024
Require a colon in `//@ normalize-*:` test headers

The previous parser for `//@ normalize-*` headers (before rust-lang#126370) was so lax that it did not require `:` after the header name. As a result, the test suite contained a mix of with-colon and without-colon normalize headers, both numbering in the hundreds.

This PR updates the without-colon headers to add a colon (matching the style used by other headers), and then updates the parser to make the colon mandatory.

(Because the normalization parser only runs *after* the header system identifies a normalize header, this will detect and issue an error for relevant headers that lack the colon.)

Addresses one of the points of rust-lang#126372.
bors added a commit to rust-lang-ci/rust that referenced this issue Jul 11, 2024
Require a colon in `//@ normalize-*:` test headers

The previous parser for `//@ normalize-*` headers (before rust-lang#126370) was so lax that it did not require `:` after the header name. As a result, the test suite contained a mix of with-colon and without-colon normalize headers, both numbering in the hundreds.

This PR updates the without-colon headers to add a colon (matching the style used by other headers), and then updates the parser to make the colon mandatory.

(Because the normalization parser only runs *after* the header system identifies a normalize header, this will detect and issue an error for relevant headers that lack the colon.)

Addresses one of the points of rust-lang#126372.
@jieyouxu jieyouxu added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) C-bug Category: This is a bug. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. and removed C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 17, 2024
jieyouxu added a commit to jieyouxu/rust that referenced this issue Dec 27, 2024
compiletest: Remove the `-test` suffix from normalize directives

This suffix was an artifact of using the same condition-checking engine as the `ignore-*` and `only-*` directives, but in practice we have only 2 tests that legitimately use a condition, and both of them only care about 32-bit vs 64-bit.

This PR detaches `normalize-*` directives from the condition checker, and replaces it with a much simpler system of four explicit `NormalizeKind` values. It then takes advantage of that simplicity to get rid of the `-test` suffix.

---

Addresses one of the points of rust-lang#126372.

The new name-checking code is a bit quaint, but I think it's a definite improvement over the status quo.

---

The corresponding dev-guide update is rust-lang/rustc-dev-guide#2172.

r? jieyouxu
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Dec 27, 2024
Rollup merge of rust-lang#134759 - Zalathar:normalize, r=jieyouxu

compiletest: Remove the `-test` suffix from normalize directives

This suffix was an artifact of using the same condition-checking engine as the `ignore-*` and `only-*` directives, but in practice we have only 2 tests that legitimately use a condition, and both of them only care about 32-bit vs 64-bit.

This PR detaches `normalize-*` directives from the condition checker, and replaces it with a much simpler system of four explicit `NormalizeKind` values. It then takes advantage of that simplicity to get rid of the `-test` suffix.

---

Addresses one of the points of rust-lang#126372.

The new name-checking code is a bit quaint, but I think it's a definite improvement over the status quo.

---

The corresponding dev-guide update is rust-lang/rustc-dev-guide#2172.

r? jieyouxu
poliorcetics pushed a commit to poliorcetics/rust that referenced this issue Dec 28, 2024
compiletest: Remove the `-test` suffix from normalize directives

This suffix was an artifact of using the same condition-checking engine as the `ignore-*` and `only-*` directives, but in practice we have only 2 tests that legitimately use a condition, and both of them only care about 32-bit vs 64-bit.

This PR detaches `normalize-*` directives from the condition checker, and replaces it with a much simpler system of four explicit `NormalizeKind` values. It then takes advantage of that simplicity to get rid of the `-test` suffix.

---

Addresses one of the points of rust-lang#126372.

The new name-checking code is a bit quaint, but I think it's a definite improvement over the status quo.

---

The corresponding dev-guide update is rust-lang/rustc-dev-guide#2172.

r? jieyouxu
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-bug Category: This is a bug. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
Development

No branches or pull requests

3 participants