Test UI tests for pass=check #72053
Merged
Test UI tests for pass=check #72053
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @kennytm (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
RalfJung
reviewed
May 9, 2020
|
@RalfJung I'm not familiar with the various UI test annotations these days, could you either suggest fixes or push a commit to my branch fixing the test cases failing in the PR builder? |
|
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 |
|
The annotations are documented at https://rustc-dev-guide.rust-lang.org/tests/adding.html#ui. Maybe I'll have time to look into this tomorrow. |
Mark-Simulacrum
force-pushed
the
32bitcheck
branch
from
cf34fd3
to
8fc1a2b
Compare
|
Unfortunately I cannot reproduce these failures locally: I cannot use that arm target as I do not have the toolchain install, and |
RalfJung
reviewed
May 10, 2020
|
Huh. I would've expected the failures to be common -- they don't look arm specific. My impression is that at least the async failures are because the error pattern doesn't get emitted if we don't run the test (and the test compiled just fine, AFAICT) -- I guess I can ignore them for now on the arm target so we can land this? |
|
I mean we have errors like That just makes no sense... |
I don't think the test runner is so dumb -- after all, |
|
Huh. Well, CI is passing now (just restarted because I applied your suggestion). Maybe there was some spurious error there? I can't imagine what it might have been though. Anyway, we'll see what the timings are -- the master runtime is around 40 minutes on GHA and it looks like this took 45 minutes last time which is probably within the normal variation; maybe that means we're a few minutes slower. IMO though definitely acceptable so presuming CI passes again I'll squash and nominate for infra team approval. |
|
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 |
Only GHA is passing, probably because your change only affects Azure? These tests are failing: Maybe there is something about this target that breaks panics and a few other things? Isn't there a less obscure 32bit target we can use? |
|
Ah, no, we are failing on both. GHA is just... not reporting that? cc @pietroalbini -- this seems quite concerning that GHA is silently succeeding despite test failures while Azure correctly fails |
|
Hm, so maybe someone can explain the interaction between error-pattern and For a specific example, take src/test/ui/vec/vec-overrun.rs -- this should compile and panic when running. But since we've specified Skimming over the failures, all of them look specifically like "we tried to run the test and failed" -- but we shouldn't be running the tests at all, right? The issue you cite -- #65865 -- does look relevant. But I don't know how to interpret it: is it saying that run-fail tests currently always check the error pattern? That definitely won't work. |
|
|
I'll look into this more in-depth tomorrow, but at a quick glance it seems like that's a compiletest bug uncovered by GHA: for some reason there was a I/O error while executing tests, and when that happens compiletest prints the error and continues as if nothing happened. rust/src/tools/compiletest/src/main.rs Lines 346 to 352 in 9912925 |
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
May 11, 2020
…ror, r=pietroalbini Fail if I/O error occurs during testing First known case of this is in rust-lang#72053 (comment) but may have happened before then. r? @pietroalbini
Mark-Simulacrum
force-pushed
the
32bitcheck
branch
from
d242d56
to
7292481
Compare
Mark-Simulacrum
changed the title
|
I've removed the 32-bit side of things here; getting that working in PR CI isn't something I quite have time for right now -- it seems like we'll probably want to wait until we can easily cross-compile to i686-unknown-linux-gnu (so that run-fail tests just work) but we can't currently do that with LLVM 8 which is used by PR CI. @RalfJung -- do you think it's worthwhile to land this (partial) solution in the meantime? Locally a |
|
You've been the one to fix my Could you also leave a FIXME to use a 32bit target once LLVM is unbroken? |
|
Hm I do not recall any pass=check breakage that I fixed... I do feel that today's 2x time win is pretty nice but not really of the right order -- if a check run was 10-20 seconds then it would be snappy enough to make sense, but with a 70 second check run I don't know that it's that much better than a 150 second check run. I get distracted over that time anyway... But it's obviously useful to some people and keeping it working makes sense to me. |
|
Ah sorry it wasn't you it was @petrochenkov. |
joelpalmer
added
S-waiting-on-review
Elinvynia
added
S-waiting-on-review
Elinvynia
added
S-waiting-on-review
We do not test cross-compilation here as the PR builder lacks a sufficiently recent LLVM to cross-compile to 32-bit linux. Once we bump the minimum LLVM version to LLVM 9, this can use normal 32-bit linux.
Mark-Simulacrum
force-pushed
the
32bitcheck
branch
from
7292481
to
562e015
Compare
|
@bors r+ rollup=never Thanks! |
|
📌 Commit 562e015 has been approved by |
bors
added
S-waiting-on-bors
|
@bors p=1 |
|
⌛ Testing commit 562e015 with merge 9ba490af2e8e3a0294dfd1c1fb3966443017b622... |
|
@bors yield |
|
@bors retry yield retry |
|
☀️ Test successful - checks-actions, checks-azure |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I'm going to just compare the builder times since I wasn't able to get this working nicely locally (hit some obscure linker error).
Fixes part of #69823