fix --filter format used by PHPStorm

[schlndh]

It was broken by PR #6272. As far as I can tell the goal was to change the test name printed by the default printer to make it easier to copy-paste it into --filter.

Example from PHPUnit's own tests:

1) PHPUnit\TestFixture\DataProviderFilterTest::testFalse@other false test with data (false)

Can be copy-pasted into --filter like this:

--filter 'testFalse@other false test'

However, IMO the PR changed the test name more broadly than necessary. This broke the --filter pattern used by PHPStorm (i.e. No tests executed!). E.g:

--filter '/PHPUnit\\TestFixture\\DataProviderFilterTest::testFalse with data set \"other false test\"$/'

It would have have an extra @ like this:

--filter '/PHPUnit\\TestFixture\\DataProviderFilterTest::testFalse with data set \"@other false test\"$/'

But the @ is not present in the teamcity output that PHPStorm uses (they seem to use the locationHint). So PHPStorm (and similar tools) would have to hack the test name to add it (which would presumably make the whole thing even more tied to PHPUnit's internal behavior).

IMO the --filter used by PHPStorm is pretty reasonable, so I'm attempting to restore support for it, while maintaining the new name used by the default printer for easy copy-pasting.

[sebastianbergmann]

CC @staabm

[staabm]

@schlndh thanks for reporting.

Could you describe how to reproduce the problem jn PHPStorm?

[staabm]

If phpstorm is using data from a formatter and feeds it back into the filter cli option we should have a test for this e2e scenario

[schlndh]

@staabm I do it like this:

• Run a test with a bunch of cases from a data provider.
• Click on one of the test cases and run just that one (especially useful for debugging failing cases one-by-one).

Here is a screen recording to make it even easier to understand:

12.3 branch:

phpstorm_test_broken.mp4

My branch:

phpstorm_test_fixed.mp4

[staabm]

I think this looks mostly good. thanks for the in-detail repro and the time you invested in making the screen recordings.

[staabm]

just to re-iterate: this reverts my change which caused the bug/BC break.

[schlndh]

The real bug/BC break was in dataSetAsString. This just has to match it, so that the short test@data filter syntax works. The PHPStorm's filter doesn't execute this code at all, because it's a full regex pattern (i.e. starting with /).

[staabm]

we need another test with a numbered dataprovider. this one here covers only named dataproviders.

and I think we should land the new tests on PHPUnit 11+ to make sure all PHPUnit versions are on the same page

[sebastianbergmann]

Maybe even for PHPUnit >= 10.

[schlndh]

I opened a separate PR that adds E2E tests both for named and numbered data providers. If you want, I can also add a simpler version of the test like this to it. Otherwise, I'd just delete the test from this PR.

[staabm]

usually we put a url to the underlying issue/pr in --TEST--.

in this case it would be

--TEST--
https://github.com/sebastianbergmann/phpunit/pull/6364

[staabm]

And as mentioned above, I think we should have a separate e2e test which feeds the output of the teamcity formatter back into the --filter option, so we mimick the phpstorm use-case as close as possible

[schlndh]

And as mentioned above, I think we should have a separate e2e test which feeds the output of the teamcity formatter back into the --filter option, so we mimick the phpstorm use-case as close as possible

Do you have any suggestion about the best way to do it? I tried first running the whole test class with --log-teamcity, parsing the locationHint and running the test again with --filter. Unfortunately, it seems that (new PHPUnit\TextUI\Application)->run($_SERVER['argv']); cannot be called twice from one process, because of all the static properties it initializes.

I suppose that a weaker alternative might be hard-coding the --filter and running the test with --teamcity to make sure that the locationHint matches the contents of the filter. But I guess it would be easy to overlook this intentional coupling if the test breaks in the future and it could be fixed incorrectly.

[staabm]

Do you have any suggestion about the best way to do it? I tried first running the whole test class with --log-teamcity, parsing the locationHint and running the test again with --filter. Unfortunately, it seems that (new PHPUnit\TextUI\Application)->run($_SERVER['argv']); cannot be called twice from one process, because of all the static properties it initializes.

I think you could either

• use exec or similar to let phpunit ran in a subprocess and work on the resulting xml (feels closer to PHPStorm)
• or use TeamCityLogger directly to let it build the xml

maybe @sebastianbergmann has a better idea

[schlndh]

@sebastianbergmann Since the PhpStorm team hasn't responded to your question on the E2E test PR, can we proceed with this PR regardless? IMO the fix makes sense in general, not just due to PhpStorm. I'd just add a simple test for the numbered dataprovider here for completeness.