Using phpunit from the vendors #197

greg0ire · 2016-08-20T12:20:53Z

@sonata-project/contributors, I have big issue with phpunit. The fact that we don't require and treat it as a dependency of our project seems to mean that as soon as we require a library the phpunit phar also bundles, test can fuck up in unexpected ways. It is the case in this PR since I required matthiasnoback/symfony-dependency-injection-test to test the Sonata Admin Symfony extension (which was not covered at all, it seems, by the way). My last commit, where I require and use phpunit provided by Composer fixes this issue. Here is what I get otherwise. This way of doing things has caused issues in the past, and will probably cause more if we continue like this. Can we agree that this is no longer relevant? As an argument against this, doing things like that also makes the travis workflow unnecessarily complicated IMO.
And finally, we were warned against this by Jordi himself, and I really think we should listen to what this guy has to say.

Related issues (each one has been very time-consuming) :

The text was updated successfully, but these errors were encountered:

core23 · 2016-08-20T17:54:46Z

Not sure if I understand the problem correctly... You want to include the phpunit dependency in the composer.json file? Sounds like a good idea, because we can define which version should be used for test classes (PHPunit 4 vs. 5). The latest 5.4 version also spams some deprecation warnings when running tests.

Adding it to composer would also improve autocompletion when writing new tests 👍

greg0ire · 2016-08-20T18:03:34Z

Not sure if I understand the problem correctly...

Yes you did!

You want to include the phpunit dependency in the composer.json file?

Right now if I don't, phpunit can get confused when requiring a class. Should it autoload it from the phpunit phar, or from the project's vendors? This is why I can type phpunit --version, and have another version be displayed when running the tests.

The main argument against using the version from the vendors is that you add some more constraints that can hold you back, because phpunit has many dependencies. Let's have a look at that claim :

cd /tmp 
mkdir test
cd test 
composer require phpunit/phpunit
Using version ^5.5 for phpunit/phpunit
./composer.json has been created
Loading composer repositories with package information
Updating dependencies (including require-dev)
  - Installing symfony/yaml (v3.1.3)
    Loading from cache

  - Installing sebastian/version (2.0.0)
    Loading from cache

  - Installing sebastian/resource-operations (1.0.0)
    Loading from cache

  - Installing sebastian/recursion-context (1.0.2)
    Loading from cache

  - Installing sebastian/object-enumerator (1.0.0)
    Loading from cache

  - Installing sebastian/global-state (1.1.1)
    Loading from cache

  - Installing sebastian/exporter (1.2.2)
    Loading from cache

  - Installing sebastian/environment (1.3.8)
    Loading from cache

  - Installing sebastian/diff (1.4.1)
    Loading from cache

  - Installing sebastian/comparator (1.2.0)
    Loading from cache

  - Installing doctrine/instantiator (1.0.5)
    Loading from cache

  - Installing phpunit/php-text-template (1.2.1)
    Loading from cache

  - Installing phpunit/phpunit-mock-objects (3.2.4)
    Loading from cache

  - Installing phpunit/php-timer (1.0.8)
    Loading from cache

  - Installing phpunit/php-file-iterator (1.4.1)
    Loading from cache

  - Installing sebastian/code-unit-reverse-lookup (1.0.0)
    Loading from cache

  - Installing phpunit/php-token-stream (1.4.8)
    Loading from cache

  - Installing phpunit/php-code-coverage (4.0.1)
    Loading from cache

  - Installing webmozart/assert (1.1.0)
    Loading from cache

  - Installing phpdocumentor/reflection-common (1.0)
    Loading from cache

  - Installing phpdocumentor/type-resolver (0.2)
    Loading from cache

  - Installing phpdocumentor/reflection-docblock (3.1.0)
    Loading from cache

  - Installing phpspec/prophecy (v1.6.1)
    Loading from cache

  - Installing myclabs/deep-copy (1.5.1)
    Loading from cache

  - Installing phpunit/phpunit (5.5.2)
    Loading from cache

sebastian/global-state suggests installing ext-uopz (*)
phpunit/phpunit suggests installing phpunit/php-invoker (~1.1)
Writing lock file
Generating autoload files

So yeah, phpunit has a lot of dependencies, but mainly to other testing libraries, so who cares? Plus, phpunit is actively maintained, and will very unlikely hold us back with its constraints. Look at the symfony/yaml version it installs, for instance! It's the latest.

Adding it to composer would also improve autocompletion when writing new tests

Indeed, I miss it a lot too!

core23 · 2016-08-20T18:19:20Z

The main argument against using the version from the vendors is that you add some more constraints that can hold you back, because phpunit has many dependencies.

Only interesting while development, but who cares? As long as a bundle user won't have to load it 👍

greg0ire · 2016-08-20T18:23:08Z

Only interesting while development, but who cares? As long as a bundle user won't have to load it 👍

Exactly. So on the one hand, we have hypothetical problems about being held back by too many deps, and on the other hand, we have the painfully real problem (happened to me personally several times) that we lose hours debugging the very strange issues this can create.

OskarStark · 2016-08-22T06:18:33Z

...Don't know if we can test this, because we have a hardcoded class in the TestCase.

true

greg0ire · 2016-08-22T06:45:36Z

Wrong issue @OskarStark?

OskarStark · 2016-08-22T06:50:34Z

... that we loose hours debugging the very strange problems this can create.

true ;-)

soullivaneuh · 2016-08-22T07:51:44Z

This issue should solve your problem: sebastianbergmann/phpunit#2015

I have to take a look to you're PR because I already used matthiasnoback/symfony-dependency-injection-test with a global .phar without any issue.

If we can't do anything else, in this case yes we have to including it sebastianbergmann/phpunit#2015. But this should be an exception and temporary.

Plus, you get more risk to require PHPunit locally or globally with composer than the .phar. Why? Simply because you will not get the dependencies versions used and tested for the .phar and take the risk to have a non-working binary.

soullivaneuh · 2016-08-22T07:52:43Z

The main argument against using the version from the vendors is that you add some more constraints that can hold you back, because phpunit has many dependencies.

Only interesting while development, but who cares?

Maybe people already having PHPUnit installation? Plus what I said on #197 (comment).

greg0ire · 2016-08-22T08:20:00Z

I have to take a look to you're PR because I already used matthiasnoback/symfony-dependency-injection-test with a global .phar without any issue.

On which package? I think the results depends on several things : the existing tests, and the environment at least (I had the issue on 5.5 only).

Plus, you get more risk to require PHPunit locally or globally with composer than the .phar. Why? Simply because you will not get the dependencies versions used and tested for the .phar and take the risk to have a non-working binary.

I really don't think this is a big risk, as long as we use stable versions, compared to the strange things that happen with the phar right now. I agree that sebastianbergmann/phpunit#2015 would be best, but we don't have an ETA for that now, do we? So in the meantime, let's use another solution ok? This one has proved several times to cause problems.

core23 · 2016-08-25T15:55:49Z

So? Any decision here?

IMO we should require PHPUnit in every bundle, at least to tell the developers which PHPUnit API (4/5/6?) is used in our bundles right now.

core23 · 2016-08-25T15:58:35Z

This would also solve #197

greg0ire · 2016-08-25T16:03:44Z

This would also solve #197

Circular reference detected

core23 · 2016-08-25T16:41:10Z

Damn, looks like my Greasemonkey scripts are broken :/

#159

greg0ire · 2016-08-25T21:27:25Z

@soullivaneuh : the build is broken on SonataDoctrineOrmBundle, and I don't know how to fix it without doing this : sonata-project/SonataDoctrineORMAdminBundle#616 and without modifying the test. I'll try to fix it by creating dummy classes for now, in another mergeable PR, but this is the kind of situation that we want to avoid.

soullivaneuh · 2016-08-29T14:02:39Z

So? Any decision here?

Keep the .phar, really. With requiring from composer (globally or not), you don't have the same used dependencies as the built version.

If we have very no choice for a special case, in this case we may consider it (after considering all the other options).

I hope sebastianbergmann/phpunit#2015 will be solved soon.

greg0ire · 2016-08-29T14:06:41Z

you don't have the same used dependencies as the built version.

That's the problem IMO : the built version might be buggy, which was exactly the case recently. Being able to blacklist the last version, or rather, blacklist the last version of one dependency b/c it has a bug that affects us is important, even if the phpunit team is pretty quick at fixing bugs.

soullivaneuh · 2016-08-29T14:08:21Z

the built version might be buggy, which was exactly the case recently.

Then report the issue and wait for the fix. I already experience this and when the bug is critical, the fix is released quite quickly.

soullivaneuh · 2016-08-29T14:09:30Z

Having phpunit on requirement would produce more risk than the .phar IMO, like I described in #197 (comment)

greg0ire · 2016-08-29T14:10:47Z

Then report the issue and wait for the fix. I already experience this and when the bug is critical, the fix is released quite quickly.

That's what happened, but it's not quick enough. Every PR is frozen in the meantime.

Bug reported 12 days ago : sebastianbergmann/phpunit-mock-objects#321
Actually fixed 3 days ago.

soullivaneuh · 2016-08-29T14:12:49Z

9 days to have a fix, you think this is slow? Well, this is the open-source rules. If you think it's to slow, propose your own! 😉

greg0ire · 2016-08-29T14:50:26Z

I don't think it is slow, if you ask me it's a pretty quick fix. What I find long is the time during which we are frozen, with no option. One week without being able to merge any PR. Were we using phpunit from the vendors, it would be as simple as

require-dev: {
    "phpunit/phpunit-mock-objects": "!=3.2.4, !=3.2.5"
}

How long do you think this takes to ship?

I hope sebastianbergmann/phpunit#2015 will be solved soon.

Zero activity on this thread since january, what gives you this hope?

Plus, you get more risk to require PHPunit locally or globally with composer than the .phar. Why? Simply because you will not get the dependencies versions used and tested for the .phar and take the risk to have a non-working binary.

By that I think you mean that there is some kind of integration test suite (BTW, do you know if there is actually one ?) that would shield us from defects. Well let's be honest, it didn't shield us from the nasty issue that is discussed here, and it prevents us from getting back to a version of phpunit-mock-objects that works. So while there might be two risks, we're optimizing for the most infrequent one IMO.

soullivaneuh · 2016-08-30T08:50:06Z

By that I think you mean that there is some kind of integration test suite (BTW, do you know if there is actually one ?)

By that, I just think if you have a build with locked dependencies you should use this to be sure to get it working well and properly report bugs if not.

Zero activity on this thread since january, what gives you this hope?

This depends of another issue. And yes it's a hope. I didn't get the question.

I hope to get this because with class prefixes, we will not have any dependencies conflicts anymore.

BTW, if I understood well, the related issue is fixed and released. So no reason to discuss more about that. Let's keep .phar for proper build and use deps for very special case (with issue of when we can rollback.)

Plus, you talk about to get stuck on one PR for 9 days. Well, I think we have a lot of PR on sonata to take care waiting this one, right? 😉

If the current issue is solved, let's close this one for now.

greg0ire · 2016-08-30T08:55:53Z

This was just the biggest out of several issues. The last commit of #4029, sonata-project/SonataAdminBundle@bf3a2d0 makes #4029 work, but you disagree with it, right? I am going to remove it to show you the bug again. I will also rebase this PR on the tip of 3.x, you never know. Also, as @core23 mentioned, there is #159 .

greg0ire · 2016-08-30T09:05:29Z

This depends of another issue. And yes it's a hope. I didn't get the question.

The question is : "What do you base your hope on?" Maybe you could link to that other issue b/c if this is solved tomorrow, then problem of #4029 solved. It would probably not help in case of a buggy release, but I guess if we end up in this kind of case, we can still rollback.

By that, I just think if you have a build with locked dependencies you should use this to be sure to get it working well and properly report bugs if not.

Did you notice that I managed to have #4029 pass (= work well), when it would have been impossible to do so with the locked dependencies?

properly report bugs if not

What? How does not using the .phar prevent you from reporting bugs properly?

greg0ire · 2016-08-30T09:12:38Z

I'm going to file a bug report to phpunit, maybe there is another solution…

greg0ire · 2016-08-30T09:28:08Z

See sebastianbergmann/phpunit#2282

greg0ire · 2016-10-12T17:56:00Z

@soullivaneuh : this won't be fixed, occurs only on the Travis machine it seems…

sstok · 2016-12-18T10:45:39Z

Symfony is suffering from the same kind of issues and solved them

How could I have missed that blog post 🙀 I really need to check my feeds more often...

"Set the SYMFONY_PHPUNIT_REMOVE env var to symfony/yaml if you need prophecy but not symfony/yaml." Nice 😊

soullivaneuh · 2017-01-09T13:42:15Z

@greg0ire Symfony is not talking about phpunit but /vendor/bin/simple-phpunit apparently...

In this case, maybe use this bridge?

On in very last case, if really no solution, yes we can add phpunit as dep for failing project.

But this should be rollback once sebastianbergmann/phpunit#2015 solved. Because the only issue is namespace conflict.

greg0ire · 2017-01-09T13:56:55Z

@greg0ire Symfony is not talking about phpunit but /vendor/bin/simple-phpunit apparently...

I think they build their own version b/c they had a very similar namespace conflict problem with the Yaml component.

I also think this should be reverted when and only when the phpunit runner code is separated from the code you extend in your tests, which you should be able to pick and choose. See this comment. So IMO in the future we should use a phar / and require whichever part of the phpunit framework we are referencing in our code. Tests are part of the code and have dependencies and a such, should absolutely require them (in the require-dev section though)

greg0ire · 2017-01-09T14:02:08Z

@greg0ire Symfony is not talking about phpunit but /vendor/bin/simple-phpunit apparently...

Apparently, they are using them on other projects than Symfony, so maybe it would indeed help. Would have to be tested on the next weird issue we get if require phpunit is such a no-no for you.

soullivaneuh · 2017-01-10T13:11:39Z

Apparently, they are using them on other projects than Symfony, so maybe it would indeed help.

Can be a good alternative IMHO.

I also think this should be reverted when and only when the phpunit runner code is separated from the code you extend in your tests

I'm not sure of what you mean, test class will always extend PHPUnit classes...

greg0ire · 2017-01-10T13:14:10Z

I'm not sure of what you mean, test class will always extend PHPUnit classes...

This means you should always at least require the package where these classes are defined. In the future, the package that defines PHPUnit_Framework_Test_Case should be much smaller and have much fewer dependencies.

greg0ire · 2017-01-26T13:44:37Z

I have a confirmation from the symfony team that simple-phpunit would indeed be the way to go for us.

greg0ire · 2017-02-04T17:30:22Z

@sonata-project/contributors : Here is a POC : sonata-project/SonataAdminBundle#4307 . Please have a look at it.

greg0ire · 2017-02-04T17:36:31Z

Recap of the sf slack :

global phpunit is bad, because of potential autoloading conflicts;
local phpunit is bad too, because the dev env adds constraints to the production env, which it shouldn not;
the ultimate solution is to use php-scoper when it is ready;
until then simple-phpunit is the best solution, because it has none of the problems introduced by 1. or 2.

I think @soullivaneuh wants to set this up only on projects that have a problem, while others might want to do it on all projects, so let's vote on this.

greg0ire · 2017-02-04T17:36:54Z

Solution 1 : switch to simple-phpunit when problem arise on libs

greg0ire · 2017-02-04T17:37:08Z

Solution 2: switch all projects to simple-phpunit

greg0ire · 2017-02-04T17:37:51Z

Solution 3: do nothing and wait for php-scoper.

greg0ire · 2017-02-04T17:39:46Z

I vote 2. b/c I think reacting on the fly is not something we are that good at, because of the nature of dev-kit, and I don't want to wait for an undetermined period of time and bang my head against the wall on weird problems. Also, it might be more simple to implement.

We do not have to care anymore about picking the right phpunit versions, and this should spare us some autoloading issues. Closing sonata-project#197

core23 · 2018-02-03T20:25:23Z

As we don't want to use composer to handle developer tools (e.g. PHPUnit), what about adding the allowed tools to the conflict section inside the composer.json file. So we have no hard dependency on PHPUnit, but we communicate the used version.

greg0ire · 2018-02-03T20:33:50Z

As we don't want to use composer as a required-dev dependency

Sorry, what?

core23 · 2018-02-03T20:38:14Z

Reword the text xD

greg0ire · 2018-02-03T20:40:29Z

Ah ok I get it now! It's a way to communicate it indeed, but it will not "do" anything, will it? Plus this will prevent people using conflicting versions of phpunit or whatever in their project, and we should have zero influence on that.

core23 · 2018-02-03T20:45:15Z

Or we use this: #372

VincentLanglet · 2020-08-15T11:09:18Z

@greg0ire I'll close this since we're now using simple-phpunit. But maybe I'm missing something.

greg0ire · 2020-08-15T11:14:06Z

No it's fine I think :)

greg0ire mentioned this issue Aug 25, 2016

Mock class that exists sonata-project/SonataDoctrineORMAdminBundle#617

Merged

greg0ire mentioned this issue Sep 6, 2016

Deprecate exporter class and service sonata-project/SonataAdminBundle#4029

Merged

7 tasks

greg0ire mentioned this issue Dec 19, 2016

add ‘blockId’ to shared block service. solves #741 sonata-project/SonataPageBundle#755

Merged

core23 mentioned this issue Jan 22, 2017

Removed deprecated getMock in unit tests sonata-project/SonataBlockBundle#357

Merged

greg0ire mentioned this issue Feb 3, 2017

Remove lower PHP versions - clean up build Matrix sonata-project/SonataDoctrinePhpcrAdminBundle#417

Closed

core23 mentioned this issue Feb 3, 2017

Use fixed php version #231

Merged

greg0ire mentioned this issue Jul 27, 2017

Prepare new release sonata-project/SonataDoctrinePhpcrAdminBundle#470

Merged

greg0ire added a commit to greg0ire/dev-kit that referenced this issue Aug 16, 2017

Use simple-phpunit to run the tests

d3636ed

We do not have to care anymore about picking the right phpunit versions, and this should spare us some autoloading issues. Closing sonata-project#197

greg0ire mentioned this issue Aug 16, 2017

WIP: Use simple-phpunit to run the tests #265

Closed

greg0ire added a commit to greg0ire/dev-kit that referenced this issue Aug 16, 2017

Use simple-phpunit to run the tests

70b5771

We do not have to care anymore about picking the right phpunit versions, and this should spare us some autoloading issues. Closing sonata-project#197

greg0ire mentioned this issue Jul 5, 2019

Add missing dev requirement against "phpspec/prophecy" sonata-project/SonataAdminBundle#5617

Closed

VincentLanglet closed this as completed Aug 15, 2020

Using phpunit from the vendors #197

Using phpunit from the vendors #197

Comments

greg0ire commented Aug 20, 2016

core23 commented Aug 20, 2016 • edited by greg0ire

greg0ire commented Aug 20, 2016 • edited

core23 commented Aug 20, 2016

greg0ire commented Aug 20, 2016 • edited

OskarStark commented Aug 22, 2016

greg0ire commented Aug 22, 2016

OskarStark commented Aug 22, 2016 • edited

soullivaneuh commented Aug 22, 2016

soullivaneuh commented Aug 22, 2016

greg0ire commented Aug 22, 2016 • edited

core23 commented Aug 25, 2016

core23 commented Aug 25, 2016

greg0ire commented Aug 25, 2016

core23 commented Aug 25, 2016

greg0ire commented Aug 25, 2016

soullivaneuh commented Aug 29, 2016

greg0ire commented Aug 29, 2016

soullivaneuh commented Aug 29, 2016

soullivaneuh commented Aug 29, 2016

greg0ire commented Aug 29, 2016

soullivaneuh commented Aug 29, 2016

greg0ire commented Aug 29, 2016 • edited

soullivaneuh commented Aug 30, 2016

greg0ire commented Aug 30, 2016 • edited

greg0ire commented Aug 30, 2016

greg0ire commented Aug 30, 2016

greg0ire commented Aug 30, 2016

greg0ire commented Oct 12, 2016

sstok commented Dec 18, 2016 • edited

soullivaneuh commented Jan 9, 2017

greg0ire commented Jan 9, 2017 • edited

greg0ire commented Jan 9, 2017

soullivaneuh commented Jan 10, 2017

greg0ire commented Jan 10, 2017

greg0ire commented Jan 26, 2017

greg0ire commented Feb 4, 2017

greg0ire commented Feb 4, 2017

greg0ire commented Feb 4, 2017

greg0ire commented Feb 4, 2017

greg0ire commented Feb 4, 2017

greg0ire commented Feb 4, 2017

core23 commented Feb 3, 2018 • edited

greg0ire commented Feb 3, 2018

core23 commented Feb 3, 2018

greg0ire commented Feb 3, 2018

core23 commented Feb 3, 2018

VincentLanglet commented Aug 15, 2020

greg0ire commented Aug 15, 2020

core23 commented Aug 20, 2016 •

edited by greg0ire

greg0ire commented Aug 20, 2016 •

edited

greg0ire commented Aug 20, 2016 •

edited

OskarStark commented Aug 22, 2016 •

edited

greg0ire commented Aug 22, 2016 •

edited

greg0ire commented Aug 29, 2016 •

edited

greg0ire commented Aug 30, 2016 •

edited

sstok commented Dec 18, 2016 •

edited

greg0ire commented Jan 9, 2017 •

edited

core23 commented Feb 3, 2018 •

edited