[5.6] Fix duplicate transactions in tests when using RefreshDatabase

[laurencei]

Ok - so bare with me here, there are a few things to explain this.

Current issue: Using the RefreshDatabase trait will cause two database transactions to start in the Illuminate\Foundation\Testing\TestCase file in Laravel 5.6. I have confirmed and can replicate this.

How did it start: it was a unintended side effect of this PR #22596

What was that PR? The offending PR was a code cleanup. It simply re-used the DatabaseTransactions trait inside the RefreshDatabase trait to save on code duplication. At the time it seemed to make sense.

So why does that trigger two transactions? The problem is in the original Illuminate\Foundation\Testing\TestCase file. If you scroll down to around line 100 - you'll see this:

    if (isset($uses[RefreshDatabase::class])) {
        $this->refreshDatabase();
    }
    ...
    if (isset($uses[DatabaseTransactions::class])) {
        $this->beginDatabaseTransaction();
    }

So if you follow the progression - RefreshDatabase will run. Currently inside RefreshDatabase it kicks off its own transaction as the last command.

But now the if (isset($uses[DatabaseTransactions::class])) will now return true (since it is used in RefreshDatabase - kicking off another database transaction.

So what's the solution?

There are realistically two options:

• Revert [5.6] Reuse code from DatabaseTransactions in RefreshDatabase #22596 - but that will leave a fair amount of code duplication
• Simply remove the start of a transaction from RefreshDatabase and let TestCase kick it off itself via DatabaseTransactions.

This PR is the second option.

I've tested locally and it works. @skollro @a-komarev, you both experienced this issue - can you please test yourself and confirm it solves your issue?

p.s. I cant work out how we can add a test to the framework for this to prevent regression? If any test gurus can make a suggestion - that would be good

[laurencei]

Not sure why Travis is failing? It's only failing on PHP7.2 and for a Redis issue? Seems completely unrelated to this PR...

[antonkomarev]

It fixed issue in my test case. And I've checked DatabaseTransactions::beginDatabaseTransaction() still executing.

Thank you @laurencei!

[crynobone]

To be honest, it should just revert the original changes and learn our lesson that we shouldn't extends between testing concerns.

[laurencei]

I thought about that as well @crynobone.

the problem is if we ever made a change to the DatabaseTransaction in the future, we would have to remember to make the same change in RefreshDatabase (which uses the exact same logic).

So I kinda think moving forward is better in this instance for that reason. But either works I guess 🤷‍♂️

[crynobone]

Then move the underlying class to Illuminate\Foundation\Testing\Concerns\DatabaseTransactions.

The simplest matter we can't have any of the traits under setUpTraits() depends on each other.

[mnabialek]

@crynobone I would say the biggest problem here are missing tests. As you see its quite easy to make a change that looks right as I did and all tests will be green but some other issues appear that are quite hard to predict.

Having multiple copies of same code is not a solution in my opinion because finally you forget to update some code in one of those places.

[laurencei]

I'll wait for guidence from Taylor on how he wants to solve it.

I'm happy to rewrite this PR to do it the other way once Taylor says which way he wants to go...

[crynobone]

I would say the biggest problem here are missing tests. As you see its quite easy to make a change that looks right as I did and all tests will be green but some other issues appear that are quite hard to predict.

<?php

namespace Illuminate\Tests\Foundation\Testing;

use PHPUnit\Framework\TestCase;
use Symfony\Component\Finder\Finder;

class SetUpTraitsTest extends TestCase
{
    /** @test */
    public function it_shouldnt_loads_setup_traits_more_than_once()
    {
        $files = [];
        $finders = Finder::create()->files()
                        ->name('*.php')
                        ->notName('HttpException.php')
                        ->notName('TestCase.php')
                        ->notName('TestResponse.php')
                        ->depth(0)
                        ->in(realpath(__DIR__.'/../../../src/Illuminate/Foundation/Testing'));

        foreach ($finders as $file) {
            $files[] = "Illuminate\\Foundation\\Testing\\".rtrim($file->getBasename(), '.php');
        }

        foreach ($files as $file) {
            $uses = class_uses_recursive($file);

            foreach ($uses as $use) {
                $this->assertNotContains($use, $files, "{$file} shouldn't extends {$use}");
            }
        }
    }
}

[skollro]

The proposed PR is a possible solution for the problem.

But if refreshInMemoryDatabase() instead of refreshTestDatabase() is called, now also a transaction is started. Is this intended? In the original version only refreshTestDatabase() started a transaction.

[taylorotwell]

Original PR should be reverted as @crynobone said.

[taylorotwell]

If someone can please do that it would be much appreciated.