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

[5.8] Fix expects output bug #29248

Closed
wants to merge 5 commits into from
Closed

[5.8] Fix expects output bug #29248

wants to merge 5 commits into from

Conversation

paulhenri-l
Copy link
Contributor

@paulhenri-l paulhenri-l commented Jul 21, 2019

Hey,

I found a bit of time to implement the afterApplicationDestroyed callback system that I talked about in #29246.

So this pr solves #29246.

I am not sure about the place where I call the afterApplicationDestroyed callbacks, maybe they could be called a bit earlier?

If you want to see the code in action it is used in the https://github.com/paulhenri-l/laravel-command-test-bug repository on the test-fix branch.

@taylorotwell taylorotwell requested a review from themsaid July 21, 2019 13:54
@paulhenri-l
Copy link
Contributor Author

Well obviously, this does bring some issues as-well :D

I'll look into that.

It also appears that the fix is not that simple, because a mockery exception is thrown when calling Mockery::close(); preventing Laravel from displaying its custom error message.

So the original bug is gone, but it brings a new one.

Maybe using some try/catch/finally could help us here.

@paulhenri-l
Copy link
Contributor Author

Using a try/catch/finally helps a bit.

It allows us to display the Laravel custom error message.

But since Mockery::close() now gets called we get both Laravel custom error message and mockery one.

I'll push the changes I've made with the try/finnaly thing and look if I can find a way to destroy the mock before Mockery::close() gets called.

@driesvints driesvints changed the title Fix expects output bug [5.8] Fix expects output bug Jul 21, 2019
@paulhenri-l
Copy link
Contributor Author

I finally made it work :)

So to sum it up.

  • afterApplicationDestroyedCallbacks have been added
  • console output mocks now do not verify their expectations
  • whatever happens the test suite should properly tearDown

Any call to the this->fail() method should be done in an afterApplicationDestroyed callback and not in a beforeApplicationDestroyed one. This allows test traits to be properly reset between tests.

As the InteractsWithConsole trait is taking care of calling the this->fail() method itself and is using mocks only in order to execute custom behavior when can safely tell mockery to not verify them. This will prevent mockery from displaying errors that Laravel have already displayed by calling this->fails()

Given that the this->fails() method call throws an exception under the hood the try/finally makes sure that the test environment is properly teared down.

Broken tests

Well now I guess I'll have to look at why the test broke

@paulhenri-l
Copy link
Contributor Author

Hum, regarding the broken tests it looks like a pr should also be made on orchestra/testbench.

So lets just see here if this solution fits everyone and if it does we'll get into making a pr on orchestra/testbench.

@GrahamCampbell
Copy link
Member

Thanks for the PR. I'm wondering if Laravel 5.9 would be a better place for this. We don't want to break existing funky test code, relying on the existing setup.

@GrahamCampbell
Copy link
Member

Hum, regarding the broken tests it looks like a pr should also be made on orchestra/testbench.

Yes, definitely Laravel 5.9 will be the right place to make these changes then. :)

@GrahamCampbell
Copy link
Member

GrahamCampbell commented Jul 21, 2019

Please send a PR to the master branch of this repo, and the 3.9 branch of testbench.

@paulhenri-l paulhenri-l deleted the fix-expects-output-bug branch July 22, 2019 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants