Skip to content

[Process] Dont test TTY if there is no TTY support #38950

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

Merged
merged 1 commit into from
Nov 2, 2020

Conversation

Nyholm
Copy link
Member

@Nyholm Nyholm commented Nov 1, 2020

Q A
Branch? 4.4
Bug fix? no
New feature? no
Deprecations? no
Tickets Fix #38946
License MIT
Doc PR

@derrabus
Copy link
Member

derrabus commented Nov 1, 2020

Well, this would make our tests pass. But my understanding is that the TTY detection that you use to skip the test is part of what we want to test here. Let's say we would break that detection logic. We would never know because the corresponding tests would flip from green to skipped. 😕

@Nyholm
Copy link
Member Author

Nyholm commented Nov 1, 2020

Is it testing Process::isTtySupported()?

Isn't it testing that the Process is using UnixPipes and handle the exit code correctly?

@Nyholm
Copy link
Member Author

Nyholm commented Nov 2, 2020

PR updated. Thank you

@derrabus
Copy link
Member

derrabus commented Nov 2, 2020

@Nyholm If I change Process::isTtySupported() to always return false, those two tests are the only ones failing.

@Nyholm
Copy link
Member Author

Nyholm commented Nov 2, 2020

If Process::isTtySupported() always returns false, then these two tests will be skipped.

If Process::isTtySupported() always returns true, yes, these will fail.

What alternative strategy do you have to solve #38946?

@derrabus
Copy link
Member

derrabus commented Nov 2, 2020

What alternative strategy do you have to solve #38946?

None. 😕

@nicolas-grekas
Copy link
Member

Thank you @Nyholm.

@nicolas-grekas nicolas-grekas merged commit ed217a1 into symfony:4.4 Nov 2, 2020
@Nyholm Nyholm deleted the tty branch November 3, 2020 16:14
@fabpot fabpot mentioned this pull request Nov 10, 2020
This was referenced Nov 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants