sebastianbergmann / phpunit

The PHP Unit Testing framework.
https://phpunit.de/
BSD 3-Clause "New" or "Revised" License
19.69k stars 2.2k forks source link

how to properly escape a string for use in --filter #5706

Open staabm opened 8 months ago

staabm commented 8 months ago
Q A
PHPUnit version 9.5.23
PHP version 8.2.16
Installation Method Composer / PHAR

Summary

we try to feed phpunit --filter with test-case names which we determine via --list-tests-xml. we are wondering, how to properly escape our input strings we want to pass to phpunit via --filter option.

Current behavior

the docs mention the filter is interpreted as a regex, therefore I went with preg_quote(), but this didn't work when we use a "full qualified test-case name" which contains a #.

How to reproduce

a test-class like

<?php
namespace TestNamespace;

use PHPUnit\Framework\TestCase;

class mytest extends TestCase
{
    /**
     * @dataProvider provider
     */
    public function testMethod($data)
    {
        $this->assertTrue($data);
    }

    public function provider()
    {
        return [
            'my name(d data' => [true],
            'my $dat)a'       => [true]
        ];
    }
}

requires a escaped filter

➜  phpstan-src git:(1.11.x) ✗ vendor/bin/phpunit mytest.php --filter 'my $dat\)a'
PHPUnit 9.5.23 #StandWithUkraine

Warning:       No code coverage driver available

No tests executed!
➜  phpstan-src git:(1.11.x) ✗ vendor/bin/phpunit mytest.php --filter 'my \$dat\)a'
PHPUnit 9.5.23 #StandWithUkraine

Warning:       No code coverage driver available

.                                                                   1 / 1 (100%)

Time: 00:00.011, Memory: 32.00 MB

OK (1 test, 1 assertion)

the escaping required looks like preg_quote and it works for the case above


the following example doesn't work with preg_quote:

<?php
namespace TestNamespace;

use PHPUnit\Framework\TestCase;

class mytest extends TestCase
{
    /**
     * @dataProvider provider
     */
    public function testMethod($data)
    {
        $this->assertTrue($data);
    }

    public function provider()
    {
        return [
            [true],
            [true]
        ];
    }
}
➜  phpstan-src git:(quote) ✗ vendor/bin/phpunit mytest.php --filter 'mytest\#1'
PHPUnit 9.5.23 #StandWithUkraine

Warning:       No code coverage driver available

No tests executed!

Expected behavior

1)

I wonder how to properly escape the value for the --filter option. this would be especially important as when using the wrong filter no tests would be executed.

2)

can we make phpunit return with a non-zero exit code, when a filter is used which does not execute any tests?

edit: I just found https://github.com/sebastianbergmann/phpunit/pull/4314 - which described how it should be done.

sebastianbergmann commented 8 months ago

I just found #4314 - which described how it should be done.

I was about to comment this:

$ ./tools/phpunit --filter foo --fail-on-empty-test-suite
PHPUnit 11.0.3 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.3.3
Configuration: /usr/local/src/raytracer/phpunit.xml

No tests executed!
$ echo $?                                                
1

I just opened #5707 as --fail-on-empty-test-suite is missing from --help output.

ondrejmirtes commented 8 months ago

We're just interested in how the value passed to --filter should be sanitized when it comes from an unknown/dynamic source. Is doubling each \ the only thing that needs to be done, or are there other rules to follow?

sebastianbergmann commented 8 months ago

I honestly do not know and need to research this (ie. look at the code). I do not have time to do that right now, sorry. I might get to it later today, but it's the weekend, you know. TL;DR: I will do my best to get you an answer ASAP.

ondrejmirtes commented 8 months ago

There's no rush, we're just trying to improve the CI pipeline :) Thank you very much.

sebastianbergmann commented 8 months ago

Doubling \ should be enough. Here you can see what PHPUnit does with the value passed to --filter.

staabm commented 8 months ago

hey sebastian,

thx for doing the research.

I think we need to do more escaping, see the very first example in the PR description above. when using data-providers with a string-key, its possible to use characters like e.g. $, (, ) which need escaping in the --filter option.

Doubling \ is only enough if your filter just contains a class-name and or test-method, but not identifier for the data-provider.

sebastianbergmann commented 8 months ago

Do you need something else from me here or can this be closed?

ondrejmirtes commented 8 months ago

I think what @staabm tried to say is that when you use a custom name for a specific data provider iteration like:

yield 'unknown filter' => [
    'mixed',
    'filter_var($mixed, $mixed)',
];

You could use a custom name that looks like a regular expression. And the question is - if we wanted to execute only this test, how should we write the argument for --filter so that it's not interpreted as a regular expression, but as a request to execute the test for this one specific data provider iteration.

@staabm's theory is that simply doubling \\ is not enough to properly escape the --filter value.

We could of course avoid this problem by using simple names for data providers (and I think we do), but if we were for example to publish a general GitHub Action that would promise you to partition your test suite and run it in parallel over multiple jobs, we could not rely on simple names anymore.

mgleska commented 3 months ago

@staabm In my opinion source of the problem is function preg_quote which you consider for escaping data extracted from --list-tests-xml. In preg_quote documentation https://www.php.net/manual/en/function.preg-quote.php we can read, that # is "special regular expression character" and will be escaped by this function. It surprises me, because in PCRE documentation https://www.php.net/manual/en/regexp.reference.meta.php # is not present on meta-character list. Exactly the same we can find in https://www.pcre.org/original/doc/html/pcrepattern.html#SEC4

I think you should consider using str_replace instead of preg_quote.