run-tests.php exit code

If behavior wasn't changed unless ENV var or CLI option was specified, then
I think it can go into 7.1 (run-test.php isn't production code part of a
build, etc...).

Behavior remaining the same of course wouldn't break anybody's existing CI.

For those who benefit from this in 7.3, they should benefit for 7.1 and 7.2
which we have to provide the same level of support for a couple of years
still.

Also, ENV var could be TRUE to change behavior, or could be a string, in
which case the exit code is non-zero only if a failing test's name contains
that string. That would allow some CI scripts to only focus on certain
tests, extensions, etc.... That would be easy to add. That wouldn't help me
but I just thought of that and maybe it helps somebody else.

Regards
-M

Hi Stas,

-----Original Message-----
From: Stanislav Malyshev [mailto:smalyshev@gmail.com]
Sent: Wednesday, February 28, 2018 9:26 PM
To: PHP Internals internals@lists.php.net
Subject: [PHP-DEV] run-tests.php exit code

Hi!

When running tests with run-tests.php, if the tests fail, the script
will exit with
non-zero exit code, but only if REPORT_EXIT_STATUS is set. This was the
case
since 2002 when this capability has been introduced.

I think it would be nice if we reversed the default and made the script
use exit
code by default, unless NO_EXIT_STATUS is set. All usages of this script
that I
know (including our own CI suite) use this setting, and I don't think
there's a
good case for not using it. I think it makes sense to default to the
common use
case. So:

1. Does it make sense to change the default?
   I've stumbled upon this a couple of times, too, especially when writing
   new scripts for non core test automation. IMO, given today there's much
   more automation than ever, defaulting to the exit code translation makes
   sense. Test fails should not be subtle by default at any levels.

2. Do we want RFC for it?
   IMHO not really deserving an RFC.

3. Can we put it in 7.1 or do we want to wait for 7.3?

If someone doesn't use it on CI already, that's probably just a blessed
ignorance as the tests runs would always pass. Depends if we want the
change to be going more soft for some non core CI tests and avoid confusion
on manual test runs as in some cases an additional output could be
produced. Given it's in many cases harder to investigate failures on CI,
perhaps 7.3 would be safe to put it with the corresponding documentation,
etc.

Regarding the option itself, we'd probably not need to introduce a new
one. Instead, a piece like below could be put at the top

If (getenv("REPORT_EXIT_STATUS") === false) putenv("REPORT_EXIT_STATUS=1")
;

Regards

Anatol