Skip to content

Convert run-make/coverage-reports tests to use a custom compiletest mode #112300

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

Merged
merged 12 commits into from
Jun 30, 2023

Conversation

Zalathar
Copy link
Contributor

@Zalathar Zalathar commented Jun 5, 2023

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
Copy link
Collaborator

rustbot commented Jun 5, 2023

r? @wesleywiser

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

@rustbot 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
@Zalathar
Copy link
Contributor Author

Zalathar commented Jun 5, 2023

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

This comment was marked as outdated.

@Zalathar
Copy link
Contributor Author

Zalathar commented Jun 5, 2023

@rustbot label +A-code-coverage

@rustbot rustbot added the A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) label Jun 5, 2023
@wesleywiser
Copy link
Member

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
Copy link
Contributor Author

Zalathar commented Jun 7, 2023

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
Copy link
Contributor Author

Zalathar commented Jun 7, 2023

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
Copy link
Contributor Author

Zalathar commented Jun 7, 2023

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
Copy link
Contributor Author

Zalathar commented Jun 7, 2023

(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 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
@Zalathar
Copy link
Contributor Author

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

@rust-log-analyzer

This comment has been minimized.

@Zalathar

This comment was marked as outdated.

@Zalathar
Copy link
Contributor Author

Looks like this is all working again now.

@jyn514 jyn514 self-assigned this Jun 12, 2023
@Zalathar
Copy link
Contributor Author

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.

Copy link
Member

@wesleywiser wesleywiser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Comment on lines +920 to +926
// FIXME(mati865): MinGW GCC miscompiles compiler-rt profiling library but with Clang it works
// properly. Since we only have GCC on the CI ignore the test for now.
"ignore-windows-gnu",
// FIXME(pietroalbini): this test currently does not work on cross-compiled
// targets because remote-test is not capable of sending back the *.profraw
// files generated by the LLVM instrumentation.
"ignore-cross-compile",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Zalathar
Copy link
Contributor Author

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

src/test/run-make*/coverage* tests should be reimplemented as a custom compiletest
7 participants