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.

Sign up for GitHub

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

Jump to bottom

Move some tests in src/test/compile-fail -> src/test/ui #62177

Closed
wants to merge 2 commits into from
Closed

Move some tests in src/test/compile-fail -> src/test/ui #62177

wants to merge 2 commits into from

Conversation

Centril
Copy link
Contributor

@Centril Centril commented Jun 27, 2019

This is a subset of #62113.

Hopefully everything here sticks, but if it doesn't I'll remove tests until it does.

r? @petrochenkov

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 27, 2019
@Centril Centril mentioned this pull request Jun 27, 2019
Closed
@Centril
Copy link
Contributor Author

Centril commented Jun 27, 2019

Yay! Initial tests seem to pass at least.

@petrochenkov
Copy link
Contributor

Some of these are going to fail on wasm, IIRC.
If you are on Linux, could you run the docker image for the wasm target and see whether it fails?

@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 Jun 27, 2019
@petrochenkov
Copy link
Contributor

r=me after the wasm check

@Centril
Copy link
Contributor Author

Centril commented Jun 27, 2019

If you are on Linux, could you run the docker image for the wasm target and see whether it fails?

@petrochenkov Will do. Should I ignore tests for the wasm target if they fail?

@petrochenkov
Copy link
Contributor

Splitting into ignore-wasm and only-wasm would be better.

@petrochenkov
Copy link
Contributor

(Since they "fail" by producing platform-dependent output, rather than by really failing.)

@Centril Centril force-pushed the move-some-compile-fail-tests branch from 477fb6c to 6d4c642 Compare June 29, 2019 17:26
@Centril
Copy link
Contributor Author

Centril commented Jun 29, 2019

Tested wasm with ./src/ci/docker/run.sh test-various and made some fixes. Hopefully this will stick now but I won't say it's a sure thing...

@bors r=petrochenkov

@bors
Copy link
Contributor

bors commented Jun 29, 2019

📌 Commit 6d4c642 has been approved by petrochenkov

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 29, 2019
RalfJung
RalfJung reviewed Jun 29, 2019
View reviewed changes
src/test/ui/imports/empty-extern-arg-wasm.stderr

error[E0463]: can't find crate for `std`
|
= note: the `wasm32-unknown-unknown` target may not be installed
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could also use normalization to get rid of these two lines. Then you don't need to duplicate tests.

Copy link
Member

@RalfJung RalfJung Jun 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have e.g. used the following to normalize away entire lines in other tests:

// normalize-stderr-test "thread.*panicked.*Metadata module not compiled.*\n" -> ""
// normalize-stderr-test "note:.*RUST_BACKTRACE=1.*\n" -> ""

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, this is great news.
For some reason I thought the normalization works per-line.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much simpler to fix platform-dependent tests this way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the matching of the normalization pattern is per-line -- but if you include the final \n, you can remove the entire line. And then that's handled correctly when comparing later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll have a look at this tomorrow; in the meantime feel free to r- or we can do this in a follow up PR if that's alright with y'all.

Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 2, 2019
@Manishearth
Rollup merge of rust-lang#62177 - Centril:move-some-compile-fail-test…
42d0eb5 
…s, r=petrochenkov

Move some tests in src/test/compile-fail -> src/test/ui

This is a subset of rust-lang#62113.

Hopefully everything here sticks, but if it doesn't I'll remove tests until it does.

r? @petrochenkov
@Manishearth Manishearth mentioned this pull request Jul 2, 2019
Closed
@Centril
Copy link
Contributor Author

Centril commented Jul 2, 2019

@bors r-

Failed in #62294 (comment).

Will try the normalization strategy instead.

@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 Jul 2, 2019
@bors
Copy link
Contributor

bors commented Jul 4, 2019

☔ The latest upstream changes (presumably #62355) made this pull request unmergeable. Please resolve the merge conflicts.

@fmckeogh
Copy link
Member

@Centril Ping from triage regarding the merge conflicts! :)

@JohnCSimon
Copy link
Member

@Centril @petrochenkov Ping from triage, this is so close! :)

@totsteps
Copy link
Member

Third ping from triage, @Centril any updates on this?

Note: Thanks for the PR. This will be closed and marked as S-inactive-closed next week. Feel free to re-open when you have time.

Thanks

@Centril
Copy link
Contributor Author

Centril commented Aug 14, 2019

Closing for now until I have time to get back to it :)

@Centril Centril closed this Aug 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petrochenkov petrochenkov petrochenkov left review comments

@RalfJung RalfJung RalfJung left review comments

Assignees

@petrochenkov petrochenkov

Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@Centril @petrochenkov @bors @fmckeogh @JohnCSimon @totsteps @RalfJung @rust-highfive