Proposal to run all tests with and without strict_types enabled

  101827
February 11, 2018 23:03 pslacerda@gmail.com (Pedro Lacerda)
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
  101828
February 11, 2018 23:34 cmbecker69@gmx.de ("Christoph M. Becker")
On 12.02.2018 at 00:03, Pedro Lacerda wrote:

> 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
  101829
February 11, 2018 23:42 pslacerda@gmail.com (Pedro Lacerda)
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>:

> On 12.02.2018 at 00:03, Pedro Lacerda wrote: > > > 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
  101830
February 12, 2018 01:32 pslacerda@gmail.com (Pedro Lacerda)
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>: > >> On 12.02.2018 at 00:03, Pedro Lacerda wrote: >> >> > 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
  101832
February 12, 2018 05:18 smalyshev@gmail.com (Stanislav Malyshev)
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
  101833
February 12, 2018 05:50 pslacerda@gmail.com (Pedro Lacerda)
> > 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
  101855
February 12, 2018 21:05 rowan.collins@gmail.com (Rowan Collins)
On 12 February 2018 05:50:56 GMT+00:00, Pedro Lacerda <pslacerda@gmail.com> wrote:
>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]
  101856
February 12, 2018 23:37 pslacerda@gmail.com (Pedro Lacerda)
2018-02-12 18:05 GMT-03:00 Rowan Collins 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