TransactionStartException will always be throwed in tests with RefreshDatabase trait

[ibrunotome]

Describe your task
The test suite in laravel are always executed inside a database transaction if you're using the LazilyRefreshDatabase trait. After v8.2, all tests that has a wallet transaction are broken

To Reproduce
Steps to reproduce the behavior:

1. write an action that has a app(DatabaseServiceInterface::class)->transaction.
2. write a test with LazilyRefreshDatabase trait.

Expected behavior
The test will broke with "Bavix\Wallet\Internal\Exceptions\TransactionStartException : Working inside an embedded transaction is not possible", but this transaction was started by the framework itself, not in userland.

Server:

• php version: 8.1
• database: postgres 14
• wallet version 8.2
• cache lock: redis
• cache wallets: redis

[rez1dent3]

Hello. Prior to laravel-wallet 8.2, there was no such error, but the tests did not work correctly either. The fact is that the wallet stores the state inside, and upon completion of the transaction, it updates the data. This is necessary to improve performance. I'll think of some testing tools that might help.

More details can be read here: #412 #455 https://bavix.github.io/laravel-wallet/#/transaction

[rez1dent3]

@ibrunotome Hello. Today I took the time and immersed myself in your problem. The error is correct, it says that it is not possible to start the package's internal transaction, since the framework transaction has already been opened above, as a result of which there will be an error in updating the data.
Right now, the only thing that can be done is to close the transaction in setUp.

laravel-wallet/tests/Infra/TestCase.php

Lines 22 to 28 in 88913b5

	use RefreshDatabase;
	protected function setUp(): void
	{
	parent::setUp();
	DB::transactionLevel() && DB::rollBack();
	}

It may not be convenient, but this is a correct check of the package. The fact is that at the end of the transaction, the package updates the data in the cache, sends events and does a bunch of other useful things that are not in the framework transactions. It's a compromise between performance and convenience.

[ibrunotome]

Tested, the error remains the same after rollback in setup:

The test is written with pest, but I think that this doesn't affect anything, right? Or I'm missing something here?

[rez1dent3]

This package is tested with phpunit, RefreshDatabase and it works. Can you make an example?
@ibrunotome

[ibrunotome]

You mean create a new repo or just post some examples like below?

TestCase.php

<?php

namespace Tests;

use Illuminate\Foundation\Testing\LazilyRefreshDatabase;
use Illuminate\Foundation\Testing\TestCase as BaseTestCase;
use Illuminate\Support\Facades\DB;

abstract class TestCase extends BaseTestCase
{
    use CreatesApplication;
    use LazilyRefreshDatabase;

    protected function setUp(): void
    {
        parent::setUp();
        DB::transactionLevel() && DB::rollBack();
    }
}

Pest.php

<?php

/*
|--------------------------------------------------------------------------
| Test Case
|--------------------------------------------------------------------------
|
| The closure you provide to your test functions is always bound to a specific PHPUnit test
| case class. By default, that class is "PHPUnit\Framework\TestCase". Of course, you may
| need to change it using the "uses()" function to bind a different classes or traits.
|
*/

uses(Tests\TestCase::class)->in(__DIR__);

TestExample.php (the usage of package transaction occurs in RateContractAsSatisfiedAction)

<?php

use App\Actions\Quests\RateContractAsSatisfiedAction;
use App\Enums\QuestContractStatus;
use App\Models\Quest;
use App\Models\User;

it('successfully rate quest contract as super satisfied', function () {
    /** @var \App\Models\Quest $quest */
    $quest = Quest::factory()->create();

    /** @var User $user */
    $user = User::factory()->create();

    $user->tasks()->attach($quest->id, [
        'days_to_rate' => $quest->days_to_rate,
        'workers_earn' => $quest->workers_earn,
        'completed_at' => now(),
    ]);

    app(RateContractAsSatisfiedAction::class)->execute($quest, [
        'bonus'         => '0.10',
        'contracts'     => [
            $user->contracts->first()->id,
        ],
    ]);

    $contract = $user->refresh()->contracts->first();

    $amountEarnedWithBonus = bcadd($quest->workers_earn, '0.10', 8);
    expect($contract->status)->toBe(QuestContractStatus::SuperSatisfied);
    expect(number_format($contract->worker_earned, 8))->toBe($amountEarnedWithBonus);
});

[rez1dent3]

@ibrunotome Well thank you. In the near future I will try to make a project with Pest.

We have an overly twisted example with a bunch of services that I don't have. It would be nice to get a clean example from you, without wrappers.

[ibrunotome]

Here is an example: https://github.com/ibrunotome/laravel-wallet-transaction-exception-example

[rez1dent3]

@ibrunotome hello. I found the problem. Thanks for your example, very detailed. The problem is precisely in the trait LazilyRefreshDatabase, which is what you said. If you replace it with Illuminate\Foundation\Testing\RefreshDatabase in your project, then everything starts working.

The problem is that the transaction starts inside the Factory when using this trait and it does not finish its work. This can be checked like this:

    dump(\Illuminate\Support\Facades\DB::transactionLevel()); // 0
    /** @var User $user */
    $user = User::factory()->create();
    dd(\Illuminate\Support\Facades\DB::transactionLevel()); // 1

[rez1dent3]

If you really need to use LazilyRefreshDatabase, then you will have to apply the commit after the Factory.

    $user = User::factory()->create();
    \Illuminate\Support\Facades\DB::commit();

I don't have any other solution.

[rst630]

I have the same issue in tests, Im using only RefreshDatabase trait

problem only with 8.2 version, on production we have 8.1.1 and it passes all tests

[rez1dent3]

@rst630 As I described above, the problem is in the tests. 8.2 added an exception that tells you the error. You need to fix the tests. I am sure that at some point you start a transaction. And inside the transaction, the package will not work correctly. This error tells you about it and will not let you shoot yourself in the foot.

[ibrunotome]

Using commit strategy after factory creation started to give me wrong results after the first test (e.g: 3 tests in the same file)

The same happens with RefreshDatabase trait (without DB::commit exactly like your test case) instead of LazyRefreshDatabase

I will try to make an example in that repo like I did before.

[rez1dent3]

@ibrunotome Let me know when you complete the example.

[rez1dent3]

@ibrunotome Your tests are passing successfully. Submitted a pull request ibrunotome/laravel-wallet-transaction-exception-example#1

[ibrunotome]

@ibrunotome Let me know when you complete the example.

I found my problem, one of my factories had:

'user_id'  => User::exists() ? User::first() : User::factory()

So, because with this rollback:

abstract class TestCase extends BaseTestCase
{
    use CreatesApplication;
    use RefreshDatabase;

    protected function setUp(): void
    {
        parent::setUp();
        DB::transactionLevel() && DB::rollBack();
    }
}

The data in database was never erased.

Using commit strategy after factory creation started to give me wrong results after the first test (e.g: 3 tests in the same file)

That's why the tests was failing after 1st successful test, the second test was taking the user created in the first test and continued to use that balance.

So now I think that this is another problem, the tests are not isolated anymore after this suggestion of DB::rollback, do you agree? Since the results of one tests are still available to the next test. An example:

Scenario with normal laravel behavior, without DB::transactionLevel() && DB::rollBack(); in setUp method

• the first test creates 20 categories and access a /categories endpoint and assert that there are 20 categories listed.
• the second test doesn't created any category and access a /categories endpoint and assert that will not list any category.

Scenario with DB::transactionLevel() && DB::rollBack(); in setUp method

• the first test creates 20 categories and access a /categories endpoint and assert that there are 20 categories listed.
• the second test doesn't created any category and access a /categories endpoint and assert that there are no categories, but the test will fail because there are 20 categories from the previous test that will be listed.

To fix this I use a truncate query in tearDown method, this increase my tests execution time from 180s to 268s 😢

Hope we can talk about this to discute if there is a better approach.

Thank you.

[rez1dent3]

@ibrunotome In the old version, your tests were faster, because there were not a lot of requests to the database to complete the transaction. Your tests were not correct, from the point of view of the package. With the new approach, you really started testing the package, not just the internal state of the package. You can speed up only by parallelizing, there are no other options.

[ibrunotome]

@rez1dent3 ok, but about the example I did in last comment, about the remaining data from first test to another, don't you agree that the data must be isolated from on test to another like before?

[rez1dent3]

@ibrunotome Tests should be written in isolation by themselves. If there is only one base, and there is only one here, then the test cannot be isolated. or you need to wrap the entire test in batch transactions.

[ibrunotome]

Good article if someone get into this issue someday: https://www.code-distortion.net/books/fast-test-databases/