Fix expectsOutput bug

[paulhenri-l]

Hey there!

In order to fix this issue: laravel/framework#29246

I've made a PR on the laravel/framework repository: laravel/framework#29252

As it is changing code in the TestCase class I need to replicate the changes here as-well in order to make the tests pass.

So as requested by @GrahamCampbell I'am opening this PR.

[coveralls]

Coverage decreased (-1.3%) to 86.517% when pulling aa26717 on paulhenri-l:fix-expects-output-bug into 2ab439a on orchestral:3.9.

[crynobone]

I would only merge this if @taylorotwell accept the PR on laravel/framework

[GrahamCampbell]

I would only merge this if @taylorotwell accept the PR on laravel/framework

Yeh. I think acceptance needs to be mutual, or not at all. :)

[crynobone]

I don't get why we need two nested try, surely one is enough since outside of if ($this->app) condition we are only unsetting the values.

[paulhenri-l]

The nested tries are from @GrahamCampbell he can correct me if I am wrong.

As far as I understand the idea is that if in the same tearDown

• a beforeApplicationDestroyed callback throws the app will still get flushed.
• an afterApplicationDestroyedCallback throws the variables will still get reset.

My initial fix had only the outter try/finally but it makes the assumption that beforeApplicationDestroyed callbacks won't throw.

While being a bit clunky the nested try/finally seems to really add value here. Or maybe there is another way?

[crynobone]

I would just use rescue() for this TBH.

if ($this->app) {
    foreach ($this->beforeApplicationDestroyedCallbacks as $callback) {
        \rescue($callback, null, false);
    }
    
    $this->app->flush();
    $this->app = null;

    foreach ($this->afterApplicationDestroyedCallbacks as $callback) {
        \rescue($callback, null, false);
    }
}

cc @GrahamCampbell

[paulhenri-l]

Oh I didn't know about this helper.

Well then I guess using rescue is definitely a better option.

I'll make the changes.

[paulhenri-l]

Ahh actually we can't use rescue here.

calls to the this->fails() method of phpunit throws. If we use rescue then phpunit won't be able to display these errors.

I guess we can call the beforeApplicationDestroyedCallbacks with rescue and the afterApplicationDestroyedCallbacks without it.

If a test trait needs to call $this->fail() it will need to do so in an afterApplicationDestroyed callback.

I'll check if laravel's provided test trait do so and update them accordingly if we go that route.

Maybe @GrahamCampbell as another idea?

[crynobone]

Please resubmit PR with changes based on accepted PR to laravel/framework

[paulhenri-l]

Yep I'll do that