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

Remove vestigial CI job msvc-aux. #73296

Merged
merged 1 commit into from
Jun 15, 2020
Merged

Remove vestigial CI job msvc-aux. #73296

merged 1 commit into from
Jun 15, 2020

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Jun 12, 2020

This CI job isn't really doing anything, so it seems prudent to remove it.

For some history:

Over time through attrition, the job was left with one lonely thing: src/test/run-pass-valgrind/pretty. And of course, this wasn't actually running the "pretty" tests. The normal run-pass-valgrind tests ran, and then when it tried to run in "pretty" mode, all the tests were ignored because compiletest thought nothing had changed (apparently compiletest isn't fingerprinting the mode? Needs more investigation…). run-pass-valgrind is already being run as part of x86_64-msvc-1, so there's no need to run it here.

I've taken the liberty of removing src/test/run-pass-valgrind/pretty as a distinct test. I'm guessing from the other PR's that the pretty tests should now live in src/test/pretty, and that the team has moved away from doing pretty tests on other parts of the src/test tree.

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 12, 2020
@mati865
Copy link
Contributor

mati865 commented Jun 12, 2020

How run-pass-valgrind can even run on MSVC?
There is no Valgrind for Windows 🤔

@ehuss
Copy link
Contributor Author

ehuss commented Jun 12, 2020

Oh, good question! It looks like the valgrind tests silently downgrade to just a normal run-pass test. AFAIK, none of the CI jobs use valgrind.

@Mark-Simulacrum
Copy link
Member

So just to make sure I understand, this isn't technically changing behavior but is sort of just affirming that pretty tests aren't being run? It seems like we should duplicate/move the pretty tests to src/test/pretty at least?

@ehuss
Copy link
Contributor Author

ehuss commented Jun 13, 2020

I'm not sure I'm understanding your questions correctly, as I'm somewhat unfamiliar with some of this.

IIUC, the pretty tests just ingest some code and verify the pretty printer spits out something that can still compile (modulo other annotations like compare-only). I'm not sure if there are any tests to "move". Previously the test suite used various parts of the src/test tree as a convenient corpus, where the src/test/pretty sub-tree was only used to verify pretty-printing. Over time, as more things were moved to src/test/ui, there were fewer and fewer subtrees that were being pretty-tested (until there were none outside of src/test/pretty). Presumably src/test/ui is too large to run pretty tests, or maybe just didn't justify the time to run, or maybe putting the correct annotations on each test would be too tedious?

Does that sound like a correct summary? I don't think there was anything special about run-pass-valgrind in terms of pretty validation, it was probably just added because it was small and fast to run.

I just tried to run pretty tests on src/test/ui, but almost every test fails (usually with some small whitespace differences).

I'm not sure how pretty-printing tests should be handled moving forward. I'm inclined to say new tests should be added to src/test/pretty whenever a pretty-specific bug is fixed. Otherwise, I think the compiler team would need to decide if they want to use a larger corpus (like src/test/ui) for pretty-printing validation, but it looks like that would take some non-trivial work.

@Mark-Simulacrum
Copy link
Member

Okay, thanks. I also took a look a the specific tests in question and it looks like there's not really anything special with regards to pretty printing there. #23616 (comment) also makes me feel like maybe we should just remove all the pretty annotations and create some tests which are specifically intended to test pretty printing (if we even care about it all that much).

I think the primary reason for wanting to test pretty printing is that proc macros currently sometimes fall back on it as a method for generating tokenstreams, but we're moving away from that model over time. We also sometimes use it in diagnostics, but those should be tested with dedicated UI tests for the most part rather than pretty tests, I think.

But, regardless of the ultimate fate of pretty tests, I agree that losing the run-pass-valgrind suite isn't going to hurt us, so I'm going to go ahead and approve this. cc @pietroalbini -- this frees up a builder slot for us which might be good for the limits you mentioned we might be hitting soon.

@bors r+

@bors
Copy link
Contributor

bors commented Jun 13, 2020

📌 Commit c0aef6d has been approved by Mark-Simulacrum

@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 Jun 13, 2020
@RalfJung
Copy link
Member

@bors rollup

RalfJung added a commit to RalfJung/rust that referenced this pull request Jun 15, 2020
@RalfJung
Rollup merge of rust-lang#73296 - ehuss:remove-msvc-aux, r=Mark-Simul…
baedb48 
…acrum

Remove vestigial CI job msvc-aux.

This CI job isn't really doing anything, so it seems prudent to remove it.

For some history:
* This was introduced in rust-lang#48809 when the msvc job was split in two to keep it under 2 hours (oh the good old days). At the time, this check-aux job did a bunch of things:
    * tidy
    * src/test/pretty
    * src/test/run-pass/pretty
    * src/test/run-fail/pretty
    * src/test/run-pass-valgrind/pretty
    * src/test/run-pass-fulldeps/pretty
    * src/test/run-fail-fulldeps/pretty
* Tidy was removed in rust-lang#60777.
* run-pass and run-pass-fulldeps moved to UI in rust-lang#63029
* src/test/pretty removed in rust-lang#58140
* src/test/run-fail moved to UI in rust-lang#71185
* run-fail-fulldeps removed in rust-lang#51285

Over time through attrition, the job was left with one lonely thing: `src/test/run-pass-valgrind/pretty`. And of course, this wasn't actually running the "pretty" tests. The normal `run-pass-valgrind` tests ran, and then when it tried to run in "pretty" mode, all the tests were ignored because compiletest thought nothing had changed (apparently compiletest isn't fingerprinting the mode?  Needs more investigation…). `run-pass-valgrind` is already being run as part of `x86_64-msvc-1`, so there's no need to run it here.

I've taken the liberty of removing `src/test/run-pass-valgrind/pretty` as a distinct test. I'm guessing from the other PR's that the pretty tests should now live in `src/test/pretty`, and that the team has moved away from doing pretty tests on other parts of the `src/test` tree.
This was referenced Jun 15, 2020
Closed
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 15, 2020
@bors
Auto merge of rust-lang#73369 - RalfJung:rollup-hl8g9zf, r=RalfJung
ff4a253 
Rollup of 10 pull requests

Successful merges:

 - rust-lang#72707 (Use min_specialization in the remaining rustc crates)
 - rust-lang#72740 (On recursive ADT, provide indirection structured suggestion)
 - rust-lang#72879 (Miri: avoid tracking current location three times)
 - rust-lang#72938 (Stabilize Option::zip)
 - rust-lang#73086 (Rename "cyclone" to "apple-a7" per changes in upstream LLVM)
 - rust-lang#73104 (Example about explicit mutex dropping)
 - rust-lang#73139 (Add methods to go from a nul-terminated Vec<u8> to a CString)
 - rust-lang#73296 (Remove vestigial CI job msvc-aux.)
 - rust-lang#73304 (Revert heterogeneous SocketAddr PartialEq impls)
 - rust-lang#73331 (extend network support for HermitCore)

Failed merges:

r? @ghost
@bors bors merged commit fb75d4a into rust-lang:master Jun 15, 2020
@cuviper cuviper added this to the 1.46 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.46
Development

Successfully merging this pull request may close these issues.
7 participants
@ehuss @rust-highfive @mati865 @Mark-Simulacrum @bors @RalfJung @cuviper