Uh oh!
There was an error while loading. Please reload this page.
Fix instrument-coverage tests by using Python to sort instantiation groups - #111179
Merged
Conversation
rustbot
commented
May 4, 2023
Collaborator
(rustbot has picked a reviewer for you, use r? to override) |
This comment was marked as resolved.
This comment was marked as resolved.
Zalatharforce-pushed
the
sort-groups
branch
2 times, most recently
from
May 4, 2023 13:32
9e18099 to
159e54fCompareZalathar
commented
May 5, 2023
MemberAuthor
@rustbot label +A-code-coverage +A-testsuite |
Mark-Simulacrum
commented
May 6, 2023
Member
@bors r+ |
bors
commented
May 6, 2023
Collaborator
matthiaskrgr added a commit
to matthiaskrgr/rust
that referenced
this pull request
May 6, 2023
…acrum Fix instrument-coverage tests by using Python to sort instantiation groups rust-lang#110942 was intended to fix a set of `-Cinstrument-coverage` tests, but it ended up silently *breaking* those tests on Linux, for annoying reasons detailed at rust-lang#111171. Dealing with `diff --ignore-matching-lines` across multiple platforms has been such a hassle that I've instead written a simple Python script that can detect instantiation groups in the output of `llvm-cov show`, and sort them in a predictable order so that they can be used as snapshots for an ordinary invocation of `diff`. This approach should be much less error-prone, because it can't accidentally ignore the wrong lines, and any unforeseen problems will tend to result in a Python exception or a failing diff.
matthiaskrgr added a commit
to matthiaskrgr/rust
that referenced
this pull request
May 6, 2023
…acrum Fix instrument-coverage tests by using Python to sort instantiation groups rust-lang#110942 was intended to fix a set of `-Cinstrument-coverage` tests, but it ended up silently *breaking* those tests on Linux, for annoying reasons detailed at rust-lang#111171. Dealing with `diff --ignore-matching-lines` across multiple platforms has been such a hassle that I've instead written a simple Python script that can detect instantiation groups in the output of `llvm-cov show`, and sort them in a predictable order so that they can be used as snapshots for an ordinary invocation of `diff`. This approach should be much less error-prone, because it can't accidentally ignore the wrong lines, and any unforeseen problems will tend to result in a Python exception or a failing diff.
matthiaskrgr
commented
May 6, 2023
Member
Zalathar
commented
May 7, 2023
MemberAuthor
@rustbot label -T-bootstrap -T-infra (Just temporarily changing CI settings to run the necessary tests for this PR; those changes won't get merged.) |
This comment has been minimized.
This comment has been minimized.
Zalathar
commented
May 7, 2023
MemberAuthor
This should be good to go now. I had thought I could get away with re-enabling two previously-disabled tests, but it turns out they had been disabled for good reason, beyond the scope of just sorting coverage report output. So those two tests remain disabled, which was already the status quo. @rustbot label -S-waiting-on-author +S-waiting-on-review |
Mark-Simulacrum
commented
May 7, 2023
Member
@bors r+ |
bors
commented
May 7, 2023
Collaborator
bors
commented
May 10, 2023
Collaborator
⌛ Testing commit 27a3ce2 with merge bfb69532ea3d505d1d92e436b6af47173c475293... |
bors
commented
May 10, 2023
Collaborator
💔 Test failed - checks-actions |
rust-log-analyzer
commented
May 10, 2023
Collaborator
This comment was marked as outdated.
This comment was marked as outdated.
bors
commented
May 10, 2023
Collaborator
@Zalathar: 🔑 Insufficient privileges: not in try users |
bjorn3
commented
May 11, 2023
Member
@bors retry |
bors added a commit
to rust-lang-ci/rust
that referenced
this pull request
May 12, 2023
…iaskrgr Rollup of 6 pull requests Successful merges: - rust-lang#111179 (Fix instrument-coverage tests by using Python to sort instantiation groups) - rust-lang#111393 (bump windows crate 0.46 -> 0.48) - rust-lang#111441 (Verify copies of mutable pointers in 2 stages in ReferencePropagation) - rust-lang#111456 (Update cargo) - rust-lang#111490 (Don't ICE in layout computation for placeholder types) - rust-lang#111492 (use by ref TokenTree iterator to avoid a few clones) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
This was referenced Jun 15, 2023
bors added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jul 9, 2023
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#110942 was intended to fix a set of
-Cinstrument-coveragetests, but it ended up silently breaking those tests on Linux, for annoying reasons detailed at #111171.Dealing with
diff --ignore-matching-linesacross multiple platforms has been such a hassle that I've instead written a simple Python script that can detect instantiation groups in the output ofllvm-cov show, and sort them in a predictable order so that they can be used as snapshots for an ordinary invocation ofdiff.This approach should be much less error-prone, because it can't accidentally ignore the wrong lines, and any unforeseen problems will tend to result in a Python exception or a failing diff.