-
Notifications
You must be signed in to change notification settings - Fork 13.7k
Add more tests for the parallel rustc #145384
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
Conversation
r? @davidtwco rustbot has assigned @davidtwco. Use |
Some changes occurred in src/tools/compiletest cc @jieyouxu |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice if the tests could have slightly more descriptive names, though I appreciate that might be tricky.
src/tools/compiletest/src/runtest.rs
Outdated
@@ -2749,7 +2749,7 @@ impl<'test> TestCx<'test> { | |||
// Wrapper tools set by `runner` might provide extra output on failure, | |||
// for example a WebAssembly runtime might print the stack trace of an | |||
// `unreachable` instruction by default. | |||
let compare_output_by_lines = self.config.runner.is_some(); | |||
let compare_output_by_lines = self.props.parallel_front_end || self.config.runner.is_some(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is the only thing that parallel_front_end
enables, maybe exposing //@ compare-output-by-lines
would be better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that might be better, it's more direct in telling the test reader what the behavior is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Some nits
src/tools/compiletest/src/runtest.rs
Outdated
@@ -2749,7 +2749,7 @@ impl<'test> TestCx<'test> { | |||
// Wrapper tools set by `runner` might provide extra output on failure, | |||
// for example a WebAssembly runtime might print the stack trace of an | |||
// `unreachable` instruction by default. | |||
let compare_output_by_lines = self.config.runner.is_some(); | |||
let compare_output_by_lines = self.props.parallel_front_end || self.config.runner.is_some(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Problem: please document this directive and what it does in rustc-dev-guide.
src/tools/compiletest/src/runtest.rs
Outdated
@@ -2749,7 +2749,7 @@ impl<'test> TestCx<'test> { | |||
// Wrapper tools set by `runner` might provide extra output on failure, | |||
// for example a WebAssembly runtime might print the stack trace of an | |||
// `unreachable` instruction by default. | |||
let compare_output_by_lines = self.config.runner.is_some(); | |||
let compare_output_by_lines = self.props.parallel_front_end || self.config.runner.is_some(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remark: note that this has two potential issues:
- This can mask genuine bugs, where a symptom may be diagnostics reordering.
- The parallel front-end tests are still potential flaky.
I don't love (2) too much, but I recognize the pragmatic need for having some test coverage, so I don't want to block this.
My reservation is primarily that these tests can potentially fail someone else's PR, or a rollup, and can be rather confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remark: this is one of those that we do not know if it was genuinely fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I remove it ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's keep it for now, will need to disable it if it's not actually fixed
@rustbot author |
Reminder, once the PR becomes ready for a review, use |
The rustc-dev-guide subtree was changed. If this PR only touches the dev guide consider submitting a PR directly to rust-lang/rustc-dev-guide otherwise thank you for updating the dev guide with your changes. |
@rustbot ready |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
The queue was particularly backed up this week (partly due to outages and bors troubles), but this PR is in reasonably close position to tip of queue. |
Add more tests for the parallel rustc At the moment, the parallel frontend test cases are severely lacking. Althought some reported issues have been resolved, they haven't been added into the tests. This PR arranges the resolved ICE issues and adds tests for them. Whether it is worthwhile to add a separate test suite for the paralel frontend still requires futher discussion. But we are trying coveraging issues being resolved through capability of the existing UI test suite. Discussion: [Zulip](https://rust-lang.zulipchat.com/#narrow/channel/233931-t-compiler.2Fmajor-changes/topic/Proposal.20for.20a.20dedicated.20test.20suite.20for.20t.E2.80.A6.20compiler-team.23906) Related issues: - #120760 - #124423 fixed by #140358 - #127971 fxied by #140358 - #120601 fixed by #127311 cc `@jieyouxu`
💔 Test failed - checks-actions |
Do the machines get offline? |
GitHub Actions had an incident last night (a different one), https://www.githubstatus.com/incidents/c7kq3ctclddp @bors retry |
Add more tests for the parallel rustc At the moment, the parallel frontend test cases are severely lacking. Althought some reported issues have been resolved, they haven't been added into the tests. This PR arranges the resolved ICE issues and adds tests for them. Whether it is worthwhile to add a separate test suite for the paralel frontend still requires futher discussion. But we are trying coveraging issues being resolved through capability of the existing UI test suite. Discussion: [Zulip](https://rust-lang.zulipchat.com/#narrow/channel/233931-t-compiler.2Fmajor-changes/topic/Proposal.20for.20a.20dedicated.20test.20suite.20for.20t.E2.80.A6.20compiler-team.23906) Related issues: - #120760 - #124423 fixed by #140358 - #127971 fxied by #140358 - #120601 fixed by #127311 cc `@jieyouxu`
The job Click to see the possible cause of the failure (guessed by this bot)
|
💔 Test failed - checks-actions |
failed in library/std/src/thread/mod.rs , I guess this is unrelated to this PR? |
Yeah, however that test failing is a bit suspicious, I'll open an issue to track it. |
@bors retry |
☀️ Test successful - checks-actions |
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing 93edf9f (parent) -> 41a79f1 (this PR) Test differencesShow 14 test diffsStage 1
Stage 2
Additionally, 2 doctest diffs were found. These are ignored, as they are noisy. Job group index
Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard 41a79f1862aa6b81bac674598e275e80e9f09eb9 --output-dir test-dashboard And then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
Finished benchmarking commit (41a79f1): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)Results (primary -5.5%, secondary -3.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 468.299s -> 468.201s (-0.02%) |
At the moment, the parallel frontend test cases are severely lacking. Althought some reported issues have been resolved, they haven't been added into the tests.
This PR arranges the resolved ICE issues and adds tests for them.
Whether it is worthwhile to add a separate test suite for the paralel frontend still requires futher discussion. But we are trying coveraging issues being resolved through capability of the existing UI test suite.
Discussion: Zulip
Related issues:
no index for a field
#120760variances_of
returns&[ty::Variance]
' #124423 fixed by Usesearch_for_cycle_permutation
to look forvariances_of
#140358only 'variances_of' returns '&[ty::Variance]'
#127971 fxied by Usesearch_for_cycle_permutation
to look forvariances_of
#140358Unexpected type for constructor
#120601 fixed by Avoid follow-up errors and ICEs after missing lifetime errors on data structures #127311cc @jieyouxu