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

Unignore tests in stage1 #25338

Merged
merged 2 commits into from
May 14, 2015
Merged

Unignore tests in stage1 #25338

merged 2 commits into from
May 14, 2015

Conversation

tamird
Copy link
Contributor

@tamird tamird commented May 12, 2015

We don't have any pending snapshot-requiring changes.

Works toward #3965.

@rust-highfive
Copy link
Collaborator

r? @alexcrichton

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

@alexcrichton
Copy link
Member

Just to confirm, have you run all these tests in stage1? Plugins fundamentally don't work in stage1, so many of these I believe are intentional.

@tamird
Copy link
Contributor Author

tamird commented May 12, 2015

I haven't, but I'll do that now!

@tamird
Copy link
Contributor Author

tamird commented May 12, 2015

Hm. make check-stage1 passed locally.

@alexcrichton
Copy link
Member

Ah right, it is possible for these tests to work at stage1 (if codegen is similar enough in stage0 and stage1), but it's not guaranteed. For example if codegen changes after stage0 then plugins will be broken.

@tamird
Copy link
Contributor Author

tamird commented May 13, 2015

So: wat do?

@tamird
Copy link
Contributor Author

tamird commented May 13, 2015

wat do

@alexcrichton
Copy link
Member

I think that this should back out the removal of // ignore-stage from all *-fulldeps tests, but all changes to src/test/{auxiliary,run-pass} look good to me!

@tamird
Copy link
Contributor Author

tamird commented May 13, 2015

I think it's probably OK to unignore compilefail-fulldeps; do you agree?

@alexcrichton
Copy link
Member

I think it kinda depends on the test. Anything which expects #![plugin] to succeed (e.g. have the dlopen be a success) must be ignored in stage1, but some of these tests are expecting a compiler error before we hit that point so they can be un-ignored yeah.

@tamird
Copy link
Contributor Author

tamird commented May 13, 2015

@alexcrichton is there a change I can make locally to induce the condition that would break these tests in stage1? feels a bit like a guessing game otherwise.

@alexcrichton
Copy link
Member

I think if you change the symbol hashing function it'll cause the symbols to be totally different at stage1 than they were at stage0, so you can try just adding symbol_hasher.input_str("foo") somewhere in there perhaps? I haven't exactly tested out this theory though, so it also may not work :(

We don't have any pending snapshot-requiring changes. Tests which
continue to be ignored are those that are broken by codegen changes.
@tamird
Copy link
Contributor Author

tamird commented May 14, 2015

Your suggestion worked! I tested these locally and they passed (with the hashing change) so this should be good to merge. Thanks for your help @alexcrichton.

@alexcrichton
Copy link
Member

@bors: r+ f548a05

Nice! Thanks!

@alexcrichton
Copy link
Member

@bors: rollup

@bors
Copy link
Contributor

bors commented May 14, 2015

⌛ Testing commit f548a05 with merge b1bd3a3...

bors added a commit that referenced this pull request May 14, 2015
We don't have any pending snapshot-requiring changes. Closes #20184.

Works toward #3965.
@bors bors merged commit f548a05 into rust-lang:master May 14, 2015
@tamird tamird deleted the unignore-stage-tests branch May 14, 2015 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants