Skip to content
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

[6.x] Delete cached config file before running tests #5091

Merged
merged 3 commits into from Sep 6, 2019

Conversation

@SjorsO
Copy link
Contributor

commented Sep 5, 2019

I ran into the problem that the bootstrapping of the unit tests crashed because the cached config file (bootstrap/cache/phpunit.config.php) contained a reference to a class that didn't exist anymore.

This problem happened to me when I removed a package that I had manually registered in my config/app.php. I think this can also happen when you rename classes.

Steps to reproduce:

  1. On a fresh Laravel install, install a package that you have to manually register in your config/app.php
    • eg: composer require clarification/sparkpost-laravel-driver
    • manually register the provider: Clarification\MailDrivers\Sparkpost\SparkpostServiceProvider::class
  2. Run the unit tests
    • bootstrap/cache/phpunit.config.php now contains the FQCN of the package
  3. Remove the provider from config/app.php and remove the package (composer remove clarification/sparkpost-laravel-driver)
  4. Try running the unit tests again

You should see the following error:

In Application.php line 671:
                                                                                  
  Class 'Clarification\MailDrivers\Sparkpost\SparkpostServiceProvider' not found  

I'm not very happy with this solution, but it works. Maybe someone has a better solution to this problem.

Also, this same bug might also exist with the other cached files.

@taylorotwell

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

Is that server variable always going to be set? If it's not we will get an error there.

tests/bootstrap.php Outdated Show resolved Hide resolved
SjorsO added 2 commits Sep 6, 2019
@SjorsO

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2019

I've included @timacdonald's suggestion on how to delete the files. Now we can be sure none of the cached files can give us any trouble.

I've also refactored the bootstrap file as a PHPunit extension, this has the following benefits:

  • We can now reuse the CreatesApplication trait
  • We now delete the cached files after the last tests, instead of before all tests (which means they are never left on the disk, and that IDEs wont try to index them)
  • It gives users a convenient place to put additional PHPunit hooks
@timacdonald

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2019

I think this should probably be 2 PRs so discussion can happen independently for the two different changes, i.e.

  1. to remove the cached files before the test run with....
array_map('unlink', glob('bootstrap/cache/*.phpunit.php'));
  1. Refactoring to the extension.

But while it is here - a couple of thoughts on this approach:

We now delete the cached files after the last tests, instead of before all tests (which means they are never left on the disk, and that IDEs wont try to index them)

This isn't really an issue. The files are .gitignored and before the introduction of this feature, running your test suite would already generate packages.php and services.php.

We will also have to check how this plays with parallel test runners, i.e. that it only runs the extension hooks once per run and not once per thread.

@taylorotwell

This comment has been minimized.

Copy link
Member

commented Sep 6, 2019

So, is this ready to merge?

@SjorsO

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2019

I think this PR can be merged. I'm pretty sure it works just like the old code, and it solves the problem described in this issue.

Like @timacdonald said, i don't know if these hooks work with parallel test runners. But since these hooks are normal phpunit features, i'm not sure if this is something we should worry about.

@taylorotwell taylorotwell merged commit 42936c6 into laravel:master Sep 6, 2019

1 check passed

continuous-integration/styleci/pr The analysis has passed
Details
@timacdonald

This comment has been minimized.

Copy link
Contributor

commented Sep 7, 2019

Looks like ParaTest is hitting some issues due to its multi threaded-ness, but I have a solution that I think can just go in the docs. Will send a follow up PR to the docs about it.

p.s. @SjorsO I dig the extension over the plain bootstrap file. Nice being able to have a specific before and after hooks.

realodix added a commit to realodix/urlhub that referenced this pull request Sep 7, 2019
realodix added a commit to realodix/urlhub that referenced this pull request Sep 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.