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

[11.x] Improved Performance for Testing with In-Memory Databases #47912

Merged
merged 2 commits into from
Aug 16, 2023

Conversation

AJenbo
Copy link
Contributor

@AJenbo AJenbo commented Jul 31, 2023

This pull request aims to enhance the performance and developer experience when running tests with an in-memory database in Laravel. By keeping a reference to the PDO object and restoring it for connections, we enable transaction-based database refresh, resulting in a significant performance boost. Test suites that used to take minutes to run are now completed in just seconds, providing a much smoother testing process.

image

I believe this change is non-breaking as it replicates the same database operations that would occur with a persisted database configuration. However, if there are any concerns, please let me know, and I can rebase and retarget the pull request on the master branch.

Please feel free to provide any feedback or preferences regarding the implementation or coding style. Your input is greatly appreciated.

Thank you.

@AJenbo AJenbo changed the title Transactional RefreshDatabase for in memory databases Improved Performance for Testing with In-Memory Databases Jul 31, 2023
@JurianArie
Copy link
Contributor

I like the idea, and I think it could work, but there is a bug in your implementation. Copy pasting the trait breaks my entire test suite.

@GrahamCampbell GrahamCampbell changed the title Improved Performance for Testing with In-Memory Databases [10.x] Improved Performance for Testing with In-Memory Databases Aug 1, 2023
Copy link
Member

@GrahamCampbell GrahamCampbell left a comment

Choose a reason for hiding this comment

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

Probably 11.x is the best place for this, as this could break things for people doing funky things such as DDL in their tests.

@AJenbo AJenbo changed the base branch from 10.x to master August 1, 2023 21:23
@AJenbo AJenbo changed the title [10.x] Improved Performance for Testing with In-Memory Databases [11.x] Improved Performance for Testing with In-Memory Databases Aug 1, 2023
@AJenbo
Copy link
Contributor Author

AJenbo commented Aug 1, 2023

@GrahamCampbell

Thank you for the feedback. I have rebased on master and set it as the target branch as well as updated the title to reflect that this is targeting Larvel 11.x.

@JurianArie Can you please be more specific here is what I just did:

  • Crate a Laravel 11 skeleton app
  • Copy .env
  • Generate keys
  • Set DB to sqlite :memory: in .env
  • Delete the unit test
  • Duplicate the feature test
  • Add sleep(5) to the create_users migration
  • Run test
  • Swap RefreshDatabase to the one from this PR
  • Run test

@AJenbo AJenbo requested a review from GrahamCampbell August 1, 2023 21:30
@JurianArie
Copy link
Contributor

JurianArie commented Aug 2, 2023

@AJenbo I copy pasted the trait (directly in the vendor folder) from this PR into a project that uses a in memory DB for testing. It looks like the database never gets migrated.

Error output
phpunit --stop-on-error
PHPUnit 10.2.6 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.1.15
Configuration: [SNIP]phpunit.xml

............E

Time: 00:04.762, Memory: 62.50 MB

There was 1 error:

1) Tests\Feature\[TESTCASE]
Illuminate\Database\QueryException: SQLSTATE[HY000]: General error: 1 no such table: files (Connection: sqlite, SQL: insert into "files" [SNIP])

vendor/laravel/framework/src/Illuminate/Database/Connection.php:574
vendor/laravel/framework/src/Illuminate/Database/Connection.php:788
vendor/laravel/framework/src/Illuminate/Database/Connection.php:755
vendor/laravel/framework/src/Illuminate/Database/Connection.php:581
vendor/laravel/framework/src/Illuminate/Database/Connection.php:533
vendor/laravel/framework/src/Illuminate/Database/Query/Processors/Processor.php:32
vendor/laravel/framework/src/Illuminate/Database/Query/Builder.php:3383
vendor/laravel/framework/src/Illuminate/Database/Eloquent/Builder.php:1925
vendor/laravel/framework/src/Illuminate/Database/Eloquent/Model.php:1333
vendor/laravel/framework/src/Illuminate/Database/Eloquent/Model.php:1298
vendor/laravel/framework/src/Illuminate/Database/Eloquent/Model.php:1137
vendor/laravel/framework/src/Illuminate/Database/Eloquent/Factories/Factory.php:330
vendor/laravel/framework/src/Illuminate/Collections/Traits/EnumeratesValues.php:236
vendor/laravel/framework/src/Illuminate/Database/Eloquent/Factories/Factory.php:339
vendor/laravel/framework/src/Illuminate/Database/Eloquent/Factories/Factory.php:279
vendor/laravel/framework/src/Illuminate/Database/Eloquent/Factories/Factory.php:475
vendor/laravel/framework/src/Illuminate/Collections/Arr.php:558
vendor/laravel/framework/src/Illuminate/Collections/Collection.php:772
vendor/laravel/framework/src/Illuminate/Database/Eloquent/Factories/Factory.php:472
vendor/laravel/framework/src/Illuminate/Database/Eloquent/Factories/Factory.php:425
vendor/laravel/framework/src/Illuminate/Database/Eloquent/Factories/Factory.php:409
vendor/laravel/framework/src/Illuminate/Database/Eloquent/Concerns/GuardsAttributes.php:155
vendor/laravel/framework/src/Illuminate/Database/Eloquent/Factories/Factory.php:414
vendor/laravel/framework/src/Illuminate/Database/Eloquent/Factories/Factory.php:382
vendor/laravel/framework/src/Illuminate/Database/Eloquent/Factories/Factory.php:276
vendor/laravel/framework/src/Illuminate/Database/Eloquent/Factories/Factory.php:301
vendor/laravel/framework/src/Illuminate/Database/Eloquent/Concerns/HasEvents.php:408
vendor/laravel/framework/src/Illuminate/Database/Eloquent/Factories/Factory.php:302
vendor/laravel/framework/src/Illuminate/Database/Eloquent/Factories/Factory.php:232
[TESTCASE]:36
vendor/laravel/framework/src/Illuminate/Foundation/Testing/TestCase.php:174

Caused by
PDOException: SQLSTATE[HY000]: General error: 1 no such table: files

vendor/laravel/framework/src/Illuminate/Database/Connection.php:574
vendor/laravel/framework/src/Illuminate/Database/Connection.php:788
vendor/laravel/framework/src/Illuminate/Database/Connection.php:755
vendor/laravel/framework/src/Illuminate/Database/Connection.php:581
vendor/laravel/framework/src/Illuminate/Database/Connection.php:533
vendor/laravel/framework/src/Illuminate/Database/Query/Processors/Processor.php:32
vendor/laravel/framework/src/Illuminate/Database/Query/Builder.php:3383
vendor/laravel/framework/src/Illuminate/Database/Eloquent/Builder.php:1925
vendor/laravel/framework/src/Illuminate/Database/Eloquent/Model.php:1333
vendor/laravel/framework/src/Illuminate/Database/Eloquent/Model.php:1298
vendor/laravel/framework/src/Illuminate/Database/Eloquent/Model.php:1137
vendor/laravel/framework/src/Illuminate/Database/Eloquent/Factories/Factory.php:330
vendor/laravel/framework/src/Illuminate/Collections/Traits/EnumeratesValues.php:236
vendor/laravel/framework/src/Illuminate/Database/Eloquent/Factories/Factory.php:339
vendor/laravel/framework/src/Illuminate/Database/Eloquent/Factories/Factory.php:279
vendor/laravel/framework/src/Illuminate/Database/Eloquent/Factories/Factory.php:475
vendor/laravel/framework/src/Illuminate/Collections/Arr.php:558
vendor/laravel/framework/src/Illuminate/Collections/Collection.php:772
vendor/laravel/framework/src/Illuminate/Database/Eloquent/Factories/Factory.php:472
vendor/laravel/framework/src/Illuminate/Database/Eloquent/Factories/Factory.php:425
vendor/laravel/framework/src/Illuminate/Database/Eloquent/Factories/Factory.php:409
vendor/laravel/framework/src/Illuminate/Database/Eloquent/Concerns/GuardsAttributes.php:155
vendor/laravel/framework/src/Illuminate/Database/Eloquent/Factories/Factory.php:414
vendor/laravel/framework/src/Illuminate/Database/Eloquent/Factories/Factory.php:382
vendor/laravel/framework/src/Illuminate/Database/Eloquent/Factories/Factory.php:276
vendor/laravel/framework/src/Illuminate/Database/Eloquent/Factories/Factory.php:301
vendor/laravel/framework/src/Illuminate/Database/Eloquent/Concerns/HasEvents.php:408
vendor/laravel/framework/src/Illuminate/Database/Eloquent/Factories/Factory.php:302
vendor/laravel/framework/src/Illuminate/Database/Eloquent/Factories/Factory.php:232
[TESTCASE]:36
vendor/laravel/framework/src/Illuminate/Foundation/Testing/TestCase.php:174

@mbardelmeijer
Copy link
Contributor

I can confirm a good speedup as well on our test suite with these changes 👍

Without change: Time: 01:45.759, Memory: 123.23 MB
With change: Time: 00:36.129, Memory: 125.00 MB

PHPUnit 10.2.6 with PHP 8.2.8

@nshiro
Copy link
Contributor

nshiro commented Aug 2, 2023

I can also confirm an increase in speed.

182 tests
Without change: Time: 2.65s
With change: Time: 1.68s

PHPUnit 10.2.6 (+ with some Pest)
PHP 8.1

@AJenbo
Since the migrateUsing function is no longer called, should we remove it?

@axlon
Copy link
Contributor

axlon commented Aug 2, 2023

This breaks my testsuite as well, running a single test or test file works fine, but running the whole suite breaks all tests that rely on the database.
I've tested with and without Paratest, both give the same result, which is an error about a non-existent table (implying the migrations weren't run)

Hope this helps.

@AJenbo
Copy link
Contributor Author

AJenbo commented Aug 2, 2023

@axlon Can you please provide a test case or steps to reproduce it.

@axlon
Copy link
Contributor

axlon commented Aug 2, 2023

@axlon Can you please provide a test case or steps to reproduce it.

I don't think I can provide a test case because every single test case passes in isolation, it only fails when running all my tests. Obviously I can't post my entire project.

Could it be that running tests that don't use the DB at all alongside tests that do maybe breaks the internal state? I also use LazilyRefreshDatabase in some tests, I don't know if that matters (did not look into this PR code in depth)

@taylorotwell taylorotwell marked this pull request as draft August 2, 2023 18:17
@taylorotwell
Copy link
Member

Please mark as ready for review when cause of breakages has been found and fixed.

@schonhoff
Copy link
Contributor

Hi,

I tried out the trait and got the same error, reported earlier. I switched LazilyRefreshDatabase to RefreshDatabase (your modified trait). No other changes were made. My configuration:

phpunit.xml:

<?xml version="1.0" encoding="UTF-8"?>
<phpunit xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
         xsi:noNamespaceSchemaLocation="https://schema.phpunit.de/10.1/phpunit.xsd"
         bootstrap="vendor/autoload.php"
         cacheResult="true"
         backupGlobals="false"
         colors="true"
         cacheDirectory="coverage/cache"
         backupStaticProperties="false"
>
    <testsuites>
        <testsuite name="Feature">
            <directory suffix="Test.php">./tests/Feature</directory>
        </testsuite>
        <testsuite name="Unit">
            <directory suffix="Test.php">./tests/Unit</directory>
        </testsuite>
    </testsuites>
    <source>
        <include>
            <directory suffix=".php">./app</directory>
        </include>
    </source>
    <php>
        <env name="APP_ENV" value="testing"/>
        <env name="BCRYPT_ROUNDS" value="4"/>
        <env name="CACHE_DRIVER" value="array"/>
        <env name="MAIL_DRIVER" value="array"/>
        <env name="QUEUE_DRIVER" value="sync"/>
        <env name="SESSION_DRIVER" value="array"/>
        <env name="TELESCOPE_ENABLED" value="false"/>
    </php>
</phpunit>

.env.testing

[...]
DB_CONNECTION=sqlite_testing
DB_HOST=127.0.0.1
DB_PORT=3306
[...]

database.php (config)

[...]
'connections' => [
    'sqlite_testing' => [
        'driver' => 'sqlite',
        'database' => ':memory:',
        'prefix' => '',
        'foreign_key_constraints' => true,
    ],
],
[...]

Error in the console after running php artisan test --stop-on-failure (this is just an example)

SQLSTATE[HY000]: General error: 1 no such table: roles (Connection: sqlite_testing, SQL: insert into "roles" [...]

I'm using pest version v2.12.2 as my testing framework with mostly unit tests (want to switch to pest soon).
My app has close to 9000 tests and 40% is using the database. That is why I'm currently working with LazilyRefreshDatabase instead RefreshDatabase. I'm implementing the trait in the TestCase abstract class.

Hope that helps to narrow down the error a bit more.

@nunomaduro
Copy link
Member

Results using Vapor is test suite: Apple M1 Pro 16 GB | 851 tests

Before this pull request:
./vendor/bin/pest: 231.74s
./vendor/bin/pest --parallel (10 processes): 23.09s

After this pull request:
./vendor/bin/pest: FAILED - SQLSTATE[HY000]: General error: 1 no such table: users...
./vendor/bin/pest --parallel (10 processes):  FAILED - SQLSTATE[HY000]: General error: 1 no such table: users...

@AJenbo
Copy link
Contributor Author

AJenbo commented Aug 11, 2023

@nunomaduro sorry I'm not sure I follow. Are you saying that using Vapor in a project will trigger the issue, or that the test suite for the Vaport project displays the issue?
(full disclaimer I have no knolage about what Vapor is other then hearing the name before)

@nunomaduro
Copy link
Member

@AJenbo Just saying that the changes on this pull request makes the Vapor's test suite to fail.

@donnysim
Copy link
Contributor

@AJenbo I also get the same exception that a table is not found. It doesn't happen if you have only one feature test with use RefreshDatabase;, but it does if you have multiple like Feature1Test, Feature2Test. And the problem seems more like the connection is always recreated between test classes so you cannot use the :memory: driver as each class will have a separate Application instance and separate sqlite memory database, which the second time it does not migrate it and it stays empty.

@AJenbo
Copy link
Contributor Author

AJenbo commented Aug 12, 2023

I managed to track down a project that exhibited the described issue and have applied the needed changes to fix it.

Turns out the difference is that it worked when RefreshDatabase was used on the abstract TestCase but not the individual test classes, since this would create individual instances of $inMemoryConnections that then go out of sync with RefreshDatabaseState::$migrated.
The solution was simply to move the property to the RefreshDatabaseState class rather then the RefreshDatabase trait so that they share scope.

Thank you to everyone who assisted in testing.

@AJenbo AJenbo marked this pull request as ready for review August 12, 2023 03:26
@axlon
Copy link
Contributor

axlon commented Aug 13, 2023

@AJenbo can confirm it is now working as expected! 👍

@schonhoff
Copy link
Contributor

Hey,

I tried to use your trait after the fix with no changes on my tests (using LazilyRefreshDatabase and pest). It worked:

Before:
image

After:
image

This would be after 10 tests runs on the before configuration and after configuration in decrease of ~35%.

@driesvints
Copy link
Member

@AJenbo Laravel.io's codebase might be a good candidate as well to try these changes: https://github.com/laravelio/laravel.io

Copy link
Contributor

@JurianArie JurianArie left a comment

Choose a reason for hiding this comment

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

I can confirm that it now works!

src/Illuminate/Foundation/Testing/RefreshDatabaseState.php Outdated Show resolved Hide resolved
src/Illuminate/Foundation/Testing/RefreshDatabase.php Outdated Show resolved Hide resolved
This provides a significant performance increase for running
tests when using an in memory database.
@taylorotwell
Copy link
Member

taylorotwell commented Aug 15, 2023

@AJenbo Sorry - it's been a while since I wrote this code. What happens currently (without this PR) if you try to treat SQLite in-memory databases like MySQL databases and let the refreshTestDatabase method run in either case? What error is thrown and why?

@AJenbo
Copy link
Contributor Author

AJenbo commented Aug 15, 2023

@taylorotwell By doing so, we would encounter a SQLSTATE[HY000]: General error: 1 no such table error for all but the first test that attempts to use the database. This issue arises because the PDO object (and consequently, the in-memory database) on which the migrations were initially run gets dropped from memory when the application is torn down between tests. As a result, a new instance is spawned for the next test, where migrations have not been executed.

The changes introduced by this PR involve retaining a persistent PDO instance in the RefreshDatabaseState object and injecting it into the application during test setup when an in-memory configuration is detected. This change enables us to treat the in-memory database the same way we treat disk-based instances, effectively extending the performance benefits of RefreshDatabase to in-memory databases as well.

@taylorotwell taylorotwell merged commit 20d6b4b into laravel:master Aug 16, 2023
@taylorotwell
Copy link
Member

Thanks!

@mokhosh
Copy link
Contributor

mokhosh commented Feb 27, 2024

@AJenbo is there any documentation on how to migrate a Laravel 10 package to adapt to this change?
We're trying to support Laravel 11 on Filament, but most tests are failing, and I ended up here after creating this issue on testbench orchestral/testbench#393

@AJenbo
Copy link
Contributor Author

AJenbo commented Feb 27, 2024

@mokhosh does this behave differently for you depending if you use MySQL or SQLite? Was that also the case prior?

In Laravel 10 RefreshDatabase behaved more like DatabaseMigrations when SQLite was used.

You might want to use DatabaseMigrations or DatabaseTruncation if your tests require that the database is reset between each test. See: https://laravel.com/docs/10.x/dusk#resetting-the-database-after-each-test

It could also happen if you manually start a transaction in a way that isn't done though ManagesTransactions.

@mokhosh
Copy link
Contributor

mokhosh commented Feb 27, 2024

@AJenbo Thanks for your reply.

If I don't touch anything except changing the driver to mysql I get Base table or view already exists

DatabaseMigrations reduced failures from almost 100% to about 50%, and I got General error: 1 no such table: users.

DatabaseTruncation reduced failures to only one test, and that's the first test, but in that test I'm still getting General error: 1 no such table: users, so it still seems related to how these traits work, rather than the tests.

@AJenbo
Copy link
Contributor Author

AJenbo commented Feb 27, 2024

If I don't touch anything except changing the driver to mysql I get Base table or view already exists

Ok that's a good indication that your tests are not compatible with RefreshDatabase in general and only worked in previous versions because the trait was mostly skipped when using SQLite (falling back to running $this->artisan('migrate');).

I would suggest you switch to DatabaseTruncation and make sure that the default migrations is run before any of your own migrations (does the failing test actually trigger migration?).

Alternatively you could try calling $this->artisan('migrate'); manually during your test setup.

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.