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

[9.x] Add DatabaseTruncation trait for testing #45726

Merged
merged 7 commits into from
Jan 31, 2023
Merged

[9.x] Add DatabaseTruncation trait for testing #45726

merged 7 commits into from
Jan 31, 2023

Conversation

patrickomeara
Copy link
Contributor

@patrickomeara patrickomeara commented Jan 19, 2023

This is a second attempt at #45630

The DatabaseTruncates trait is a drop in replacement for DatabaseMigrations however it only truncates tables with data instead of dropping and migrating the whole database, this is a more performant approach in most cases.

In this PR I have added support for multiple connections, just like the RefreshDatabase trait.

Excluding tables can be done per connection or across all connections depending on what the developer needs.

@emargareten raised the issue of foreign keys in the last PR. As this PR moves the truncating to the start of the test, I have added a usesForeignKeyChecks() which allows the developer to set the check per connection, defaulting to true.

Once this PR is resolved I will PR this change that cleans up the logic around the event dispatcher

Migrate the database in the first run, then truncate the affected tables and use the seeder for subsequent runs.
* allow excluding tables per connection
* unset the event dispatcher before truncating, re-set afterwards
@patrickomeara patrickomeara changed the title [9.x] Add DatabaseTruncates trait for testing [9.x] Add DatabaseTruncates trait for testing Jan 20, 2023
* This removes potential conflicts with other beforeApplicationDestroyed callbacks, where a database is needed
* This matches the behaviour of the other tests instead of removing data afterwards.
* It also makes it easier to check the data after failed tests, especially when a dusk test fails.
@patrickomeara patrickomeara marked this pull request as draft January 20, 2023 00:55
@patrickomeara
Copy link
Contributor Author

patrickomeara commented Jan 20, 2023

Here are some result comparisons against DatabaseMigrations (top) and DatabaseTruncates (bottom)

19 Dusk Tests
image

49 Feature Tests
image

This is for demonstration purposes only. RefreshDatabase is a much better choice for Feature Tests.

@patrickomeara patrickomeara marked this pull request as ready for review January 20, 2023 02:04
* allow the developer to specify which connections use foreign key checks, default to all connections
* allwo the developer to create an excludeTables property instead of having to override the method
@nshiro
Copy link
Contributor

nshiro commented Jan 26, 2023

Nice approach. 😄

But this DatabaseTruncates does not seem to work in parallel mode.
php artisan test --parallel
I tested on Laravel 9.48.0 / MySQL 8.0.30.

While DatabaseMigrations works.

P.S.
This was my mistake.🙇‍♂️

@taylorotwell
Copy link
Member

@nshiro can you elaborate? error messages?

@nshiro
Copy link
Contributor

nshiro commented Jan 27, 2023

(Edited)
I removed the error messages because it was my mistake. And I also wanted to make this PR readable.

@nshiro
Copy link
Contributor

nshiro commented Jan 27, 2023

I also noticed I needed doctrine/dbal to use DatabaseTruncates.
composer require doctrine/dbal

@patrickomeara
Copy link
Contributor Author

patrickomeara commented Jan 28, 2023

Thanks for your feedback @nshiro

Are you able to create a repository with the failing tests?

I have created one here based on what you mentioned and all tests pass with and without parallel.

image

Here is the diff from a fresh install
patrickomeara/truncates@194771c...main

Yes doctrine/dbal is a dev requirement to get the table names for all database grammars. This should be mentioned in the Dusk docs if this PR is accepted. As it is here.

@nshiro
Copy link
Contributor

nshiro commented Jan 28, 2023

@patrickomeara
I apologise to you. It was all my fault. 🙇‍♂️🙇‍♂️
I forgot to copy and paste the last bit of your code.
Now the test works in parallel mode too.
Great work !

@patrickomeara
Copy link
Contributor Author

@nshiro Awesome. Thanks for testing this PR out.

@laserhybiz
Copy link

Why not extract truncateTablesForConnection and its methods into a console command so it can be used in other places?
like @emargareten mentioned

@taylorotwell taylorotwell merged commit 5273fac into laravel:9.x Jan 31, 2023
@taylorotwell
Copy link
Member

Thanks - could you PR some docs to laravel/docs where you think this is relevant?

@mpyw
Copy link
Contributor

mpyw commented Feb 1, 2023

@patrickomeara It looks to be useful for testing native-transaction-related libraries: (e.g. https://github.com/mpyw/laravel-database-advisory-lock/blob/master/tests/PostgresTransactionErrorRecoveryTest.php)
Thank you!


if ($seeder = $this->seeder()) {
// Use a specific seeder class...
$this->artisan('db:seed', ['--class' => $seeder]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it will be better to use FQCN instead of the command name?

@patrickomeara
Copy link
Contributor Author

Thanks - could you PR some docs to laravel/docs where you think this is relevant?

@taylorotwell Yes, will do this ASAP.

@patrickomeara patrickomeara changed the title [9.x] Add DatabaseTruncates trait for testing [9.x] Add DatabaseTruncation trait for testing Feb 6, 2023
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.

6 participants