-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
ui
ify run-pass
#54223
ui
ify run-pass
#54223
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
r=me |
but travis is unhappy |
Oops. Well I suppose this is what I was asking for by putting that panic in there Will fix |
@nikomatsakis by the way, this PR does seem very susceptible to conflicts, in the sense that any PR that adds a new Any objection to me throwing on a p=10 after I placate Travis? |
284c934
to
a49d7d5
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Oh, I guess the mode changes must apply to Okay well I'll just fix those as well. |
25fd4ab
to
e829515
Compare
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.
Well, this feels hacky, but I guess that was the point. compile-test definitely needs an owner to clean up and simplify the model.
src/tools/compiletest/src/runtest.rs
Outdated
@@ -263,7 +263,7 @@ impl<'test> TestCx<'test> { | |||
|
|||
fn should_run_successfully(&self) -> bool { | |||
match self.config.mode { | |||
RunPass => true, | |||
RunPass => !self.props.skip_codegen, |
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'm not crazy about having RunPass
be a distinct member of this enum, rather than just having it set flags on self.props
(and/or adding appropriate flags to all the tests). e.g.,, does this mean that skip_codegen
.. doesn't work on // run-pass
UI tests, but does work on run-pass
mode tests?
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've been assuming that self.props
was meant to reflect the actual set of directives on the header.
I can certainly change this code so that // skip-codegen
works in UI tests too.
But yes, I would personally prefer if things were rearchitected so that the set of properties were established up front, based on a combination of the header (under current revision/compare-mode), and then queries could look solely at those properties.
I think the one thing that would be nice is some kind of comment on the mode enum explaining the special role of "run-pass", but otherwise r=me and p=10 makes sense. |
e829515
to
180ae2e
Compare
@bors r=nikomatsakis p=10 (Setting high priority because any new run-pass test will cause this to PR to fail in its own testing.) |
📌 Commit 180ae2e8f3ff78eecbf704d15f586f38bf896242 has been approved by |
@bors r=nikomatsakis p=10 (wait, I remembered there's a chance that the newly added run-pass test(s) will not cause any compiler diagnostics to be emitted. So re-r+'ing until I double-check locally (after my local build finishes).) |
📌 Commit 180ae2e8f3ff78eecbf704d15f586f38bf896242 has been approved by |
💡 This pull request was already approved, no need to approve it again.
|
📌 Commit 180ae2e8f3ff78eecbf704d15f586f38bf896242 has been approved by |
⌛ Testing commit 180ae2e8f3ff78eecbf704d15f586f38bf896242 with merge 33f03528ea9deb4e093fb14b9b472a21530389e1... |
* In the case of `derive-same-struct`, it seemed cleaner to add the output than to try to modify the macro itself (which is where the output is coming from). * In the case of `custom-derive-partial-eq`, it was just easier to add the output than to attempt to port the test to use a procedural macro.
b5af1d7
to
a79db05
Compare
@bors r=nikomatsakis p=10 |
📋 Looks like this PR is still in progress, ignoring approval. Hint: Remove [WIP] from this PR's title when it is ready for review. |
@bors r=nikomatsakis p=10 |
📌 Commit a79db05 has been approved by |
@bors p=99 rollup fairness |
⌛ Testing commit a79db05 with merge e43129b9cc33abdc3ddf1517dfb4080cb57a339d... |
💔 Test failed - status-appveyor |
@bors retry |
…sakis `ui`ify run-pass This addresses the remainder of #53764 by first converting `src/test/run-pass` into another `ui`-style test suite, and then turning on `compare-mode=nll` for that new suite. After this lands, we can address #54047 for the short term by moving all the `src/test/ui/run-pass` tests back to `src/test/run-pass`. (Longer term, the compiler team's current (hypothetical sketch of a) plan (see [1][], [2][]) is unify all the tests by embedding these meta-properties like "// run-pass` into their headers explicitly and dropping the significance of their location on the file system.) [1]: https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/subject/weekly.20meeting.202018-09-13/near/133889370 [2]: #54047 (comment)
☀️ Test successful - status-appveyor, status-travis |
This addresses the remainder of #53764 by first converting
src/test/run-pass
into anotherui
-style test suite, and then turning oncompare-mode=nll
for that new suite.After this lands, we can address #54047 for the short term by moving all the
src/test/ui/run-pass
tests back tosrc/test/run-pass
.(Longer term, the compiler team's current (hypothetical sketch of a) plan (see 1, 2) is unify all the tests by embedding these meta-properties like "// run-pass` into their headers explicitly and dropping the significance of their location on the file system.)