Starting recommending to run PHPUnit directly by weaverryan · Pull Request #906 · symfony/recipes

weaverryan · 2021-02-27T16:13:04Z

Q	A
License	MIT
Doc issue/PR	TODO - important

Hi!

This is tricky, and probably controversial. My goal is to make running PHPUnit in Symfony projects "not special". This means that users can run ./vendor/bin/phpunit like any other project (though, we will keep bin/phpunit as a shortcut and smooth mechanism to not break docs, etc).

My understanding (please correct me if I'm wrong) is that as long as users have <listener class="Symfony\Bridge\PhpUnit\SymfonyTestsListener"/> in their phpunit.xml.dist file, all of the features of phpunit-bridge are available to them. The one exception is the ability to run your tests against multiple PHPUnit versions, which is not a need that many projects have.

On a high level, here is how the new system would work:

When you composer require test --dev, you actually download symfony/test-pack (this is already the case). In Adding phpunit directly to the pack test-pack#10, I've added phpunit/phpunit. This means you'll get both symfony/phpunit-bridge and phpunit/phpunit.
When the recipes are executed, the symfony/phpunit-bridge runs first. This gives the user the phpunit.xml.dist file from phpunit-bridge, which is good because that has the <listener class="Symfony\Bridge\PhpUnit\SymfonyTestsListener"/> already in it.
At this point, the user can run ./vendor/bin/phpunit directly. But, of course, they could also choose to continue executing simple-phpunit directly. But, as a shortcut (and to make this change smoother), the bin/phpunit file still exists, but now executes PHPUnit directly if it's installed directly.

Details

A) The experience has been optimized for the "main" user that just wants to run PHPUnit directly. In the phpunit.xml.dist file from the phpunit-bridge recipe, we have 2 spots where we favor config that assumes you're running things directly.

Cheers!

…ctly

ghost

Pull request passes validation.

weaverryan · 2021-02-27T16:13:59Z

+    <extensions>
+        <extension class="Symfony\Component\Panther\ServerExtension" />
+    </extensions>
+    -->


This is just to make this file consistent with the phpunit.xml.dist file from the phpunit-bridge recipe, in case someone installs phpunit directly (and not via the pack, where you will get the phpunit.xml.dist file from phpunit-bridge)

A nice addition would be to have a PHPUnit configurator in Flex.

weaverryan · 2021-02-27T16:14:27Z

+#!/usr/bin/env php
+<?php
+if (file_exists(dirname(__DIR__).'/vendor/phpunit/phpunit/phpunit')) {
+    require(dirname(__DIR__).'/vendor/phpunit/phpunit/phpunit');


This is the new part: if PHPUnit is install directly, use it.

I tried adding this to my bin/phpunit file and I'm getting

PHP Fatal error: strict_types declaration must be the very first statement in the script

Did you try without strict type declaration?
This file don't include any.

Already fixed in #952

Already fixed in #952

Indeed, thanks

kbond · 2021-02-27T16:25:10Z

I recently switched a large (4.4) app over to using paratest (with excellent results btw). Since paratest (and I believe we want to start promoting this) requires phpunit I switched to requiring phpunit directly in my app without issue.

wouterj · 2021-02-27T16:41:24Z

👍 I like this approach! I'm not sure what my approval triggers on flex recipes, so I'll leave this as a normal comment for now :)

A couple important takeaways from this PR imho:

The simple way is now targeted at application developers, rather than library maintainers.
This removes Symfony being "special with PHPunit". This allows removing some docs about PHPunit, and also allows referencing official PHPunit docs and blog posts.
This opens up using many community packages build for/on-top-of PHPunit in a Symfony application, such as Paratest, Collision or PestPHP.
The "old way" is still supported (vendor/bin/simple-test), so no feature is lost with this change

wouterj · 2021-02-27T16:43:08Z

Btw, I'm going to work on a testing docs rewrite soon (see symfony/symfony-docs#14731). It would be great to coordinate merging this together with the doc changes, so less resources are spent and a bigger "announcement" can be made.

javiereguiluz · 2021-02-27T17:19:22Z

In my opinion, this hits the sweet spot: it allows to use standard PHPUnit practices while enjoying all the cool features of Symfony's PHPUnit Bridge. A big 👍 form me. Thanks Ryan!

Nyholm · 2021-03-02T16:27:28Z

This PR is marked as "[WIP]", what is the current blocker?

nicolas-grekas · 2021-03-23T23:02:29Z

I'm certainly not happy with this PR, because requiring phpunit/phpunit as a direct dep is conceptually so wrong. Sebastian Bergmann himself discourages doing so. Yet everybody happily does it and 🙈 with the issues it creates. Dependency hell is the first and major one, but since the majority doesn't use --no-dev when installing deps on prod servers, it also means runtime overhead to load functions declared by phpunit et al., exposition to possible unserialize()+destructor exploits, etc.

I don't have the will to block this move, because I also understand ppl that complain about simple-phpunit - it's not a perfect solution either. I think the tradeoffs it offers are worth it, but if the majority doesn't agree, let's merge.

👍 from the purely technical aspect for the patch.

nicolas-grekas · 2021-03-23T23:06:47Z

For the record, see composer/composer#9636, which looks for a better solution to these issues. No ready, so not a blocker for this PR.

Nyholm · 2021-03-23T23:11:11Z

I do see your point and I agree that this is an issue that should be solved. But it is not a "symfony issue", it is more an issue of how the php community is using tools like phpunit.

I think symfony would benefit to do "like everybody else" in this case, even though it is a technically worse solution.

nicolas-grekas · 2021-03-24T12:13:43Z

See also composer/composer#9792

ghost

Pull request passes validation.

weaverryan · 2021-03-24T14:36:31Z

Here's the plan:

A) Merge this recipe. It applies to phpunit-bridge 5.3 only.

B) When 5.3 comes out, THEN merge and tag adding phpunit/phpunit to test-pack symfony/test-pack#10

The timing on (B) is just to reduce (but definitely not eliminate, think 4.4. users) the number of people who would install test-pack but not get the new recipe. This recipe updates bin/phpunit to use phpunit/phpunit directly. So if you don't get this recipe, then you will still be using phpunit-bridge, which is fine... except that it could cause some class versioning issues where phpunit-bridge downloads one version of PHPUnit, then the user's app has another version autoloading from their own vendor/.

I have just tested this recipe, both with and without phpunit/phpunit directly installed.

wouterj · 2021-03-24T14:48:58Z

The timing on (B) is just to reduce (but definitely not eliminate, think 4.4. users) the number of people who would install test-pack but not get the new recipe.

Note that we unpack packs by default. So you would need to install a fresh test-pack in a <5.3 application to get this situation afaics

weaverryan · 2021-03-24T15:32:24Z

I'm coordinating with Wouter to make sure the docs will be ready for 5.3

weaverryan added 3 commits February 27, 2021 10:38

Making phpunit direct recipe match phpunit-bridge

053077a

removing the recommendation when installing phpunit directly

59f0b39

Adding phpunit-bridge 5.3 recipe, which recommends using PHPUnit dire…

1a88b62

…ctly

ghost deployed to staging February 27, 2021 16:13 View deployment

ghost approved these changes Feb 27, 2021

View reviewed changes

weaverryan commented Feb 27, 2021

View reviewed changes

weaverryan mentioned this pull request Feb 27, 2021

Adding phpunit directly to the pack symfony/test-pack#10

Merged

noniagriconomie reviewed Mar 11, 2021

View reviewed changes

Comment thread symfony/phpunit-bridge/5.3/phpunit.xml.dist

weaverryan changed the title ~~[WIP] Starting recommending to run PHPUnit directly~~ Starting recommending to run PHPUnit directly Mar 24, 2021

ghost deployed to staging March 24, 2021 14:34 View deployment

ghost approved these changes Mar 24, 2021

View reviewed changes

nicolas-grekas approved these changes Mar 24, 2021

View reviewed changes

Nyholm approved these changes Mar 24, 2021

View reviewed changes

ghost merged commit 43bdac2 into symfony:master Mar 24, 2021

weaverryan deleted the phpunit-updates branch March 24, 2021 15:29

VincentLanglet mentioned this pull request Jun 6, 2021

Use both phpunit/phpunit and simple-phpunit. sonata-project/dev-kit#1559

Closed

Tjeerd mentioned this pull request Jun 9, 2021

Prefer phpunit over symfony bridge ibuildingsnl/flex-recipes#15

Merged

franmomu mentioned this pull request Oct 8, 2021

Fix tests deprecations KnpLabs/KnpMenu#354

Merged

VincentLanglet mentioned this pull request Oct 16, 2021

3.35.0 sonata-project/SonataMediaBundle#2167

Merged

alexander-schranz mentioned this pull request Dec 10, 2021

Upgrade sulu skeleton with symfony recipe defaults sulu/skeleton#150

Merged

wouterj mentioned this pull request Apr 12, 2025

Add PHPUnit 11 recipe, treat PHPUnitBridge only as a PHPUnit extension #1401

Merged

This pull request was closed.

Uh oh!

Conversation

weaverryan commented Feb 27, 2021

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

weaverryan Feb 27, 2021

Choose a reason for hiding this comment

Uh oh!

dunglas Mar 1, 2021

Choose a reason for hiding this comment

Uh oh!

weaverryan Feb 27, 2021

Choose a reason for hiding this comment

Uh oh!

VincentLanglet Jun 3, 2021

Choose a reason for hiding this comment

Uh oh!

Nyholm Jun 3, 2021

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas Jun 3, 2021

Choose a reason for hiding this comment

Uh oh!

VincentLanglet Jun 3, 2021

Choose a reason for hiding this comment

Uh oh!

kbond commented Feb 27, 2021

Uh oh!

wouterj commented Feb 27, 2021

Uh oh!

wouterj commented Feb 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

javiereguiluz commented Feb 27, 2021

Uh oh!

Nyholm commented Mar 2, 2021

Uh oh!

Uh oh!

nicolas-grekas commented Mar 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolas-grekas commented Mar 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Nyholm commented Mar 23, 2021

Uh oh!

nicolas-grekas commented Mar 24, 2021

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

weaverryan commented Mar 24, 2021

Uh oh!

wouterj commented Mar 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weaverryan commented Mar 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

wouterj commented Feb 27, 2021 •

edited

Loading

nicolas-grekas commented Mar 23, 2021 •

edited

Loading

nicolas-grekas commented Mar 23, 2021 •

edited

Loading

wouterj commented Mar 24, 2021 •

edited

Loading