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] Allows to persist database connection between tests and only reset PDO in afterClassTearDown() #49385

Closed
wants to merge 43 commits into from
Closed
Show file tree
Hide file tree
Changes from 29 commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
0451e43
wip
crynobone Dec 15, 2023
1062ef9
wip
crynobone Dec 15, 2023
e9436fe
wip
crynobone Dec 15, 2023
36a42a3
Apply fixes from StyleCI
StyleCIBot Dec 15, 2023
18c50b0
wip
crynobone Dec 15, 2023
1f68c0c
wip
crynobone Dec 15, 2023
6e4cdcf
Apply fixes from StyleCI
StyleCIBot Dec 15, 2023
39ed316
wip
crynobone Dec 15, 2023
9055cfe
Merge remote-tracking branch 'upstream/11/persist-database-connection…
crynobone Dec 15, 2023
16b3042
wip
crynobone Dec 15, 2023
dbcd76b
wip
crynobone Dec 15, 2023
12cde1f
wip
crynobone Dec 15, 2023
2c81e2c
wip
crynobone Dec 15, 2023
5e2ffc7
wip
crynobone Dec 15, 2023
41b17df
wip
crynobone Dec 15, 2023
f4e0451
wip
crynobone Dec 15, 2023
e2e9b2b
wip
crynobone Dec 15, 2023
c360bcf
wip
crynobone Dec 15, 2023
c675d2a
wip
crynobone Dec 15, 2023
42d9c15
wip
crynobone Dec 15, 2023
82b0eac
wip
crynobone Dec 15, 2023
2a6d4f1
wip
crynobone Dec 15, 2023
7c87079
wip
crynobone Dec 15, 2023
342bed4
wip
crynobone Dec 15, 2023
aa85d7a
wip
crynobone Dec 15, 2023
73cf607
Update SchemaBuilderTest.php
crynobone Dec 15, 2023
3926aa8
Update SchemaBuilderTest.php
crynobone Dec 16, 2023
13c39e5
Update src/Illuminate/Foundation/Testing/DatabaseConnectionFactory.php
crynobone Dec 16, 2023
e9ddabf
wip
crynobone Dec 17, 2023
6a30bee
Merge branch 'master' into 11/persist-database-connections
crynobone Dec 21, 2023
ff0862c
Apply suggestions from code review
crynobone Dec 21, 2023
3399a59
Merge branch 'master' into 11/persist-database-connections
crynobone Dec 21, 2023
16b0b0d
wip
crynobone Dec 26, 2023
3ca65a1
Apply fixes from StyleCI
StyleCIBot Dec 26, 2023
d76e1fb
Merge remote-tracking branch 'upstream/master' into 11/persist-databa…
crynobone Dec 26, 2023
6e88d3c
Merge remote-tracking branch 'upstream/11/persist-database-connection…
crynobone Dec 26, 2023
3240703
wip
crynobone Dec 26, 2023
3171da6
Merge branch 'master' into 11/persist-database-connections
crynobone Jan 10, 2024
e2f32d3
Merge remote-tracking branch 'upstream/master' into 11/persist-databa…
crynobone Jan 15, 2024
f55d238
wip
crynobone Jan 15, 2024
738a2cb
wip
crynobone Jan 15, 2024
2a14d0a
wip
crynobone Jan 15, 2024
3c3ce3e
Merge remote-tracking branch 'upstream/master' into 11/persist-databa…
crynobone Jan 18, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions .github/workflows/databases.yml
Original file line number Diff line number Diff line change
Expand Up @@ -235,3 +235,40 @@ jobs:
DB_DATABASE: master
DB_USERNAME: SA
DB_PASSWORD: Forge123

sqlite:
runs-on: ubuntu-20.04

strategy:
fail-fast: true

name: SQLite

steps:
- name: Checkout code
uses: actions/checkout@v4
with:
fetch-depth: 0

- name: Setup PHP
uses: shivammathur/setup-php@v2
with:
php-version: 8.2
extensions: dom, curl, libxml, mbstring, zip, pcntl, sqlsrv, pdo, pdo_sqlsrv, odbc, pdo_odbc, :php-psr
tools: composer:v2
coverage: none

- name: Install dependencies
uses: nick-fields/retry@v2
with:
timeout_minutes: 5
max_attempts: 5
command: composer update --prefer-stable --prefer-dist --no-interaction --no-progress

- name: Create database
run: vendor/bin/testbench package:create-sqlite-db

- name: Execute tests
run: vendor/bin/phpunit tests/Integration/Database
env:
DB_CONNECTION: sqlite
2 changes: 1 addition & 1 deletion composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@
"league/flysystem-sftp-v3": "^3.0",
"mockery/mockery": "^1.5.1",
"nyholm/psr7": "^1.2",
"orchestra/testbench-core": "^9.0",
"orchestra/testbench-core": "dev-persist-db-connections",
Copy link
Member

Choose a reason for hiding this comment

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

@crynobone should we put this PR in draft while this dependency isn't ready yet?

Copy link
Member Author

@crynobone crynobone Dec 18, 2023

Choose a reason for hiding this comment

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

I can't merge Testbench Core branch to stable unless Laravel commit with this changes.

Testbench will depends on the new Illuminate\Foundation\Testing\DatabaseConnectionFactory added in this PR.

"pda/pheanstalk": "^5.0",
"phpstan/phpstan": "^1.4.7",
"phpunit/phpunit": "^10.1",
Expand Down
15 changes: 15 additions & 0 deletions src/Illuminate/Contracts/Database/Connectors/ConnectionFactory.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
<?php

namespace Illuminate\Contracts\Database\Connectors;

interface ConnectionFactory
{
/**
* Establish a PDO connection based on the configuration.
*
* @param array $config
* @param string $name
* @return \Illuminate\Database\Connection
*/
public function make(array $config, $name);
}
7 changes: 4 additions & 3 deletions src/Illuminate/Database/Connectors/ConnectionFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
namespace Illuminate\Database\Connectors;

use Illuminate\Contracts\Container\Container;
use Illuminate\Contracts\Database\Connectors\ConnectionFactory as ConnectionFactoryContract;
use Illuminate\Database\Connection;
use Illuminate\Database\MySqlConnection;
use Illuminate\Database\PostgresConnection;
Expand All @@ -12,7 +13,7 @@
use InvalidArgumentException;
use PDOException;

class ConnectionFactory
class ConnectionFactory implements ConnectionFactoryContract
{
/**
* The IoC container instance.
Expand All @@ -36,10 +37,10 @@ public function __construct(Container $container)
* Establish a PDO connection based on the configuration.
*
* @param array $config
* @param string|null $name
* @param string $name
* @return \Illuminate\Database\Connection
*/
public function make(array $config, $name = null)
public function make(array $config, $name)
Copy link
Member Author

@crynobone crynobone Dec 15, 2023

Choose a reason for hiding this comment

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

$name shouldn't accept null since parseConfig() can only accept string for $name

Copy link
Contributor

Choose a reason for hiding this comment

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

parseConfig() is only restricted via doc-block documentation. Since the property isn't really strictly typed, it actually works fine by using $factory->make($config) on userland code.

Copy link
Member Author

Choose a reason for hiding this comment

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

For multi-tenant applications where you may want to change the default connections, this might not be the best usage:

CleanShot 2024-01-02 at 10 24 39

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry I didn't see your reply here, unfortunately I don't get notifications on the Laravel repository anymore (GitHub bug).

Whether you change or not the default connection in that script it's irrelevant since the configuration is being explicitly passed to the $factory->make(). The variable you hold is the one that is relevant to which database connection you have.

the getName() method is returning exactly what was provided when constructing the connection (null), which is my point about this breaking change. It works as intended and it doesn't seem relevant to break it

{
$config = $this->parseConfig($config, $name);

Expand Down
8 changes: 4 additions & 4 deletions src/Illuminate/Database/DatabaseManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
namespace Illuminate\Database;

use Doctrine\DBAL\Types\Type;
use Illuminate\Database\Connectors\ConnectionFactory;
use Illuminate\Contracts\Database\Connectors\ConnectionFactory;
use Illuminate\Database\Events\ConnectionEstablished;
use Illuminate\Support\Arr;
use Illuminate\Support\ConfigurationUrlParser;
Expand Down Expand Up @@ -32,7 +32,7 @@ class DatabaseManager implements ConnectionResolverInterface
/**
* The database connection factory instance.
*
* @var \Illuminate\Database\Connectors\ConnectionFactory
* @var \Illuminate\Contracts\Database\Connectors\ConnectionFactory
*/
protected $factory;

Expand Down Expand Up @@ -68,7 +68,7 @@ class DatabaseManager implements ConnectionResolverInterface
* Create a new database manager instance.
*
* @param \Illuminate\Contracts\Foundation\Application $app
* @param \Illuminate\Database\Connectors\ConnectionFactory $factory
* @param \Illuminate\Contracts\Database\Connectors\ConnectionFactory $factory
* @return void
*/
public function __construct($app, ConnectionFactory $factory)
Expand Down Expand Up @@ -149,7 +149,7 @@ protected function makeConnection($name)
return call_user_func($this->extensions[$driver], $config, $name);
}

return $this->factory->make($config, $name);
return $this->factory->make($config, $name ?? $this->getDefaultConnection());
}

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
<?php

namespace Illuminate\Foundation\Bootstrap;

use Illuminate\Contracts\Foundation\Application;
use Illuminate\Foundation\Testing\DatabaseConnectionFactory;

class OverrideProvidersForTesting
{
/**
* Bootstrap the given application.
*
* @param \Illuminate\Contracts\Foundation\Application $app
* @return void
*/
public function bootstrap(Application $app)
{
if (! $app->runningUnitTests()) {
return;
}

if ($app->bound('db.factory')) {
tap($app['db.factory'], function ($factory) use ($app) {
$app->instance('db.factory', new DatabaseConnectionFactory($app, $factory));
});
}
}
}
1 change: 1 addition & 0 deletions src/Illuminate/Foundation/Console/Kernel.php
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@ class Kernel implements KernelContract
\Illuminate\Foundation\Bootstrap\RegisterFacades::class,
\Illuminate\Foundation\Bootstrap\SetRequestForConsole::class,
\Illuminate\Foundation\Bootstrap\RegisterProviders::class,
\Illuminate\Foundation\Bootstrap\OverrideProvidersForTesting::class,
\Illuminate\Foundation\Bootstrap\BootProviders::class,
];

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,13 @@

trait InteractsWithDatabase
{
/**
* Determine whether database connection should be disconnected between tests.
*
* @var bool
*/
protected $disconnectDatabaseConnections = false;

/**
* Assert that a given where condition exists in the database.
*
Expand Down
78 changes: 78 additions & 0 deletions src/Illuminate/Foundation/Testing/DatabaseConnectionFactory.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
<?php

namespace Illuminate\Foundation\Testing;

use Illuminate\Contracts\Container\Container;
use Illuminate\Contracts\Database\Connectors\ConnectionFactory as ConnectionFactoryContract;
use Illuminate\Database\Connectors\ConnectionFactory;

/**
* @internal
*/
class DatabaseConnectionFactory extends ConnectionFactory
{
/**
* List of cached database connections.
*
* @var array<string, \Illuminate\Database\Connection>
*/
protected static array $cachedConnections = [];

/**
* Create a new connection factory instance.
*
* @param \Illuminate\Contracts\Container\Container $container
* @return void
*/
public function __construct(
Container $container,
protected ConnectionFactoryContract $factory
) {
parent::__construct($container);
}

/**
* Establish a PDO connection based on the configuration.
*
* @param array $config
* @param string $name
* @return \Illuminate\Database\Connection
*/
#[\Override]
public function make(array $config, $name)
{
$key = $name ?? $config['name'];

// In-Memory Databases doesn't have any max connections limitation so it should be safe to just create a new connection between tests.
// Because some tests may be depend on thier volatile, we should always create new connections to avoid carrying over previous data.
if ($config['driver'] === 'sqlite' && $config['database'] === ':memory:') {
return $this->factory->make($config, $name);
}

if (! isset(static::$cachedConnections[$key]) || is_null(static::$cachedConnections[$key]->getRawPdo() ?? null)) {
return static::$cachedConnections[$key] = $this->factory->make($config, $name);
}

$config = $this->parseConfig($config, $name);

$connection = $this->createConnection(
$config['driver'], static::$cachedConnections[$key]->getRawPdo(), $config['database'], $config['prefix'], $config
)->setReadPdo(static::$cachedConnections[$key]->getRawReadPdo());

return static::$cachedConnections[$key] = $connection;
crynobone marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* Flush the current state.
*
* @return void
*/
public static function flushState(): void
{
foreach (static::$cachedConnections as $connection) {
$connection->disconnect();
}

static::$cachedConnections = [];
}
}
1 change: 0 additions & 1 deletion src/Illuminate/Foundation/Testing/DatabaseTransactions.php
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ public function beginDatabaseTransaction()
$connection->unsetEventDispatcher();
$connection->rollBack();
$connection->setEventDispatcher($dispatcher);
$connection->disconnect();
}
});
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ class DatabaseTransactionsManager extends BaseManager
* @param callable $callback
* @return void
*/
#[\Override]
public function addCallback($callback)
{
// If there are no transactions, we'll run the callbacks right away. Also, we'll run it
Expand All @@ -29,6 +30,7 @@ public function addCallback($callback)
*
* @return \Illuminate\Support\Collection<int, \Illuminate\Database\DatabaseTransactionRecord>
*/
#[\Override]
public function callbackApplicableTransactions()
{
return $this->pendingTransactions->skip(1)->values();
Expand All @@ -40,6 +42,7 @@ public function callbackApplicableTransactions()
* @param int $level
* @return bool
*/
#[\Override]
public function afterCommitCallbacksShouldBeExecuted($level)
{
return $level === 1;
Expand Down
1 change: 0 additions & 1 deletion src/Illuminate/Foundation/Testing/RefreshDatabase.php
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,6 @@ public function beginDatabaseTransaction()
$connection->unsetEventDispatcher();
$connection->rollBack();
$connection->setEventDispatcher($dispatcher);
$connection->disconnect();
}
});
}
Expand Down
8 changes: 8 additions & 0 deletions src/Illuminate/Foundation/Testing/TestCase.php
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,13 @@ protected function tearDown(): void

ParallelTesting::callTearDownTestCaseCallbacks($this);

if (
property_exists($this, 'disconnectDatabaseConnections')
&& $this->disconnectDatabaseConnections === true
) {
DatabaseConnectionFactory::flushState();
}

$this->app->flush();

$this->app = null;
Expand Down Expand Up @@ -255,6 +262,7 @@ protected function tearDown(): void
public static function tearDownAfterClass(): void
{
static::$latestResponse = null;
DatabaseConnectionFactory::flushState();

foreach ([
\PHPUnit\Util\Annotation\Registry::class,
Expand Down
11 changes: 0 additions & 11 deletions tests/Integration/Database/DatabaseTestCase.php
Original file line number Diff line number Diff line change
Expand Up @@ -16,17 +16,6 @@ abstract class DatabaseTestCase extends TestCase
*/
protected $driver;

protected function setUp(): void
{
$this->beforeApplicationDestroyed(function () {
foreach (array_keys($this->app['db']->getConnections()) as $name) {
$this->app['db']->purge($name);
}
});

parent::setUp();
}

protected function defineEnvironment($app)
{
$connection = $app['config']->get('database.default');
Expand Down
7 changes: 0 additions & 7 deletions tests/Integration/Database/EloquentBelongsToManyTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,6 @@

class EloquentBelongsToManyTest extends DatabaseTestCase
{
protected function tearDown(): void
{
parent::tearDown();

Carbon::setTestNow(null);
}

protected function afterRefreshingDatabase()
{
Schema::create('users', function (Blueprint $table) {
Expand Down
9 changes: 0 additions & 9 deletions tests/Integration/Database/EloquentMassPrunableTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -93,15 +93,6 @@ public function testPrunesSoftDeletedRecords()
$this->assertEquals(0, MassPrunableSoftDeleteTestModel::count());
$this->assertEquals(2000, MassPrunableSoftDeleteTestModel::withTrashed()->count());
}

protected function tearDown(): void
{
parent::tearDown();

Container::setInstance(null);

m::close();
}
}

class MassPrunableTestModel extends Model
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,6 @@ class EloquentTransactionWithAfterCommitUsingDatabaseTransactionsTest extends Te

protected function setUp(): void
{
$this->beforeApplicationDestroyed(function () {
foreach (array_keys($this->app['db']->getConnections()) as $name) {
$this->app['db']->purge($name);
}
});

parent::setUp();

if ($this->usesSqliteInMemoryDatabaseConnection()) {
Expand Down
Loading