Convert `run-make/coverage-reports` tests to use a custom compiletest mode by Zalathar · Pull Request #112300 · rust-lang/rust

Zalathar · 2023-06-05T03:19:05Z

I was frustrated by the fact that most of the coverage tests are glued together with makefiles and shell scripts, so I tried my hand at converting most of them over to a newly-implemented run-coverage mode/suite in compiletest.

This ~~mostly~~ resolves #85009, ~~though I've left a small number of the existing tests as-is because they would require more work to fix/support~~.

I had time to go back and add support for the more troublesome tests that I had initially skipped over, so this PR now manages to completely get rid of run-make/coverage-reports.

The patches are arranged as follows:

Declare the new mode/suite in bootstrap
Small changes to compiletest that will be used by the new mode
Implement the new mode in compiletest
Migrate most of the tests over
Add more code to bootstrap and compiletest to support the remaining tests
Migrate the remaining tests (with some temporary hacks to avoid re-blessing them)
Remove the temporary hacks and re-bless the migrated tests
Remove the unused remnants of run-make/coverage-reports

rustbot · 2023-06-05T03:19:13Z

r? @wesleywiser

(rustbot has picked a reviewer for you, use r? to override)

Zalathar · 2023-06-05T03:23:56Z

The part I’m least confident about is the implied needs-profiler-support check for each test, since the existing needs-checking code doesn't really anticipate that sort of use-case.

Zalathar · 2023-06-05T03:29:34Z

@rustbot label +A-code-coverage

src/bootstrap/test.rs

src/tools/compiletest/src/header/needs.rs

wesleywiser · 2023-06-06T15:31:19Z

The part I’m least confident about is the implied needs-profiler-support check for each test, since the existing needs-checking code doesn't really anticipate that sort of use-case.

This part looks ok to me but if @jyn514 wants to check as well, that would be awesome!

ignore-windows-gnu

If I recall correctly, code coverage isn't supported on windows-gnu which is why the tests ignore it.

Zalathar · 2023-06-07T00:45:27Z

Rebased and added some small tweaks based on feedback:

Adjusted comments and control flow in the bootstrap code that supplies --llvm-bin-dir
profiler_support is made pub(super) in the patch that actually needs it
Added a comment to the profiler_support check

I haven't looked into the ignore-windows-gnu situation.

Zalathar · 2023-06-07T02:51:12Z

In light of still needing to support ignore-windows-gnu, what I might do is add a way to inject “extra” directives into iter_header, so that they are processed as though they appeared directly in the file. That seems like the least intrusive way to support arbitrary mode-implied directives.

Zalathar · 2023-06-07T03:23:57Z

OK, I’ve fundamentally changed how the mode-implied directives work.

iter_header_extra now takes a list of caller-supplied directives that should be processed as though they appeared on “line 0” of the file being inspected. There's a hard-coded check that adds three extra directives for Mode::RunCoverage tests, and does nothing for all other modes.

This is still a little bit gross, but I think it’s the best compromise for now.

Zalathar · 2023-06-07T03:26:45Z

(A side-effect of the above is that it’s no longer necessary to move profiler_support into the needs cache struct, but I’ve left that change intact because I think it’s a reasonable little improvement in its own right.)

Zalathar · 2023-06-12T03:05:15Z

I've now migrated all of the run-make/coverage-reports tests, so some code has been changed/added since the last review.

Zalathar · 2023-06-12T12:29:51Z

Looks like this is all working again now.

Zalathar · 2023-06-12T22:43:36Z

Dealing with doctest.rs was tricky, since it’s inherently a lot more complicated than the other tests.

I was tempted to leave it as its own run-make test, but that would ultimately leave us with make/shell/Python code duplicating much of run-coverage.
I thought about including it in the main run-coverage suite, but that would force the whole suite to require rustdoc (which is a real cost from a clean build), and require a new directive to actually trigger running the doctests (or running them unconditionally for everything).

So while it feels a bit weird to introduce a whole extra suite for one test, I think it ends up being the more natural solution.

wesleywiser

This is looking pretty good to me! I noticed there used to be a mechanism in the coverage makefile to bless the tests, does --bless still work?

wesleywiser · 2023-06-13T14:31:55Z

src/tools/compiletest/src/header.rs

Thanks for including these FIXMEs here, that will make it easier to understand in the future when they can be removed!

tests/run-coverage/uses_crate.rs

tests/run-coverage/uses_inline_crate.rs

Zalathar · 2023-06-13T22:18:01Z

This is looking pretty good to me! I noticed there used to be a mechanism in the coverage makefile to bless the tests, does --bless still work?

Yes, --bless is now handled automatically by the compiletest methods for comparing output. I’ve used it on these migrated tests to manually verify that it still works (e.g. in the patch that re-blesses some tests).

Re-enable some coverage tests on Linux These tests were originally disabled (on all platforms) in rust-lang#110393, because those changes had made them start failing on Linux for unclear reasons. I tried to re-enable them unconditionally in rust-lang#111179, since they worked locally on my Mac, but I found that they were still failing on Linux, so I gave up at that time. Later while working on rust-lang#112300 I was able to re-enable them on Windows and Mac, since those changes made it possible to add specific `ignore-` directives to individual tests. I noticed at the time that the tests actually seemed to be working again on Linux, but by that point I didn't want to risk more CI failures, so I left them disabled on Linux with an intention to re-enable them later. Now I'm going back to re-enable them on Linux too, since they seem to work fine. --- Because `run-coverage` tests are sensitive to line numbers, and `x test tidy` doesn't like leading blank lines, I've replaced the old comment/ignore with an informative comment that occupies the same number of lines.

Prior to rust-lang#114875, these tests were very sensitive to lines being added/removed, so the migration to `run-coverage` in rust-lang#112300 tried hard to avoid disturbing the existing line numbers. That resulted in some awkward reshuffling when certain comments/directives needed to be added or moved. Now that we don't have to worry about preserving line numbers, we can rearrange those comments into a more conventional layout.

The demangler was only needed by coverage tests, but those tests were migrated into their own custom test mode in rust-lang#112300. This avoids having to build the demangler just for run-make tests. It will still be built as needed by run-coverage tests or for other purposes.

Avoid unnecessary builds/rebuilds of `rust-demangler` This is a combination of two loosely-related changes: - Don't build `rust-demangler` as a dependency of `tests/run-make`, because after rust-lang#112300 none of the remaining run-make tests actually use it. (If future run-make tests ever do need the demangler, it'll be easy to add it back.) - For `tests/run-coverage`, build the demangler with the stage 0 compiler instead of the current-stage compiler. This avoids having to uselessly rebuild the demangler after modifying and rebuilding the compiler itself.

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: Unify `cmd2procres` with `run_command_to_procres` Historical context: I originally added `run_command_to_procres` in rust-lang#112300 and rust-lang#114843, because I didn't like the overly-specific failure message in `cmd2procres`, but at the time I didn't feel confident enough to change the existing code, so I just added my own similar code. Now I'm going back to remove this redundancy by eliminating `cmd2procress`, and adjusting all callers to use a slightly-tweaked `run_command_to_procres` instead.

Rollup merge of rust-lang#125753 - Zalathar:procres, r=jieyouxu compiletest: Unify `cmd2procres` with `run_command_to_procres` Historical context: I originally added `run_command_to_procres` in rust-lang#112300 and rust-lang#114843, because I didn't like the overly-specific failure message in `cmd2procres`, but at the time I didn't feel confident enough to change the existing code, so I just added my own similar code. Now I'm going back to remove this redundancy by eliminating `cmd2procress`, and adjusting all callers to use a slightly-tweaked `run_command_to_procres` instead.

rustbot assigned wesleywiser Jun 5, 2023

rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Jun 5, 2023

This comment was marked as outdated.

Sign in to view

rustbot added the A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) label Jun 5, 2023

wesleywiser reviewed Jun 6, 2023

View reviewed changes

src/bootstrap/test.rs Outdated Show resolved Hide resolved

src/tools/compiletest/src/header/needs.rs Outdated Show resolved Hide resolved

Zalathar force-pushed the run-coverage branch from 0a730d8 to 551e74f Compare June 7, 2023 00:43

Zalathar force-pushed the run-coverage branch from 551e74f to b1910eb Compare June 7, 2023 03:20

Zalathar force-pushed the run-coverage branch from b1910eb to d96cc6e Compare June 12, 2023 02:59

Zalathar changed the title ~~Convert most run-make/coverage-reports tests to use a custom compiletest mode~~ Convert run-make/coverage-reports tests to use a custom compiletest mode Jun 12, 2023

This comment has been minimized.

Sign in to view

Zalathar force-pushed the run-coverage branch from d96cc6e to 610d9ba Compare June 12, 2023 03:07

This comment was marked as outdated.

Sign in to view

Zalathar mentioned this pull request Jun 12, 2023

Dummy PR to test coverage test changes in CI #112514

Closed

Zalathar force-pushed the run-coverage branch from 610d9ba to 87107a3 Compare June 12, 2023 08:07

jyn514 self-assigned this Jun 12, 2023

wesleywiser reviewed Jun 13, 2023

View reviewed changes

Zalathar mentioned this pull request Jul 6, 2023

Re-enable some coverage tests on Linux #113389

Merged

Zalathar mentioned this pull request Oct 26, 2023

Avoid unnecessary builds/rebuilds of rust-demangler #117197

Merged

Zalathar mentioned this pull request Feb 17, 2024

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

Merged

Zalathar mentioned this pull request May 30, 2024

compiletest: Unify cmd2procres with run_command_to_procres #125753

Merged

Uh oh!

Conversation

Zalathar commented Jun 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Jun 5, 2023

Uh oh!

Zalathar commented Jun 5, 2023

Uh oh!

This comment was marked as outdated.

Zalathar commented Jun 5, 2023

Uh oh!

Uh oh!

Uh oh!

wesleywiser commented Jun 6, 2023

Uh oh!

Zalathar commented Jun 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Zalathar commented Jun 7, 2023

Uh oh!

Zalathar commented Jun 7, 2023

Uh oh!

Zalathar commented Jun 7, 2023

Uh oh!

Zalathar commented Jun 12, 2023

Uh oh!

This comment has been minimized.

This comment was marked as outdated.

Zalathar commented Jun 12, 2023

Uh oh!

Zalathar commented Jun 12, 2023

Uh oh!

wesleywiser left a comment

Choose a reason for hiding this comment

Uh oh!

wesleywiser Jun 13, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Zalathar commented Jun 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Zalathar commented Jun 5, 2023 •

edited

Loading

Zalathar commented Jun 7, 2023 •

edited

Loading