Update to PHPUnit namespaces #21564

peterrehm · 2017-02-08T07:27:06Z

Q	A
Branch?	2.7+
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#21534
License	MIT
Doc PR	-

Replaces #21540

@nicolas-grekas Is the update of the cache-id like this sufficient? Do we maybe have to specifiy 4.8.35 in simple-phpunit?

peterrehm · 2017-02-08T07:34:32Z

The FC layer is basically restricted to a few classes as you can see here: sebastianbergmann/phpunit@a55235c

PHPUnit_Framework_Assert
PHPUnit_Framework_BaseTestListener
PHPUnit_Framework_TestCase
PHPUnit_Framework_TestListener

In order to fully switch to namespaces we must upgrade to 6.0.

Meanwhile to Bridge will not work with 6.0.

peterrehm · 2017-02-08T08:23:04Z

The PHP 5.3 test fails due to the wrong phpunit version.

stof · 2017-02-08T09:19:15Z

Well, you say you are replacing #21540, but our remarks on it are still valid. We have more public base test cases (and we have some listeners in the PHPUnit bridge)

peterrehm · 2017-02-08T09:45:27Z

@stof Yes. Sorry, it looks like I did not elaborate enough information about your Bridge remark.

The issue is since only the above mentioned 4 classes are in the FC layer, the Bridge cannot be updated to 6.0 as it uses the following classes:

/symfony/src/Symfony/Bridge/PhpUnit$ pt PHPUnit_
bootstrap.php
16:if (!defined('PHPUNIT_COMPOSER_INSTALL') && !class_exists('PHPUnit_TextUI_Command', false)) {

DeprecationErrorHandler.php
54:                return \PHPUnit_Util_ErrorHandler::handleError($type, $msg, $file, $line, $context);
62:            while (1 < $i && (!isset($trace[--$i]['class']) || ('ReflectionMethod' === $trace[$i]['class'] || 0 === strpos($trace[$i]['class'], 'PHPUnit_')))) {
76:                    || in_array('legacy', \PHPUnit_Util_Test::getGroups($class, $method), true)
99:            if (array('PHPUnit_Util_ErrorHandler', 'handleError') === $oldErrorHandler) {

Tests/DeprecationErrorHandler/default.phpt
21:class PHPUnit_Util_Test

TextUI/Command.php
17:class Command extends \PHPUnit_TextUI_Command

Are those the listeners/handlers you meant? I do not see a way other then adding classes to the bridge which should then work like the FC layer here sebastianbergmann/phpunit@a55235c.

peterrehm · 2017-02-08T09:47:47Z

I would think all the other remarks are included in the PR.

stof · 2017-02-08T09:47:53Z

@peterrehm it cannot be switched to namespaces entirely to add compat, I know. But several places can be changed.

And we also have base test cases elsewhere than the KernelTestCase (I know that the Form and Validator components have some)

peterrehm · 2017-02-08T09:49:35Z

How about migrating at least ALL those 4 classes to the namespaced version that the FC layer provides?

stof · 2017-02-08T10:35:37Z

src/Symfony/Bridge/ProxyManager/Tests/LazyProxy/ContainerBuilderTest.php

@@ -13,6 +13,7 @@

 require_once __DIR__.'/Fixtures/includes/foo.php';

+use PhpUnit\Framework\TestCase;


wrong namespace. It is PHPUnit (same in many files, and it breaks on the CI server)

Fixed with the Migration of the Components

peterrehm · 2017-02-08T11:23:12Z

And PHPUnit_Framework_Assert is also migrated.

peterrehm · 2017-02-08T14:19:20Z

I dont get why the PHP 7 tests are failing, on my local machine I do not have any issues.
Any other remarks?

peterrehm · 2017-02-10T07:03:48Z

/ping @symfony-deciders Any remarks? Can we get this merged?

Jean85 · 2017-02-10T10:04:23Z

If this gets merged, due to the new conflicts with the older PHPUnit versions, I can completely rewrite my PR for the PHPUnit bridge too (#21221)

jderusse · 2017-02-10T10:03:59Z

composer.json

@@ -24,6 +24,9 @@
        "twig/twig": "~1.28|~2.0",
        "psr/log": "~1.0"
    },
+    "conflict": {
+        "phpunit/phpunit": "< 4.8.35 || >=5.0 <=5.4.2"


Symfony 2.7 supports php 5.3
This rule prevent people to install a version of phpunit compatible with php < 5.6 alongside Symfony

Should not as it allows 4.8.35?

Indeed, I missed the strict comparison

should written as <4.8.35|<5.4.3,>=5.0 for consistency with what we do elsewhere IMHO

jderusse · 2017-02-10T10:07:20Z

If this PR break unit tests for a supported version, you should find a way to fix it

peterrehm · 2017-02-10T10:10:09Z

@jderusse As mentioned I do not get why they are failing, it might be related to the rebuild of the bridge, the subtree split, etc. Feel free to take over if you know better.

xabbuh · 2017-02-10T18:50:02Z

phpunit

@@ -1,7 +1,7 @@
 #!/usr/bin/env php
 <?php

-// Cache-Id: https://github.com/symfony/phpunit-bridge/commit/4f2aa873d2872a3b9782b25879fd628a0c418bc9
+// Cache-Id: https://github.com/symfony/phpunit-bridge/commit/d3157d942a4590121dfd23f9cadf519ca6ad4ac7


This change is not necessary. The issue with the failing tests is that the PhpUnitBridge itself checks whether or not it needs an update. However, this doesn't work here as the simple-phpunit file is part of the bridge installed as a vendor library.

It still is necessary I think to trigger a phpunit cache invalidation on appveyor.

nicolas-grekas · 2017-02-11T11:30:00Z

I cleared the phpunit cache and restarted tests. There are still a few failures to take care of.

peterrehm · 2017-02-11T12:54:29Z

Hm, when quickly scanning the logs I do not understand the reason for the failures. I see some missing autoloadphp's and some command failures.

Do you have a clue what the reason might be?

nicolas-grekas · 2017-02-11T12:58:14Z

Dunno yet about the autoload failure.
You should be able to reproduce (then debug) this one easily locally, isn't it?
https://travis-ci.org/symfony/symfony/jobs/199575966#L3178

peterrehm · 2017-02-12T08:33:56Z

Actually not at all:

With PHPUnit 5.7.
./phpunit src/Symfony/Bundle/TwigBundle/Tests/Functional/CacheWarmingTest.php
#!/usr/bin/env php
PHPUnit 5.7.12 by Sebastian Bergmann and contributors.

Testing Symfony\Bundle\TwigBundle\Tests\CacheWarmingTest
..                                                                  2 / 2 (100%)

Time: 430 ms, Memory: 14.00MB

With PHPUnit 4.8

./phpunit src/Symfony/Bundle/TwigBundle/Tests/Functional/CacheWarmingTest.php
#!/usr/bin/env php
PHPUnit 4.8.35 by Sebastian Bergmann and contributors.

Testing Symfony\Bundle\TwigBundle\Tests\CacheWarmingTest
..

Time: 456 ms, Memory: 14.00MB

I tried some adjustment, it just makes no sense. Localy the tests pass on PHP7 as well.

peterrehm · 2017-02-12T11:29:08Z

With the latest commits I fixed 5.3 which IMHO makes no sense but who cares.

No there are the issues with 7.x ledt which seem more related with the test setup rather than the actual changes? Is this maybe due to the subtree split?

nicolas-grekas

LGTM - would be great to have a rule on fabbot that ensures that we won't introduce non-namespaced phpunit things anymore thought.

nicolas-grekas · 2017-02-13T09:36:04Z

composer.json

@@ -24,6 +24,9 @@
        "twig/twig": "~1.28|~2.0",
        "psr/log": "~1.0"
    },
+    "conflict": {
+        "phpunit/phpunit": "< 4.8.35 || >=5.0 <=5.4.2"


should written as <4.8.35|<5.4.3,>=5.0 for consistency with what we do elsewhere IMHO

nicolas-grekas · 2017-02-13T09:37:40Z

travis failures are unrelated (known composer bug when too many local packages are created)

peterrehm · 2017-02-13T10:27:19Z

@nicolas-grekas Updated the constraint. For now some PHPUnit_ classes are still needed due to the limited scope of the FC layer.

If we manage to provide a full FC layer we could enforce that. Next would be to update the bridge to full PHP6 compatibility.

nicolas-grekas · 2017-02-13T10:41:20Z

Just did that: grep PHPUnit_ * -r, which leads to two things that remain:
all doc block annotations should be updated to namespaces, even if the BC layer doesn't provide them.
all instanceof/catch checks should be doubled with both namespaced and underscore versions.

Jean85 · 2017-02-13T11:20:32Z

@nicolas-grekas what about the usages of static methods in the bridge, like those?

We don't have an FC layer for that..

peterrehm · 2017-02-13T11:23:00Z

See my attempt here: sebastianbergmann/phpunit#2502 Lets see how it goes.

peterrehm · 2017-02-17T08:31:04Z

@nicolas-grekas Rebased. Once this is merged I would like to check first the 3.x branches then we should care about the bridge. Once we have this done we could look into how to prevent those issues. Does fabbot rely on php-cs-fixer?

Jean85 · 2017-02-17T08:42:33Z

@peterrehm I this get merged, I would like to help with the bridhe.
I'll close and attempt a rewrite to my prev PR (#21221) but I think it'll be a hard think to do... The main issue that I was unable to overcome was the usage of the TestCase in argument typehints, and the use of the FC layer should be helpful!

peterrehm · 2017-02-17T10:25:53Z

@Jean85 Lets discuss once this is done. I think we have to look into providing a FC layer as PHPUnit does.

nicolas-grekas · 2017-02-18T08:35:59Z

👍

xabbuh · 2017-02-18T08:53:00Z

👍 The changes look good. I just don't understand why the build is failing.

peterrehm · 2017-02-18T09:59:36Z

@xabbuh #21564 (comment)

Tobion · 2017-02-18T11:26:50Z

👍

fabpot · 2017-02-18T16:03:03Z

Thank you @peterrehm.

@nicolas-grekas

This PR was squashed before being merged into the 2.7 branch (closes #21564). Discussion ---------- Update to PHPUnit namespaces | Q | A | ------------- | --- | Branch? | 2.7+ | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #21534 | License | MIT | Doc PR | - Replaces #21540 @nicolas-grekas Is the update of the cache-id like this sufficient? Do we maybe have to specifiy 4.8.35 in simple-phpunit? Commits ------- ddd2dff Update to PHPUnit namespaces

nicolas-grekas · 2017-02-18T17:08:09Z

@peterrehm can you please check branch 2.8 now? I just merged 2.7 into 2.8. Thanks for your help.

This PR was squashed before being merged into the 2.8 branch (closes #21663). Discussion ---------- Updated PHPUnit namespaces | Q | A | ------------- | --- | Branch? | 2.8 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Follow Up of #21564 Commits ------- 205ced4 Updated PHPUnit namespaces

This PR was merged into the 2.7 branch. Discussion ---------- Add missing conflict rules for phpunit | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - We forgot them in #21564 Commits ------- 3e83e02 Add missing conflict rules for phpunit

This PR was merged into the 3.2 branch. Discussion ---------- Use PHPUnit 5.4 instead of 5.3 | Q | A | ------------- | --- | Branch? | 3.2 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | PHPUnit 5.3 doesn't have the forward compatibility layer for PHPUnit 6 so that `PHPUnit\Framework\TestCase` can be used instead of `PHPUnit_Framework_TestCase`. This generates an error when upgrading to Symfony 3.2.5 without forcing the `SYMFONY_PHPUNIT_VERSION` const: ``` Class 'PHPUnit\Framework\TestCase' not found in vendor/symfony/symfony/src/Symfony/Bundle/FrameworkBundle/Test/KernelTestCase.php on line 25 ``` This was introduced by c9684ad (from #21564) in 3.2.5 (Discussion started on Slack: https://symfony-devs.slack.com/archives/support/p1489058629011510) Commits ------- fca16ba Use PHPUnit 5.4 instead of 5.3

carsonbot added the Status: Needs Review label Feb 8, 2017

peterrehm mentioned this pull request Feb 8, 2017

Fix invalid parent class reference with PhpUnit 6 #21540

Closed

stof reviewed Feb 8, 2017

View reviewed changes

jderusse reviewed Feb 10, 2017

View reviewed changes

xabbuh reviewed Feb 10, 2017

View reviewed changes

nicolas-grekas approved these changes Feb 13, 2017

View reviewed changes

peterrehm mentioned this pull request Feb 13, 2017

Symfony PHPUnit6 Compatibility sebastianbergmann/phpunit#2502

Closed

peterrehm added 9 commits February 17, 2017 09:28

Updated all bridges to the namespaced TestCase

5aa18fc

Migrated all Bundles to the namespaces TestCase

0d0e886

Migrated Components to namespaced TestCase

bc7ce67

Migrated to namespaced Assert

cfb6107

Try CacheWarmingTest with TwigBundle TestCase instead of the PHPUnit one

fa3de4a

Same for NoTemplatingEntryTest

211c7dc

Updated composer conflict constraint

500cd4b

Added conflict header for the bridge

2fb207b

Updated doc blocks

6b4544e

nicolas-grekas added this to the 2.7 milestone Feb 18, 2017

Tobion self-requested a review February 18, 2017 11:27

Tobion approved these changes Feb 18, 2017

View reviewed changes

fabpot closed this Feb 18, 2017

peterrehm deleted the phpunit-6 branch February 18, 2017 17:32

peterrehm mentioned this pull request Feb 18, 2017

Updated PHPUnit namespaces #21663

Closed

nicolas-grekas mentioned this pull request Feb 20, 2017

Add missing conflict rules for phpunit #21684

Merged

j0k3r mentioned this pull request Mar 9, 2017

Use PHPUnit 5.4 instead of 5.3 #21946

Merged

		@@ -13,6 +13,7 @@

		require_once __DIR__.'/Fixtures/includes/foo.php';

		use PhpUnit\Framework\TestCase;

Update to PHPUnit namespaces #21564

Update to PHPUnit namespaces #21564

Conversation

peterrehm commented Feb 8, 2017

peterrehm commented Feb 8, 2017 • edited Loading

peterrehm commented Feb 8, 2017

stof commented Feb 8, 2017

peterrehm commented Feb 8, 2017

peterrehm commented Feb 8, 2017

stof commented Feb 8, 2017

peterrehm commented Feb 8, 2017

stof Feb 8, 2017

Choose a reason for hiding this comment

peterrehm Feb 8, 2017

Choose a reason for hiding this comment

peterrehm commented Feb 8, 2017

peterrehm commented Feb 8, 2017

peterrehm commented Feb 10, 2017

Jean85 commented Feb 10, 2017

jderusse Feb 10, 2017 • edited Loading

Choose a reason for hiding this comment

peterrehm Feb 10, 2017

Choose a reason for hiding this comment

jderusse Feb 11, 2017

Choose a reason for hiding this comment

nicolas-grekas Feb 13, 2017 • edited Loading

Choose a reason for hiding this comment

jderusse commented Feb 10, 2017

peterrehm commented Feb 10, 2017

xabbuh Feb 10, 2017

Choose a reason for hiding this comment

nicolas-grekas Feb 11, 2017

Choose a reason for hiding this comment

nicolas-grekas commented Feb 11, 2017

peterrehm commented Feb 11, 2017

nicolas-grekas commented Feb 11, 2017

peterrehm commented Feb 12, 2017

peterrehm commented Feb 12, 2017

nicolas-grekas left a comment

Choose a reason for hiding this comment

nicolas-grekas Feb 13, 2017 • edited Loading

Choose a reason for hiding this comment

nicolas-grekas commented Feb 13, 2017

peterrehm commented Feb 13, 2017

nicolas-grekas commented Feb 13, 2017

Jean85 commented Feb 13, 2017

peterrehm commented Feb 13, 2017

peterrehm commented Feb 17, 2017

Jean85 commented Feb 17, 2017

peterrehm commented Feb 17, 2017

nicolas-grekas commented Feb 18, 2017

xabbuh commented Feb 18, 2017

peterrehm commented Feb 18, 2017

Tobion commented Feb 18, 2017

fabpot commented Feb 18, 2017

nicolas-grekas commented Feb 18, 2017

peterrehm commented Feb 8, 2017 •

edited

Loading

jderusse Feb 10, 2017 •

edited

Loading

nicolas-grekas Feb 13, 2017 •

edited

Loading

nicolas-grekas Feb 13, 2017 •

edited

Loading