Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[WIP][PHPUnitBridge] Cross compatibility between PHPUnit 5 and 6#21158
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Jean85 commentedJan 4, 2017
Status: needs work |
| namespaceSymfony\Bridge\PhpUnit\Compat\Framework; | ||
| if (class_exists(PHPUnit\Framework\BaseTestListener::class)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this is a no-go. the bridge must stay compatible with PHP 5.3 to be usable in the Symfony LTS CI setup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Good catch! Already fixed, the Travis build reported this.
composer.json Outdated
| "psr-4": { | ||
| "Symfony\\Bridge\\Doctrine\\":"src/Symfony/Bridge/Doctrine/", | ||
| "Symfony\\Bridge\\Monolog\\":"src/Symfony/Bridge/Monolog/", | ||
| "Symfony\\Bridge\\PhpUnit\\Compat\\":"src/Symfony/Bridge/PhpUnit/Compat/", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
-1 for this. The PHPUnit bridge is not meant to be autoloaded from the main Symfony repo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm not autoloading the whole bridge but just the Compat classes, otherwise this will not work. Do you have any suggestions for an alternate approach?
[EDIT] I've done this here since the normal autoload was failing during the SecurityBundle tests in the build. Maybe it's just a build issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
These classes must be autoloaded by the bridge only. Anything else is wrong.
A failure may be related to cases mixing a downloaded version of the bridge and your patched version here. But I have not seen the failure you are talking about. The only build failures on Travis on the commit before this one are about broken signatures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You're right, we have to load that just in the bridge. I've reverted that but the build is failing:https://travis-ci.org/Jean85/symfony/jobs/188845916#L1983
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
but ths has nothing to do with SecurityBundle. It is a failure when testing the bridge itself
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yeah, you're right. In fact, the PR build is green, great!https://travis-ci.org/symfony/symfony/builds/188846004
| namespaceSymfony\Bridge\PhpUnit\Compat\Framework; | ||
| if (class_exists(PHPUnit\Framework\BaseTestListener::class)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
and if it was kept, there is a missing\ here, meaning you check forSymfony\Bridge\PhpUnit\Compat\Framework\PHPUnit\Framework\BaseTestListener
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Good catch! Already fixed, the Travis build reported this too.
| class Blacklistextends \PHPUnit\Util\Blacklist | ||
| {} | ||
| }else { | ||
| class Blacklistextends \PHPUnit_Util_Blacklist |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
there is no reason to create this class. We need compat classes only when extending them. Otherwise, we can do the class_exists check around the usage of the class, which is much more readable
| $skipped =array(); | ||
| foreach ($suite->tests()as$test) { | ||
| if (!$testinstanceof \PHPUnit_Framework_TestCase | ||
| if (!$this->isInstanceOfPHPUnitTestSuite($test) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
method name is weird, as the check was for TestCase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Good catch! Thanks, fixing it
| namespaceSymfony\Bridge\PhpUnit\Compat\Framework; | ||
| if (class_exists(PHPUnit\Framework\BaseTestListener::class)) { | ||
| abstractclass BaseTestListenerextends \PHPUnit\Framework\BaseTestListener |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this class (and all other classes here) must be tagged as@internal so that there are not covered by BC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Right! Doing it RN
Jean85 commentedJan 4, 2017
Ok, the build is green, but still I don't know how to proceed for the class in typehints... |
…or the PHPUnit Bridge
a0d8870 todbd94a2CompareJean85 commentedJan 9, 2017
Closing in favor of#21221 |
Still WIP... I have doubt on how to solve (preserving BC) the case where the target class that I want to replace is a public method's argument typehint, like in
SymfonyTestsListener::addSkippedTest:symfony/src/Symfony/Bridge/PhpUnit/SymfonyTestsListener.php
Line 132 in12a4cfe