-
Notifications
You must be signed in to change notification settings - Fork 8k
Automatically skip tty tests if not on tty #20013
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
Conversation
Is there a better way to determine tty? The tty tests obviously also test the tty functions, of which you use one. If it were broken (e.g. return false if it shouldn't), well, the tests to indicate that would just not run either.
@bwoebi Well, any concrete suggestions? The tests do more than just check for istty()
. I'm not sure what other solution we have.
@iluuu1994 Having the shall check for tty via [ -t 0 ] && [ -t 1 ] && [ -t 2 ]
Well, it would need to be portable and I don't think the risk of hiding true failures is big enough to invest too much time here, at least on my part.
@iluuu1994 I don't mind doing it the proposed way, it was merely a question / suggestion.
Okay, thanks. I understand the concern, I had the same thought. I just couldn't think of a simpler solution that's also portable.
Under circumstances, STDIN, STDOUT or STDERR may not be available as constants in PHP cli SAPI.
For the check to become more robust, it should check if the constants are actually define()-ed.
This might be useful if you are concerned about the stability, so just FYI @iluuu1994
@hakre Such as? I.e. when would that ever happen on cli?
I quickly asked Niels since he's more knowledgeable on streams, and this can only happen with preloading or explicitly closing stdin, out or err, which seems rather intentional. E.g. in bash you can do php test.php 2>&-
. But this is not a realistic scenario we need to guard against.
@hakre Such as? I.e. when would that ever happen on cli?
When for example the PHP script is read from standard input. This is not the normal invocation of the run-tests.php script but a prominent example over PHP versions.
$ < run-tests.php | php -- -j2 ... PHP 5. stream_socket_accept($socket = resource(20621) of type (stream), $timeout = 5) Standard input code:1417 ERROR: Failed to accept connection from worker.
This example is construed, using the workes (-j2) provokes the FAILURE here.
So this is merely an FYI, we can always argue with GIGO but how I read the check as care-taking it might still be useful as the behaviour changed often in the past. The example on passing the script via standard input is stable because PHP can't offer the standard input to the script as STDIN as it has to consume it itself to read the script - this would just not make sense.
These three constants are comewhat special as they are added to the end of the Core and the overall constant table:
$ sapi/cli/php -r 'print_r(array_slice(get_defined_constants(true)["Core"], -3));' Array ( [STDIN] => Resource id #1 [STDOUT] => Resource id #2 [STDERR] => Resource id #3 ) $ sapi/cli/php -r 'print_r(array_slice(get_defined_constants(), -3));' Array ( [STDIN] => Resource id #1 [STDOUT] => Resource id #2 [STDERR] => Resource id #3 )
explicitly closing stdin, out or err
@iluuu1994 The primary STDIN/STDERR/STDOUT constants will look at the stdin/stdout/stderr C symbols, which are always defined. Only explicitly opening php://stderr (etc.) at runtime may actually fail.
The constants are always defined, but they may be null, which will crash run-tests.php. But that seems acceptable, as it seems quite intentional.
The constants are always defined, but they may be null, which will crash run-tests.php. But that seems acceptable, as it seems quite intentional.
It's even more esoteric in the details from time to time. I've seen tests failing when invoking CLI where (one of) the constants actually were not defined (and therefore fatal error on using them), but as you wrote it's a consideration thing and we don't want to consider it here. All fine. (and yes normally the constants are always defined even if the descriptor is closed, as @bwoebi wrote).
Ok, I added defined()
tests, they shouldn't hurt at least.
Uh oh!
There was an error while loading. Please reload this page.
Without this, one can trivially run into test failures by not connecting all tty pipes to the run-tests.php process, or by running it inside a bash script. E.g. this will cause test failures:
See GH-19975