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

rustbuild: Delete extraneous pretty test suites #38420

Closed
wants to merge 1 commit into from

Conversation

alexcrichton
Copy link
Member

The pretty printer in the compiler isn't really heavily used, and there's not
really many stability guarantees around it. In that sense we don't need to
exhaustively test it on every single build of Rust. Additionally these test
suites typically take around 10-15 minutes of each CI run, which certainly adds
up over time.

This commit deletes the pretty passes over the standard test suites, e.g.
run-pass, run-pass-fulldeps, etc. The src/test/pretty folder is still kept and
we can retain test cases there, but this should cut down on the vast majority of
pretty test case related test time.

The pretty printer in the compiler isn't really heavily used, and there's not
really many stability guarantees around it. In that sense we don't need to
exhaustively test it on every single build of Rust. Additionally these test
suites typically take around 10-15 minutes of each CI run, which certainly adds
up over time.

This commit deletes the pretty passes over the standard test suites, e.g.
run-pass, run-pass-fulldeps, etc. The `src/test/pretty` folder is still kept and
we can retain test cases there, but this should cut down on the vast majority of
pretty test case related test time.
@alexcrichton
Copy link
Member Author

r? @brson

@rust-highfive rust-highfive assigned brson and unassigned nikomatsakis Dec 17, 2016
@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

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

@brson
Copy link
Contributor

brson commented Dec 17, 2016

Oh it's on.

@petrochenkov
Copy link
Contributor

Pass rate for pretty printing tests is in relatively good state now, I recently fixed and enabled many previously failing tests and the few remaining tests are covered by issues #37195, #37199, #37201.
I'm not really sure if they should stay or not and wouldn't mind removing. In theory testing is good, but 15 minutes is 15 minutes...
In any case, I think this needs some attention from people before merging.

cc @nrc

@alexcrichton
Copy link
Member Author

Ah so I should clarify that I'm currently under the impression that we use the pretty printer very little, if at all, in the compiler. Please correct me if I'm wrong though! If it's critical that the pretty printer works in every way possible then we should keep testing everything, of course.

@brson
Copy link
Contributor

brson commented Dec 29, 2016

The pretty printer is used for example to print arbitrary expressions generated by procedural macros, in error messages.

I'd prefer to keep running pretty on the 'pretty' tests and the 'run-pass' tests, but punt them to the cargotest bot (maybe renaming cargotest to 'aux' or something), so they don't compete for ci time. We can have a single aux test target that tests these minor things that should be tested, but shouldn't break.

@steveklabnik
Copy link
Member

I'm in favor of removing pretty entirely, but I remember @erickt saying "nooooooooo"

@alexcrichton
Copy link
Member Author

@brson ok that approach seems reasonable to me, I'll close this and reopen with that strategy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants