Using phpunit from the vendors

[greg0ire]

@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) :

• https://github.com/matthiasnoback/SymfonyDependencyInjectionTest/issues/48
• https://github.com/sebastianbergmann/phpunit/issues/1757

[core23]

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]

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]

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]

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

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]

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

true

[greg0ire]

Wrong issue @OskarStark?

[OskarStark]

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

true ;-)

[soullivaneuh]

This issue should solve your problem: https://github.com/sebastianbergmann/phpunit/issues/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 https://github.com/sebastianbergmann/phpunit/issues/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]

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 https://github.com/sonata-project/dev-kit/issues/197#issuecomment-241338004.

[greg0ire]

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]

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]

This would also solve https://github.com/sonata-project/dev-kit/issues/197

[greg0ire]

This would also solve #197

Circular reference detected

[core23]

Damn, looks like my Greasemonkey scripts are broken :/

https://github.com/sonata-project/dev-kit/issues/159

[greg0ire]

@Soullivaneuh : the build is broken on SonataDoctrineOrmBundle, and I don't know how to fix it without doing this : https://github.com/sonata-project/SonataDoctrineORMAdminBundle/pull/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]

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 https://github.com/sebastianbergmann/phpunit/issues/2015 will be solved soon.

[greg0ire]

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]

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]

Having phpunit on requirement would produce more risk than the .phar IMO, like I described in https://github.com/sonata-project/dev-kit/issues/197#issuecomment-241338004

[greg0ire]

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 : https://github.com/sebastianbergmann/phpunit-mock-objects/issues/321 Actually fixed 3 days ago.

[soullivaneuh]

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! :wink:

[greg0ire]

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]

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? :wink:

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

[greg0ire]

This was just the biggest out of several issues. The last commit of #4029, https://github.com/sonata-project/SonataAdminBundle/pull/4029/commits/bf3a2d0d1ff383efa1be07a6364055edaaf85894 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]

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]

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

[greg0ire]

See https://github.com/sebastianbergmann/phpunit/issues/2282

[greg0ire]

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

[greg0ire]

Should I skip this test on php 5.5 ?

[soullivaneuh]

If you could skip only this case, yes.

[greg0ire]

Ok, I'll try that!

[greg0ire]

Wait I remember now that it's not even my test that is failing… another test is failing b/c phpunit is required in another version by the test library I use in my test. If I comment that test out, I think another one will fail, and so on. What should I do?

[OskarStark]

What should I do?

pray? 😄 Sorry for sarcasm.

Hmm, not really an idea

[greg0ire]

Lol I know what I should do : change @Soullivaneuh's mind. But it's hard!

[core23]

Same probleme here... https://github.com/sonata-project/SonataMediaBundle/pull/1107

[greg0ire]

@Soullivaneuh : see? This has to stop, it's getting hard to contribute. How many problems do you think we are going to have to fix because of this?

[core23]

Let's start a vote for phpunit @sonata-project/contributors

👍 for adding phpunit as a composer dependency 👎 for using the system phpunit version

[greg0ire]

Yet another build that suspiciously fails for php 5.5 only cc @pix-art

[greg0ire]

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

[sstok]

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]

@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 https://github.com/sebastianbergmann/phpunit/issues/2015 solved. Because the only issue is namespace conflict.

[greg0ire]

@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]

@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]

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]

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]

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

[greg0ire]

@sonata-project/contributors : Here is a POC : https://github.com/sonata-project/SonataAdminBundle/pull/4307 . Please have a look at it.

[greg0ire]

Recap of the sf slack :

1. global phpunit is bad, because of potential autoloading conflicts;
2. local phpunit is bad too, because the dev env adds constraints to the production env, which it shouldn not;
3. the ultimate solution is to use php-scoper when it is ready;
4. 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]

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

[greg0ire]

Solution 2: switch all projects to simple-phpunit