search

paratestphp / paratest

:computer: Parallel testing for PHPUnit
MIT License
2.32k stars 229 forks source link

Percentage going over 100. #58

Closed gnutix closed 9 years ago

gnutix commented 11 years ago

Small issue I've noticed in the output for paratests : the percentage is often over 100. Exemple: 

$ $(pwd)/vendor/bin/paratest -c $(pwd)/app/phpunit.xml.dist $(pwd)/src/
Running phpunit in 5 processes with /home/travis/build/project/vendor/bin/phpunit
Configuration read from /home/travis/build/project/app/phpunit.xml.dist
...............................................................  63 / 159 ( 39%)
............................................................... 126 / 159 ( 79%)
............................................................... 189 / 159 (118%)
......................
Time: 53 seconds, Memory: 12.75Mb
OK (211 tests, 1091 assertions)

As you can see here, the percentage shown is 118%. Any idea how this can happen? Could it be because of skipped / incomplete / @depend tests ?

dbaltas commented 11 years ago

This is an issue caused by the @ depends attribute, but in the functional mode paratest -f. I just pushed a fix at #40 suggested by @zerkms, Can you check out this branch and see if it works for you?

dbaltas commented 11 years ago

(closed by mistake) The fix has been pushed in the master branch. For the record, the lack of support for incomplete and skipped tests of the current version has the opposite effect of the one you are experiencing. Those tests are not accounted for. For example if you have 450 regular tests and 50 incomplete or skipped for a total of 500, you are expected to see the progress bar going 63/500, 126/500, where the 126 are the regular ones only. So the progress bar will stop before 100% at 450 and in the end you will see OK (450 tests, X assertions)

After seeing your example, it seems that you have 159 tests, 52 (211-159) regular tests from those are called twice and 22 incomplete or skipped (211-189).

If the fix works you will then see `OK (128 tests, X assertions)... until the skipped/incomplete tests issue is addressed :)

brianium commented 11 years ago

On the subject of skipped/incomplete tests - for completeness sake, it might be a good reason to implement. However, those tests are not logged in JUnit XML. It seems like the only other way to report on those would be to do code analysis (possibly via the SuiteLoader?).

gnutix commented 11 years ago

@dbaltas So do you need me to test the latest master branch ? Or this one ? https://github.com/brianium/paratest/tree/issues/17-dependent-tests-on-functional-mode

dbaltas commented 11 years ago

the master branch. thanks.

gnutix commented 11 years ago

Here's the result :

With paratest v0.4.4:

$ bin/paratest -c app/phpunit.xml.dist src/
Running phpunit in 5 processes with /home/user/project/vendor/bin/phpunit

Configuration read from /home/user/project/app/phpunit.xml.dist
...............................................................  63 / 207 ( 30%)
............................................................... 126 / 207 ( 60%)
............................................................... 189 / 207 ( 91%)
............................................................... 252 / 207 (121%)
............................................................... 315 / 207 (152%)
.........................................................

Time: 03:41, Memory: 16.50Mb
OK (372 tests, 1305 assertions)

With paratest dev-master:

$ bin/paratest -c app/phpunit.xml.dist src/
Running phpunit in 5 processes with /home/user/project/vendor/bin/phpunit

Configuration read from /home/user/project/app/phpunit.xml.dist
...............................................................  63 / 173 ( 36%)
............................................................... 126 / 173 ( 72%)
............................................................... 189 / 173 (109%)
............................................................... 252 / 173 (145%)
............................................................... 315 / 173 (182%)
.........................................................

Time: 04:31, Memory: 20.00Mb
OK (372 tests, 1305 assertions)

With PHPUnit v3.7.22:

$ bin/phpunit -c app/ src/
PHPUnit 3.7.22 by Sebastian Bergmann.

Configuration read from /home/user/project/app/phpunit.xml

...............................................................  63 / 375 ( 16%)
............................................................... 126 / 375 ( 33%)
.S............................................................. 189 / 375 ( 50%)
.........................................S..................... 252 / 375 ( 67%)
...............S............................................... 315 / 375 ( 84%)
............................................................

Time: 09:02, Memory: 4715.00Mb
OK, but incomplete or skipped tests!
Tests: 375, Assertions: 1306, Skipped: 3.

I have found 34 occurences of @depends in my code and there's 3 skipped tests (as we can see from PHPUnit's output). One strange thing is the assertions count too : 1305 with paratest and 1306 with phpunit.

dbaltas commented 11 years ago

Unfortunately I can't reproduce the problem. Can you nail it down to the smallest combination of tests (maybe in a single test file) that produces different output with phpunit and dev-master?

Thanks for the feedback and for the patience!

Note: I hope that the fact that running with dev-master was almost 1 minute slower than with 0.4.4 is a coincidence related to system resources usage :)

gnutix commented 11 years ago

Note: it probably was, as I was running phpunit at the same time. ;)

I'll try to do that as soon as I get some time (quite busy at work those next days).

gnutix commented 10 years ago

@dbaltas Still had no time to get a small reproducible test suite, sorry. The issue remains though.

With paratest v0.6.0 : 

$ bin/paratest -c $(pwd)/app/phpunit.xml.dist $(pwd)/src/
Running phpunit in 5 processes with phpunit
Configuration read from /home/travis/build/app/phpunit.xml.dist
...............................................................  63 / 257 ( 24%)
............................................................... 126 / 257 ( 49%)
............................................................... 189 / 257 ( 73%)
............................................................... 252 / 257 ( 98%)
............................................................... 315 / 257 (122%)
............................................................... 378 / 257 (147%)
............................................................... 441 / 257 (171%)
............................................................... 504 / 257 (196%)
......................
Time: 5.1 minutes, Memory: 22.50Mb
OK (526 tests, 1930 assertions)

With phpunit :

PHPUnit 3.7.28 by Sebastian Bergmann.

Configuration read from /home/travis/build/app/phpunit.xml.dist

.......................I.......................................  63 / 529 ( 11%)
............................................................... 126 / 529 ( 23%)
............................................................... 189 / 529 ( 35%)
..................................................S............ 252 / 529 ( 47%)
............................................................... 315 / 529 ( 59%)
............................................................... 378 / 529 ( 71%)
......S........................................................ 441 / 529 ( 83%)
............................................................... 504 / 529 ( 95%)
.........................

Time: 9.92 minutes, Memory: 6114.25Mb

There was 1 incomplete test:
1) ...

There were 2 skipped tests:
1) ...
2) ...

OK, but incomplete or skipped tests!
Tests: 529, Assertions: 1932, Incomplete: 1, Skipped: 2.
aik099 commented 10 years ago

I'm getting 423 of 313 tests executed (120%) here https://travis-ci.org/aik099/qa-tools/jobs/15085005 when executed with paratest (dev-master):

$ ./vendor/bin/paratest --coverage-clover build/logs/clover.xml

Running phpunit in 5 processes with /home/travis/build/aik099/qa-tools/vendor/bin/phpunit

Configuration read from /home/travis/build/aik099/qa-tools/phpunit.xml.dist

...............................................................  63 / 313 ( 20%)
............................................................... 126 / 313 ( 40%)
............................................................... 189 / 313 ( 60%)
............................................................... 252 / 313 ( 80%)
............................................................... 315 / 313 (100%)
............................................................... 378 / 313 (120%)
.............................................

Time: 8.82 seconds, Memory: 10.75Mb

OK (423 tests, 3211 assertions)

You can easily clone library from here https://github.com/aik099/qa-tools and run paratest on it.

With phpunit:

PHPUnit 3.7.28 by Sebastian Bergmann.

Configuration read from /mnt/hd/home/alex/web/g/mine/qa-tools/phpunit.xml

...............................................................  63 / 373 ( 16%)
............................................................... 126 / 373 ( 33%)
............................................................... 189 / 373 ( 50%)
............................................................... 252 / 373 ( 67%)
............................................................... 315 / 373 ( 84%)
..........................................................

Time: 47.68 seconds, Memory: 297.75Mb

OK (373 tests, 2804 assertions)

Running tests without paratest I'm getting total of 373 tests executed. Coverage report seems to valid though. I'm not using -f parameter.

This seems to be an issue with counting how much tests there are in total.

aik099 commented 10 years ago

Either paratest doesn't display stuff correctly or it really runs more tests/assertions, then it should.

johnpbloch commented 10 years ago

Just as a guess, this looks to me like it's caused by data providers. PHPUnit treats each dataset from a data provider as one test and paratest only looks at the number of test methods in a class.

aik099 commented 10 years ago

I'm using data providers a lot. But if problem is, that paratest thinks, that there are less tests, then how the total test*data_provider count is larger, than during regular phpunit run?

johnpbloch commented 10 years ago

Yeah, paratest merely counts the number of test methods: https://github.com/brianium/paratest/blob/87aea81277bf54f523f951677e7d11d833ed9f73/src/ParaTest/Parser/Parser.php#L83

PHPUnit has a pretty complex setup for the whole thing, including adding each dataset for a test as a separate test: https://github.com/sebastianbergmann/phpunit/blob/c517984099c588d7cbe04bea9fb756a764769f5f/PHPUnit/Framework/TestSuite.php#L500

aik099 commented 10 years ago

That doesn't explain why assertion count is too high either.

Maybe best approach would be to implement own test runner, based command line test runner, that would:

  1. collect tests based on given files
  2. get counts from them
  3. execute these runners in parallel

Or just process @dataProvider annotation as well manually.

julianseeger commented 9 years ago

I would like to propose a fix for this issue at https://github.com/brianium/paratest/tree/percentage-fix With the fix, every ExecutableTest knows the expected count of testmethods to be contained in the result. If the real results contain more tests than expected, the total count of tests is increased accordingly. The phenomenon you can observe is that the total test count increases while running the dataProviders with an increasing percentage that will never get above 100%.

With https://github.com/aik099/qa-tools that now contains a total of 671 tests, it looks like this: Paratest:

Running phpunit in 10 processes with */vendor/bin/phpunit

Configuration read from */qa-tools/phpunit.xml.dist

...............................................................  63 / 510 ( 12%)
............................................................... 126 / 538 ( 23%)
............................................................... 189 / 556 ( 33%)
............................................................... 252 / 576 ( 43%)
............................................................... 315 / 587 ( 53%)
............................................................... 378 / 602 ( 62%)
............................................................... 441 / 617 ( 71%)
............................................................... 504 / 631 ( 79%)
............................................................... 567 / 644 ( 88%)
............................................................... 630 / 662 ( 95%)
.........................................

Time: 2.64 seconds, Memory: 11.50Mb

OK (671 tests, 5153 assertions)

PHPUnit:

PHPUnit 4.4.1 by Sebastian Bergmann.

Configuration read from */qa-tools/phpunit.xml.dist

...............................................................  63 / 671 (  9%)
............................................................... 126 / 671 ( 18%)
............................................................... 189 / 671 ( 28%)
............................................................... 252 / 671 ( 37%)
............................................................... 315 / 671 ( 46%)
............................................................... 378 / 671 ( 56%)
............................................................... 441 / 671 ( 65%)
............................................................... 504 / 671 ( 75%)
............................................................... 567 / 671 ( 84%)
............................................................... 630 / 671 ( 93%)
.........................................

Time: 3.8 seconds, Memory: 45.50Mb

OK (671 tests, 5153 assertions)

Summary: Cons:

Pros:

Why not execute dataProviders in advance? Executing the dataProviders from paratest provides a different environment than executing them from phpunit. So this might harm the stability and applicability of paratest only in favor of a "better" text output (bad trade in my opinion). Likewise, executing phpunit twice per testcase only to get the correct count up-front will harm the effectiveness of paratest because process spawning is expensive.... again in favor of a better text output. I think this fix is a fair tradeoff between representative progress output and simplicity.

Sorry for the wall of text but I would really like to get feedback on this ;)

aik099 commented 9 years ago

In any case having <100% is better (except for shop discount :smile: ) then we have currently.

brianium commented 9 years ago

This is definitely an improvement. I don't think I am a fan of the idea of executing dataProviders in advance, for pretty much the same reasons @julianseeger mentioned above.

I wonder if we need perfect parity with PHPUnit here on text output? It seems reasonable to not have a total count available for concurrent tests. Would the usefulness of ParaTest be hurt if we removed the right side of the counts and the percentage?

PHPUnit 4.4.1 by Sebastian Bergmann.

Configuration read from */qa-tools/phpunit.xml.dist

...............................................................  63
............................................................... 126
............................................................... 189
............................................................... 252
............................................................... 315
............................................................... 378
............................................................... 441
............................................................... 504
............................................................... 567
............................................................... 630
.........................................

Time: 2.64 seconds, Memory: 11.50Mb

OK (671 tests, 5153 assertions)
julianseeger commented 9 years ago

I wonder if we need perfect parity with PHPUnit here on text output? It seems reasonable to not have a total count available for concurrent tests. Would the usefulness of ParaTest be hurt if we removed the right side of the counts and the percentage?

The total count probably doesn't matter but the percentage seems to be of interest as a measure of progress. In #129 , @grongor even mentioned that he would prefer his own fork (that counts all dataProvider executions of a testmethod as one single test) over the paratest origin. The result is exactly this: correct/usable percentage but completely incorrect total count.

So we could make a tradeoff:

...............................................................  63 ( 12%)
............................................................... 126 ( 23%)
............................................................... 189 ( 33%)
............................................................... 252 ( 43%)
............................................................... 315 ( 53%)
............................................................... 378 ( 62%)
............................................................... 441 ( 71%)
............................................................... 504 ( 79%)
............................................................... 567 ( 88%)
............................................................... 630 ( 95%)
.........................................
brianium commented 9 years ago

I think that is totally acceptable. I certainly don't want to step on any toes if the majority of people using this prefer the 0/0 format - even if the numbers are incorrect.