Welcome! Log In Create A New Profile

Advanced

[PHP-DEV] run-tests.php exit code

Posted by Stanislav Malyshev 
Stanislav Malyshev
[PHP-DEV] run-tests.php exit code
February 28, 2018 09:30PM
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?
2. Do we want RFC for it?
3. Can we put it in 7.1 or do we want to wait for 7.3?

Would be glad to hear everybody's thought on this.
--
Stas Malyshev
smalyshev@gmail.com

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Anatol Belski
RE: [PHP-DEV] run-tests.php exit code
March 01, 2018 11:20AM
Hi Stas,

> -----Original Message-----
> From: Stanislav Malyshev [mailto:[email protected]]
> Sent: Wednesday, February 28, 2018 9:26 PM
> To: PHP Internals <[email protected]>
> 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
Matt Ficken
Re: [PHP-DEV] run-tests.php exit code
March 01, 2018 11:30PM
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

On Thu, Mar 1, 2018 at 2:15 AM, Anatol Belski <[email protected]> wrote:

> Hi Stas,
>
> > -----Original Message-----
> > From: Stanislav Malyshev [mailto:[email protected]]
> > Sent: Wednesday, February 28, 2018 9:26 PM
> > To: PHP Internals <[email protected]>
> > 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
>
Stanislav Malyshev
Re: [PHP-DEV] run-tests.php exit code
March 02, 2018 12:30AM
Hi!

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

The point is to change behavior - namely, make run-tests return proper
exit code even if no variables are set. True, run-tests is not part of
PHP language as such, so one can claim BC guarantees do not extend to
it, but I'd like to hear if somebody knows any reason why this still
might be a problem.

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

Yes, the options are 7.1 or 7.3. I'm fine with both, though would like
7.1 better.
--
Stas Malyshev
smalyshev@gmail.com

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Sorry, only registered users may post in this forum.

Click here to login