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.

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

coverage: Harmonize the coverage-map and run-coverage test directories #117340

Closed
wants to merge 5 commits into from

Conversation

Zalathar
Copy link
Contributor

@Zalathar Zalathar commented Oct 29, 2023

While exploring ways to merge these two separate test directories into one directory, I quickly found that an important first step is to harmonize the two directories so that their sets of enclosed *.rs files are completely identical, instead of just mostly-identical.


This has been split off into its own PR so that it can be reviewed independently as a smaller unit. I’m still working on ways to actually combine these two test directories into one shared directory; that work will build on top of this PR. The two directories will be fully unified by #117484, which builds on top of these changes.


Closed in favour of #117484, which incorporates all of these changes.

@rustbot
Copy link
Collaborator

rustbot commented Oct 29, 2023

r? @compiler-errors

(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 Oct 29, 2023
@Zalathar
Copy link
Contributor Author

@rustbot label +A-code-coverage

@rustbot rustbot added the A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) label Oct 29, 2023
@Zalathar Zalathar force-pushed the harmonize branch 2 times, most recently from 6aea535 to 2856753 Compare October 31, 2023 00:36
@cjgillot
Copy link
Contributor

Could we have a single directory doing both testsuites?

@Zalathar
Copy link
Contributor Author

That's what I'm working towards, but I'll need to make some changes to compiletest for that to work properly.

So this PR is cleaning up the two test directories so that when I do make those changes, actually merging the two directories becomes a trivial step.

@Zalathar
Copy link
Contributor Author

(E.g. right now bootstrap can't actually handle a test suite having a different name from its test directory, so the obvious approach of “just point two different suites at the same directory” doesn't actually work.)

@Zalathar Zalathar force-pushed the harmonize branch 2 times, most recently from 9c38bb3 to 0520bce Compare November 1, 2023 10:43
@cjgillot
Copy link
Contributor

cjgillot commented Nov 1, 2023

(E.g. right now bootstrap can't actually handle a test suite having a different name from its test directory, so the obvious approach of “just point two different suites at the same directory” doesn't actually work.)

What about a single directory = testsuite which runs the checks from both current testsuites?

@Zalathar
Copy link
Contributor Author

Zalathar commented Nov 1, 2023

I figured out how to make the necessary changes in bootstrap without touching compiletest at all, so I've filed the full unification as #117484.

@Zalathar Zalathar force-pushed the harmonize branch 2 times, most recently from bbe6d66 to be743a9 Compare November 3, 2023 01:18
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 4, 2023
coverage: Unify `tests/coverage-map` and `tests/run-coverage` into `tests/coverage`

Ever since the introduction of the `coverage-map` suite, it's been awkward to have to manage two separate coverage test directories containing dozens of mostly-identical files.

However, those two suites were separate for good reasons. They have very different requirements (since only one of them requires actually running the test program), running only one suite is noticeably faster than running both, and having separate suites allows them to be blessed separately if desired. So while unifying them was an obvious idea, actually doing so was non-trivial.

---

Nevertheless, this PR finds a way to merge the two suites into one directory while retaining almost all of the developer-experience benefits of having two suites. This required non-trivial implementations of `Step`, but the end result works very smoothly.

---

The first 5 commits are a copy of rust-lang#117340, which is a necessary prerequisite, though they can be reviewed directly as part of this PR if desired.
This is a step towards being able to unify the two coverage test directories.

There are two tests that require adjustment:

- `overflow.rs` requires an explicit `-Coverflow-checks=yes`
- `sort_groups.rs` is sensitive to provably unused instantiations
There is another test named `if.rs` in `tests/coverage-map/status-quo/`, so
this test stands in the way of flattening that directory into its parent.

Fortunately both tests are more-or-less equivalent, so removing this one is
fine.
These multi-file tests were not copied over in rust-lang#114843 because they weren't
working, but it turns out that they just need the correct crate-type.
@compiler-errors
Copy link
Member

r? cjgillot

@Zalathar
Copy link
Contributor Author

Zalathar commented Nov 7, 2023

These changes have been incorporated into #117484, which has received an r+ but encountered test failures on Windows that have now been fixed.

So instead of approving this PR, it might make more sense to re-approve the combined PR and merge both parts in one go.

@compiler-errors
Copy link
Member

Yeah, sounds good, let's close this PR then, if you don't mind.

bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 8, 2023
coverage: Unify `tests/coverage-map` and `tests/run-coverage` into `tests/coverage`

Ever since the introduction of the `coverage-map` suite, it's been awkward to have to manage two separate coverage test directories containing dozens of mostly-identical files.

However, those two suites were separate for good reasons. They have very different requirements (since only one of them requires actually running the test program), running only one suite is noticeably faster than running both, and having separate suites allows them to be blessed separately if desired. So while unifying them was an obvious idea, actually doing so was non-trivial.

---

Nevertheless, this PR finds a way to merge the two suites into one directory while retaining almost all of the developer-experience benefits of having two suites. This required non-trivial implementations of `Step`, but the end result works very smoothly.

---

The first 5 commits are a copy of rust-lang#117340, which has been closed in favour of this PR.
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-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)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants