Potential improvement: add support for custom PDO implementation #20

M1ke · 2021-02-03T11:37:12Z

This is quite a speculative PR so please consider it one for discussion rather than immediate merge or refusal either way. I thought it better to open as a PR rather than an issue so that we can discuss the code implications.

There's a new tool released that provides a fake implementation of PDO allowing for database driven tests to be run against essentially a fake database implemented in PHP. This could significantly reduce test runtimes by avoiding database network connections/sockets and by reducing disk i/o. It is currently limited in some ways - it doesn't support triggers or views, and some parsing is incomplete, but the core is there.

I have worked on implementing this in Codeception and have been able to run an initial database creation from a schema file, and have got to running tests. It fails at haveInDatabase for mysql specifically because the parser lacks support for show indexes but this can be worked on.

To implement it in the Codeception module I added a keyword fake: to the start of the DSN which gets stripped off and used to start the fake PDO driver rather than a real one. FakePdo uses static storage under the hood to represent a database, meaning that the same FakePdo created on module initialization would be available for userland access if test code injected FakePdo instead of real PDO into application code.

As far as I can see merging this would have no impacts unless someone chose to add fake: to the start of their DSN; at this point they'd need to have installed the vimeo/php-mysql-engine package or Codeception would crash on startup. I thought that preferable to making that package a dependency.

Let me know if this is something we might merge, or if there are better ways to implement this as an optional plugin without having to re-implement all the other features of this module.

M1ke · 2021-02-03T11:40:39Z

src/Codeception/Lib/Driver/Db.php

@@ -3,9 +3,12 @@
 namespace Codeception\Lib\Driver;

 use Codeception\Exception\ModuleException;
+use Vimeo\MysqlEngine\FakePdo;


As far as I am aware this won't break even if the package isn't installed

This change is no longer present

M1ke · 2021-02-03T18:01:58Z

Fast moving target; the php-mysql-enging package has a change queued up for next release that splits the class for PHP7 or PHP8; when that hits I'll update this PR if not yet merged, or follow up a new PR if this did get merged.

Naktibalda · 2021-02-03T22:27:44Z

I see a big problem here. This module does not interact with application which is being tested, but with database only.

How are you going to make application code use the same instance of FakePdo?

M1ke · 2021-02-03T23:31:26Z

Thanks for checking this out! So I might have mentioned it above, but the actual fake "server" is a static class so any instance of its use in the code (test harness or application) will use the same test data. It's worth also pointing out therefore that this will only work for integration tests where Codeception is calling application code - it won't work for functional or acceptance tests because there's a network request gap. However given that different suites may (and generally should) use different module configurations this is still usable for those types of test without negative effects on acceptance tests. Hope that makes sense, I've got a working demo that can set data in the database but just need to implement one more piece of SQL syntax in the parser to work with how Codeception does MySQL inserts.

…

On Wed, 3 Feb 2021, 10:28 pm Gintautas Miselis, ***@***.***> wrote: I see a big problem here. This module does not interact with application which is being tested, but with database only. How are you going to make application code use the same instance of FakePdo? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#20 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJLLCYFNCEACFMWDAYPCZ3S5HEW5ANCNFSM4XAWEKHA> .

Naktibalda · 2021-02-04T07:44:15Z

the actual fake "server" is a static class so any instance of its use in the
code (test harness or application) will use the same test data.

If application uses PDO directly, you have to make the same changes in application code as you made here and come up with a way to pass fake: DSN to application when running it in tests.
But a lot of applications don't use PDO directly, they use Doctrine, Eloquent, LaminasDb or some other ORM/abstraction layer.
How are you going to make them use FakePDO?

There will still be differences between behaviour of FakePDO and real mysql.
How does using FakePDO compare against using Sqlite with in-memory storage?

Finally, php-mysql-engine is so new that I won't merge this PR for at least a month until it stabilizes.
Disabled issues page is a big downside to me too.
You are welcome to publish your fork on Packagist.

…lass name. Also add option to config docs at the top

M1ke · 2021-02-04T09:48:34Z

But a lot of applications don't use PDO directly, they use Doctrine, Eloquent, LaminasDb or some other ORM/abstraction layer.
How are you going to make them use FakePDO?

Ultimately this will vary on a case-by-case basis. Many frameworks will support Dependency Injection and allow a PDO instance to be passed in to the framework itself, or in the case of an ORM, may allow the ORM to be injected and this also allows PDO to be injected. If a framework creates a PDO instance within the application then it's not possible to stub it out - however that would suggest the framework could have other problems being tested.

For example in the main (private) framework I maintain, our Codeception tests already inject a custom PDO library which has extra debug logging built in to analyse failed tests. So I imagine this is a use case people might already be handling. So far however we've never required that of the PDO instance in Codeception, because the module already provides debug output.

In short though - many frameworks can inject PDO, but if they can't then the whole concept of fake PDO is useless to them anyway.

There will still be differences between behaviour of FakePDO and real mysql.
How does using FakePDO compare against using Sqlite with in-memory storage?

Yes, this is true. Not everything is implemented. So it will not work for every case. A user can't necessarily plug and play with Fake PDO if their application makes a lot of use of triggers or views. However this is already the case with SqlLite - if your application utilises any specific features of one database (e.g. MySQL) then many commands will just fail with SqlLite. However, in the case of FakePdo it would be possible to fix these yourself - it stands to reason if you're developing a large PHP application that your team would be able to write PHP code to resolve incompatibilities, whereas if I meet a problem with SqlLite I likely lack the skills or ability to ask the maintainers to make it more compatible with MySQL.

Finally, php-mysql-engine is so new that I won't merge this PR for at least a month until it stabilizes.

These are very reasonable concerns. I've modified my implementation significantly in light of this.

The implementation now simply allows the module configuration to declare a config item pdo_class which can be a fully qualified class name for a child of \PDO. This way the module itself makes no recommendation of a particular tool or its usefullness/stability but instead allows a user to choose any implementation, or make their own, as they wish. If the config is not provided or is blank it will fall back to regular \PDO. It will throw a module exception if the provided class name does not exist as a class, or resolves to a class which does not extend \PDO.

Do you think this would be suitable for merging sooner?

…o modify class not exists to throw exception

M1ke

I've added a few notes to why I made certain decisions which should make reviewing easier.

M1ke · 2021-02-04T10:01:03Z

.gitignore

@@ -1,3 +1,4 @@
 /.idea/
 /vendor/
 /composer.lock
+composer.phar


I can remove this if needed but seemed sensible to include it

M1ke · 2021-02-04T10:01:47Z

src/Codeception/Lib/Driver/Db.php

+        }
+
+        if (!class_exists($pdo_class)){
+            throw new ModuleException(


Initially this returned the fallback value of \PDO::class but actually throwing an exception is more helpful to the user - they can resolve by correcting the class name or by simply removing the config item.

M1ke · 2021-02-04T10:02:21Z

src/Codeception/Lib/Driver/Db.php

-        $dbh = new \PDO($dsn, $user, $password, $options);
+        $class_name = self::pdoClass($pdo_class);
+        $dbh = new $class_name($dsn, $user, $password, $options);
+        self::assertIsPdo($dbh, $pdo_class);


This assert is necessary so we don't get something that accepts the right arguments but doesn't provided expected methods and/or fails expected typehints down the line.

M1ke · 2021-02-04T10:03:39Z

src/Codeception/Lib/Driver/Db.php

        $this->dbh->setAttribute(\PDO::ATTR_ERRMODE, \PDO::ERRMODE_EXCEPTION);

        $this->dsn = $dsn;
        $this->user = $user;
        $this->password = $password;
        $this->options = $options;
+        $this->pdo_class = $pdo_class;


Storing this the same as the rest so it can be passed back in if necessary (e.g. sqllite reconnection)

M1ke · 2021-02-04T10:04:12Z

src/Codeception/Module/Db.php

@@ -570,7 +571,8 @@ private function connect($databaseKey, $databaseConfig)

        try {
            $this->debugSection('Connecting To Db', ['config' => $databaseConfig, 'options' => $options]);
-            $this->drivers[$databaseKey] = Driver::create($databaseConfig['dsn'], $databaseConfig['user'], $databaseConfig['password'], $options);
+            $pdo_class = array_key_exists('pdo_class', $databaseConfig) ? $databaseConfig['pdo_class'] : null;


This adds a bit more bulk but without it a lot of tests fail due to notices.

DavertMik · 2021-02-09T19:48:32Z

Thanks, @M1ke
It's really an interesting library but I want to raise the same concern. Db module was designed to work with an external database, for acceptance tests mostly. While the intention of FakePDO is to repace PDO internally. The difference here that Db module is expected to have real data storage for tests and that it can be used with PhpBrowser or WebDriver tests.

So I like php-mysql-engine but I am not sure this is the right place to use it.

I expect the typical use case would be a PHP application that uses a lot of PDO statement and we need to have an integration tests for such kind of application. In this case same instance of FakePDO should be injected into app and into tests. For instance, creating a new connection from FakePDO is not an option as we must rely on the same connection an application already uses.

So my suggestion is following:

create a standalone module for FakePDO that follows the syntax of Db module
make FakePDO module configurable the way, a connection can be injected into it from an application. Maybe via connecting to dependency injection, or using closure? Depends on the application how to make it right.
publish it as an independent package but mention it in Codeception docs in Data section. Because this approach seems really good for some applications!

M1ke · 2021-02-09T20:02:08Z

Hi @DavertMik

Thanks for looking over this. As it stands this implementation doesn't prejudice the module to any specific PDO implementation. It simply allows the class to be named up front, rather than created in the module constructor as a fixed class name.

This would make it suitable for any PDO implementation, whilst avoiding coupling of the module to fake pdo or any other library.

In the specific case of fake PDO the change as made here will work "as is" for integration testing. Thus for the addition of a single config value we get the ability to use fake PDO or any other library.

If it's not possible to merge this then most likely I'll just fork it, as the work required to make a new module fully compatible would be quite high and require updates if this implementation changed. Whereas if this PR can be merged then it just makes this module slightly more flexible without any burden of extra maintenance.

M1ke · 2021-02-24T15:48:44Z

@DavertMik what do we think to merging this on the basis that it has no dependency on FakePDO (or any other PDO implementation) but that it simply allows an extra degree of flexibility for someone who would like to use an alternative PDO implementation (and without having any negative effect for those who don't)?

andrew-svirin · 2021-09-13T19:05:19Z

May be exists some alternative for pgsql?

M1ke added 3 commits February 3, 2021 09:24

Add fake pdo option to driver

1eec70d

ignore composer.phar in gitignore

a47ec7c

Ensure correct sub-driver is used when fake is chosen

d25537d

M1ke commented Feb 3, 2021

View reviewed changes

M1ke added 2 commits February 3, 2021 11:44

Spaces instead of tabs

d4e77bd

Spaces instead of tabs (again)

de7413c

M1ke mentioned this pull request Feb 3, 2021

Add Exec and Query methods to FakePDO vimeo/php-mysql-engine#6

Merged

M1ke added 3 commits February 4, 2021 09:34

Rebuild concept around pdo injection to allow class fqn to be provided

322f5ab

Add asserts to ensure we get an instance of PDO

762375a

Changed config 'pdo' to 'pdo_class' to make it clearer that it is a c…

36bea69

…lass name. Also add option to config docs at the top

M1ke added 2 commits February 4, 2021 09:52

Ensure phpunit tests pass by declaring optional config value, and als…

36d39c6

…o modify class not exists to throw exception

Another attempt to fix config issues, passes locally

4f15f4d

M1ke commented Feb 4, 2021

View reviewed changes

M1ke changed the title ~~Potential improvement: add support for fake PDO [not ready to merge]~~ Potential improvement: add support for custom PDO implementation Feb 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Potential improvement: add support for custom PDO implementation #20

Potential improvement: add support for custom PDO implementation #20

M1ke commented Feb 3, 2021

M1ke Feb 3, 2021

M1ke Feb 9, 2021

M1ke commented Feb 3, 2021

Naktibalda commented Feb 3, 2021

M1ke commented Feb 3, 2021 via email

Naktibalda commented Feb 4, 2021

M1ke commented Feb 4, 2021 •

edited

Loading

M1ke left a comment

M1ke Feb 4, 2021

M1ke Feb 4, 2021

M1ke Feb 4, 2021

M1ke Feb 4, 2021

M1ke Feb 4, 2021

DavertMik commented Feb 9, 2021

M1ke commented Feb 9, 2021

M1ke commented Feb 24, 2021

andrew-svirin commented Sep 13, 2021

Potential improvement: add support for custom PDO implementation #20

Are you sure you want to change the base?

Potential improvement: add support for custom PDO implementation #20

Conversation

M1ke commented Feb 3, 2021

M1ke Feb 3, 2021

Choose a reason for hiding this comment

M1ke Feb 9, 2021

Choose a reason for hiding this comment

M1ke commented Feb 3, 2021

Naktibalda commented Feb 3, 2021

M1ke commented Feb 3, 2021 via email

Naktibalda commented Feb 4, 2021

M1ke commented Feb 4, 2021 • edited Loading

M1ke left a comment

Choose a reason for hiding this comment

M1ke Feb 4, 2021

Choose a reason for hiding this comment

M1ke Feb 4, 2021

Choose a reason for hiding this comment

M1ke Feb 4, 2021

Choose a reason for hiding this comment

M1ke Feb 4, 2021

Choose a reason for hiding this comment

M1ke Feb 4, 2021

Choose a reason for hiding this comment

DavertMik commented Feb 9, 2021

M1ke commented Feb 9, 2021

M1ke commented Feb 24, 2021

andrew-svirin commented Sep 13, 2021

M1ke commented Feb 4, 2021 •

edited

Loading