[WIP][PHPUnitBridge] Cross compatibility between PHPUnit 5 and 6 #21158

Jean85 · 2017-01-04T11:35:41Z

Q	A
Branch?	master
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#21125
License	MIT

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 in 12a4cfe

    
           public function addSkippedTest(\PHPUnit_Framework_Test $test, \Exception $e, $time)

Jean85 · 2017-01-04T11:36:38Z

Status: needs work

stof · 2017-01-04T11:45:07Z

src/Symfony/Bridge/PhpUnit/Compat/Framework/BaseTestListener.php

+
+namespace Symfony\Bridge\PhpUnit\Compat\Framework;
+
+if (class_exists(PHPUnit\Framework\BaseTestListener::class)) {


this is a no-go. the bridge must stay compatible with PHP 5.3 to be usable in the Symfony LTS CI setup.

Good catch! Already fixed, the Travis build reported this.

stof · 2017-01-04T11:46:06Z

composer.json

@@ -103,6 +103,7 @@
        "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/",


-1 for this. The PHPUnit bridge is not meant to be autoloaded from the main Symfony repo

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?

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.

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

but ths has nothing to do with SecurityBundle. It is a failure when testing the bridge itself

Yeah, you're right. In fact, the PR build is green, great! https://travis-ci.org/symfony/symfony/builds/188846004

stof · 2017-01-04T11:46:52Z

src/Symfony/Bridge/PhpUnit/Compat/Framework/BaseTestListener.php

+
+namespace Symfony\Bridge\PhpUnit\Compat\Framework;
+
+if (class_exists(PHPUnit\Framework\BaseTestListener::class)) {


and if it was kept, there is a missing \ here, meaning you check for Symfony\Bridge\PhpUnit\Compat\Framework\PHPUnit\Framework\BaseTestListener

Good catch! Already fixed, the Travis build reported this too.

stof · 2017-01-04T11:48:01Z

src/Symfony/Bridge/PhpUnit/Compat/Util/Blacklist.php

+    class Blacklist extends \PHPUnit\Util\Blacklist
+    {}
+} else {
+    class Blacklist extends \PHPUnit_Util_Blacklist


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

stof · 2017-01-04T11:49:45Z

src/Symfony/Bridge/PhpUnit/SymfonyTestsListener.php

@@ -119,7 +128,7 @@ public function startTestSuite(\PHPUnit_Framework_TestSuite $suite)
        } elseif (2 === $this->state) {
            $skipped = array();
            foreach ($suite->tests() as $test) {
-                if (!$test instanceof \PHPUnit_Framework_TestCase
+                if (!$this->isInstanceOfPHPUnitTestSuite($test)


method name is weird, as the check was for TestCase

Good catch! Thanks, fixing it

stof · 2017-01-04T11:52:22Z

src/Symfony/Bridge/PhpUnit/Compat/Framework/BaseTestListener.php

+namespace Symfony\Bridge\PhpUnit\Compat\Framework;
+
+if (class_exists(PHPUnit\Framework\BaseTestListener::class)) {
+    abstract class BaseTestListener extends \PHPUnit\Framework\BaseTestListener


this class (and all other classes here) must be tagged as @internal so that there are not covered by BC.

Right! Doing it RN

Jean85 · 2017-01-04T14:03:51Z

Ok, the build is green, but still I don't know how to proceed for the class in typehints...
I've suggested an approach here: #21125 (comment)

…or the PHPUnit Bridge

Jean85 · 2017-01-09T18:53:53Z

Closing in favor of #21221

carsonbot added Status: Needs Review PhpUnitBridge Bug labels Jan 4, 2017

carsonbot added Status: Needs Work and removed Status: Needs Review labels Jan 4, 2017

Jean85 mentioned this pull request Jan 4, 2017

[PhpUnitBridge] not compatible with latest phpunit #21125

Closed

stof requested changes Jan 4, 2017

View reviewed changes

nicolas-grekas added this to the 3.x milestone Jan 6, 2017

Jean85 and others added 11 commits January 9, 2017 14:52

Implement compat classes to have cross-compat between PHPUnit 5 & 6 f…

a085a0e

…or the PHPUnit Bridge

Add compat classes to PSR-4 autoloader

1a56153

Revert wrong edit of typehint

abb1f91

Avoid ::class to maintain PHP 5.3 compat

a75694e

Fix wrong method as per review

22ca4c0

Mark all compat classes as @internal to avoid BC policy

73946f5

Fix

5065e87

Revert composer.json

c5a77d5

Change approach to conditional use statements; ditch Compat classes

61f68a1

Fix argument typehint

e43f288

Fix new usages after rebase

dbd94a2

Jean85 force-pushed the phpunit_6_bridge_compat branch from a0d8870 to dbd94a2 Compare January 9, 2017 13:57

Jean85 mentioned this pull request Jan 9, 2017

[PHPUnitBridge] Cross compatibility between PHPUnit 5 and 6 #21221

Closed

Jean85 closed this Jan 9, 2017

nicolas-grekas modified the milestones: 3.x, 3.3 Mar 24, 2017


		namespace Symfony\Bridge\PhpUnit\Compat\Framework;

		if (class_exists(PHPUnit\Framework\BaseTestListener::class)) {

Uh oh!

[WIP][PHPUnitBridge] Cross compatibility between PHPUnit 5 and 6 #21158

[WIP][PHPUnitBridge] Cross compatibility between PHPUnit 5 and 6 #21158

Uh oh!

Conversation

Jean85 commented Jan 4, 2017

Uh oh!

Jean85 commented Jan 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jean85 Jan 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jean85 commented Jan 4, 2017

Uh oh!

Jean85 commented Jan 9, 2017

Uh oh!

Uh oh!

Jean85 Jan 4, 2017 •

edited

Loading