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.

Sign up for GitHub

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

Jump to bottom

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

Closed
richkadel opened this issue May 6, 2021 · 4 comments · Fixed by #112300
Closed

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

richkadel opened this issue May 6, 2021 · 4 comments · Fixed by #112300
Labels
A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) C-enhancement Category: An issue proposing an enhancement or a PR with one.

Comments

@richkadel
Copy link
Contributor

The coverage tests (for -Z instrument-coverage) are implemented using Makefiles and embedded shell scripts, which is fragile.

Some cross-platform issues are unavoidable, but there have been some cross-platform issues, due to differences in the shell scripts and external commands (like sed).

Also, there is a known bug in some versions of make on MacOS that corrupts shell scripts during script interpretation.

Some of these issues could be avoided in the future if the test logic was rewritten/ported to Rust as a compiletest.
@richkadel
Copy link
Contributor Author

@tmandry

@richkadel richkadel mentioned this issue May 6, 2021
Closed
@richkadel
Copy link
Contributor Author

@rustbot +A-code-coverage

@richkadel richkadel mentioned this issue May 14, 2021
Open
@Dylan-DPC Dylan-DPC added C-enhancement Category: An issue proposing an enhancement or a PR with one. A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) labels Feb 13, 2023
@Zalathar
Copy link
Contributor

Zalathar commented May 6, 2023

I was bitten pretty badly by cross-platform differences in #110942/#111179, so I'm investigating what it would take to integrate the coverage tests into compiletest as a new test category.

@Zalathar Zalathar mentioned this issue Jun 5, 2023
Merged
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jun 29, 2023
@matthiaskrgr
Rollup merge of rust-lang#112300 - Zalathar:run-coverage, r=wesleywiser
ccc6b35 
Convert `run-make/coverage-reports` tests to use a custom compiletest mode

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 rust-lang#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`
compiler-errors added a commit to compiler-errors/rust that referenced this issue Jun 30, 2023
@compiler-errors
Rollup merge of rust-lang#112300 - Zalathar:run-coverage, r=wesleywiser
33a0812 
Convert `run-make/coverage-reports` tests to use a custom compiletest mode

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 rust-lang#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`
@bors bors closed this as completed in f00db43 Jun 30, 2023
@Zalathar
Copy link
Contributor

There is still one test left in tests/run-make/coverage-llvmir, but that's much less complex than the coverage-reports tests that were migrated.

It mostly delegates to $(LLVM_FILECHECK) anyway, so it seems fine as-is for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) C-enhancement Category: An issue proposing an enhancement or a PR with one.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Convert run-make/coverage-reports tests to use a custom compiletest mode Zalathar/rust
3 participants
@richkadel @Zalathar @Dylan-DPC