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

tests: alter integration tests for stdio to not use a handrolled cat implementation #4803

Merged
merged 2 commits into from
Jul 2, 2022

Conversation

Noah-Kennedy
Copy link
Contributor

This fixes #4801, where, as a result of rust-lang/rust#95469, our implementation of cat used for this test no longer works, as stdio functions on windows now can abort the process if the pipe is set to nonblocking mode.

Unfortunately in windows, setting one end of the pipe to be nonblocking makes the whole thing nonblocking, so when, in tokio::process we set the child pipes to nonblocking mode, it causes serious problems for any rust program at the other end.

Fixing this issue is for another day, but fixing the tests is for today.

…implementation

This fixes #4801, where, as a result of rust-lang/rust#95469, our implementation of cat used for this test no longer works, as stdio functions on windows now can abort the process if the pipe is set to nonblocking mode.

Unfortunately in windows, setting one end of the pipe to be nonblocking makes the whole thing nonblocking, so when, in tokio::process we set the child pipes to nonblocking mode, it causes serious problems for any rust program at the other end.

Fixing this issue is for another day, but fixing the tests is for today.
@Noah-Kennedy Noah-Kennedy changed the title tests: alter integration tests for stdio to not use a handrolled cat … tests: alter integration tests for stdio to not use a handrolled cat implementation Jul 2, 2022
@Noah-Kennedy Noah-Kennedy enabled auto-merge (squash) July 2, 2022 01:54
@Noah-Kennedy Noah-Kennedy merged commit d8cad13 into master Jul 2, 2022
@Noah-Kennedy Noah-Kennedy deleted the noah/ci-test-fail branch July 2, 2022 03:32
Copy link
Member

@ipetkov ipetkov left a comment

Choose a reason for hiding this comment

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

Overall LGTM! Though I do wonder, should we keep our cat utility blocking on Unix systems?

It would be good to maintain it as a regression test for Unix behavior (even though the stdlib maintains this for us we can find out if it breaks)

@Noah-Kennedy
Copy link
Contributor Author

@ipetkov It might not be a bad idea to split the pipes test into two tests, one with sync, one with async.

ipetkov added a commit that referenced this pull request Jul 23, 2022
ipetkov added a commit that referenced this pull request Jul 23, 2022
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.

process: Windows tests failing on 1.62
3 participants