phpunit icon indicating copy to clipboard operation
phpunit copied to clipboard

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

Open staabm opened this issue 1 year ago • 9 comments

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

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.

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

staabm avatar Feb 18 '24 09:02 staabm

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.

sebastianbergmann avatar Feb 18 '24 09:02 sebastianbergmann

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?

ondrejmirtes avatar Feb 18 '24 09:02 ondrejmirtes

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.

sebastianbergmann avatar Feb 18 '24 09:02 sebastianbergmann

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

ondrejmirtes avatar Feb 18 '24 10:02 ondrejmirtes

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

sebastianbergmann avatar Feb 18 '24 12:02 sebastianbergmann

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.

staabm avatar Feb 18 '24 13:02 staabm

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

sebastianbergmann avatar Feb 27 '24 13:02 sebastianbergmann

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.

ondrejmirtes avatar Feb 27 '24 20:02 ondrejmirtes

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

mgleska avatar Aug 10 '24 16:08 mgleska