Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Execution order "by-defect" does not support parameterized tests with @dataProvider #3246

Closed
reinholdfuereder opened this issue Aug 7, 2018 · 17 comments

Comments

@reinholdfuereder
Copy link

Q A
PHPUnit version 7.3.1
PHP version 7.2.8
Installation Method PHAR

In my test set (executed via XML configuration) there is (coincidentally) (1) one incomplete, (2) one skipped and (3) one failing test. I just started playing around with the new cool feature of running tests first that failed in a previous run, but it does not behave as expected:

acmedev@dev56w1:~$ rm .phpunit.result.cache
rm: remove regular file ‘.phpunit.result.cache’? y
acmedev@dev56w1:~$ php /mnt/data/Kunden/acme/components/phpunit/7.3.1/phpunit.phar --configuration /home/data/acmedev/unittests/AllPlainPHPUnitTests.xml --order-by=defects --cache-result
Execution according to PHPUnit XML configuration file '/home/data/acmedev/unittests/AllPlainPHPUnitTests.xml'...
PHPUnit 7.3.1 by Sebastian Bergmann and contributors.

.................I.............................................  63 / 524 ( 12%)
............................................................... 126 / 524 ( 24%)
............................................................... 189 / 524 ( 36%)
............................................................... 252 / 524 ( 48%)
............................................................... 315 / 524 ( 60%)
............................................................... 378 / 524 ( 72%)
..........................................................F.... 441 / 524 ( 84%)
............................................................... 504 / 524 ( 96%)
.S..................                                            524 / 524 (100%)

Time: 6.41 seconds, Memory: 94.25MB

There was 1 failure:

1) Acme_Phone_FormatNumberTest::formatNumberForMobileTelLink with data set #8 ('  123-456-789', '123456789')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'123456789'
+'      123456789'

/home/data/acmedev/unittests/unit/app/admin/classes/phone/formatnumberTest.php:40

FAILURES!
Tests: 524, Assertions: 1031, Failures: 1, Skipped: 1, Incomplete: 1.
acmedev@dev56w1:~$ php /mnt/data/Kunden/acme/components/phpunit/7.3.1/phpunit.phar --configuration /home/data/acmedev/unittests/AllPlainPHPUnitTests.xml --order-by=defects --cache-result
Execution according to PHPUnit XML configuration file '/home/data/acmedev/unittests/AllPlainPHPUnitTests.xml'...
PHPUnit 7.3.1 by Sebastian Bergmann and contributors.

I..............................................................  63 / 524 ( 12%)
............................................................... 126 / 524 ( 24%)
............................................................... 189 / 524 ( 36%)
............................................................... 252 / 524 ( 48%)
............................................................... 315 / 524 ( 60%)
............................................................... 378 / 524 ( 72%)
..................................................F............ 441 / 524 ( 84%)
.....................................................S......... 504 / 524 ( 96%)
....................                                            524 / 524 (100%)

Time: 6.63 seconds, Memory: 94.25MB

There was 1 failure:

1) Acme_Phone_FormatNumberTest::formatNumberForMobileTelLink with data set #8 ('  123-456-789', '123456789')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'123456789'
+'      123456789'

/home/data/acmedev/unittests/unit/app/admin/classes/phone/formatnumberTest.php:40

FAILURES!
Tests: 524, Assertions: 1031, Failures: 1, Skipped: 1, Incomplete: 1.

=> My expectation would have been, that it starts with the single failing test (or in general all failing test cases or maybe all test classes with failing test cases?); and executing the other tests (including incomplete or skipped ones) in the previous/default order.

Not sure if it is needed or helpful, but the cache result file starts like this:

C:30:"PHPUnit\Runner\TestResultCache":35938:{a:2:{s:7:"defects";a:3:{s:20:"sendQuoteRequestTest";i:2;s:45:"formatNumberForMobileTelLink with data set #8";i:3;s:44:"testPhpWarningIsConvertedToPhpUnitException3";i:1;}...

...whereas:

  1. sendQuoteRequestTest is the incomplete test
  2. formatNumberForMobileTelLink with data set #8 is the failing test (see above)
  3. testPhpWarningIsConvertedToPhpUnitException3 is the skipped test
@reinholdfuereder
Copy link
Author

Hm, when deleting the incomplete test it still behaves badly and does not change the execution order at all; maybe this is because the failing test is a parameterized one?

acmedev@dev56w1:~$ rm .phpunit.result.cache
rm: remove regular file ‘.phpunit.result.cache’? y
acmedev@dev56w1:~$ php /mnt/data/Kunden/acme/components/phpunit/7.3.1/phpunit.phar --configuration /home/data/acmedev/unittests/AllPlainPHPUnitTests.xml --order-by=defects --cache-result
Execution according to PHPUnit XML configuration file '/home/data/acmedev/unittests/AllPlainPHPUnitTests.xml'...
PHPUnit 7.3.1 by Sebastian Bergmann and contributors.

...............................................................  63 / 523 ( 12%)
............................................................... 126 / 523 ( 24%)
............................................................... 189 / 523 ( 36%)
............................................................... 252 / 523 ( 48%)
............................................................... 315 / 523 ( 60%)
............................................................... 378 / 523 ( 72%)
.........................................................F..... 441 / 523 ( 84%)
............................................................... 504 / 523 ( 96%)
S..................                                             523 / 523 (100%)

Time: 5.99 seconds, Memory: 94.25MB

There was 1 failure:

1) Acme_Phone_FormatNumberTest::formatNumberForMobileTelLink with data set #8 ('  123-456-789', '123456789')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'123456789'
+'      123456789'

/home/data/acmedev/unittests/unit/app/admin/classes/phone/formatnumberTest.php:40

FAILURES!
Tests: 523, Assertions: 1031, Failures: 1, Skipped: 1.
acmedev@dev56w1:~$ php /mnt/data/Kunden/acme/components/phpunit/7.3.1/phpunit.phar --configuration /home/data/acmedev/unittests/AllPlainPHPUnitTests.xml --order-by=defects --cache-result
Execution according to PHPUnit XML configuration file '/home/data/acmedev/unittests/AllPlainPHPUnitTests.xml'...
PHPUnit 7.3.1 by Sebastian Bergmann and contributors.

...............................................................  63 / 523 ( 12%)
............................................................... 126 / 523 ( 24%)
............................................................... 189 / 523 ( 36%)
............................................................... 252 / 523 ( 48%)
............................................................... 315 / 523 ( 60%)
............................................................... 378 / 523 ( 72%)
.................................................F............. 441 / 523 ( 84%)
....................................................S.......... 504 / 523 ( 96%)
...................                                             523 / 523 (100%)

Time: 6.69 seconds, Memory: 94.25MB

There was 1 failure:

1) Acme_Phone_FormatNumberTest::formatNumberForMobileTelLink with data set #8 ('  123-456-789', '123456789')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'123456789'
+'      123456789'

/home/data/acmedev/unittests/unit/app/admin/classes/phone/formatnumberTest.php:40

FAILURES!
Tests: 523, Assertions: 1031, Failures: 1, Skipped: 1.

@reinholdfuereder
Copy link
Author

reinholdfuereder commented Aug 7, 2018

Oh yes, it looks like it is the @dataProvider:

  • I have extracted the failing parameterization from @dataProvider and added a dedicated test case with that values => now it works:
acmedev@dev56w1:~$ rm .phpunit.result.cache
rm: remove regular file ‘.phpunit.result.cache’? y
acmedev@dev56w1:~$ php /mnt/data/Kunden/acme/components/phpunit/7.3.1/phpunit.phar --configuration /home/data/acmedev/unittests/AllPlainPHPUnitTests.xml --order-by=defects --cache-result
Execution according to PHPUnit XML configuration file '/home/data/acmedev/unittests/AllPlainPHPUnitTests.xml'...
PHPUnit 7.3.1 by Sebastian Bergmann and contributors.

...............................................................  63 / 523 ( 12%)
............................................................... 126 / 523 ( 24%)
............................................................... 189 / 523 ( 36%)
............................................................... 252 / 523 ( 48%)
............................................................... 315 / 523 ( 60%)
............................................................... 378 / 523 ( 72%)
..........................................................F.... 441 / 523 ( 84%)
............................................................... 504 / 523 ( 96%)
S..................                                             523 / 523 (100%)

Time: 5.9 seconds, Memory: 94.25MB

There was 1 failure:

1) Acme_Phone_FormatNumberTest::formatNumberForMobileTelLink_failingWithoutDataProvider
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'123456789'
+'      123456789'

/home/data/acmedev/unittests/unit/app/admin/classes/phone/formatnumberTest.php:48

FAILURES!
Tests: 523, Assertions: 1031, Failures: 1, Skipped: 1.
acmedev@dev56w1:~$ php /mnt/data/Kunden/acme/components/phpunit/7.3.1/phpunit.phar --configuration /home/data/acmedev/unittests/AllPlainPHPUnitTests.xml --order-by=defects --cache-result
Execution according to PHPUnit XML configuration file '/home/data/acmedev/unittests/AllPlainPHPUnitTests.xml'...
PHPUnit 7.3.1 by Sebastian Bergmann and contributors.

F..............................................................  63 / 523 ( 12%)
............................................................... 126 / 523 ( 24%)
............................................................... 189 / 523 ( 36%)
............................................................... 252 / 523 ( 48%)
............................................................... 315 / 523 ( 60%)
............................................................... 378 / 523 ( 72%)
............................................................... 441 / 523 ( 84%)
....................................................S.......... 504 / 523 ( 96%)
...................                                             523 / 523 (100%)

Time: 5.92 seconds, Memory: 94.25MB

There was 1 failure:

1) Acme_Phone_FormatNumberTest::formatNumberForMobileTelLink_failingWithoutDataProvider
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'123456789'
+'      123456789'

/home/data/acmedev/unittests/unit/app/admin/classes/phone/formatnumberTest.php:48

FAILURES!
Tests: 523, Assertions: 1031, Failures: 1, Skipped: 1.

@reinholdfuereder reinholdfuereder changed the title Execution order "by-defect" starts with incomplete test Execution order "by-defect" does not support parameterized tests with @dataProvider Aug 7, 2018
@reinholdfuereder
Copy link
Author

Rather irrelevant by comparison, but I am also wondering why the execution order "by-defect" also seems to consider incomplete tests:

acmedev@dev56w1:~$ rm .phpunit.result.cache
rm: remove regular file ‘.phpunit.result.cache’? y
acmedev@dev56w1:~$ php /mnt/data/Kunden/acme/components/phpunit/7.3.1/phpunit.phar --configuration /home/data/acmedev/unittests/AllPlainPHPUnitTests.xml --cache-result           Execution according to PHPUnit XML configuration file '/home/data/acmedev/unittests/AllPlainPHPUnitTests.xml'...
PHPUnit 7.3.1 by Sebastian Bergmann and contributors.

...............................................................  63 / 524 ( 12%)
............................................................... 126 / 524 ( 24%)
............................................................... 189 / 524 ( 36%)
............................................................... 252 / 524 ( 48%)
....................................F.......................... 315 / 524 ( 60%)
...................................I........................... 378 / 524 ( 72%)
............................................................... 441 / 524 ( 84%)
............................................................... 504 / 524 ( 96%)
.....S..............                                            524 / 524 (100%)

Time: 5.95 seconds, Memory: 94.25MB

There was 1 failure:

1) Acme_Phone_FormatNumberTest::formatNumberForMobileTelLink_failingWithoutDataProvider
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'123456789'
+'      123456789'

/home/data/acmedev/unittests/unit/app/admin/classes/phone/formatnumberTest.php:48

FAILURES!
Tests: 524, Assertions: 1031, Failures: 1, Skipped: 1, Incomplete: 1.
acmedev@dev56w1:~$ php /mnt/data/Kunden/acme/components/phpunit/7.3.1/phpunit.phar --configuration /home/data/acmedev/unittests/AllPlainPHPUnitTests.xml --cache-result           Execution according to PHPUnit XML configuration file '/home/data/acmedev/unittests/AllPlainPHPUnitTests.xml'...
PHPUnit 7.3.1 by Sebastian Bergmann and contributors.

...............................................................  63 / 524 ( 12%)
............................................................... 126 / 524 ( 24%)
............................................................... 189 / 524 ( 36%)
............................................................... 252 / 524 ( 48%)
....................................F.......................... 315 / 524 ( 60%)
...................................I........................... 378 / 524 ( 72%)
............................................................... 441 / 524 ( 84%)
............................................................... 504 / 524 ( 96%)
.....S..............                                            524 / 524 (100%)

Time: 5.96 seconds, Memory: 94.25MB

There was 1 failure:

1) Acme_Phone_FormatNumberTest::formatNumberForMobileTelLink_failingWithoutDataProvider
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'123456789'
+'      123456789'

/home/data/acmedev/unittests/unit/app/admin/classes/phone/formatnumberTest.php:48

FAILURES!
Tests: 524, Assertions: 1031, Failures: 1, Skipped: 1, Incomplete: 1.
acmedev@dev56w1:~$ php /mnt/data/Kunden/acme/components/phpunit/7.3.1/phpunit.phar --configuration /home/data/acmedev/unittests/AllPlainPHPUnitTests.xml --order-by=defects --cache-result
Execution according to PHPUnit XML configuration file '/home/data/acmedev/unittests/AllPlainPHPUnitTests.xml'...
PHPUnit 7.3.1 by Sebastian Bergmann and contributors.

F.........I....................................................  63 / 524 ( 12%)
............................................................... 126 / 524 ( 24%)
............................................................... 189 / 524 ( 36%)
............................................................... 252 / 524 ( 48%)
............................................................... 315 / 524 ( 60%)
............................................................... 378 / 524 ( 72%)
............................................................... 441 / 524 ( 84%)
.....................................................S......... 504 / 524 ( 96%)
....................                                            524 / 524 (100%)

Time: 5.93 seconds, Memory: 94.25MB

There was 1 failure:

1) Acme_Phone_FormatNumberTest::formatNumberForMobileTelLink_failingWithoutDataProvider
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'123456789'
+'      123456789'

/home/data/acmedev/unittests/unit/app/admin/classes/phone/formatnumberTest.php:48

FAILURES!
Tests: 524, Assertions: 1031, Failures: 1, Skipped: 1, Incomplete: 1.

@sebastianbergmann
Copy link
Owner

@epdenouden ping

@epdenouden
Copy link
Contributor

@reinholdfuereder thank you for the super detailed issue! I will look into it later this week, it is currently not impeding anything, right?

@reinholdfuereder
Copy link
Author

@epdenouden No stress at all from my side, just take the time you need; thanks in advance...

@epdenouden
Copy link
Contributor

@sebastianbergmann Can you assign this issue to me? makes it much easier to keep track of it. :)

@sebastianbergmann
Copy link
Owner

@epdenouden GitHub only allows assigning of issues to people that have push access, sorry.

@epdenouden
Copy link
Contributor

@sebastianbergmann Thanks for the clarification. I had no idea. Quickfix by tracking it via my own issues list.

@epdenouden
Copy link
Contributor

epdenouden commented Oct 5, 2018

@reinholdfuereder Apologies for the delay, I have some time this weekend to finally look into this.

To answer one question quickly: but I am also wondering why the execution order "by-defect" also seems to consider incomplete tests?
The sorting algorithm only considers tests that would require fixing business logic to be defective:

  • errors: PHP errors, for example syntax, missing require(), runtime errors
  • failing assertions

Other modes of failure like skipping and incomplete are considered to be non-blocking annoyances. We tried a few different settings for this and quickly found out that sorting skipped/incomplete tests to the front would result in some TestSuites being first every single run. The real-world reason for this is often environment dependencies like "only run this test when we have run on a Unix-like system". Issue #2085 is an example of this as it requires OS-specific functionality.

@reinholdfuereder
Copy link
Author

@epdenouden Thanks for the explanation: this sounds very reasonable

@epdenouden
Copy link
Contributor

epdenouden commented Oct 19, 2018

@reinholdfuereder

  • Is the repo or application you are using for your test public, can I find it on Github somewhere?
  • Does your test collection have multiple @dataprovider driven tests? The sorting algorithm can only sort individual dataprovider-rows within the test.

In general, the TestSuiteSorter has a bunch of limitations that are a direct result of the internal structure of PHPUnit. Sebastian has recently done a nice presentation about that. (I wish I had had those graphs earlier this year :)

  • @dataprovider driven TestCases are converted to DataproviderTestSuites before the run starts
  • TestCases can only be sorted within the context of their parent TestSuite
  • TestSuites are run in sequence, or when using something like Paratest, independently of eachother

@reinholdfuereder
Copy link
Author

@epdenouden

  • No, sorry, the code is not public; if necessary, I could try to write some small re-producer examples (but judging from your lines below concerning quite specific limitations it may not be that easy!?)
  • Yes, IIRC there are a lot of different PHPUnit test classes with typically quite often more than one different data provider (within each PHPUnit test class) -- is the latter vital for the re-producer, while the other test classes should be irrelevant (they just need to be around)?

Okay, I nonetheless assume it should not be too difficult to write a small re-producer based on the aforementioned failing one. Unfortunately I will be in the office and have access to the code no earlier than October 30. However, I guess any further delay does not really matter... So please be patient...

@epdenouden
Copy link
Contributor

epdenouden commented Oct 30, 2018

@reinholdfuereder No worries about the timing. As you can see, I'm also not as quick as I like with working on this, with the new(ish) job and all.

I will just go ahead and try to break the edge cases I have sketched out above ^^^ and report back here.

@epdenouden
Copy link
Contributor

epdenouden commented Nov 2, 2018

@reinholdfuereder I have found a scenario that fails in a way that looks a lot like your report. Have added a test that reproduces the failure. Caching previous test results does not sort tests with defective rows to the front.

Remove the cache and do a first test run:

./phpunit --testdox --order-by=defects tests/_files/MultiDependencyExecutionOrderTest.php
[...]
MultiDependencyExecutionOrder
 ✔ First test that always works [12.30 ms]
 ✔ Add numbers with a dataprovider with data set "1+2=3" [1.07 ms]
 ✔ Add numbers with a dataprovider with data set "2+1=3" [0.24 ms]
 ✘ Add numbers with a dataprovider with data set "1+1=3" [4.52 ms]
   │
   │ Failed asserting that 2 is identical to 3.
   │ 

 ✔ Test in the middle that always works [0.34 ms]
 ✔ Add more numbers with a dataprovider with data set "1+2=3" [0.35 ms]
 ✔ Add more numbers with a dataprovider with data set "2+1=3" [0.49 ms]
 ✘ Add more numbers with a dataprovider with data set "1+1=3" [0.51 ms]
   │
   │ Failed asserting that 2 is identical to 3.
   │ 

A second run will sort the failing data rows to the front of their parent dataprovider, but the tests using those with @dataprovider as not recognized as defective:

./phpunit --testdox --order-by=defects tests/_files/MultiDependencyExecutionOrderTest.php
[...]
MultiDependencyExecutionOrder
 ✔ First test that always works [2.72 ms]
 ✘ Add numbers with a dataprovider with data set "1+1=3" [1.10 ms]
   │
   │ Failed asserting that 2 is identical to 3.
   │ 

 ✔ Add numbers with a dataprovider with data set "1+2=3" [0.16 ms]
 ✔ Add numbers with a dataprovider with data set "2+1=3" [0.15 ms]
 ✔ Test in the middle that always works [0.18 ms]
 ✘ Add more numbers with a dataprovider with data set "1+1=3" [0.56 ms]
   │
   │ Failed asserting that 2 is identical to 3.
   │ 

 ✔ Add more numbers with a dataprovider with data set "1+2=3" [0.29 ms]
 ✔ Add more numbers with a dataprovider with data set "2+1=3" [0.15 ms]

The issue can be found somewhere in the logic that bubbles cached test results up the search tree. Although it's just a minor bug, it can make the tool feel unpredictable or make it look like the cache doesn't work. Good to have it fixed. It might even be a nice performance gain for developers working on unit tests for e.g. input validation logic with sizeable dataproviders like \PHPUnit\Util\XmlTest::charProvider.

Thank you for providing such detailed and ongoing feedback!

@epdenouden
Copy link
Contributor

@reinholdfuereder After some more investigation I have opened #3396 and will provide an implementation for this in the coming week.

@epdenouden
Copy link
Contributor

@sebastianbergmann This ticket can be closed now. Everything here is covered by #3396 and its related pull request #3401 :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants