Skip to content

Remove unneeded error check in ui tests #48562

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed

Conversation

GuillaumeGomez
Copy link
Member

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 26, 2018
@GuillaumeGomez
Copy link
Member Author

And it also solves the double compilation issue.

@Mark-Simulacrum
Copy link
Member

I don't feel qualified to understand the check we're removing and why it's not necessary.

r? @petrochenkov

@estebank
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented Feb 27, 2018

📌 Commit 3aca663 has been approved by estebank

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 27, 2018
@petrochenkov
Copy link
Contributor

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 27, 2018
@petrochenkov
Copy link
Contributor

You can't remove this, UI tests should check ERROR/WARN/etc annotations.
See #46116 and linked issues for the background.

@GuillaumeGomez
Copy link
Member Author

Even with the background, I don't understand its purpose. For this we typically had compile-fail tests. What's the point to check the compiler errors if you compare outputs? If an error/warning/help/whatever isn't thrown out by the compiler, the test will fail.

The other solution which I quickly started would be to actually parse the compiler stderr. And I can assure you that this is not gonna be fun or efficient...

@estebank
Copy link
Contributor

I thought that ui tests were using the rendered field in the json output...

@petrochenkov petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 27, 2018
@petrochenkov
Copy link
Contributor

petrochenkov commented Feb 27, 2018

@GuillaumeGomez

Even with the background, I don't understand its purpose.

TLDR: UI testing alone is very unreliable.
Suppose you have a test with N lines that should report an error and M lines that should not report an error. How do you ensure this by having only the auto-generated .stderr file? "Manually" looking through two files trying to find corresponding lines is not reliable, it's very easy to miss something, it's like separating asserts from run-pass tests into separate files. ERROR/WARN annotations must be inline and also not auto-generated.

For this we typically had compile-fail tests.

The plan so far is to gradually migrate them to UI to consistently detect changes in secondary stuff like spans in addition to ERROR annotations (#44844).

@estebank

I thought that ui tests were using the rendered field in the json output...

This was the case until #48337 got merged.

@petrochenkov
Copy link
Contributor

petrochenkov commented Feb 27, 2018

@GuillaumeGomez
I see two ways to resolve #48550:

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 27, 2018
@GuillaumeGomez
Copy link
Member Author

Then I'll just go for the --error-format=human output checking. I had someone working in most cases so I think it's doable.

@GuillaumeGomez GuillaumeGomez mentioned this pull request Feb 28, 2018
@petrochenkov
Copy link
Contributor

Closing in favor of #48684

@GuillaumeGomez GuillaumeGomez deleted the remove-useless-check branch March 6, 2018 09:07
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants