Migrate "ui" test suite from legacy compiletest-style directives `//` to `ui_test`-style directives `//@` #120881

jieyouxu · 2024-02-10T14:25:15Z

Preface

There's an on-going effort to rewrite parts of or the entirety of compiletest
(rust-lang/compiler-team#536). A step towards this involve migrating
ui tests to use the ui_test framework, which involves
changing compiletest directives in // <directive-name> style to ui_test //@ <directive-name>
style (rust-lang/compiler-team#512).

This PR aims to implement the directive-style change from // to //@ for ui tests only and
make compiletest only accept //@ directives in the "ui" test suite (only).

Key Changes

All ui test // directives are replaced by //@ directives.
Only accept //@ directives for "ui" test suite.
Errors if a comment could be interpreted as a legacy-style // directive.

Diff Generation

The diff is generated by:

Collecting directives from ui tests via hacking on compiletest.
Using a migration tool to replace // directives in ui tests with //@.

Reproduction Steps

Delete the temporary directory $RUSTC_REPO_PATH/build/x86_64-apple-darwin/test/ui/__directive_lines and the temporary file $RUSTC_REPO_PATH/build/x86_64-apple-darwin/test/ui/__directive_lines.txt (if you ran the collection script before).
Use the https://github.com/jieyouxu/rust/tree/collect-test-directives collect-test-directives
script, which outputs temporary files recording headers occuring in each ui test.
- You need to checkout this branch: git checkout collect-test-directives.
- You might need to rebase on lastest master and ensure there are no conflicts.
- You likely need to run ./x test tests/ui --stage 1 --force-rerun to generate the temporary
  files consistently.
Checkout the migrate-ui-test-directives branch.
Run the migration tool https://github.com/jieyouxu/compiletest-ui_test-header-migration.
- You will need to first generate a migration_config.toml via cargo run -- generate-config under $CWD.
- Then, update manual_directives = ["// should-fail"] in migration_config.toml. This is required because the collection script doesn't deal with some special meta ui tests and there are no other // should-fail occurrences.
Check that the migration at least does not cause UI test failures if you change compiletest to
accept //@ directives for ui tests only.
- RUSTC_TEST_FAIL_FAST=1 ./x test tests/ui --stage 1 --bless
Confirm that there is no difference after running the migration tool when you are on the
migrate-ui-test-directives branch.

Next Steps

Need to implement some kind of warning or tidy script to help contributors catch old-style
// <directive-name> directives, while only accepting ui_test-style //@ <directive-name>
directives. An error is emitted if a comment that could be interpreted as legacy-style test directive is encountered.
Need to properly document this change in e.g. rustc-dev-guide (Update docs about ui tests now using //@ headers rustc-dev-guide#1885).
- Add a README.md to tests/ui describing the directive style change.

fmease · 2024-02-10T15:17:19Z

Lol, did rustbot just give up?
r? compiler

oli-obk · 2024-02-10T18:30:31Z

r? @oli-obk

oli-obk · 2024-02-10T18:31:00Z

cc @asquared31415 @pietroalbini

asquared31415 · 2024-02-10T19:23:12Z

I no longer have the energy to work on my efforts for that, but I have pushed the rest of my changes to my fork and this was part of an effort to allow compiletest to properly parse ui_test style headers, not just change the syntax. Feel free to use any of this code for any purpose.

I think it's a better move to just move to ui_test style comments without changing the actual commands. However I would like to see a tidy warning and fix suggestion (that can auto apply on bless), to make sure that developers don't accidentally use the old style comments and the test doesn't actually test everything it's meant to.

(I have some code in my branch that has tidy fixing things, for reference. However I ended up completely changing how the parsing works, so it may or may not be usable.)

jieyouxu · 2024-02-12T09:48:11Z

I think it's a better move to just move to ui_test style comments without changing the actual commands. However I would like to see a tidy warning and fix suggestion (that can auto apply on bless), to make sure that developers don't accidentally use the old style comments and the test doesn't actually test everything it's meant to.

I agree that a tidy warning would be a good idea. I'm not entirely sure on the auto-apply fix suggestion though, because there might be unfortunate cases like

// This comment serves to mention that
// ignore-test is a very useful directive
// which ignores the test.

and then the test can become unintentionally ignored. (Even though is this the current behavior.)

oli-obk · 2024-02-12T09:54:34Z

It may be simpler to parse twice, once with @ and once without, and error if any successfully parse without.

jieyouxu · 2024-02-12T11:55:31Z

It may be simpler to parse twice, once with @ and once without, and error if any successfully parse without.

I added a very dumb check, trying to parse a // comment as a legacy directive. Currently it simply reports an error message and panics on the first such encountered directive.

tests/ui/abi/stack-protector.rs

bors · 2024-02-12T20:33:44Z

☔ The latest upstream changes (presumably #120980) made this pull request unmergeable. Please resolve the merge conflicts.

rustbot · 2024-02-13T17:55:49Z

Changes to the code generated for builtin derived traits.

cc @nnethercote

jieyouxu · 2024-02-13T18:11:28Z

Sorry there's something weird with my diff, might have accidentally done something...

tests/ui/asm/inline-syntax.rs

jieyouxu · 2024-02-15T11:59:17Z

I can squash the commits after the relevant migration parts are reviewed.
@rustbot review

oli-obk · 2024-02-16T10:02:22Z

lgtm! Please squash.

will the collect-test-directives branch still work on top of this or does it include the relevant parts to skip tidy?

rust-timer · 2024-02-17T00:17:00Z

Finished benchmarking commit (bccb9bb): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.3%	[-0.3%, -0.3%]	1
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.2%	[-1.2%, -1.2%]	1
All ❌✅ (primary)	-	-	0

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 638.862s -> 639.086s (0.04%)
Artifact size: 306.32 MiB -> 308.39 MiB (0.67%)

When these extra directives were ported over as part of rust-lang#112300, it made sense to introduce `iter_header_extra` and pass them in as an extra argument. But now that rust-lang#120881 has added a `mode` parameter to `iter_header` for its own purposes, it's slightly simpler to move the coverage special-case code directly into `iter_header` as well. This lets us get rid of `iter_header_extra`.

Move the extra directives for `Mode::CoverageRun` into `iter_header` When these extra directives were ported over as part of rust-lang#112300, it made sense to introduce `iter_header_extra` and pass them in as an extra argument. But now that rust-lang#120881 has added a `mode` parameter to `iter_header` for its own purposes, it's slightly simpler to move the coverage special-case code directly into `iter_header` as well. This lets us get rid of `iter_header_extra`.

Rollup merge of rust-lang#121233 - Zalathar:extra-directives, r=oli-obk Move the extra directives for `Mode::CoverageRun` into `iter_header` When these extra directives were ported over as part of rust-lang#112300, it made sense to introduce `iter_header_extra` and pass them in as an extra argument. But now that rust-lang#120881 has added a `mode` parameter to `iter_header` for its own purposes, it's slightly simpler to move the coverage special-case code directly into `iter_header` as well. This lets us get rid of `iter_header_extra`.

compiletest: Remove the one thing that was checking a directive's `original_line` This special handling of `ignore-tidy*` was introduced during the migration to `//`@`` directives (rust-lang#120881), and has become unnecessary after the subsequent removal of the legacy directive check (rust-lang#131392).

Rollup merge of rust-lang#131585 - Zalathar:original-line, r=jieyouxu compiletest: Remove the one thing that was checking a directive's `original_line` This special handling of `ignore-tidy*` was introduced during the migration to `//`@`` directives (rust-lang#120881), and has become unnecessary after the subsequent removal of the legacy directive check (rust-lang#131392).

jieyouxu force-pushed the migrate-ui-test-directives branch from 602d658 to c633baa Compare February 10, 2024 14:57

rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 10, 2024

rustbot assigned davidtwco Feb 10, 2024

rustbot assigned oli-obk and unassigned davidtwco Feb 10, 2024

oli-obk reviewed Feb 12, 2024

View reviewed changes

tests/ui/abi/stack-protector.rs Outdated Show resolved Hide resolved

oli-obk mentioned this pull request Feb 12, 2024

Update docs about ui tests now using //@ headers rust-lang/rustc-dev-guide#1885

Merged

oli-obk force-pushed the migrate-ui-test-directives branch from ff1704e to ba9995f Compare February 12, 2024 17:35

This comment has been minimized.

Sign in to view

bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Feb 12, 2024

jieyouxu force-pushed the migrate-ui-test-directives branch from ba9995f to e6fb22f Compare February 13, 2024 17:55

oli-obk reviewed Feb 13, 2024

View reviewed changes

tests/ui/asm/inline-syntax.rs Outdated Show resolved Hide resolved

jieyouxu force-pushed the migrate-ui-test-directives branch from e6fb22f to cbfb5e4 Compare February 13, 2024 18:35

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 15, 2024

This comment was marked as outdated.

Sign in to view

This was referenced Feb 17, 2024

const_mut_refs: allow mutable pointers to statics #120932

Merged

Rollup of 7 pull requests #121222

Closed

Zalathar mentioned this pull request Feb 17, 2024

Move the extra directives for Mode::CoverageRun into iter_header #121233

Merged

This was referenced Feb 21, 2024

Migrate compiletest to use ui_test-style //@ directives #121370

Merged

Rollup of 6 pull requests #121458

Closed

Zalathar mentioned this pull request Oct 12, 2024

compiletest: Remove the one thing that was checking a directive's original_line #131585

Merged

jieyouxu deleted the migrate-ui-test-directives branch November 7, 2024 14:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate "ui" test suite from legacy compiletest-style directives `//` to `ui_test`-style directives `//@` #120881

Migrate "ui" test suite from legacy compiletest-style directives `//` to `ui_test`-style directives `//@` #120881

jieyouxu commented Feb 10, 2024 •

edited

Loading

fmease commented Feb 10, 2024

oli-obk commented Feb 10, 2024

oli-obk commented Feb 10, 2024

asquared31415 commented Feb 10, 2024 •

edited

Loading

jieyouxu commented Feb 12, 2024

oli-obk commented Feb 12, 2024

jieyouxu commented Feb 12, 2024 •

edited

Loading

This comment has been minimized.

bors commented Feb 12, 2024

rustbot commented Feb 13, 2024

jieyouxu commented Feb 13, 2024

jieyouxu commented Feb 15, 2024 •

edited

Loading

oli-obk commented Feb 16, 2024

This comment was marked as outdated.

rust-timer commented Feb 17, 2024

Migrate "ui" test suite from legacy compiletest-style directives // to ui_test-style directives //@ #120881

Migrate "ui" test suite from legacy compiletest-style directives // to ui_test-style directives //@ #120881

Conversation

jieyouxu commented Feb 10, 2024 • edited Loading

Preface

Key Changes

Diff Generation

Reproduction Steps

Next Steps

fmease commented Feb 10, 2024

oli-obk commented Feb 10, 2024

oli-obk commented Feb 10, 2024

asquared31415 commented Feb 10, 2024 • edited Loading

jieyouxu commented Feb 12, 2024

oli-obk commented Feb 12, 2024

jieyouxu commented Feb 12, 2024 • edited Loading

This comment has been minimized.

bors commented Feb 12, 2024

rustbot commented Feb 13, 2024

jieyouxu commented Feb 13, 2024

jieyouxu commented Feb 15, 2024 • edited Loading

oli-obk commented Feb 16, 2024

This comment was marked as outdated.

rust-timer commented Feb 17, 2024

Overall result: ✅ improvements - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

Migrate "ui" test suite from legacy compiletest-style directives `//` to `ui_test`-style directives `//@` #120881

Migrate "ui" test suite from legacy compiletest-style directives `//` to `ui_test`-style directives `//@` #120881

jieyouxu commented Feb 10, 2024 •

edited

Loading

asquared31415 commented Feb 10, 2024 •

edited

Loading

jieyouxu commented Feb 12, 2024 •

edited

Loading

jieyouxu commented Feb 15, 2024 •

edited

Loading