-
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
Move compile-fail
tests to ui
#44844
Comments
Has the relative performance of compile-fail vs. UI tests been evaluated? I worry that we'd be hurting our merge times if there's too much of a regression. I doubt it's anything serious, though, so no need to block this. |
Why are we doing this when compile-fail are easier to use, more readable and maintainable? |
I don't think anyone has (including me), but my gut feeling is that the difference isn't big enough to make a dent. I'll try to keep track of it though, as well as investigate any performance enhancements we might be able to make to the test suite itself.
I could debate all three points:
Beyond those three points, we want to add support for the current inline tests to There is another underlying reason, witch I haven't stated so far: I want to be able to audit the compiler output in it's totality in order to find common places for improvement, to write down unifying guidelines and applying them to any inconsistent output. Having only |
I tried to convert everything once and encountered two issues to solve
Most of tests still can be converted though. I think it's critical to implement and enable checking of |
It seems like a really strange idea... |
I'm counting on that.
But it should be easier for reviewers to catch them when they occur, IMO.
How's so? |
The goal of the compile-fail tests is different than the ui tests'. The first one is to ensure the expected error(s) come(s) out, even if the output changes whereas the second is to watch over the compiler output. That's why it seems strange to me that we're trying to merge the both of them. |
It's very difficult to write new compile-fail style tests testing more than one error without support of |
Which seems very complicated in case there are multiple lines for one error (help, notes, suggestions...). |
Do we have a way to regenerate these tests on Windows? Also we need to document how to regenerate the tests somewhere discoverable, perhaps in the output of tests themselves? I'd also prefer just the diff of output be printed. These tests seem quite noisy. |
cc @pnkfelix @nikomatsakis (I precise again I'm against this change.) |
I think the compiler team should discuss: 1. whether we want to go in this direction, and 2. if we do want to go in this direction, what prerequisites need to be met (e.g., consider @petrochenkov 's comment that we should implement and enable checking of |
My opinion:
(I wish we had a lighterweight way to run the test runner, incidentally, without e.g. invoking it from |
@nikomatsakis Where is the test runner for Edit: Found the ui test runner and the compile-fail test runner. I'm going to do my best to understand what they're doing. |
Alright, I've upgraded the test runner for
I'm going through and fixing each one right now. This might take some time. |
Also, there doesn't appear to be any way to check "did you mean" stuff using commands... it's categorized by the json as kind: None, but there's no command //~ NONE. I'm just leaving them in as comments; they're still checked by stderr formatting diffs. |
There's 78 ui tests to fix, and a few features and special cases need to be added to my ui test runner code. Here's my work-in-progress stuff: https://github.com/Phlosioneer/rust Should I set up a pull request and just let it sit there while I work on this, for feedback? Or should I wait until I'm actually ready to submit the code? |
You are awesome. Thank you.
I had expected us not to parse json anymore but instead to go back to scraping from the output (which we used to do before we parsed JSON). Are you compiling everything twice? (I guess I can check your branch.) |
Opening up a |
@rfcbot fcp merge I'd like to propose transition compile-fail (and eventually run-pass) to UI tests, subject to the conditions that I described here. In particular, subject to the conditions that I've made my "positive" case before, in [this internals thread]. It is very similar to what @estebank had to say. @rust-lang/compiler, feel free to tell me why this is such a terrible idea. =) |
Move some `compile-fail` tests to `ui` Re: #44844.
I would like to re-raise this issue in importance. We have great tooling for comparing the AST and MIR borrow checkers, but it is ALL based on |
As such, I am marking this as an NLL Milestone issue. |
I'm currently taking a stab at this, will self-assign. |
@davidtwco I'd advice doing it piece meal, not more than a couple hundred at a time, for your sanity's sake. You'll find that there are some tests with platform dependent output, which will require using some of the more esoteric features of ui tests (literal replacement), but there're tons of low hanging fruit that only needs to be moved with no changes. Also, the more files you involve the harder it'll be for you to merge it back as things change in master. |
Move `compile-fail` tests to `ui` Fixes #46841, #52531, #44844. r? @nikomatsakis
Move `compile-fail` tests to `ui` Fixes #46841, #52531, #44844. r? @nikomatsakis
Move `compile-fail` tests to `ui` Fixes #46841, #52531, #44844. r? @nikomatsakis
This is completed as of #53196. As there are still some tests in |
There are some cases where
compile-fail
errors are still needed, asui
errors require the output to be exact and consistent across executions and platforms. That being said, the vast majority of errors can be safely moved.The process is straightforward:
git mv src/test/compile-fail/$FILE.rs src/test/ui/$FEATURE-DIR/$FILE.rs
./x.py test --stage 1 src/test/ui
./src/test/ui/update-all-references ./build/x86_64-apple-darwin/test/ui
(replacex86_64-apple-darwin
with whatever environment you're working on)git add src/test/ui/$FEATURE-DIR/$FILE.stderr
git commit -m "Moving $FILE test to ui"
You don't need to move only one file at a time, but avoid trying to do all at once, as that will probably cause too many conflicts with PRs being merged precluding these changes from landing.
The following is a list of all files and directories (without checking wether they can be moved at this time, I will audit this list at a later time):
Click to expand
The text was updated successfully, but these errors were encountered: