Skip to content

Automatically skip tty tests if not on tty - #20013

Closed
iluuu1994 wants to merge 1 commit into
php:PHP-8.3from
iluuu1994:auto-skip-tty-tests
Closed

iluuu1994 wants to merge 1 commit into
php:PHP-8.3from
iluuu1994:auto-skip-tty-tests

Conversation

@iluuu1994

@iluuu1994 iluuu1994 commented Sep 30, 2025

Copy link
Copy Markdown
Member

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:

$ php run-tests.php tests/output 2> /dev/null

See GH-19975

@iluuu1994
iluuu1994 requested a review from Girgias September 30, 2025 14:36
@bwoebi

bwoebi commented Sep 30, 2025

Copy link
Copy Markdown
Member

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.

@iluuu1994

Copy link
Copy Markdown
Member Author

@bwoebi Well, any concrete suggestions? The tests do more than just check for istty(). I'm not sure what other solution we have.

@bwoebi

bwoebi commented Oct 1, 2025

Copy link
Copy Markdown
Member

@iluuu1994 Having the shall check for tty via [ -t 0 ] && [ -t 1 ] && [ -t 2 ]

@iluuu1994

Copy link
Copy Markdown
Member Author

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.

@bwoebi

bwoebi commented Oct 1, 2025

Copy link
Copy Markdown
Member

@iluuu1994 I don't mind doing it the proposed way, it was merely a question / suggestion.

@iluuu1994

Copy link
Copy Markdown
Member Author

Okay, thanks. I understand the concern, I had the same thought. I just couldn't think of a simpler solution that's also portable.

@hakre

hakre commented Oct 1, 2025

Copy link
Copy Markdown
Contributor

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

@bwoebi

bwoebi commented Oct 1, 2025

Copy link
Copy Markdown
Member

@hakre Such as? I.e. when would that ever happen on cli?

@iluuu1994

Copy link
Copy Markdown
Member Author

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

hakre commented Oct 1, 2025

Copy link
Copy Markdown
Contributor

@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
)

@bwoebi

bwoebi commented Oct 1, 2025

Copy link
Copy Markdown
Member

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.

@iluuu1994

Copy link
Copy Markdown
Member Author

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.

@hakre

hakre commented Oct 1, 2025

Copy link
Copy Markdown
Contributor

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).

@iluuu1994 iluuu1994 closed this in 8315977 Oct 2, 2025
@iluuu1994

Copy link
Copy Markdown
Member Author

Ok, I added defined() tests, they shouldn't hurt at least.

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.

4 participants