Conversation
Codecov Report
@@ Coverage Diff @@
## master #10257 +/- ##
============================================
+ Coverage 94.88% 94.92% +0.03%
- Complexity 12067 12100 +33
============================================
Files 422 421 -1
Lines 29959 30040 +81
============================================
+ Hits 28427 28514 +87
+ Misses 1532 1526 -6
Continue to review full report at Codecov.
|
|
I think the proposed warning makes sense 👍 |
src/TestSuite/TestSuite.php
Outdated
| trigger_error(sprintf('Your PHPUnit Version must be at least 5.7.0 to use CakePHP Testsuite, found %s', \PHPUnit_Runner_Version::id()), E_USER_ERROR); | ||
| } | ||
| class_alias('PHPUnit_Framework_TestSuite', 'PHPUnit\Framework\TestSuite'); | ||
|
|
There was a problem hiding this comment.
Blank line found at end of control structure
antograssiot
force-pushed
the
phpunit
branch
from
60a1d08 to
9b7c7e8
Compare
|
Ok I've added the warning and hopefully fix the issue users have. @markstory Please don't merge this just yet, I'll try to get feedback from #10260 |
| trigger_error(sprintf('Your PHPUnit Version must be at least 5.7.0 to use CakePHP Testsuite, found %s', \PHPUnit_Runner_Version::id()), E_USER_ERROR); | ||
| } | ||
| class_alias('PHPUnit_Framework_Test', 'PHPUnit\Framework\Test'); | ||
| class_alias('PHPUnit_Framework_Warning', 'PHPUnit\Framework\Warning'); |
There was a problem hiding this comment.
Why don't these have class_exists() calls?
There was a problem hiding this comment.
@markstory my reasoning was the following:
If a old PHPUNIT version class is found, I need to define aliases
If I need to define the same aliases in several places, I check if it was not done already.
There was a problem hiding this comment.
Using the PHPUnit_Framework_Version class also determines if PHPUnit <6 is present at all and only defines those aliases in this case
| if (!class_exists('PHPUnit\Framework\TestSuite')) { | ||
| class_alias('PHPUnit_Framework_TestSuite', 'PHPUnit\Framework\TestSuite'); | ||
| } | ||
| if (class_exists('PHPUnit_Runner_Version') && !class_exists('PHPUnit\Framework\AssertionFailedError')) { |
There was a problem hiding this comment.
Why does one depend on the other?
There was a problem hiding this comment.
I can remove the first check it is already nested in
|
Great job @antograssiot |
5 participants
Another attempt to support PHPUnit without forcing file loading for every request.
I tested it with composer installation and .phar for PHPUnit 5.7 and 6 but I would appreciate if one can quickly check one of is app using this branch to confirm it does work properly before it (cc @dereuromark )
I added a "conflict" definition in the
composer.jsonto alert composer users.Finally, what fo you think of adding the following code inside the ifs:
I think it can help people to better understand what's going on than getting an standard error that could looks like