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

Run x test --stage 1 in CI #99135

Closed
jyn514 opened this issue Jul 10, 2022 · 11 comments · Fixed by #99529
Closed

Run x test --stage 1 in CI #99135

jyn514 opened this issue Jul 10, 2022 · 11 comments · Fixed by #99529
Assignees
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.

Comments

@jyn514
Copy link
Member

jyn514 commented Jul 10, 2022

Similar to #95996, we expect contributors to run this frequently, so it should be tested in CI.

@eddyb suggested using the defaults in

# These defaults are meant for contributors to the compiler who do not modify codegen or LLVM
but I'm a little hesitant to do that because some of them are very wrong for CI (e.g. download-ci-llvm) and others shouldn't affect tests but make the compiler slower (e.g. debug-logging = true). I do agree we should enable incremental for these tests, though.

Not sure which builder to add these to - adding it to mingw-check would make it significantly slower, and I don't know which of the full builders has spare capacity. @pietroalbini do you have suggestions?

_Originally posted by @eddyb in #98660 (comment)

@jyn514 jyn514 added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. A-testsuite Area: The testsuite used to check the correctness of rustc T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Jul 10, 2022
@eddyb
Copy link
Member

eddyb commented Jul 10, 2022

To be clear, I was mistakenly under the impression that only stage1 tests were ran for PR CI.
But I could see how that may be an issue for ignore-stage1 tests (though, is it easy to only run those in stage2?).

@jyn514
Copy link
Member Author

jyn514 commented Jul 10, 2022

To be clear, I was mistakenly under the impression that only stage1 tests were ran for PR CI.

ah yeah no, we currently run x test --stage 2 in PR CI. I think it might make sense to only run --stage 1 now that you mention it though! would be a lot faster and in practice there are very few tests that only fail in stage 2.

though, is it easy to only run those in stage2?

not currently, no - I think it wouldn't be impossible to add to compiletest though, it already does filtering and parses the headers, so it should be possible to combine the two. That would mean we have to build the stage 2 compiler though, so if we switch to stage 1 I think it would be ok to only run those in a full bors merge, not in PR CI.

@pietroalbini
Copy link
Member

@jyn514 just add another builder, we have capacity for that.

@eddyb
Copy link
Member

eddyb commented Jul 17, 2022

If we do get another builder, can we have incremental enabled on it (or rather, whichever dev profile enables that)?

Some codegen tests haven't been passing for a short while because of the difference, and might be good to be tracking something that strongly resembles local workflows.

@jyn514
Copy link
Member Author

jyn514 commented Jul 18, 2022

@eddyb

I'm a little hesitant to do that because some of them are very wrong for CI (e.g. download-ci-llvm) and others shouldn't affect tests but make the compiler slower (e.g. debug-logging = true). I do agree we should enable incremental for these tests, though.

@rustbot
Copy link
Collaborator

rustbot commented Jul 18, 2022

Error: Parsing assign command in comment failed: ...'bot assign' | error: specify user to assign to at >| ''...

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #t-infra on Zulip.

@Milo123459
Copy link
Contributor

@rustbot claim

@Milo123459
Copy link
Contributor

What is the correct course of action here? Adding another builder?

@jyn514
Copy link
Member Author

jyn514 commented Jul 20, 2022

Yes, adding another builder is enough. I want to investigate running --stage 1 in PR CI instead of --stage 2 long term, but that's harder and shouldn't block testing this.

@RalfJung
Copy link
Member

This would have caught #99619, I assume?

@eddyb
Copy link
Member

eddyb commented Jul 22, 2022

@RalfJung Yes, despite not being filed as an issue, that's exactly the kind of test failure I was referencing in #99135 (comment) (it does require that we do enable incremental, not just test at stage 1).

@bors bors closed this as completed in 482153b Jul 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants