New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixed up the phpunit config #3725
Conversation
Can you describe more what was going on and what this fixes? |
I personally see only cosmetic changes there. He just added suffix for test files which is useful when you have some non-test file there. 👍 |
@taylorotwell @shehi I think this fixes laravel/framework#10808 |
@scrubmx Can't be! The cause for that problem can't be THIS SIMPLE! /runs to test it UPDATE: You know what's weird? I don't know who did what, but right now, all my tests run smoothly with PHPUnit-5, without this change. I don't know how. UPDATE-2: How part = I don't have routes.php files in my app, it's all inside RouteServiceProvider. That's why I stopped receiving these errors. |
Not at all!
Because our routes.php file is not PSR-4 compliant, we can't safely "require" it. PHPUnit's code coverage will attempt to require all files as if they have no "consequences", but our routes file will execute code when you require it. That fact means that when PHPUnit goes through the stages of checking for uncovered files, it will require the routes file directly, thus causing this exception. NB This error will only happen if your test suite doesn't require the routes file itself at one point. So, if you don't have any tests that boot the routing layer, then you'll see the bug, or, if you ran a subset of your tests that don't load the routes file, you'll see this error. |
<whitelist processUncoveredFilesFromWhitelist="true"> | ||
<directory suffix=".php">./app</directory> | ||
<exclude> | ||
<file>./app/Http/routes.php</file> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the really important change - it is NOT cosmetic - it stops phpunit requiring this file when searching for uncovered files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I retract my statement of it being cosmetic :D LOL, thanks for awesome solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we just exclude ./app/Http/routes.php
file without introducing other changes (we could make that the new default on 5.3)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have Laravel-5.2 based code (AudithSoftworks/Basis) and I always have
it up-to-date with the latest Laravel. I update it to the letter, as far
as I can, unless those updates are not necessary due to my own
modifications.
On 01-04-2016 08:03, Joseph Silber wrote:
In phpunit.xml
#3725 (comment):</testsuite> </testsuites> <filter>
<whitelist>
<directory suffix=".php">app/</directory>
<whitelist processUncoveredFilesFromWhitelist="true">
<directory suffix=".php">./app</directory>
<exclude>
Why are we so concerned about breaking BC in the app skeleton? Do<file>./app/Http/routes.php</file>
people actually update their existing apps? How?—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/laravel/laravel/pull/3725/files/28ea52d10b69b5cdf6fe72fe2acb5594022e1a8e#r58162725
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd recommend adding some kind of XML comment into that file, notifying
users. We have comments everywhere, why not in this file as well?
On 01-04-2016 07:02, Mior Muhammad Zaki wrote:
In phpunit.xml
#3725 (comment):</testsuite> </testsuites> <filter>
<whitelist>
<directory suffix=".php">app/</directory>
<whitelist processUncoveredFilesFromWhitelist="true">
<directory suffix=".php">./app</directory>
<exclude>
<file>./app/Http/routes.php</file>
Shouldn't we just exclude |./app/Http/routes.php| file without
introducing other changes (we could make that the new default on 5.3)—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/laravel/laravel/pull/3725/files/28ea52d10b69b5cdf6fe72fe2acb5594022e1a8e#r58160091
I still get exactly the same error after updating the config file. I am running phpunit 4.8.2, php 7.0 and Laravel 5.2. Is there a step I am missing? I have run composer dump-autoload & php artisan clear-compiled etc. |
There might be other files you need to exclude too that you've added. |
Thats actually a very good point, thanks for the tip. |
What error are y'all even talking about? |
I am still working on my phpunit.xml processUncoveredFilesFromWhitelist whitelist, which should fix this error that I get when running phpunit. I think its related to this closed issue: laravel/framework#10808 PHPUnit 4.8.24 by Sebastian Bergmann and contributors. Runtime: PHP 7.0.3-13+deb.sury.org~trusty+1 with Xdebug 2.4.0RC4 ........... 11 / 11 (100%) Time: 16.74 seconds, Memory: 28.00Mb PHP Fatal error: Uncaught Illuminate\Contracts\Container\BindingResolutionException: Target [Illuminate\Contracts\Debug\ExceptionHandler] is not instantiable. in /home/vagrant/Code/bootstrap/cache/compiled.php:1349 Fatal error: Uncaught Illuminate\Contracts\Container\BindingResolutionException: Target [Illuminate\Contracts\Debug\ExceptionHandler] is not instantiable. in /home/vagrant/Code/bootstrap/cache/compiled.php on line 1349 Illuminate\Contracts\Container\BindingResolutionException: Target [Illuminate\Contracts\Debug\ExceptionHandler] is not instantiable. in /home/vagrant/Code/bootstrap/cache/compiled.php on line 1349 Call Stack: |
@taylorotwell Don't you remember the case when PHPUnit 5 wasn't working in Laravel 5? Well, @GrahamCampbell and the team solved that issue, apparently. @StephenWattbike , this fix definitely solves the problem we had specifically with PHPUnit 5 (not 4). Not sure if your case is related though. I never had any issues with PHPUnit 4 even when I had routes.php in place, or any other file for that matter. I'd recommend profiling your code and check for every included, required file during the test run. |
This fixes that closed issue. |
You'll need to add any other files to the whilelist excludes that are of the same nature as routes.php. |
Not doing so is not a bug in laravel, it's just misconfiguration on you part I'm afraid. |
So I should use phpunit 5 to avoid this issue altogether? I think @GrahamCampbell said that only phpunit version 4.8 was supported which is why I'm running that version. I will run 5 if I can avoid this whitelisting which may work for a blank project but its becoming painful for me. I will continue to try the whitelist with 4.8 but I still get the error after excluding my own additions, and other combinations to get it to work. Sorry to drop in this thread I am trying my best to get past this issue. |
Both are broken. |
Well, neither are broken as long as you configure them correctly... |
So long as code coverage uncovered file searching has been enabled, you have always needed this to prevent this from happening, in all versions. |
Is it possible this is a breaking change at all? |
@@ -10,12 +10,15 @@ | |||
stopOnFailure="false"> | |||
<testsuites> | |||
<testsuite name="Application Test Suite"> | |||
<directory>./tests/</directory> | |||
<directory suffix="Test.php">./tests</directory> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary to have this changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That one is not necessary, but is more useful.
I don't think it can be a breaking change. Noone would go and copy paste On 01-04-2016 06:17, Taylor Otwell wrote:
|
Nothing made to laravel/laravel is breaking really, like your web middleware changes. |
very strange: if i use:
I do not get an error anymore, I got the idea from this stackoverflow:
Also I removed the filter stuff from the XML and run ./vendor/bin/phpunit without errors. |
The problem with changes on patch release is that when people upgrade from say |
Why would you use a piece of code (in this case, the system's phpunit) On 01-04-2016 13:14, Stephen Wattbike wrote:
|
Agreed, that's why I recommended adding inline XML comments. I still am On 01-04-2016 13:19, Mior Muhammad Zaki wrote:
|
It's hard to debug and control the flow as you debug, which is the Also, wasn't it [!CDATA[ ... ]] blah blah the correct way of commenting in first place). It's been while since I read XML-Bible, like 6 years On 01-04-2016 13:32, Stephen Wattbike wrote:
|
vagrant@homestead:~/Code/vendor/bin$ ./phpunit --version vagrant@homestead:~/Code$ phpunit --version When I run phpunit from the project root its using my "system" phpunit |
I didn't suggest the other way around. Command "phpunit" uses On 01-04-2016 14:28, Stephen Wattbike wrote:
|
I still seem to be getting this error. Excluded all stand-alone files in the exclude list. Running PHPUnit 4.8.24. |
@driesvints Tested this on a fresh install of Laravel 5.2, and received this error when running the No idea why this works, but that's for smarter people than me to figure out. |
@derrekbertrand it doesn't really matter if you use global or local phpunit but it's a matter of which version you're using I believe. Can you run the following two commands and paste the output here?
|
@driesvints It may be the case, or it could not... Checking the version of the global binary yields 5.1, but actually running it - it thinks it is 4.8? That's the version Laravel installed! This is some chicanery, but nothing that actually has to do with Laravel. As you can see, I can reproduce the error every time, however, phpunit may be lying about its version or misconfigured somehow. Check this out:
|
@derrekbertrand that's odd. Although you've confirmed that the problem happens when running at least PHPUnit |
@driesvints Breaks like you'd expect, and gets confused about its version.
|
@derrekbertrand super weird :-| Don't know what's going on here. |
I had the same issue. (Using Laravel 5.2) Solutions:
Hope somebody finds this useful. |
Or just don't globally require PHPUnit. |
Fixed up the phpunit config
@GrahamCampbell Is this no longer an issue because it was reverted in 288e361? I would have instead expected the path to be updated to something like: <exclude>
<directory>./routes</directory>
</exclude> I only ask because I'm trying to investigate why code coverage reports are throwing exceptions again in php 7.2 for laravel tests. It doesn't seem to be related to the routes files, but I just thought I'd clarify this particular issue along the way. Thanks |
The routes directory is outside of PHP 7.2.1
|
We'll need to update the docs to let people know about this.