[5.9] Fix expects output bug

[paulhenri-l]

As discussed in #29248

Here's the PR targeting Laravel 5.9 instead of 5.8

This PR is fixing #29246

I'm also going to open a pr on orchestral/testbench in order to make the tests pass.

---

Steps to reproduce

The simplest way to reproduce it is to add this test into a brand new laravel application

laravel new expose-bug
cd expose-bug
php artisan make:test ExposeBugCommand

Paste this inside of the tests/Feature/ExposeBugCommand.php file.

<?php

namespace Tests\Feature;

use Illuminate\Support\Facades\Artisan;
use Tests\TestCase;

class ExposeBugCommandTest extends TestCase
{
    protected function setUp(): void
    {
        parent::setUp();

        Artisan::command('hello', function () {
            $this->info('hello world');
        });
    }

    public function test_hello_says_hola_mundo()
    {
        $this->artisan('hello')
            ->expectsOutput('hola mundo');
    }

    public function test_hello_says_hello_world()
    {
        $this->artisan('hello')
            ->expectsOutput('hello world');
    }
}

phpunit

Explanations

By simply looking at this test you can see that the first test method should fail and the second one pass.

But if you run phpunit oddly both tests fails (1 failure and 1 exception)

Now if you run phpunit --filter test_hello_says_hello_world you'll see that running only the working test works.

What I have discovered is that InteractsWithConsole when used with expectsOutput is calling this->fail() inside of its beforeApplicationDestroyedCallback as this->fails() throws it is breaking the execution flow of the tearDown method and preventing it from properly tearing down the test suite.

Inside of the tearDown method there is a call to Mockery::close() but because of the this->fail() exception this line is never reached.

On subsequent tests Mockery::close() gets called and throws an error related to the previous test.

Notifications

If there are other test traits using this->fail() in their callbacks then they are also preventing the test suite from properly tearing down and should be fixed. I can do that.

PRs

We've been discussing the fix with @GrahamCampbell and @crynobone inside of these pr:

• [5.8] Fix expects output bug #29248
• [5.9] Fix expects output bug #29252
• Fix expectsOutput bug orchestral/testbench-core#25

[paulhenri-l]

Here's a link to the orchestral/testbench-core pr: orchestral/testbench-core#25

[paulhenri-l]

@GrahamCampbell I've replicated your commit in orchestral/testbench-core#25 😉

[themsaid]

Check #29267 ... it works for me, do you see any reason why my PR wouldn't work?

[paulhenri-l]

@themsaid This can’t work it really is missing the afterApplicationDestroyed callbacks.

When using InteractsWithConsole alongside DatabaseTransactions the fail() call will prevent the DatabaseTransactions's beforeApplicationDestroyed callback from being called.

If the callback is not executed the transaction is never rolled back thus locking the db.

Whatever happens we need to let every beforeApplicationDestroy perform its tear down.

I can remove the once() calls though it is definitely better than using the _ignore_verification

[themsaid]

@paulhenri-l database transaction callback run before any other callbacks, can you show me a test where this lock happens for you?

[themsaid]

@paulhenri-l I'm closing this since the other approach won't require changes in orchestral/testbench, the less changes the better. However, I'd like to fix the issue so can you please help me replicate the databases test?

[paulhenri-l]

@themsaid Yes sure thing, the bug only happened at work though.

I'll do my best to replicate it

I'll keep you updated in the other pr.