Fix renamed file loader in phpunit #127

lsimeonov · 2018-02-28T08:12:58Z

It was renamed from PHPUnit\Util\Fileloader to PHPUnit\Util\FileLoader in sebastianbergmann/phpunit@3a9494b#diff-7862e6eec2d5923e62d57728b79a0480

It was renamed from `Fileloader` to `FileLoader`

Some case sensitive issues is not allowing to add class alias. Added phpunit ^7.0 tests om php version 7.2 Another run with `-x` on travis, as there was a problem with autoloading `\PHPUnit\Util\FileLoader` only if through phpunit.xml file

DonCallisto · 2018-02-28T10:09:07Z

src/Queue/CreateTestsQueueFromPhpUnitXML.php

@@ -61,6 +63,10 @@ private static function handleBootstrap(array $config)
    {
        $filename = isset($config['bootstrap']) ? $config['bootstrap'] : 'vendor/autoload.php';

-        \PHPUnit\Util\Fileloader::checkAndLoad($filename);
+        if (class_exists('\PHPUnit\Util\Fileloader')) {


Could you please threat this if as it is at line 18? Just add the alias as well and use it as before

There is a problem with that.

Adding:

if (class_exists('\PHPUnit\Util\Fileloader')) { class_alias('\PHPUnit\Util\Fileloader', '\PHPUnit\Util\FileLoader'); }

is resulting in odd behaviour on phpunit < 7.0 , class_exists will trigger autoload on Fileloader which at least on MacOs (not sure if OS case insensitive problem), is also thinking FileLoader is there thus class_alias('\PHPUnit\Util\Fileloader', '\PHPUnit\Util\FileLoader'); would throw

Cannot declare class \PHPUnit\Util\FileLoader, because the name is already in use in reateTestsQueueFromPhpUnitXML.php on line 23

You can see the failing tests on the firs commit in the PR.

And if you run class_exists(\PHPUnit\Util\FileLoader) it will return false.
So the only way to fix this problem was to actually check what is loaded before call.

Strangely enough class_exists will behave normally on cli piping. Didn't had time to dig deep into the issue and find what exactly is happening.

I'll be glad to help if you need more information.

Just saying you should put it on top of the class, like other alias, just for having them all in the same place.
BTW the problem you're facing is probably due to the fact that mac OSX (if you didn't specify a different fashion) run with case insensitive filesystem: this could lead you to this kind of troubles.

I suppose that, also, we should not include it as fastest problem but a filesystem one.
So, even travis file should not be modified, right?

I isolated the problem so you can take a look at it. I also tested on Windows and it seems it is not case sensitive problem. It would be PHP internal behaviour.
Take a look at this repo and run the test to see what the problem is.

We can remove the added ./fastest -x phpunit.xml.dist -v "vendor/phpunit/phpunit/phpunit {};" but then you can never test the class aliasing and see if they are working. Probably isolating the aliased classes in separate tests for different phpunit versions, not sure.

Still don't understand why ./fastest -x phpunit.xml.dist -v "vendor/phpunit/phpunit/phpunit {};" won't test the alias.
I mean, if we still run phpunit 7, we should fallback to alias with camel case whereas with phpunit 6 we should fallback to alias with non-camel case.
I didn't tried it, did you?

Ok, got the problem even for the command: it's due to composer autoload that is case sensitive so we have two different issues: PHP is case insensitive so we can't define at the top the alias whereas composer is case sensitive so we need to run the command explicitly

I'm still checking for a solution, but I'm starting to think that's the only one

Exactly the problem finally cleared out also for me! Thanks for your patience.

Another solution that comes to mind is to just trigger the autoload with two class_exists on both cases. Then the command will execute without problem no matter camel case or not as composer will load the file

@lsimeonov I don't thinks it's gonna work. Only solution is to let all the code be bootstrapped by composer, like symfony does (for instance). This is the only way to be able to try to load both, but if you wanna try with both, go ahead and le me know.

PS.: I'm gonna merge this one at the end of this week end so feel free to try - as well as I will - other solutions 'till that time. For other point of this discussion, I suppose it's better to separate them into single "issue" and continue there; I'm gonna do this either

DonCallisto · 2018-02-28T10:10:05Z

.travis.yml

    fi
  - composer update $PREFER_LOWEST

 script:
  - find tests/ -name "*Test.php" | php fastest -v
+  - ./fastest -x phpunit.xml.dist -v "vendor/phpunit/phpunit/phpunit {};"


Is this really needed? I mean, line 43 should run the same tests: isn't it?

I added it because the test behave differently in terms of autoloading when running trough pipping and running it with phpunit.xml.
The old travisci way would pass for phpunit ^7.0 as it won't trigger into the autoload problem I described on the previous comment and where fastest is failing with phpunit ^7.0

Of course the best way to do it is just include it in the tests which I can't do at the moment.
We need to cover all the nuances of missing classes and aliases.

DonCallisto · 2018-03-01T18:17:07Z

Ok, this seems to be the only way to fix this, I didn't find a better solution.
Just to more things, I would like to tag recently active collaborators like @tarlepp and @francoispluchino to have four more eyes on it before merging.

Moreover it would be great to start a discussion about this kind of BC that we want to keep: it's fantastic but, as we provide support for behat and phpunit, maybe we should have different major releases for majors of these libraries, with end of support for them, when them gonna stop support.

Moreover we should start thinking about "splitting" the project in three parts:

a core one (agnostic about tool that's used)
a phpunit one
a behat one

This way we could have more freedom for changes. Still don't know if core is "contaminated" with PHPUnit concepts though

Just random thoughts, let me know what do you guys think.

…or phpunit 7.0 ProcessorCounter now has new class for getting the OS so we can mock it. Tests were failing on MacOS

lsimeonov · 2018-03-04T11:05:35Z

Added more elegant way of handling case sensitive problem between PHP classes and composer autoloader.
I also fixed a failing test where we don't mock the PHP_OS.

DonCallisto · 2018-03-04T11:29:45Z

Ok, I'm gonna merge this.
TBH we should extend those tests even for other cases (don't know if it's easy to test the else branch actually) but this should not be included in this PR so it's perfectly fine to merge this.

Thank you for great effort!

DonCallisto · 2018-03-04T11:40:07Z

@lsimeonov here the release https://github.com/liuggio/fastest/releases/tag/v1.6.1 thank you again

lsimeonov added 2 commits February 28, 2018 09:57

Use latest file loader from phpunit 7.0.

7f9bae6

It was renamed from `Fileloader` to `FileLoader`

DonCallisto requested changes Feb 28, 2018

View reviewed changes

DonCallisto approved these changes Mar 1, 2018

View reviewed changes

Different handling on autloading for different cases in file loader f…

c553a36

…or phpunit 7.0 ProcessorCounter now has new class for getting the OS so we can mock it. Tests were failing on MacOS

DonCallisto merged commit f61c3b0 into liuggio:master Mar 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix renamed file loader in phpunit #127

Fix renamed file loader in phpunit #127

lsimeonov commented Feb 28, 2018

DonCallisto Feb 28, 2018

lsimeonov Feb 28, 2018 •

edited

Loading

DonCallisto Feb 28, 2018

lsimeonov Mar 1, 2018 •

edited

Loading

DonCallisto Mar 1, 2018

DonCallisto Mar 1, 2018

DonCallisto Mar 1, 2018

lsimeonov Mar 1, 2018

lsimeonov Mar 1, 2018

DonCallisto Mar 2, 2018

DonCallisto Feb 28, 2018

lsimeonov Feb 28, 2018

lsimeonov Feb 28, 2018

DonCallisto commented Mar 1, 2018 •

edited

Loading

lsimeonov commented Mar 4, 2018

DonCallisto commented Mar 4, 2018

DonCallisto commented Mar 4, 2018 •

edited

Loading

Fix renamed file loader in phpunit #127

Fix renamed file loader in phpunit #127

Conversation

lsimeonov commented Feb 28, 2018

Choose a reason for hiding this comment

lsimeonov Feb 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lsimeonov Mar 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DonCallisto commented Mar 1, 2018 • edited Loading

lsimeonov commented Mar 4, 2018

DonCallisto commented Mar 4, 2018

DonCallisto commented Mar 4, 2018 • edited Loading

lsimeonov Feb 28, 2018 •

edited

Loading

lsimeonov Mar 1, 2018 •

edited

Loading

DonCallisto commented Mar 1, 2018 •

edited

Loading

DonCallisto commented Mar 4, 2018 •

edited

Loading