First tests might not be covered by the extension
Closed this issue · 11 comments
Hello there!
I try to develop a test bundle for Symfony, and I am using your extension with LiipFixturesBundle. I know, they seem uncompatible for some cases, but I'm pretty stubborn and we did not encountered these cases for now, so I continue!
Just a quick presentation to explain the flow of the bundle. It uses your PHPUnitExtension and extends from it to load some DataFixtures before the first test. Then, it creates a wrapper for a many things such as managing container to mock services and stuff, but all in one, it just manages services.
I noticed that during the loading of the fixtures, SQLite driver is use. Same thing for the first tests, it does not use your StaticDriver. So I try to investigate, why the StaticConnectionFactory is not used at the first place. I'm still investigating, maybe I will dig something useful :p
Anyway, I have a few question about your implementation. I noticed that the Doctrine bundle provides a wrapper_class configuration to wrap the Dbal connection. Wouldn't it be more appropriate to use this configuration to wrap the Doctrine\DBAL\Connection class rather than decorating the Doctrine\DBAL\Driver\Connection ?
I hope this issue will help somebody and if I find the bug by myself, I will post it here.
Cheers guys and thank you very much for this bundle. I discovered PHPUnit extension thanks to you.
I noticed that during the loading of the fixtures, SQLite driver is use. Same thing for the first tests, it does not use your StaticDriver. So I try to investigate, why the StaticConnectionFactory is not used at the first place. I'm still investigating, maybe I will dig something useful :p
so the StaticConnectionFactory will only be used when tests have been started. See https://github.com/dmaicher/doctrine-test-bundle/blob/master/src/DAMA/DoctrineTestBundle/PHPUnit/PHPUnitExtension.php#L15
Anyway, I have a few question about your implementation. I noticed that the Doctrine bundle provides a wrapper_class configuration to wrap the Dbal connection. Wouldn't it be more appropriate to use this configuration to wrap the Doctrine\DBAL\Connection class rather than decorating the Doctrine\DBAL\Driver\Connection ?
But what if people are already using a custom wrapper_class? This would break stuff for them. Decorating the driver connection is the more robust approach that will cause less issues.
so the
StaticConnectionFactorywill only be used when tests have been started. See https://github.com/dmaicher/doctrine-test-bundle/blob/master/src/DAMA/DoctrineTestBundle/PHPUnit/PHPUnitExtension.php#L15
The bug I noticed is that, during the test, the EntityManager uses directly the SQLite driver instead of the StaticDriver that should wrap it. So I try to figure out why it does not use the StaticConnectionFactory.
But what if people are already using a custom
wrapper_class? This would break stuff for them. Decorating the driver connection is the more robust approach that will cause less issues.
That's the reason, good point. I would rather decorate both the Doctrine\DBAL\Connection and Doctrine\DBAL\Driver\Connection to fix my bug. I will try something the next hour and let you know if it fixes anything.
Thanks for the quick response :)
Okay, after some pretty messy stuff, I'm almost sure that LiipFixturesBundle does not use the StaticDriver and I guess something somehow is cached somewhere.
I don't have the time to continue the investigation today, but I will come back tomorrow with a fix hopefully.
Ever if it's not your bundles that creates the bug, I hope it will be useful to somebody. :)
Sadly I failed to find the reason why the EntityManager or the Doctrine\DBAL\Connection points directly to the SQLite driver instead of the StaticDriver. The weird thing is that I looked at the cache and it seems correct. Your bundle overrides the default connection factory. Moreover, it is used in the early container loading.
As I am migrating from Symfony 3.4 to Symfony 4.4 on my company's project, I must move forward and compose with this bug for now. I open an issue on my bundle aswell and link this one with it.
Thanks for your patience, I hope I'll find the reason of this bug one day :)
PS: Decorating the Doctrine\DBAL\Connection made a beautiful error that I couldn't manage as my understanding of doctrine is pretty limited. Sorry :(
I will close here for now as I cannot see any issue with this bundle in your case
For the record, I solved it. Apparently some components were loaded before the PHPUnit runs the PHPUnitExtension::executeBeforeFirstTest. So the StaticDriver was not configured to keep static connection. By the time this function is called, some EntityManager and Connection were instanciated and therefore not registered in the StaticDriver.
I solved it by using a ConnectionFactory that decorates the StaticConnectionFactory and sets the keepStaticConnection before creating the connection.
https://github.com/richcongress/unit-bundle/blob/v2.0.0/Doctrine/TestConnectionFactory.php
Anyway thanks for your time, I love your bundle ;) See ya !
@NicolasGuilloux Hello, I think in a different context we have a similar problem of keepStaticConnection being false at the beginning of things and thus a connection is created without the StaticDriver.
My idea is totally different but ends up by setting the param to true before what is done currently.
Would you mind to give me some feedback here : https://github.com/jeanlucc/doctrine-test-bundle/commit/9d47c36b61cd8e43925dad1e19113aff33942700
In short I do it at bundle boot.
Hi @jeanlucc,
I'm not sure that it will be set before all DB manipulations as the boot function may be called after another boot function from another bundle.
This is why I used the TestConnectionFactory which will be called before all calls of the StaticDriver. As I know that the ConnectionFactory is the only classes that will create the DB connection and therefore use the StaticDriver, I think it is safe.
But I like you way to put in the configuration though. It is far less aggressive than what I did :p
Ok thank you for your feedback. I saw that your method was more safe/aggresive. For my use case the configuration seemed enough and I could not think of a use case (because I definitely don't know endless possibilities of Symfony that much) where a service would be instantiated before kernel have finished to boot with all its bundles. I guess if my solution does not work yours cannot fail :)
@NicolasGuilloux After looking into Symfony a bit more I'm convinced boot is not a suitable moment for everyone and "sadly" I did not find any moment between the moment we have the configuration and the moment the container can be used allowing me to execute this bit of code before any service instanciation (except putting it in an overidden boot method but that is not what I want).
So I stole your approach, (I hope you do not mind) and made it configurable https://github.com/jeanlucc/doctrine-test-bundle/commit/d65cc675f0ea485272d7f3604e7c04a3cb9ee7a4
My hypothesis is that once you can change the static driver yourself it should be this manual configuration at runtime that takes precedence.
I would like to have your approval before I submit this idea as a merge request. I will definitely cite you as a contributor of this (unless you don't want to).
Steal me, go on, it is OpenSource I don't actually own anything :D
You can actually create the PR and make it draft so we can comment it directly from the PR instead of this issue, which is supposed to be closed.
NB: I'm not an expert at all in Symfony. My voice is not very trusty I guess. I guess sir Daicher will check it when the PR will be submited