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

Re-enable some coverage tests on Linux #113389

Merged
merged 1 commit into from
Jul 10, 2023
Merged

Conversation

Zalathar
Copy link
Contributor

@Zalathar Zalathar commented Jul 6, 2023

These tests were originally disabled (on all platforms) in #110393, because those changes had made them start failing on Linux for unclear reasons.

I tried to re-enable them unconditionally in #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 #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.

@rustbot
Copy link
Collaborator

rustbot commented Jul 6, 2023

r? @Mark-Simulacrum

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

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 6, 2023
@Zalathar
Copy link
Contributor Author

Zalathar commented Jul 6, 2023

@rustbot label +A-code-coverage

@rustbot rustbot added the A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) label Jul 6, 2023
@Zalathar
Copy link
Contributor Author

Zalathar commented Jul 6, 2023

From what I can tell, #110393 (comment) is referring to disabling these tests temporarily, in the hopes that future const changes would make them work again. So that seems to be the most likely explanation of why they're magically working again (though it's still a mystery why they broke in the first place).

@Zalathar
Copy link
Contributor Author

Zalathar commented Jul 6, 2023

(I'm using #112514 to verify that these tests will indeed work on Linux, since the x86_64-gnu-llvm-14 job that normally runs on PRs doesn't run the coverage tests.)

→ success https://github.com/rust-lang/rust/actions/runs/5470324669/jobs/9960279192

@Mark-Simulacrum
Copy link
Member

@bors r+ rollup=iffy

@bors
Copy link
Contributor

bors commented Jul 8, 2023

📌 Commit e4b81f6 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 8, 2023
@bors
Copy link
Contributor

bors commented Jul 9, 2023

⌛ Testing commit e4b81f6 with merge 70d7283...

@bors
Copy link
Contributor

bors commented Jul 10, 2023

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 70d7283 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 10, 2023
@bors bors merged commit 70d7283 into rust-lang:master Jul 10, 2023
@rustbot rustbot added this to the 1.73.0 milestone Jul 10, 2023
@Zalathar Zalathar deleted the re-enable branch July 10, 2023 01:22
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (70d7283): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
4.6% [4.6%, 4.6%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
1.0% [1.0%, 1.0%] 1
Regressions ❌
(secondary)
2.6% [2.6%, 2.6%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 1.0% [1.0%, 1.0%] 1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
3.4% [3.2%, 3.6%] 3
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 659.399s -> 658.067s (-0.20%)

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) merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants