Proposal to run all tests with and without strict_types enabled

6 years ago by Pedro Lacerda — view source

unread

Hi PHP developers,

I'm new to the PHP development world and enjoying to learn the internals of
this popular language. I'm also very interested in testing and typing.

Most if not all of the few tests in ext/ with strict_types=1 are related to
bugs. Function argument parsing is done explicitly (eg with
Z_PARAM_OPTIONAL or Z_PARAM_LONG) and this is prone to errors.

sodium/tests/sodium_error_001.phpt
date/tests/timezones-list-strict.phpt
mysqli/tests/bug74547.phpt
zlib/tests/zlib_wrapper_level.phpt
gettext/tests/bug73730.phpt

Currently all other tests are done with strict_types=0, so coverage can be
increased simply by enabling strict_types. On a pull request discussion,
Nikic said that enable it would fail for many tests of invalid inputs so
the signal/noise ratio would be too low.

Than I modified run-tests.php to accept a -t flag that inserts
"declare(strict_types=1);" in the --FILE-- section of .phpt tests. Really
happened what he said, tests for invalid input failed.

So a new function check_strict_types() was needed to expose
ZEND_ARG_USES_STRICT_TYPES() enabling .phpt tests to check for it in the
--SKIPIF-- section if needed.

I suppose that a function check_strict_types() didn't existed before
because had no valid use cases and maybe also avoid the user to trick the
interpreter. But seems really useful to run all possible tests with
strict_types=1.

You can start checking the implementation correctness with:

$ ./run-tests.php -t ext/date/tests/timezone_transitions_get_variation3.phpt

And the implementation draft is at this repository:

https://github.com/php/php-src/compare/master...pslacerda:experimental/strict_testing?diff=split

My proposal is to run all tests with and without strict_types, skipping if
necessary, and increasing the code coverage. Depending of you overral
reception I'll create an RFC for it.

PHP for president!

--
Atenciosamente,
Pedro Lacerda

6 years ago by Christoph M. Becker — view source

unread

So a new function check_strict_types() was needed to expose
ZEND_ARG_USES_STRICT_TYPES() enabling .phpt tests to check for it in the
--SKIPIF-- section if needed.

Umm, I wonder whether a magic constant (say, __STRICT_TYPES__) would
be more appropriate.

And the implementation draft is at this repository:

https://github.com/php/php-src/compare/master...pslacerda:experimental/strict_testing?diff=split

Please have a look at the die()s – "skip" is omitted, but the rest of
the message is printed. Something like

die('skip strict_types is not enabled');

might be more appropriate.

--
Christoph M. Becker

6 years ago by Pedro Lacerda — view source

unread

I'll check magic constants and give you a response.

The skip message is up to the test developer, however at first I'll
probably need to change all relevant tests and your message seems
appropriated to put.

2018-02-11 20:34 GMT-03:00 Christoph M. Becker cmbecker69@gmx.de:

So a new function check_strict_types() was needed to expose
ZEND_ARG_USES_STRICT_TYPES() enabling .phpt tests to check for it in the
--SKIPIF-- section if needed.

Umm, I wonder whether a magic constant (say, __STRICT_TYPES__) would
be more appropriate.

And the implementation draft is at this repository:

https://github.com/php/php-src/compare/master...
pslacerda:experimental/strict_testing?diff=split

Please have a look at the die()s – "skip" is omitted, but the rest of
the message is printed. Something like

die('skip strict_types is not enabled');

might be more appropriate.

--
Christoph M. Becker

--
Atenciosamente,
Pedro Lacerda

6 years ago by Pedro Lacerda — view source

unread

I agree with __STRICT_TYPES__, give me some time so I can try reimplement
check_strict_types() as a magic constant.

Maybe the idiom if (expr) die("reason"); is a bit verbose or cluttered
for a single expression SKIPIF section. A die_if(expr, reason) function
could be available in that context, or even skip_if(expr, reason) because
makes more sense there, albeit is a bit repetitive SKIPIF skip_if.

2018-02-11 20:42 GMT-03:00 Pedro Lacerda pslacerda@gmail.com:

I'll check magic constants and give you a response.

The skip message is up to the test developer, however at first I'll
probably need to change all relevant tests and your message seems
appropriated to put.

2018-02-11 20:34 GMT-03:00 Christoph M. Becker cmbecker69@gmx.de:

So a new function check_strict_types() was needed to expose
ZEND_ARG_USES_STRICT_TYPES() enabling .phpt tests to check for it in the
--SKIPIF-- section if needed.

Umm, I wonder whether a magic constant (say, __STRICT_TYPES__) would
be more appropriate.

And the implementation draft is at this repository:

https://github.com/php/php-src/compare/master...pslacerda:
experimental/strict_testing?diff=split

Please have a look at the die()s – "skip" is omitted, but the rest of
the message is printed. Something like

die('skip strict_types is not enabled');

might be more appropriate.

--
Christoph M. Becker

--
Atenciosamente,
Pedro Lacerda

--
Atenciosamente,
Pedro Lacerda

6 years ago by Stanislav Malyshev — view source

unread

Hi!

My proposal is to run all tests with and without strict_types, skipping if
necessary, and increasing the code coverage. Depending of you overral
reception I'll create an RFC for it.

I am not sure what would be the advantage of this. Beyond testing
strict_types functionality itself (which of course should have its own
unit tests), the tests that test standard functioning of any function
would either supply correct arguments and then strict_types would be
irrelevant (provided it works as supposed to, which is tested by its own
tests) or provide incorrect arguments, and then strict_type tests should
be different from regular ones since the errors would be different, so
we'd have to write separate tests.
The only class of errors that could be found this way would be if we
somehow made such a mistake in defining the arguments of certain
function that strict_type version of it doesn't work but regular version
works fine. Which I guess is possible but does it worth the effort to
convert all tests? Not sure.

Stas Malyshev
smalyshev@gmail.com

6 years ago by Pedro Lacerda — view source

unread

2018-02-11 20:34 GMT-03:00 Christoph M. Becker cmbecker69@gmx.de:

Umm, I wonder whether a magic constant (say, __STRICT_TYPES__) would
be more appropriate.

Implement __STRICT_TYPES__ was a breeze, very hackable codebase.

A magic constant indeed sounds more appropriate.

Hi Stanislav,

2018-02-12 2:18 GMT-03:00 Stanislav Malyshev smalyshev@gmail.com:

I am not sure what would be the advantage of this. Beyond testing
strict_types functionality itself (which of course should have its own
unit tests), the tests that test standard functioning of any function
would either supply correct arguments and then strict_types would be
irrelevant (provided it works as supposed to, which is tested by its own
tests) or provide incorrect arguments, and then strict_type tests should
be different from regular ones since the errors would be different, so
we'd have to write separate tests.

So it's a lot less useful than what I thought.

The only class of errors that could be found this way would be if we
somehow made such a mistake in defining the arguments of certain
function that strict_type version of it doesn't work but regular version
works fine. Which I guess is possible but does it worth the effort to
convert all tests? Not sure.

But it's still useful, I just fixed a bug exactly about it. Given that
there is very
few tests that use strict_types, to "convert" all tests with run-test -t
is not
too hard. Anyway all tests other than for incorrect arguments should run
correctly with -t, so it may be a default for testing. The problem than
is to
know wich tests mus be runned without it.

https://github.com/php/php-src/commit/fddd7e38bd01bc6dbc473166dd6f92
e9f81a6eab

Besides testing, may or may not be valuable expose a __STRICT_TYPES__
constant.

https://github.com/php/php-src/compare/master...
pslacerda:experimental/strict_testing?diff=split

--
Atenciosamente,
Pedro Lacerda

6 years ago by Rowan Collins — view source

unread

Besides testing, may or may not be valuable expose a __STRICT_TYPES__
constant.

I think adding this to the language would be very controversial, because it opens up the ability to undermine the "caller decides" concept of the two scalar type modes. People on this list have openly said that they would like to force users to call their library from strict mode, and others (myself included) would rather that wasn't possible. On the other hand, there were a handful of cases suggested where such an indicator would be useful to emulate the two modes in functions with more complex signatures.

For testing purposes, it would presumably be enough for run-tests.php itself to expose whether the strict types override was in effect, which would sidestep the wider issue.

Regards,

--
Rowan Collins
[IMSoP]

6 years ago by Pedro Lacerda — view source

unread

2018-02-12 18:05 GMT-03:00 Rowan Collins rowan.collins@gmail.com:

I think adding this to the language would be very controversial, because
it opens up the ability to undermine the "caller decides" concept of the
two scalar type modes. People on this list have openly said that they would
like to force users to call their library from strict mode, and others
(myself included) would rather that wasn't possible. On the other hand,
there were a handful of cases suggested where such an indicator would be
useful to emulate the two modes in functions with more complex signatures.

The current beharviour makes sense to me, if the caller don't know the
concept of strict types he must not be forced to handle it. I don't know
such examples where would be useful the callee emulate both signatures, can
you provide some? Something like correctly cast to the correct type? That
can make sense. But restrict the caller doesn't seems very valuable.

For testing purposes, it would presumably be enough for run-tests.php

itself to expose whether the strict types override was in effect, which
would sidestep the wider issue.

Expose only through run-tests.php is safer for sure, it will not be
highlighted like other constants because zend_highlight.c wasn't modified
but whatever, it's only for tests.

--
Atenciosamente,
Pedro Lacerda