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

Only golden arches #114225

Merged
merged 3 commits into from
Jul 31, 2023
Merged

Only golden arches #114225

merged 3 commits into from
Jul 31, 2023

Conversation

workingjubilee
Copy link
Member

A number of tests in the test suite have applied the somewhat comedic practice of ignoring every single target architecture that rustc has ever supported. This is silly, when they are clearly tests built around certain assumptions, primarily of the x86-64 architecture, or in one case when they are only relevant for a handful of 32-bit targets. This has even resulted, in one case, in the same architecture being ignored twice!

Document these better, and use a "revision + only-arch" idiom in the test headers to denote the "golden arches" that actually pass these tests.

Slightly more complicated: also give them appropriate names
that somewhat describe the cases they are trying to cover,
using information from PR chatter in rust-lang#47158
@rustbot
Copy link
Collaborator

rustbot commented Jul 29, 2023

r? @compiler-errors

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 29, 2023
Combining revisions with only-arch allows specifying
that a test only applies to a handful of targets.
This allows removing a large amount of repetition
in the test suite for tests that do not benefit.
The revisions are suboptimal for this for some tests,
so they aren't preferred in those cases.
@ChrisDenton
Copy link
Member

Huh, I didn't know only-* was additive like that.

@workingjubilee
Copy link
Member Author

@ChrisDenton It is not.

This pattern requires the use of revisions. The revision only applies a directive for that revision, and reruns the test (including ignoring it) for each revision.

@compiler-errors
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jul 31, 2023

📌 Commit 20ebbf4 has been approved by compiler-errors

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 31, 2023
@bors
Copy link
Contributor

bors commented Jul 31, 2023

⌛ Testing commit 20ebbf4 with merge 03b8b50...

@bors
Copy link
Contributor

bors commented Jul 31, 2023

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 03b8b50 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 31, 2023
@bors bors merged commit 03b8b50 into rust-lang:master Jul 31, 2023
@rustbot rustbot added this to the 1.73.0 milestone Jul 31, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (03b8b50): comparison URL.

Overall result: ✅ improvements - 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)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-5.4% [-5.4%, -5.4%] 1
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)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.6% [-2.6%, -2.6%] 1
All ❌✅ (primary) - - 0

Cycles

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

Binary size

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

Bootstrap: 653.862s -> 652.303s (-0.24%)

@RalfJung
Copy link
Member

Do multiple "only" not have the semantics "test runs if any of the architectures match"? That is the semantics I would expect (and it is what the ui-test crate does).

@workingjubilee
Copy link
Member Author

Hmm, when I talked with someone who knows more about ui_test and compiletest than me, they said it didn't do unions of only-*, but now I'm hearing conflicting reports so...!

@RalfJung
Copy link
Member

RalfJung commented Jul 31, 2023

Ah, seems like I was wrong for ui_test

https://github.com/oli-obk/ui_test/blob/5065d1c71e3382e65f422919d020843fc63b6914/src/lib.rs#L1120

The revisions are a clever trick. :)

@workingjubilee workingjubilee deleted the only-golden-arches branch July 31, 2023 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants