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

Add DatabaseTransactions trait for transactional events #28

Merged
merged 1 commit into from
Sep 4, 2019

Conversation

mfn
Copy link
Contributor

@mfn mfn commented Aug 28, 2019

In my test suite I'm not using RefreshDatabase but DatabaseTransactions.

The reason is that I've a custom system in place which primes the database once before all tests are run and not for every tests. Because I can't use sqlite but the actual DB driver, this is much more faster.

With this trait I can replace the vanilla one and transactional events and their effect can be tested like anything else.

Mildly related question

I've a question regarding https://github.com/fntneves/laravel-transactional-events#known-issues though: I re-read the paragraph multiple times but TBH, it's not clear whether this packages RefreshDatabase is basically required to be used or not.

I saw that you landed laravel/framework#23832 some time ago but this package actually does not use any of this:

  • the trait has a slightly differently logic then the one in this package
    (I can't test this, not using it)
  • the introduced unsetEventDispatcher is also not used anywhere

Can you clarify: if it recommended/required to use this package RefreshDatabase trait?

If so, I think the README could be clarified and also if this PR gets accepted, this should be added too (willing to do in this PR once this is cleared up for me).

If not so, then I don't understand why it exists :-)

I certainly needed this modified DatabaseTransactions otherwise the actual transactional events where never dispatched in tests.

@duxthefux
Copy link
Contributor

Hey, as far as I understand you need this. That's what we use in our project in order to normally dispatch events if we assert them.

use Illuminate\Foundation\Testing\DatabaseTransactions;

trait DatabaseTransactionWithoutTransactionalEvents
{
    use DatabaseTransactions;

    /**
     * Begin a database transaction on the testing database.
     */
    public function beginDatabaseTransaction()
    {
        $emptyDispatcher = new \Illuminate\Events\Dispatcher();
        $database = $this->app->make('db');

        foreach ($this->connectionsToTransact() as $name) {
            $connection = $database->connection($name);

            $currentDispatcher = $connection->getEventDispatcher();
            $connection->setEventDispatcher($emptyDispatcher);
            $connection->beginTransaction();
            $connection->setEventDispatcher($currentDispatcher);
        }

        $this->beforeApplicationDestroyed(function () use ($database, $emptyDispatcher) {
            foreach ($this->connectionsToTransact() as $name) {
                $connection = $database->connection($name);

                $currentDispatcher = $connection->getEventDispatcher();
                $connection->setEventDispatcher($emptyDispatcher);
                $connection->rollBack();
                $connection->setEventDispatcher($currentDispatcher);

                $connection->disconnect();
            }
        });
    }
}

@mfn
Copy link
Contributor Author

mfn commented Aug 28, 2019

Yeah, isn't that the same code as in my PR? 😄

@duxthefux
Copy link
Contributor

duxthefux commented Aug 28, 2019

@mfn sry, i obviously missed that this was a PR instead of an issue. Sorry for that. But I can confirm this code works :D for better clarity I would name the trait DatabaseTransactionWithoutTransactionalEvents to be more explicit, because otherwise you might think you use that trait but actually you use the laravel's trait.

@mfn
Copy link
Contributor Author

mfn commented Aug 28, 2019

DatabaseTransactionWithoutTransactionalEvents

But that's not what the trait does 🤔

It actually is there to handle them, because otherwise you use the vanilla trait then they won't work.

@fntneves
Copy link
Owner

fntneves commented Aug 29, 2019

Hello @mfn,

TL;DR: The RefreshDatabase provided by this package is a trait that is required for Laravel versions before 5.6.16, so the refresh logic (using transactions) does not interfer with this package. You should not see any difference in performance using DatabaseTransactions or RefreshDatabase trait, since the latter chooses the best refresh logic for your database configuration.

In my test suite I'm not using RefreshDatabase but DatabaseTransactions.

The reason is that I've a custom system in place which primes the database once before all tests are run and not for every tests. Because I can't use sqlite but the actual DB driver, this is much more faster.

The RefreshDatabase trait, available since Laravel 5.6, is smart enough to choose whether to use transactions or migrations as a way to refresh database. For in-memory databases like SQLite, it uses migrations to refresh the database. But for other databases such as MySQL and PostgreSQL, it uses transactions under the hood. Therefore, there's no difference in using RefreshDatabase or DatabaseTransactions, unless you're using a version lower than 5.6.

You just need to use the custom trait provided by this package if you're running a Laravel version that does not have my fix applied. (This fix is available since Laravel 5.6.16)

The thing is that every transaction used for keeping the database clean fires events that interfer with the behavior of this package. Concretely, as each test runs under a transaction managed by utility traits and the application-specific transactional events are actually dispatched only after the root transaction commits, the expected events are never dispatched because the root transaction (managed by utility traits) rolls back in the end.

@mfn
Copy link
Contributor Author

mfn commented Aug 29, 2019

I can't use RefreshDatabase because it only handles a single database connection (it calls artisan migrate:fresh which only deals with a single database per invocation).

As such, I can only use DatabaseTransactions.

But the vanilla one does not work with this package and I needed the same adjustments for that trait as this package has in RefreshDatabase 🤷‍♀️

Is it maybe because your past PR https://github.com/laravel/framework/pull/23832/files#diff-13ccf3cc47ede3e4128a008b8547ade0R70 only targeted RefreshDatabase?

@fntneves
Copy link
Owner

fntneves commented Aug 30, 2019

Humm, I see.

Ok, I will approve this as a temporary fix for people who use DatabaseTransactions trait.

Thank you for your PR! Next week I will release a new version, also ready for Laravel 6.

@fntneves fntneves merged commit 42a27bf into fntneves:master Sep 4, 2019
@mfn mfn deleted the mfn-db-trx branch September 4, 2019 09:59
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.

None yet

3 participants