feat: skip resetDatabase when migrations are up-to-date

[haase-fabian]

I use the DamaDoctrineTestBundle in combination with database_resetter.orm.reset_mode: migrate.

Everytime the test suite is run, when there is a TestCase using the ResetDatabase trait, the database is reset.

This change skips the step when there are no new migrations and no unregistered migrations in the database.

This eliminates about 10s of initialization time to 0s for every test suit run in my case.

This assumes, that when DamaDoctrineTestBundle is installed, it is also used and correctly configured.

I use ddev for my projects, so I haven't locally tested the changes as I don't have php locally installed.

[nikophil]

Hi @haase-fabian

Everytime the test suite is run, when there is a TestCase using the ResetDatabase trait, the database is reset.

This statement is not true when using dama

when we encounter a TestCase using ResetDatabase for the first time, we reset the database, but it is only made once per test suite, thanks to ResetDatabase::shouldReset() and the static property DatabaseResetter::$hasBeenReset.

Afterwards, if using dama, we never reset the schema, thanks to this condition:
https://github.com/zenstruck/foundry/blob/1.x/src/Test/ORMDatabaseResetter.php#L55-L58

Nonetheless, I do think your PR improves the perfs, but it also breaks the promise of the ResetDatabase which is to start from a fresh db. What if I have called StaticDriver::commit();die; somewhere in my tests for debugging purpose? The next time, the db won't be empty... I'm wondering how we can mitigate this?

This assumes, that when DamaDoctrineTestBundle is installed, it is also used and correctly configured.

I think this is also problematic: dama is enabled from phpunit.xml.dist and could be disabled locally by creating a phpunit.xml. Actually, a coworker of mine disables it because he does not like using dama for some reasons. This PR would break the tests on his computer, because it just test the presence of StaticDriver class. I don't know if we can access some PHPUnit's internals, in order to know whether or not the extension is enabled 🤔

[haase-fabian]

Hi @nikophil,

Everytime the test suite is run, when there is a TestCase using the ResetDatabase trait, the database is reset.

This statement is not true when using dama

I'm sorry for my ambiguous language. I was trying to say it's reset once if the suite encounters a TestCase with set DatabaseTrait.

I made the isDAMADoctrineTestBundleEnabled cachable.
This correctly identifies disabled DamaDoctrineTestBundles.

And I truncate all registered tables in case there are any leftover entries.
I do not know whether the Postgres version for disabling Foreign Key checks works correctly, however if it fails it will fall back to recreating the whole database. (same for other database platforms)

In my project, this takes about 0.8s now.

Thinking about introducing a flag to opt-in skipping the truncation part in a follow-up PR.

    private function isResetUsingTruncate(): bool {
        if(isset($_SERVER['FOUNDRY_RESET_DETECTION'])) {
            return $_SERVER['FOUNDRY_RESET_DETECTION'] !== 'unsafe';
        }

        return true;
    }

Disabling the truncate results in performances around 0.08s.

[nikophil]

Hey @haase-fabian

this is just an idea, but do you think we could somehow leverage the proposal here?

and manage what you want to achieve is userland based on an event system?
I feel like this would be the silver bullet solution, giving each user the flexibility s.he wants

[haase-fabian]

Yes, I would definitely appreciate extensibility of any kind of bundle.
I don't understand why many bundles violate the Open–closed principle.

However, I also think, that bundles should provide reasonable default behavior to be as close to zero-configuration as possible.
Therefore, making the database reset in 'migration' mode as fast as possible without having to set up each new project.
At least for the most common use cases.

In regard to an event based system:
I don't know exactly the capabilities, but from a search in the documentation, I think it's lacking decorating around the method.
Therefore, I would opt for decorators rather than events.

A rough idea:

• 1. Make ORMDatabaseResetter etc. implement DatabaseResetterInterface
• 2. Use protected methods and make them non-final
• 3. Inject the DatabaseResetter implementation via the container
• 4. Utilize Decorator stacks

The interface gets passed a context variable:

interface DatabaseResetterInterface
{
    public function resetDatabase(array $context): void;
    public function resetSchema(array $context): void;
}

Then the stack itself could look similar to this:

stack:
    - DamaDatabaseResetter::class //adds ["isDAMADoctrineTestBundleEnabled" => true|false] to $context
    - KernelProvider::class // adds ["kernel" => $kernel, ...$configuration], maybe also ["application" => $app]
    - ChainResetter::class // delegates to both ORMDatabaseResetter and ODMDatabaseResetter if available
        -
            - MigrationDatabaseResetter::class
            - ORMDatabaseResetter::class // holds actual reset logic
        - ODMDatabaseResetter::class // holds actual reset logic

And in user land, one can easily override specific behavior:

#[When(env: 'test')]
#[AsDecorator(ORMDatabaseResetter::class)]
class ViewResetter implements DatabaseResetterInterface
{
    public function __construct(#[AutowireDecorated] private object $inner, private ManagerRegistry $registry, ...)
    {

    }
    public function resetDatabase(array $context): void {
        if($this->shouldReset()){
            //install postgres extensions
            $this->inner->resetDatabase($context);
            //add missing views
        }
    }
    ...
}