--filter format used by PhpStorm stopped working#6364
--filter format used by PhpStorm stopped working#6364sebastianbergmann merged 3 commits intosebastianbergmann:12.4from
--filter format used by PhpStorm stopped working#6364Conversation
|
CC @staabm |
|
@schlndh thanks for reporting. Could you describe how to reproduce the problem jn PHPStorm? |
|
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 |
|
@staabm I do it like this:
Here is a screen recording to make it even easier to understand: 12.3 branch: phpstorm_test_broken.mp4My branch: phpstorm_test_fixed.mp4 |
staabm
left a comment
There was a problem hiding this comment.
I think this looks mostly good. thanks for the in-detail repro and the time you invested in making the screen recordings.
| elseif (preg_match('/^(.*?)@(.+)$/', $filter, $matches)) { | ||
| $filter = sprintf( | ||
| '%s.*with data set "@%s"$', | ||
| '%s.*with data set "%s"$', |
There was a problem hiding this comment.
just to re-iterate: this reverts my change which caused the bug/BC break.
There was a problem hiding this comment.
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 /).
| @@ -0,0 +1,22 @@ | |||
| --TEST-- | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Maybe even for PHPUnit >= 10.
There was a problem hiding this comment.
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.
| @@ -0,0 +1,22 @@ | |||
| --TEST-- | |||
| phpunit --filter '/PHPUnit\\TestFixture\\DataProviderFilterTest::testFalse with data set \"other false test\"$/' ../../_files/DataProviderFilterTest.php | |||
There was a problem hiding this comment.
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
|
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 I suppose that a weaker alternative might be hard-coding the |
I think you could either
maybe @sebastianbergmann has a better idea |
|
@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. |
* 12.4: Mark test as expected to fail for #6364 fix E2E phpstorm test on windows fix E2E phpstorm test on windows add E2E test simulating running individual tests in PHPStorm
* 12.5: Mark test as expected to fail for #6364 fix E2E phpstorm test on windows fix E2E phpstorm test on windows add E2E test simulating running individual tests in PHPStorm
|
I have merged the tests from #6367 after they were approved by @smirok on behalf of the PhpStorm team. On the |
It was broken by PR sebastianbergmann#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: 1) PHPUnit\TestFixture\DataProviderFilterTest::testFalse@other false test2 with data (false) Into: --filter 'testFalse@other false test2' However, the PR changed the test name more broadly than necessary. This broke --filter pattern used by PHPStorm. E.g: --filter '/PHPUnit\\TestFixture\\DataProviderFilterTest::testFalse with data set \"other false test\"$/'
|
@staabm What do you think? Thanks! |
|
So I justed tested this against the goals of #6272: after applying this diff (to make tests fail) diff --git a/tests/_files/DataProviderFilterTest.php b/tests/_files/DataProviderFilterTest.php
index 1c05d2eca..f26af6d9f 100644
--- a/tests/_files/DataProviderFilterTest.php
+++ b/tests/_files/DataProviderFilterTest.php
@@ -14,11 +14,12 @@
final class DataProviderFilterTest extends TestCase
{
+
public static function truthProvider(): array
{
return [
[true],
- [true],
+ [false],
[true],
[true],
];
@@ -28,7 +29,7 @@ public static function falseProvider(): array
{
return [
'false test' => [false],
- 'false test 2' => [false],
+ 'false test 2' => [true],
'other false test' => [false],
'other false test2' => [false],
];I can run which is what the initial PR wanted to implement. therefore I think this PR is ready to land. thanks @schlndh |
--filter format used by PhpStorm stopped working
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:
Can be copy-pasted into
--filterlike this: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:It would have have an extra
@like this:But the
@is not present in the teamcity output that PHPStorm uses (they seem to use thelocationHint). 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
--filterused 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.