dmaicher / doctrine-test-bundle

Symfony bundle to isolate your app's doctrine database tests and improve the test performance
MIT License
1.08k stars 60 forks source link

Bundle seems incompatible with latest releases of doctrine/dbal (2.9.3 and 2.10.0) #94

Closed evertharmeling closed 4 years ago

evertharmeling commented 4 years ago

Problem arises with upgrading to doctrine/dbal:2.9.3 and doctrine/dbal:2.10.0.

PHP Fatal error:  Uncaught PDOException: There is no active transaction in /projects/xxx/vendor/dama/doctrine-test-bundle/src/DAMA/DoctrineTestBundle/Doctrine/DBAL/StaticDriver.php:140
Stack trace:
#0 /projects/xxx/vendor/dama/doctrine-test-bundle/src/DAMA/DoctrineTestBundle/Doctrine/DBAL/StaticDriver.php(140): PDO->rollBack()
#1 /projects/xxx/vendor/dama/doctrine-test-bundle/src/DAMA/DoctrineTestBundle/PHPUnit/PHPUnitExtension.php(25): DAMA\DoctrineTestBundle\Doctrine\DBAL\StaticDriver::rollBack()
#2 /projects/xxx/vendor/phpunit/phpunit/src/Runner/Hook/TestListenerAdapter.php(128): DAMA\DoctrineTestBundle\PHPUnit\PHPUnitExtension->executeAfterTest('Tests\\XXX\\Appli...', 0.28703999519348)
#3 /projects/xxx/vendor/phpunit/phpunit/src/Framework/TestResult.php(423): PHPUnit\Runner\TestListenerAdapter->endTest(Object(Tests\XXX\Application\Website\Controller\Article\DetailControllerTest), 0.28703999519348)
#4 /projects/xxx/vendor/phpunit/phpunit/src/Framework/TestResult.php(933) in /projects/xxx/templates/website/base.html.twig on line 143

Reverting back to doctrine/dbal:2.9.2 makes my testsuite run again.

Probably the few(er) changes in https://github.com/doctrine/dbal/releases/tag/v2.9.3 (in stead of v2.10.0) narrows the problem down, as both new releases throw the same exception.

dmaicher commented 4 years ago

I just checked and the functional tests of the bundle itself are passing fine with 2.10 :confused:

Also on one of my work projects it works fine with 2.10 and mysql 5.7.

Which db are you using?

evertharmeling commented 4 years ago

Hmmm SERVER_VERSION: "5.5.5-10.1.41-MariaDB-1~bionic", would that be the problem then...

dmaicher commented 4 years ago

Can you check if reverting this change makes a difference?

https://github.com/doctrine/dbal/commit/a9b7bf855c6f391f96773f844d7158500394bcef#diff-7b1e94fb7dd408985f7b7973330dc2edR359

evertharmeling commented 4 years ago

Yep, removing this line https://github.com/doctrine/dbal/commit/a9b7bf855c6f391f96773f844d7158500394bcef#diff-7b1e94fb7dd408985f7b7973330dc2edR359 makes my testsuite run again...

jaikdean commented 4 years ago

After upgrading from 2.9.2 to 2.10.0 we've run into an issue calling $em->refresh($entity); in our test suite. After making a request that updates the entity's values in the database then calling refresh() we don't see any exceptions, but the entity doesn't reflect the changes that were made.

We've checked logs for the same test in 2.9.2 and 2.10.0 and didn't see any difference.

Removing the line mentioned above doesn't seem to fix the issue in our case.

Any ideas on what we could try to investigate this further?

dmaicher commented 4 years ago

@jaikdean does it work with 2.9.3?

Also @jaikdean @evertharmeling which version of this bundle are you actually using?

marliotto commented 4 years ago

Removing this line https://github.com/doctrine/dbal/commit/a9b7bf855c6f391f96773f844d7158500394bcef#diff-7b1e94fb7dd408985f7b7973330dc2edR359 fixes the issue. dama/doctrine-test-bundle v5.0.2 and v6.2.0 doctrine/dbal v2.10.0

dmaicher commented 4 years ago

unfortunately so far I cannot reproduce the issue :cry: @evertharmeling is trying to investigate further

jaikdean commented 4 years ago

@dmaicher Our particular issue isn't there with doctrine/dbal:2.9.3. We're using dama/doctrine-test-bundle:6.2.0.

I'll try to come up with a test case and will update with anything I can figure out, we just have a lot of moving parts to look at on this project.

evertharmeling commented 4 years ago

Also @jaikdean @evertharmeling which version of this bundle are you actually using?

Using the latest, dama/doctrine-test-bundle:6.2.0

evertharmeling commented 4 years ago

Adding the config:

dama_doctrine_test:
    enable_static_connection: false

Solves my problem, which seems logically as I'm testing an internal controller via the KernelBrowser and that also invokes it 'externally' via HTTP, as the https://github.com/dmaicher/doctrine-test-bundle#behat state. But not sure if it's a legit fix...?

Why only one test fails in my suite, could be because in the setUp method I add 2 entities separately (the relation is not configured as 'persisting') and therefor flush the EntityManager 2 times...

EDIT: Just refactored my unit test a bit by first adding the first entity as a DoctrineFixture, then query the entity in my setUp and related it to the second entity (so only 1 flush should happen in my unit test). And it still throws the same exception (when enable_static_connection: true the default), so no luck there.

dmaicher commented 4 years ago

Using enable_static_connection: false essentially disables the main purpose of the bundle and disables transaction roll-back on tests. So we should find a proper fix :smile:

@evertharmeling can you try this change and see if there was an exception thrown in your case that was silenced?

https://github.com/dmaicher/doctrine-test-bundle/compare/small_cleanup?expand=1#diff-e67e9cb1cc462500be1b82d2d5f56fe0L129

I tried one more private project and also symfony/demo and I cannot reproduce any errors unfortunately :confused:

evertharmeling commented 4 years ago

Ah ok :)

@evertharmeling can you try this change and see if there was an exception thrown in your case that was silenced?

Unfortunately not, the exception is thrown when calling $con->rollBack(); (src/DAMA/DoctrineTestBundle/Doctrine/DBAL/StaticDriver.php:140) in my case.

evertharmeling commented 4 years ago

I've tried to rewrite my testsuite but unfortunately without luck... Also tried to reproduce it on a symfony/demo but couldn't. So I guess if I'm the only one with this specific problem it's probably related to the setup of my testsuite and it's fine to close this issue... Don't have the time atm to dive in it any further. Thanks again for the fast follow up and your time!

dmaicher commented 4 years ago

@marliotto @jaikdean any chance you can provide a reproducer?

bobdenotter commented 4 years ago

Hi all,

Just chiming in, we've bumped into the same issue. Details here: https://github.com/bolt/core/issues/702

I can confirm commenting the line $this->transactionNestingLevel = 0; "fixes" it.

Modifying beginTransaction in StaticDriver to this:

    public static function beginTransaction(): void
    {
        foreach (self::$connections as $con) {
            $con->beginTransaction();
        }
    }

Still gives the error.

Additional notes: first broke on Doctrine/dbal 2.9.3, also tried 2.10.0.. Initially had it locked on dama/doctrine-test-bundle 5.x (to keep parity with SF 4), tried updating it to 6.2, but the results are unchanged.

jaikdean commented 4 years ago

@marliotto @jaikdean any chance you can provide a reproducer?

I've tried and failed to create one. Our issue might be a side effect of something else in our app or test suite that's wrong.

dmaicher commented 4 years ago

The issue on bolt/core was related to running DDL statements during tests and implicitly committing the open transaction on the connection.

See https://github.com/bolt/core/pull/715

evertharmeling commented 4 years ago

The issue on bolt/core was related to running DDL statements during tests and implicitly committing the open transaction on the connection.

See bolt/core#715

Ah this is probably my problem too, I will check it out tomorrow!

evertharmeling commented 4 years ago

Unfortunately it isn't, was already running dropping/creating database in custom Extension class (which used the BeforeFirstTestHook implementation), but isolating it via a bootstrap still throws the same exception 😞

dmaicher commented 4 years ago

@evertharmeling then I would need a reproducer :cry: Otherwise nothing I can do.

evertharmeling commented 4 years ago

Sure I get it, as I commented before, if I remain the only one with the problem it's fine to close this ;)

binarious commented 4 years ago

You're not the only one. I was hit by this today, too. Every functional test that calls flush() of the EntityManager raises this error for me. I'm currently trying to extract a simple test case.

binarious commented 4 years ago

Got one. This one does it for me: https://gist.github.com/binarious/51044562e232fce0a91940188287a6e8

The combination of static::createClient() and $this->em->flush() seems to be problematic. If one of these lines is removed, the error goes away. doctrine/dbal 2.9.2 works. 2.9.3+ not.

Output:

2019-11-14T10:35:45+01:00 [debug] "START TRANSACTION"
2019-11-14T10:35:45+01:00 [debug] "START TRANSACTION"
2019-11-14T10:35:46+01:00 [debug] "START TRANSACTION"
2019-11-14T10:35:46+01:00 [debug] INSERT INTO name (name) VALUES (?)
2019-11-14T10:35:46+01:00 [debug] "COMMIT"
PHP Fatal error:  Uncaught PDOException: There is no active transaction in /private/tmp/dama-error/vendor/dama/doctrine-test-bundle/src/DAMA/DoctrineTestBundle/Doctrine/DBAL/StaticDriver.php:140
evertharmeling commented 4 years ago

Nice catch @binarious! At least I can confirm that I'm also using the KernelBrowser in combination with the EntityManager in my failing test.

dmaicher commented 4 years ago

I can reproduce it now :+1: investigating what is going on

dmaicher commented 4 years ago

@evertharmeling @binarious @jaikdean can you test this fix?

https://github.com/dmaicher/doctrine-test-bundle/compare/issue-94?expand=1

I might have found a solution for the problem

binarious commented 4 years ago

Confirmed working. Thanks for the super quick fix, @dmaicher!

PHPUnit 8.4.3 by Sebastian Bergmann and contributors.

2019-11-14T13:58:36+01:00 [debug] "START TRANSACTION"
2019-11-14T13:58:36+01:00 [debug] "SAVEPOINT"
2019-11-14T13:58:36+01:00 [debug] INSERT INTO name (name) VALUES (?)
2019-11-14T13:58:36+01:00 [debug] "RELEASE SAVEPOINT"
.                                                                   1 / 1 (100%)

Time: 80 ms, Memory: 10.00 MB

OK (1 test, 1 assertion)
evertharmeling commented 4 years ago

Confirmed working too! Nice job @dmaicher!

jaikdean commented 4 years ago

Sterling work, that's fixed the issue for us too, @dmaicher

dmaicher commented 4 years ago

Perfect :blush: Thanks for the fast feedback.

I will release this tomorrow as patch releases 5.0.4 (just revived 5.x https://github.com/dmaicher/doctrine-test-bundle/pull/96) and 6.2.1.