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

Combining Sentry with this bundle breaks test when data is inserted #196

Closed bobvandevijver closed 2 years ago

bobvandevijver commented 2 years ago

Follow-up from #194.

It appears that using Sentry in combination with this bundle, the tests break when new data is inserted into the database, with the cryptic (probably PDO) error "There is no active transaction". Disabling Sentry in the test environment solves the issue for us.

It seemed the following caused the transaction to trigger:

class SomeTest extends KernelTestCase
{
  private Registry $doctrine;

  protected function setUp(): void
  {
    self::bootKernel();
    $this->doctrine = self::getContainer()->get('doctrine');
  }

  public function testSomething()
  {
    $entity = new Entity();

    $this->doctrine->getManager()->persist($entity);
    $this->doctrine->getManager()->flush();

    $this->assertTrue(true); // To avoid risky test warning
  }
}
dmaicher commented 2 years ago

Unfortunately I cannot reproduce it. I tried installing https://github.com/getsentry/sentry-symfony on one of my apps. Tests are passing fine.

In case you can provide a minimal reproducer please let me know. Until then I will close here for now.

Marmelatze commented 2 years ago

Same error for me with 6.7.3 in PostConnectEventListener the $connection->getDriver() is an instance of \Sentry\SentryBundle\Tracing\Doctrine\DBAL\TracingDriverForV2. I'm not sure if this is the problem?

dmaicher commented 2 years ago

Happy to look into it in case you can provide a small reproducer. Might indeed be related to the driver :thinking:

Can maybe be fixed with priorities somehow for decorating the doctrine dbal driver

Marmelatze commented 2 years ago

disabling sentry tracing for the test environment fixed it for me:

# config/test/sentry.yaml
sentry:
  tracing:
    enabled: false
dmaicher commented 2 years ago

Actually this seems hard to fix :confused:

The driver instance is replaced in a rather "hacky" way here:

https://github.com/getsentry/sentry-symfony/blob/master/src/Tracing/Doctrine/DBAL/ConnectionConfigurator.php#L43

With DBAL v3 this will be fine as Sentry is using middlewares then

Marmelatze commented 2 years ago

yes, just noticed when setting up a small project to reproduce, that it works with DBAL 3 :) Disabling sentry in test env is a suitable workaround (at least for me, as there is no error logging needed usually)

dmaicher commented 2 years ago

I still think that I need to fix this somehow as potentially decorating the driver might still happen in some other cases.

See https://github.com/dmaicher/doctrine-test-bundle/pull/197

Does this fix the issue for you with enabled Sentry tracing @Marmelatze ?

Marmelatze commented 2 years ago

Yes that works for me with enabled tracing.

dmaicher commented 2 years ago

I currently don't see a way to fix this :confused: In the end I decided to revert the fix that caused the issues. There are too many possible side-effects.

bobvandevijver commented 2 years ago

@dmaicher Thank you for your effort looking into this! Really appreciated 👏🏻

I still disabled Sentry tracing for our test environment (thank you for the suggestion @Marmelatze, I previously just disabled the whole bundle for test, but this has less impact).

Tofandel commented 2 years ago

Hmm I'm getting this issue as well, but

# config/test/sentry.yaml
sentry:
  tracing:
    enabled: false

doesn't fix it

I'm also getting several warnings

  2x: The "Sentry\SentryBundle\Tracing\Doctrine\DBAL\TracingDriverForV3" class implements "Doctrine\DBAL\VersionAwarePlatformDriver" that is deprecated All drivers will have to be aware of the server version in the next major release
.
    2x in AppTest::testDefaultRedirect from App\Tests

  1x: The "DAMA\DoctrineTestBundle\Doctrine\DBAL\AbstractStaticDriver" class implements "Doctrine\DBAL\VersionAwarePlatformDriver" that is deprecated All drivers will have to be aware of the server version in the next major release.
    1x in PHPUnitExtension::executeBeforeFirstTest from DAMA\DoctrineTestBundle\PHPUnit

Has there been any developement on this that hasn't made it to this thread?

namespace App\Tests;

use App\Repository\UserRepository;
use Symfony\Bundle\FrameworkBundle\Test\WebTestCase;

class AppTest extends WebTestCase
{
    public function testDefaultRedirect(): void
    {

        $userRepository = static::getContainer()->get(UserRepository::class);
        $userRepository->findOneByEmail('role_admin@test.com');
        $this->assertTrue(true);
      }
}

This is the kind of test that will trigger this error in my case (just fetching data)

dmaicher commented 2 years ago

@Tofandel what is the issue exactly for you? You can some There is no active transaction exception?

Tofandel commented 2 years ago

Yes this is the exact same error and if I disable sentry completely it's gone, but just disabling tracing doesn't solve it anymore

dmaicher commented 2 years ago

And you are using v6.7.5 of this bundle? :thinking:

Tofandel commented 2 years ago

Yes

            "name": "dama/doctrine-test-bundle",
            "version": "v6.7.5",
            "source": {
                "type": "git",
                "url": "https://github.com/dmaicher/doctrine-test-bundle.git",
                "reference": "af6f8e8c56fcfdf2ae039b97607883961a14af9c"
            },
            "name": "sentry/sentry-symfony",
            "version": "4.2.7",
            "source": {
                "type": "git",
                "url": "https://github.com/getsentry/sentry-symfony.git",
                "reference": "c231f5c6204fe9d428ab4db94ca4c9d1a9491e62"
            },
dmaicher commented 2 years ago

Can you provide a minimal reproducer?