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

Migrate test suite to use run-pass stdout/stderr testing #63924

Closed
Mark-Simulacrum opened this issue Aug 26, 2019 · 13 comments
Closed

Migrate test suite to use run-pass stdout/stderr testing #63924

Mark-Simulacrum opened this issue Aug 26, 2019 · 13 comments
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion.

Comments

@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Aug 26, 2019

Where possible, we should prefer to use the functionality added by #63825 instead of std::process and the like. At least one such test is known, but it's possible more can be found.

@Mark-Simulacrum Mark-Simulacrum added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. labels Aug 26, 2019
@jonas-schievink jonas-schievink added A-testsuite Area: The testsuite used to check the correctness of rustc C-cleanup Category: PRs that clean code up or issues documenting cleanup. labels Aug 26, 2019
@Mark-Simulacrum
Copy link
Member Author

I'll try to get mentoring instructions up for the actual move but the first step here is to look for all run-pass tests (// run-pass in src/test/ui I believe) and verify we're ignoring cloudabi, emscripten, and sgx in all of them. This is pretty annoying to do manually I suspect but I'd recommend comparing the results of rg -o ignore-\w+ src/test/ui and rg -c run-pass src/test/ui (exact commands may need to differ)

@RalfJung
Copy link
Member

verify we're ignoring cloudabi, emscripten, and sgx in all of them

It does not at all seem like we do:

$ rg run-pass src/test/ui | wc -l
3200
$ rg ignore-cloudabi src/test/ui | wc -l
82

@RalfJung
Copy link
Member

RalfJung commented Aug 27, 2019

I think that file changed in #63825 was ignored just because it uses process -- but just running the test seems to work fine on those platforms. I suppose CI has some way to cross-run them and compiletest uses that?

It does not use process any more so probably it can be unignored.

@Mark-Simulacrum
Copy link
Member Author

Ah, so the comment is just misleading in that this is not "no processes" but rather no "std::process"?

@Mark-Simulacrum Mark-Simulacrum removed E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. labels Aug 27, 2019
@RalfJung
Copy link
Member

That's how I initially read that comment, and I still do, yes.

@Mark-Simulacrum
Copy link
Member Author

Aha, okay, then probably just misread on my part. Closing since we probably don't actually want to do anything here.

@RalfJung
Copy link
Member

Well I think we want to remove those ignore from the test that got changed in #63825.

@Mark-Simulacrum Mark-Simulacrum added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. labels Aug 27, 2019
@Mark-Simulacrum Mark-Simulacrum changed the title Move ignores for run-pass test on platforms to compiletest without process support Remove no longer needed ignores from tests Aug 27, 2019
@Mark-Simulacrum Mark-Simulacrum changed the title Remove no longer needed ignores from tests Migrate test suite to use run-pass stdout/stderr testing Aug 27, 2019
@Mark-Simulacrum
Copy link
Member Author

I guess so. I also don't care too much, it seems fine to ignore on those platforms too. But reopening.

@RalfJung
Copy link
Member

Well, that PR hasn't landed yet, so you could just r- and ask the author to remove the ignore?

I can't even open a PR to fix this until that PR landed.^^

@nathanwhit
Copy link
Member

(Author of #63825) Since the PR is still open, and I went ahead and re-removed the ignores and pushed again, so that should be resolved.

@RalfJung
Copy link
Member

Thanks a lot!

@RalfJung
Copy link
Member

@Mark-Simulacrum I only just saw you changed the bug title... I guess I should reopen as you repurposed the issue? Or would it make more sense to open a new one, as the existing discussion here will be rather confusing?

@Mark-Simulacrum
Copy link
Member Author

New issue is fine, I don't care too much realistically :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion.
Projects
None yet
Development

No branches or pull requests

4 participants