Skip to content
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

Ignore gated-plugin test on stage1 #75404

Closed
wants to merge 1 commit into from

Conversation

mati865
Copy link
Contributor

@mati865 mati865 commented Aug 11, 2020

Just like other #![plugin(...)] tests it requires stage2 to work correctly.

Just like other `#![plugin(...)]` tests it requires stage2 to work correctly.
@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 11, 2020
@petrochenkov
Copy link
Contributor

petrochenkov commented Aug 11, 2020

What is the point of running ui-fulldeps tests on stage 1?
All of them are supposed to not pass. (The passing ones, if any, should be moved ui.)

@mati865
Copy link
Contributor Author

mati865 commented Aug 11, 2020

I think since #73964 running ./x.py test will use stage1 for ui-fulldeps and this is the only error I have noticed.
cc @jyn514

@petrochenkov
Copy link
Contributor

Interesting. I never tried to run full x.py test on stage 1.

this is the only error I have noticed

I also noticed that x.py test src/tools/clippy fails (despite building the compiler twice) and needs specifying --stage 2 explicitly, but it's not run by x.py test by default.

@jyn514
Copy link
Member

jyn514 commented Aug 11, 2020

IIUC this was always broken with --stage 1 but nobody noticed because it wasn't the default? This fix seems fine to me then.

@petrochenkov
Copy link
Contributor

This is not fine because x.py test <component> is supposed to test the component.
This was true previously, but not true now.
The whole default stage change caused more confusion than benefit, IMO.

@mati865
Copy link
Contributor Author

mati865 commented Aug 12, 2020

Like half of the ui-fulldeps tests is ignored in stage1 so the issue is much more than this single test.

@jyn514
Copy link
Member

jyn514 commented Aug 12, 2020

This is not fine because x.py test <component> is supposed to test the component.
This was true previously, but not true now.
The whole default stage change caused more confusion than benefit, IMO.

I wish you'd raised this concern during the MCP :/

AFAIK x.py test ui-fulldeps will show that the tests are being ignored, which might push people towards --stage 2. Is that clear enough or do you think it would be helpful to print a warning from x.py as well? Something like 'x.py test --stage 1 ui-fulldeps inherently cannot work; try using --stage 2 instead'.

@Mark-Simulacrum
Copy link
Member

If we truly need stage 2, then it might make sense to just error in stage 1. I don't know that it's a hard requirement that test suites pass in stage 1. We could annotate them with "needs-stage2" or so such that you can get a nicer error message (e.g., in stage 1 we just list failures in those tests rather than actually trying to run them).

@petrochenkov
Copy link
Contributor

Yeah, if stage 1 stays a default, then we should probably

  • error on x.py test <suite>/x.py test --stage 1 <suite> if the test suite requiring stage 2 is specified explicitly,
  • ignore it with a message in "glob" runs like x.py test without arguments.

@petrochenkov
Copy link
Contributor

(Also, I'm pretty sure at least some tests are in *-fulldeps directories accidentally and can be moved to stage 1 test suites.)

@mati865
Copy link
Contributor Author

mati865 commented Aug 12, 2020

@petrochenkov and remove ignore-stage1 from already existing ui-fulldeps tests.

I'm going to close this PR since it's not going to be accepted.

@mati865
Copy link
Contributor Author

mati865 commented Aug 25, 2020

Opened #75905

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants