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

Fix spawn tests in different environments #1667

Merged
merged 1 commit into from
Aug 7, 2024
Merged

Fix spawn tests in different environments #1667

merged 1 commit into from
Aug 7, 2024

Conversation

lucc
Copy link
Collaborator

@lucc lucc commented Jul 31, 2024

We are running our tests in different environments, sometimes with a terminal as stdin and sometimes with /dev/null as stdin. Both are seen as "not a pipe" by test -p so we use that to test if our python code redirects text into an external process.

This fixes #1580.

This pull request is based on my comment #1580 (comment)

@lucc
Copy link
Collaborator Author

lucc commented Jul 31, 2024

@dcbaker are you available to review this?

We are running our tests in different environments, sometimes with a
terminal as stdin and sometimes with /dev/null as stdin.  Both are seen
as "not a pipe" by `test -p` so we use that to test if our python code
redirects text into an external process.
@lucc
Copy link
Collaborator Author

lucc commented Aug 5, 2024

It seems that /dev/stdin is not portable (https://unix.stackexchange.com/questions/36403/how-portable-are-dev-stdin-dev-stdout-and-dev-stderr). @pazz is that ok for us?

@pazz
Copy link
Owner

pazz commented Aug 6, 2024

It seems that /dev/stdin is not portable (https://unix.stackexchange.com/questions/36403/how-portable-are-dev-stdin-dev-stdout-and-dev-stderr). @pazz is that ok for us?

I think we can assume that the intersection of alot and SunOS or similar is empty. Let's not make an issue of it and just use /dev/stderr and friends and reconsider should anyone ever run into issue with it?

@lucc
Copy link
Collaborator Author

lucc commented Aug 6, 2024

@pazz agreed, can you do a review if @dcbaker is not available then we can merge it.

Copy link
Owner

@pazz pazz left a comment

Choose a reason for hiding this comment

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

Looks OK to me.
I am not 100% sure if the intended semantics remains the same for the test case but the change looks safe and if it is the only blocker for us to run tests in the CI than I favour pushing this.

@lucc lucc merged commit 2831d1f into master Aug 7, 2024
9 checks passed
@lucc lucc deleted the issue-1580 branch August 7, 2024 11:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test_no_spawn_no_stdin_attached tests the build environment
2 participants