Everything
php
Learn Community Releases The PHP Project
Home » Project » Newsgroups » php.internals » Remove zpp variation tests »

Remove zpp variation tests

php.internals

Nikita Popov

7 years ago
Hi internals, We currently have a large number of zpp "variation" tests, which essentially only test the behavior of zend_parse_parameters plugged into different functions. A random example of such a test: https://github.com/php/php-src/blob/master/ext/standard/tests/strings/addslashes_variation1.phpt This test passes a certain set of input values of different types to a function with a zpp string argument and observes the behavior. Of course, there are also hundreds of other functions that accept strings through zpp and the behavior is always going to be the same. Based on a quick grep I would estimate that we have around 700 of these tests. We have a hard policy against adding new tests for zpp behavior, but I would like to get rid of the existing ones as well. They provide essentially no value, but are a major chore to update for even for minor changes. Regards, Nikita

Christoph Becker

7 years ago
On 30.01.2019 at 10:45, Nikita Popov wrote:
> Based on a quick grep I would estimate that we have around 700 of these > tests. We have a hard policy against adding new tests for zpp behavior, but > I would like to get rid of the existing ones as well. They provide > essentially no value, but are a major chore to update for even for minor > changes.
I'm +1 on removing (parts of) tests which only test for ZPP failures.
-- Christoph M. Becker

Gabriel Caruso

7 years ago
Em qua, 30 de jan de 2019 às 07:46, Nikita Popov <nikita.ppv@gmail.com> escreveu:
> Hi internals, > > We currently have a large number of zpp "variation" tests, which > essentially only test the behavior of zend_parse_parameters plugged into > different functions. A random example of such a test: > > > https://github.com/php/php-src/blob/master/ext/standard/tests/strings/addslashes_variation1.phpt > > This test passes a certain set of input values of different types to a > function with a zpp string argument and observes the behavior. Of course, > there are also hundreds of other functions that accept strings through zpp > and the behavior is always going to be the same. > > Based on a quick grep I would estimate that we have around 700 of these > tests. We have a hard policy against adding new tests for zpp behavior, but > I would like to get rid of the existing ones as well. They provide > essentially no value, but are a major chore to update for even for minor > changes. > > Regards, > Nikita >
Yes, yes, yes! When I was implementing the https://github.com/php/php-src/pull/3429, was a pain fixing all those tests.
-- Gabriel Caruso

Stas Malyshev

7 years ago
Hi!
> This test passes a certain set of input values of different types to a > function with a zpp string argument and observes the behavior. Of course, > there are also hundreds of other functions that accept strings through zpp > and the behavior is always going to be the same.
This is true for functions that use standard zpp handling, but there are a number of them that use custom zpp handling, and for those there could be corner cases which aren't handled properly. In fact, I've seen some zpp-using functions that so not properly handle nulls, empty strings, etc. as arguments (zpp works fine, it's the part past zpp that fails). So we need to be careful and remove only those tests that are indeed handled by zpp and not the code beyond it that does need to be tested, and not remove tests which cover custom argument handling.
-- Stas Malyshev smalyshev@gmail.com

Nikita Popov

7 years ago
On Wed, Jan 30, 2019 at 7:39 PM Stanislav Malyshev <smalyshev@gmail.com> wrote:
> Hi! > > > This test passes a certain set of input values of different types to a > > function with a zpp string argument and observes the behavior. Of course, > > there are also hundreds of other functions that accept strings through > zpp > > and the behavior is always going to be the same. > > This is true for functions that use standard zpp handling, but there are > a number of them that use custom zpp handling, and for those there could > be corner cases which aren't handled properly. In fact, I've seen some > zpp-using functions that so not properly handle nulls, empty strings, > etc. as arguments (zpp works fine, it's the part past zpp that fails). > So we need to be careful and remove only those tests that are indeed > handled by zpp and not the code beyond it that does need to be tested, > and not remove tests which cover custom argument handling. >
Yes, we shouldn't remove tests for custom argument handling. I've started working on this in https://github.com/php/php-src/pull/3783 and try to check for all tests whether this is a simple zpp argument or not. I'm also switching some cases to use zpp on the PHP 8 branch, where functions had some custom argument handling for no good reason. Nikita