-
Notifications
You must be signed in to change notification settings - Fork 69
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
Rewrite compiletest out of tree #536
Comments
This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed. cc @rust-lang/compiler @rust-lang/compiler-contributors |
@rustbot second |
@rustbot label -final-comment-period +major-change-accepted |
… r=oli-obk Migrate ui tests from legacy compiletest-style directives `//` to `ui_test`-style directives `//@` ## 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`](https://github.com/oli-obk/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 ui test mode (only). ## Key Changes 1. All ui test `//` directives are replaced by `//`@`` directives. 2. Only accept `//`@`` directives for ui tests (specifically ui tests). 3. 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 0. 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). 1. 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. 2. Checkout the `migrate-ui-test-directives` branch. 4. 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. 5. 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` 6. Confirm that there is no difference after running the migration tool when you are on the `migrate-ui-test-directives` branch. ## Next Steps - [x] ~~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 (rust-lang/rustc-dev-guide#1885). - [x] Add a `README.md` to `tests/ui` describing the directive style change.
… r=oli-obk Migrate ui tests from legacy compiletest-style directives `//` to `ui_test`-style directives `//@` ## 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`](https://github.com/oli-obk/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 1. All ui test `//` directives are replaced by `//`@`` directives. 2. Only accept `//`@`` directives for "ui" test suite. 3. 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 0. 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). 1. 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. 2. Checkout the `migrate-ui-test-directives` branch. 4. 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. 5. 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` 6. Confirm that there is no difference after running the migration tool when you are on the `migrate-ui-test-directives` branch. ## Next Steps - [x] ~~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 (rust-lang/rustc-dev-guide#1885). - [x] Add a `README.md` to `tests/ui` describing the directive style change.
…ves, r=oli-obk Migrate compiletest to use `ui_test`-style `//@` directives ## 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 compiletest tests to use the [`ui_test`](https://github.com/oli-obk/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 the remaining non-"ui" test suite tests. ## Key Changes 1. All `tests/` tests now use `//`@`` directives. 2. Compiletest only accepts `//`@`` and issues an error if an old-style directive is detected. 3. `// ignore-tidy` and `// ignore-tidy-*` are considered tidy directives and are ignored by compiletest header parsing. ## Diff Generation The diff is generated by: - Collecting directives from `tests/` via hijacking compiletest to emit successfully parsed directive lines. - Using a migration tool (<https://github.com/jieyouxu/compiletest-ui_test-header-migration/tree/master>) to replace `//` directives in compiletest tests with `//`@`.` ### Reproduction Steps 0. Delete the temporary file `$RUSTC_REPO_PATH/build/<target_triple>/test/__directive_lines.txt`, if the collection script was previously ran. 1. Use the <https://github.com/jieyouxu/rust/tree/collect-test-directives> collect-test-directives script, which outputs a temporary file recording headers occuring in each compiletest test. - You need to checkout this branch: `git checkout collect-test-directives`. - This needs to be rebased on latest master to ensure up-to-date test directives can be collected. - You need to run `./x test` on each of the `test/*` subfolders once: ```bash ./x test tests/assembly/ --stage 1 --force-rerun ./x test tests/codegen/ --stage 1 --force-rerun ./x test tests/codegen-units/ --stage 1 --force-rerun ./x test tests/coverage/ --stage 1 --force-rerun ./x test tests/coverage-run-rustdoc/ --stage 1 --force-rerun ./x test tests/debuginfo/ --stage 1 --force-rerun ./x test tests/incremental/ --stage 1 --force-rerun ./x test tests/mir-opt/ --stage 1 --force-rerun ./x test tests/pretty/ --stage 1 --force-rerun ./x test tests/run-make/ --stage 1 --force-rerun ./x test tests/run-make-fulldeps/ --stage 1 --force-rerun ./x test tests/run-pass-valgrind/ --stage 1 --force-rerun ./x test tests/rustdoc/ --stage 1 --force-rerun TARGET=<target-triple> ./x test tests/rustdoc-gui/ --stage 1 --force-rerun ./x test tests/rustdoc-js/ --stage 1 --force-rerun ./x test tests/rustdoc-js-std/ --stage 1 --force-rerun ./x test tests/rustdoc-json/ --stage 1 --force-rerun ./x test tests/rustdoc-ui/ --stage 1 --force-rerun ./x test tests/ui/ --stage 1 --force-rerun ./x test tests/ui-fulldeps/ --stage 1 --force-rerun ``` 2. Checkout the `migrate-compiletest-directives` branch. 3. Run the migration tool <https://github.com/jieyouxu/compiletest-ui_test-header-migration>. 4. Check that the migration at least does not cause test failures if you change compiletest to accept `//`@`` directives only. This is also required if the test outputs somehow need to be blessed. - `RUSTC_TEST_FAIL_FAST=1 ./x test tests/<secondary-directory> --stage 1 --bless` 5. Confirm that there is no difference after running the migration tool when you are on the `migrate-compiletest-directives` branch. ## Follow Up Work - [ ] Adjust rustc-dev-guide docs for compiletest directives (this time for all the other suites and modes).
…ves, r=oli-obk Migrate compiletest to use `ui_test`-style `//@` directives ## 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 compiletest tests to use the [`ui_test`](https://github.com/oli-obk/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 the remaining non-"ui" test suite tests. ## Key Changes 1. All `tests/` tests now use `//`@`` directives. 2. Compiletest only accepts `//`@`` and issues an error if an old-style directive is detected. 3. `// ignore-tidy` and `// ignore-tidy-*` are considered tidy directives and are ignored by compiletest header parsing. ## Diff Generation The diff is generated by: - Collecting directives from `tests/` via hijacking compiletest to emit successfully parsed directive lines. - Using a migration tool (<https://github.com/jieyouxu/compiletest-ui_test-header-migration/tree/master>) to replace `//` directives in compiletest tests with `//`@`.` ### Reproduction Steps 0. Delete the temporary file `$RUSTC_REPO_PATH/build/<target_triple>/test/__directive_lines.txt`, if the collection script was previously ran. 1. Use the <https://github.com/jieyouxu/rust/tree/collect-test-directives> collect-test-directives script, which outputs a temporary file recording headers occuring in each compiletest test. - You need to checkout this branch: `git checkout collect-test-directives`. - This needs to be rebased on latest master to ensure up-to-date test directives can be collected. - You need to run `./x test` on each of the `test/*` subfolders once: ```bash ./x test tests/assembly/ --stage 1 --force-rerun ./x test tests/codegen/ --stage 1 --force-rerun ./x test tests/codegen-units/ --stage 1 --force-rerun ./x test tests/coverage/ --stage 1 --force-rerun ./x test tests/coverage-run-rustdoc/ --stage 1 --force-rerun ./x test tests/debuginfo/ --stage 1 --force-rerun ./x test tests/incremental/ --stage 1 --force-rerun ./x test tests/mir-opt/ --stage 1 --force-rerun ./x test tests/pretty/ --stage 1 --force-rerun ./x test tests/run-make/ --stage 1 --force-rerun ./x test tests/run-make-fulldeps/ --stage 1 --force-rerun ./x test tests/run-pass-valgrind/ --stage 1 --force-rerun ./x test tests/rustdoc/ --stage 1 --force-rerun TARGET=<target-triple> ./x test tests/rustdoc-gui/ --stage 1 --force-rerun ./x test tests/rustdoc-js/ --stage 1 --force-rerun ./x test tests/rustdoc-js-std/ --stage 1 --force-rerun ./x test tests/rustdoc-json/ --stage 1 --force-rerun ./x test tests/rustdoc-ui/ --stage 1 --force-rerun ./x test tests/ui/ --stage 1 --force-rerun ./x test tests/ui-fulldeps/ --stage 1 --force-rerun ``` 2. Checkout the `migrate-compiletest-directives` branch. 3. Run the migration tool <https://github.com/jieyouxu/compiletest-ui_test-header-migration>. 4. Check that the migration at least does not cause test failures if you change compiletest to accept `//`@`` directives only. This is also required if the test outputs somehow need to be blessed. - `RUSTC_TEST_FAIL_FAST=1 ./x test tests/<secondary-directory> --stage 1 --bless` 5. Confirm that there is no difference after running the migration tool when you are on the `migrate-compiletest-directives` branch. ## Follow Up Work - [ ] Adjust rustc-dev-guide docs for compiletest directives (this time for all the other suites and modes).
…ves, r=oli-obk Migrate compiletest to use `ui_test`-style `//@` directives ## 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 compiletest tests to use the [`ui_test`](https://github.com/oli-obk/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 the remaining non-"ui" test suite tests. ## Key Changes 1. All `tests/` tests now use `//`@`` directives. 2. Compiletest only accepts `//`@`` and issues an error if an old-style directive is detected. 3. `// ignore-tidy` and `// ignore-tidy-*` are considered tidy directives and are ignored by compiletest header parsing. ## Diff Generation The diff is generated by: - Collecting directives from `tests/` via hijacking compiletest to emit successfully parsed directive lines. - Using a migration tool (<https://github.com/jieyouxu/compiletest-ui_test-header-migration/tree/master>) to replace `//` directives in compiletest tests with `//`@`.` ### Reproduction Steps 0. Delete the temporary file `$RUSTC_REPO_PATH/build/<target_triple>/test/__directive_lines.txt`, if the collection script was previously ran. 1. Use the <https://github.com/jieyouxu/rust/tree/collect-test-directives> collect-test-directives script, which outputs a temporary file recording headers occuring in each compiletest test. - You need to checkout this branch: `git checkout collect-test-directives`. - This needs to be rebased on latest master to ensure up-to-date test directives can be collected. - You need to run `./x test` on each of the `test/*` subfolders once: ```bash ./x test tests/assembly/ --stage 1 --force-rerun ./x test tests/codegen/ --stage 1 --force-rerun ./x test tests/codegen-units/ --stage 1 --force-rerun ./x test tests/coverage/ --stage 1 --force-rerun ./x test tests/coverage-run-rustdoc/ --stage 1 --force-rerun ./x test tests/debuginfo/ --stage 1 --force-rerun ./x test tests/incremental/ --stage 1 --force-rerun ./x test tests/mir-opt/ --stage 1 --force-rerun ./x test tests/pretty/ --stage 1 --force-rerun ./x test tests/run-make/ --stage 1 --force-rerun ./x test tests/run-make-fulldeps/ --stage 1 --force-rerun ./x test tests/run-pass-valgrind/ --stage 1 --force-rerun ./x test tests/rustdoc/ --stage 1 --force-rerun TARGET=<target-triple> ./x test tests/rustdoc-gui/ --stage 1 --force-rerun ./x test tests/rustdoc-js/ --stage 1 --force-rerun ./x test tests/rustdoc-js-std/ --stage 1 --force-rerun ./x test tests/rustdoc-json/ --stage 1 --force-rerun ./x test tests/rustdoc-ui/ --stage 1 --force-rerun ./x test tests/ui/ --stage 1 --force-rerun ./x test tests/ui-fulldeps/ --stage 1 --force-rerun ``` 2. Checkout the `migrate-compiletest-directives` branch. 3. Run the migration tool <https://github.com/jieyouxu/compiletest-ui_test-header-migration>. 4. Check that the migration at least does not cause test failures if you change compiletest to accept `//`@`` directives only. This is also required if the test outputs somehow need to be blessed. - `RUSTC_TEST_FAIL_FAST=1 ./x test tests/<secondary-directory> --stage 1 --bless` 5. Confirm that there is no difference after running the migration tool when you are on the `migrate-compiletest-directives` branch. ## Follow Up Work - [ ] Adjust rustc-dev-guide docs for compiletest directives (this time for all the other suites and modes).
…ves, r=oli-obk Migrate compiletest to use `ui_test`-style `//@` directives ## 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 compiletest tests to use the [`ui_test`](https://github.com/oli-obk/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 the remaining non-"ui" test suite tests. ## Key Changes 1. All `tests/` tests now use `//`@`` directives. 2. Compiletest only accepts `//`@`` and issues an error if an old-style directive is detected. 3. `// ignore-tidy` and `// ignore-tidy-*` are considered tidy directives and are ignored by compiletest header parsing. ## Diff Generation The diff is generated by: - Collecting directives from `tests/` via hijacking compiletest to emit successfully parsed directive lines. - Using a migration tool (<https://github.com/jieyouxu/compiletest-ui_test-header-migration/tree/master>) to replace `//` directives in compiletest tests with `//`@`.` ### Reproduction Steps 0. Delete the temporary file `$RUSTC_REPO_PATH/build/<target_triple>/test/__directive_lines.txt`, if the collection script was previously ran. 1. Use the <https://github.com/jieyouxu/rust/tree/collect-test-directives> collect-test-directives script, which outputs a temporary file recording headers occuring in each compiletest test. - You need to checkout this branch: `git checkout collect-test-directives`. - This needs to be rebased on latest master to ensure up-to-date test directives can be collected. - You need to run `./x test` on each of the `test/*` subfolders once: ```bash ./x test tests/assembly/ --stage 1 --force-rerun ./x test tests/codegen/ --stage 1 --force-rerun ./x test tests/codegen-units/ --stage 1 --force-rerun ./x test tests/coverage/ --stage 1 --force-rerun ./x test tests/coverage-run-rustdoc/ --stage 1 --force-rerun ./x test tests/debuginfo/ --stage 1 --force-rerun ./x test tests/incremental/ --stage 1 --force-rerun ./x test tests/mir-opt/ --stage 1 --force-rerun ./x test tests/pretty/ --stage 1 --force-rerun ./x test tests/run-make/ --stage 1 --force-rerun ./x test tests/run-make-fulldeps/ --stage 1 --force-rerun ./x test tests/run-pass-valgrind/ --stage 1 --force-rerun ./x test tests/rustdoc/ --stage 1 --force-rerun TARGET=<target-triple> ./x test tests/rustdoc-gui/ --stage 1 --force-rerun ./x test tests/rustdoc-js/ --stage 1 --force-rerun ./x test tests/rustdoc-js-std/ --stage 1 --force-rerun ./x test tests/rustdoc-json/ --stage 1 --force-rerun ./x test tests/rustdoc-ui/ --stage 1 --force-rerun ./x test tests/ui/ --stage 1 --force-rerun ./x test tests/ui-fulldeps/ --stage 1 --force-rerun ``` 2. Checkout the `migrate-compiletest-directives` branch. 3. Run the migration tool <https://github.com/jieyouxu/compiletest-ui_test-header-migration>. 4. Check that the migration at least does not cause test failures if you change compiletest to accept `//`@`` directives only. This is also required if the test outputs somehow need to be blessed. - `RUSTC_TEST_FAIL_FAST=1 ./x test tests/<secondary-directory> --stage 1 --bless` 5. Confirm that there is no difference after running the migration tool when you are on the `migrate-compiletest-directives` branch. ## Follow Up Work - [ ] Adjust rustc-dev-guide docs for compiletest directives (this time for all the other suites and modes). <rust-lang/rustc-dev-guide#1895>.
Proposal
Rewrite compiletest from scratch as a stable crate on crates.io. Miri is already using the MVP rewrite called
ui_test
(it currently lives in the miri repo, but will soon move out viagit subtree
). I got preliminary approval to get clippy to use it (probably via crates.io), and I am optimistic that it will get merged if I open a PR.There is a lot more work to be done to get it usable inside rustc (currently only ui tests are supported), but I am planning on doing this in a modular way so that rustc can have the parts that no one else wants live entirely in-tree without any syncs with another repo.
As an MVP I would like to replace the ui test suite logic with the new library (once feature parity has been reached). This will likely happen by changing the in-tree
compiletest
and the out of treeui_test
until they are in sync behaviour wise and then swapping them out in a PR that has no functional changes.Advantages
Cargo.toml
shared by all testsDisadvantages
git subtree
, still need to sync)Mentors or Reviewers
any volunteers? 😆
Process
The main points of the Major Change Process are as follows:
@rustbot second
.-C flag
, then full team check-off is required.@rfcbot fcp merge
on either the MCP or the PR.You can read more about Major Change Proposals on forge.
Comments
This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.
The text was updated successfully, but these errors were encountered: