All CI runs are using the sqlite fallback connection instead of the expected driver #8532

acoulton · 2021-03-11T11:37:09Z

While writing a test for an unrelated issue, I noticed that the unit tests are not loading the expected database connection details. Regardless of the config file provided, the TestUtil class is providing the fallback sqlite driver.

As a result a) none of the code is being tested against other platforms and b) tests that rely on platform-specific features (e.g. pessimistic locking) are being skipped on all builds.

It looks like this has been the case ever since the migration from Travis CI. Therefore some of the consistently-skipped tests are also not valid with phpunit 9 - e.g. Doctrine\Tests\ORM\Functional\Locking\LockTest has two tests that fail due to using ->assertContains with a string argument, which need to be upgraded to ->assertStringContainsString.

The root cause is that when the driver-specific phpunit.xml files were updated for github actions, the various tmpdb_type / tmpdb_username etc with the config for the privileged temporary connection were removed. It looks like this might be valid for doctrine/dbal because it falls back to using the main db params however the TestUtil class in doctrine/orm does not and it has a much stricter list of required parameters before it will use the specified driver.

Update test configuration to prove the issue and hard-fail if the job is not using the expected driver
Either reinstate the extra variables in the phpunit.xml files or update TestUtil to match the behaviour of the doctrine/dbal version
Identify tests that fail once the actual drivers are being used and fix (upgrade to phpunit 9) as required.

I can finish this off, but could you advise whether you'd prefer me to add the missing vars to the phpunit.xml or update TestUtil to work as expected with the current configs?

acoulton · 2021-03-11T17:15:13Z

OK - I've now got this far enough to identify all the issues.

The more I think about it the more I think it's better to move towards structuring the config the way doctrine/dbal does so that the github actions and phpunit configs are equivalent (as they are now). So I've rejigged the logic and config key naming in TestUtil to work the same way. This will mean that if developers have a local phpunit.xml that was working with the older code, they'll have to rename some variables to get their connection working again. Obviously doesn't affect end-users at all.

There are still some naming differences between TestUtil here and in dbal, e.g. dbal has getPrivilegedConnectionParameters and getTestConnectionParameters where orm has getParamsForTemporaryConnection and getParamsForMainConnection. These are all private / internal that I can see, not sure if there's any value in making the naming more consistent between the projects?

There were 3 very minor phpunit failures that I've fixed in 7388aa7.

The only thing I'm stuck on is the GH7941 test which I'll annotate separately.

Obviously I will deal with coding standards, squashing up commits sensibly etc, just wanted to get a steer first.

tests/Doctrine/Tests/ORM/Functional/Ticket/GH7941Test.php

tests/Doctrine/Tests/TestUtil.php

greg0ire · 2021-03-11T21:29:23Z

Well… this is embarrassing… thankfully you spotted it before things became too hard to fix, although there are already pretty weird things like this type difference between pdo_mysql and mysqli. Anyway, great job with this!

I can finish this off, but could you advise whether you'd prefer me to add the missing vars to the phpunit.xml or update TestUtil to work as expected with the current configs?

I'm not sure why we had 2 sets of parameters in the first place, and I'd go with the second solution for the sake of simplicity.

Successful in 1s — 88.32% (+0.88%) compared to 23dc804

Nice coverage improvement!

acoulton · 2021-03-11T22:18:12Z

I'm not sure why we had 2 sets of parameters in the first place, and I'd go with the second solution for the sake of simplicity.

It looks like it's on the assumption you might have a less-privileged user for running the main tests and then a more privileged one to create / drop the databases and run setup etc. I'm not sure how crucial that is. Maybe dbal has some tests that need to run as a less-privileged user to assert permission handling or something and this came from there but isn't really necessary?

I'd thought maybe it was in case there were devs / environments where you were happy to provide limited-permission creds but couldn't give root access to the DB. But AFAICS the temporary connection is still actually required for every test run, and it needs root-like permissions to wipe and recreate if you're running more than once, so it looks a bit like complexity for no (remaining) purpose...

I can't see any case where it would make sense to be using a different driver or speaking to a different database platform though.

Again if you're happy to diverge from dbal it looks like I could just remove the whole temporary connection concept and just have a single set of connection parameters. That would further simplify things / reduce the scope for future unexpected behaviour.

greg0ire · 2021-03-11T23:18:34Z

@beberlei @morozov do you maybe have some context about this?

morozov · 2021-03-12T01:02:58Z

The privileged credentials are used to create a test database. The regular credentials should be scoped to the test database to run tests there.

greg0ire · 2021-03-12T06:52:41Z

The regular credentials should be scoped to the test database to run tests there.

Looks like they are not, since they were apparently used to do that creation. Also, what is the point of this? I know it's good to be close to a real situation, and people will scope restrict credentials in real life to avoid disasters, but I can't imagine what bugs we want to catch here, and there certainly can't be a disaster in the case of the CI. Maybe it's in the case of a local run on the development environment?

morozov · 2021-03-12T07:53:59Z

Another reason is that when the test starts, the test database will likely not exist, so if it's used as a parameter of the only connection, this only connection will fail. So we temporarily connect to some other database or don't use any (if the platform allows) and create the database to which the test connection will be able to connect.

greg0ire · 2021-03-12T08:08:03Z

So the question is: why does it currently work? Currently, we only have one connection, and it specifies the database.

For mysql and mariadb, there is a simple answer, the database is created because we specify its name as a standard env variable of the docker image:

orm/.github/workflows/continuous-integration.yml

Line 135 in 93f31d2

MYSQL_DATABASE: "doctrine_tests"

orm/.github/workflows/continuous-integration.yml

Line 135 in 93f31d2

MYSQL_DATABASE: "doctrine_tests"

For PostgreSQL, I have no idea why it works. We have one set of credentials:

orm/ci/github/phpunit/pdo_pgsql.xml

Lines 10 to 14 in 93f31d2

    
           <var name="db_driver" value="pdo_pgsql"/> 
        
           <var name="db_host" value="localhost" /> 
        
           <var name="db_user" value="postgres" /> 
        
           <var name="db_password" value="postgres" /> 
        
           <var name="db_dbname" value="doctrine_tests" />

And there is no mention on the database name in the github workflow.

UPD: apparently the database is dropped and re-created every time, so having it created with a Docker variable does not explain how it works for Mysql and MariaDB:

orm/tests/Doctrine/Tests/TestUtil.php

Lines 38 to 40 in 93f31d2

    
                * IMPORTANT: 
        
                * 1) Each invocation of this method returns a NEW database connection. 
        
                * 2) The database is dropped and recreated to ensure it's clean.

Ah I think I have a good explanation! Although we can indeed specify a database, it will not be passed on when creating the temporary connection:

orm/tests/Doctrine/Tests/TestUtil.php

Lines 162 to 171 in 93f31d2

    
           private static function getParamsForTemporaryConnection() 
        
           { 
        
               $connectionParams = [ 
        
                   'driver' => $GLOBALS['tmpdb_type'], 
        
                   'user' => $GLOBALS['tmpdb_username'], 
        
                   'password' => $GLOBALS['tmpdb_password'], 
        
                   'host' => $GLOBALS['tmpdb_host'], 
        
                   'dbname' => null, 
        
                   'port' => $GLOBALS['tmpdb_port'], 
        
               ];

@acoulton IMO you do not need to restore the tmp_ variables, it's good as is and would only be useful to a developer willing to run the tests on an instance that has other databases that might be precious.

acoulton · 2021-03-12T08:19:26Z

OK, so this is potentially a platform-specific thing, it did fail on the db not existing on an at least one of the builds, possibly postgres, earlier in this branch.

Then I updated TestUtil to the full method from dbal, which does this:

    private static function getParamsForTemporaryConnection()
    {
        if (isset($GLOBALS['tmpdb_driver'])) {
            return self::mapConnectionParameters($GLOBALS, 'tmpdb_');
        }

        $parameters = self::mapConnectionParameters($GLOBALS, 'db_');
        unset($parameters['dbname']);

        return $parameters;
    }

So basically if you don't provide explicit details for a privileged connection, it just uses the normal connection config but removes the dbname before connecting. That's how postgres is now able to connect before the database exists, but still only uses one set of credentials / config.

In orm, the public static function getTempConnection() is never called. The "temporary DB" params are only ever used to drop / create the database before the tests.

In dbal, the method is called public static function getPrivilegedConnection() and it is called from some of the Oracle tests. However it is only the oracle tests that actually provide different config for the tmpdb_ values. All other drivers in theory could use them but are in practice falling back to use the db_ values without the dbname and running all of the tests with root-level credentials.

acoulton · 2021-03-12T08:32:11Z

@greg0ire sorry my post crossed with your edit.

So yeah, AFAICS the only theoretical use for the tmpdb_ values would be to run tests locally against a db that contained other things. However as it stands you can run the tests with a less privileged-account but you still have to provide root permissions in the tmpdb_ values for most platforms.

That's because TestUtil always attempts to drop and recreate the database at the start of the run - https://github.com/doctrine/orm/blob/2.8.x/tests/Doctrine/Tests/TestUtil.php#L110 - that will ignore a failure on the DROP DATABASE command but it will throw if CREATE DATABASE fails.

So there's no scenario at the moment where you can actually run anything without giving doctrine root-level permissions in one or both of those configs. I can't see a case where:

You're sensitive about what else is in your database so you don't want doctrine to run the tests as root
But you don't mind giving doctrine root to run DROP DATABASE / CREATE DATABASE at the start of the test run

In my mind either you don't care about the db and doctrine can be root, or you care and doctrine can't run at all?

acoulton · 2021-03-12T08:47:03Z

I guess what I'm saying is, if this was my project I'd remove the entire tmpdb_ concept and all the code that supports it from TestUtil. And just use the main db_ config vars for everything, removing the dbname for the initial connection as now.

If the ambition is that a developer should also be able to run locally against a system they care about it would be better that they provide restricted db_ creds and a flag to tell doctrine to skip the DROP / CREATE step entirely on the basis they will manage creating / resetting the database outside the doctrine test process. But I'm not sure how crucial that is in the world of docker...

Or I can just tidy up what's here and leave tmpdb_ as optional but not required. Your call.

greg0ire · 2021-03-12T10:44:29Z

I can't see a case
You're sensitive about what else is in your database so you don't want doctrine to run the tests as root
But you don't mind giving doctrine root to run DROP DATABASE / CREATE DATABASE at the start of the test run

I think it hardly makes sense indeed. The only way I could see it having sense is if you decide to trust the code that does the create/drop because it rarely changes,and don't trust code from tests because it might not be as well vetted, and every new line makes it more prone to error. Or you don't want contributors to be able to write tests that perform admin tasks from a concrete tests. IMO these concerns are not super valid.

My call would be: let's remove it, but it might be just me. WDYT @beberlei @morozov ?

acoulton · 2021-03-12T10:53:07Z

OK, I have squashed and tidied what I have so far, which solves the actual current problems.

The outstanding questions are whether I should:

Remove the concept of tmpdb_
Remove the silent/automatic fallback to sqlite in favour of always requiring phpunit.xml to specify the driver you want. I think this would avoid the need for the separate EXPECT_DB_DRIVER check in CI and be more future-proof. I'd update phpunit.xml.dist so it works as-now "out of the box".

greg0ire · 2021-03-14T20:53:17Z

IMO these concerns are not super valid.

I've given this more thought, and I think you can compare this to something we do often as developers, which is installing a piece of software like follows:

make
sudo make install

I think the issue is not one of security, we are not assuming malice here, we are just being cautious with privileges, which can sometimes spare some disasters like this one.

I would be (bandwagon) fallacious of me to say that this suddenly makes these concerns valid, but we can't pretend this kind of practice is unheard of, quite the contrary. In the case of the CI, we are ourselves not following that best practice since apparently the main credentials are enough, but that mechanism gives contributors the option to follow it while developing. So I'd keep that tmpdb_, but maybe rename it to privileged_ or something like that

Remove the silent/automatic fallback to sqlite in favour of always requiring phpunit.xml to specify the driver you want. I think this would avoid the need for the separate EXPECT_DB_DRIVER check in CI and be more future-proof. I'd update phpunit.xml.dist so it works as-now "out of the box".

I agree it would be best to remove that fallback mechanism. 👍 I'm not sure about removing the EXPECT_DB_DRIVER check, hopefully other maintainers will chime in.

greg0ire · 2021-03-17T07:15:36Z

Hey @acoulton ! I discussed internally with Benjamin and Sergei, and got the answers you needed:

they agree with you on EXPECT_DB_DRIVER being overkill now, let's remove it
about the privileged creds, let's keep them, they might be necessary if we add an Oracle build in the future (which is totally doable on GA, see the DBAL), or for devs that want to use Oracle
they agree that the fallback mechanism can go 👍

Builds using the github actions phpunit.xml files were not properly recognising driver-specific configuration values, so were all falling back to use the in-memory sqlite database instead of the expected driver. This also meant a number of tests were skipped as they rely on functionality not available in sqlite. This commit addresses that by: * REMOVING the automatic fallback to the sqlite memory database - phpunit.xml must now always specify explicit parameters for the desired connection. * Displaying the active driver in the build output for visibility and debugging. * Changing the way TestUtil loads the database config in line with the equivalent logic in doctrine/dbal, and to support the way that the config is/was specified in the phpunit.xml files for CI. Note that this means a couple of the expected config variable names have changed. Developers that are using customised phpunit.xml files locally will need to update them to provide: * Database config variables if they want to use the sqlite/memory driver - see phpunit.xml.dist for details. * `db_driver` instead of `db_type` * `db_user` instead of `db_username` * `db_dbname` instead of `db_name` * And, if in use, the equivalent changes to the `tmpdb_` values The other change is that now if you provide any value for `db_driver` we will attempt to create that connection type and that will throw if other details (username / password / etc as required by the driver) are not provided. Previously providing partial configuration would cause TestUtil to silently fall back to the in-memory sqlite driver.

These tests had not been running in CI so missed the previous phpunit upgrade. Note that assertions in decimal/floaty values in GH7941Test have been changed to compare numerically with a reasonable level of precision, instead of using regex. This is because the types returned by the different drivers are inconsistent, and phpunit now only allows regex comparisons on strings.

To avoid confusion, the `tmpdb_` test config values are now named `privileged_db_` and better documented in the phpunit.xml.dist. The TestUtil class has been refactored to more closely mirror the structure and method / variable naming of the equivalent in doctrine/dbal. This does not introduce any significant functional changes. The only real difference is that the test output now prints the selected database driver the first time it is referenced, rather than repeating this through the test run.

acoulton · 2021-03-17T10:55:51Z

@greg0ire great - that's all done and tidied up now.

I thought it made sense to get rid of the redundant / arbitrary differences between the TestUtil class here and in doctrine/dbal (and their method etc naming is clearer) so have done that too - sorry that makes the diff a little bit harder to follow. The only real diff between those classes now is that we have removed the fallback connection and changed the naming of the tmpdb_ params - probably worth porting that the other way so that the two projects are consistent? I can do that if you agree.

greg0ire · 2021-03-17T18:50:26Z

Thanks a lot for the top-notch quality PR!

probably worth porting that the other way so that the two projects are consistent? I can do that if you agree.

Yes please 🙏

This commit follows doctrine/orm#8532 to reduce the potential for confusion / error when configuring database connections for unit testing. In particular it: * REMOVES the automatic fallback to the sqlite memory database - phpunit.xml must now always specify explicit parameters for the desired connection. * Displays the active driver in the build output for visibility and debugging. * RENAMES the previous `tmpdb_xxx` test config values to `privileged_db_xxx` to clarify their meaning and usage. Developers that are using customised phpunit.xml files locally will need to update them as follows: * Add new database config variables if they want to use the sqlite/memory driver - see phpunit.xml.dist for details. * Rename the variables for the temporary / privileged database connection if they are using it.

acoulton commented Mar 11, 2021

View reviewed changes

tests/Doctrine/Tests/ORM/Functional/Ticket/GH7941Test.php Outdated Show resolved Hide resolved

greg0ire reviewed Mar 11, 2021

View reviewed changes

tests/Doctrine/Tests/TestUtil.php Outdated Show resolved Hide resolved

acoulton force-pushed the bug-fix-ci-db-connection branch from 6c00742 to 35dbfb6 Compare March 12, 2021 10:47

greg0ire previously approved these changes Mar 14, 2021

View reviewed changes

acoulton dismissed greg0ire’s stale review via e96c64b March 17, 2021 09:49

acoulton force-pushed the bug-fix-ci-db-connection branch from 48a855a to f29f3e4 Compare March 17, 2021 09:59

acoulton added 3 commits March 17, 2021 10:31

acoulton force-pushed the bug-fix-ci-db-connection branch from dc08543 to 61c4a5d Compare March 17, 2021 10:36

greg0ire approved these changes Mar 17, 2021

View reviewed changes

greg0ire merged commit 47475f3 into doctrine:2.8.x Mar 17, 2021

acoulton mentioned this pull request Mar 17, 2021

Update test DB config: rename privileged db and require explicit driver doctrine/dbal#4550

Closed

acoulton deleted the bug-fix-ci-db-connection branch March 17, 2021 22:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

All CI runs are using the sqlite fallback connection instead of the expected driver #8532

All CI runs are using the sqlite fallback connection instead of the expected driver #8532

acoulton commented Mar 11, 2021 •

edited

acoulton commented Mar 11, 2021

greg0ire commented Mar 11, 2021 •

edited

acoulton commented Mar 11, 2021 •

edited

greg0ire commented Mar 11, 2021

morozov commented Mar 12, 2021

greg0ire commented Mar 12, 2021

morozov commented Mar 12, 2021

greg0ire commented Mar 12, 2021 •

edited

acoulton commented Mar 12, 2021

acoulton commented Mar 12, 2021 •

edited

acoulton commented Mar 12, 2021

greg0ire commented Mar 12, 2021

acoulton commented Mar 12, 2021

greg0ire commented Mar 14, 2021

greg0ire commented Mar 17, 2021

acoulton commented Mar 17, 2021

greg0ire commented Mar 17, 2021

All CI runs are using the sqlite fallback connection instead of the expected driver #8532

All CI runs are using the sqlite fallback connection instead of the expected driver #8532

Conversation

acoulton commented Mar 11, 2021 • edited

acoulton commented Mar 11, 2021

greg0ire commented Mar 11, 2021 • edited

acoulton commented Mar 11, 2021 • edited

greg0ire commented Mar 11, 2021

morozov commented Mar 12, 2021

greg0ire commented Mar 12, 2021

morozov commented Mar 12, 2021

greg0ire commented Mar 12, 2021 • edited

acoulton commented Mar 12, 2021

acoulton commented Mar 12, 2021 • edited

acoulton commented Mar 12, 2021

greg0ire commented Mar 12, 2021

acoulton commented Mar 12, 2021

greg0ire commented Mar 14, 2021

greg0ire commented Mar 17, 2021

acoulton commented Mar 17, 2021

greg0ire commented Mar 17, 2021

acoulton commented Mar 11, 2021 •

edited

greg0ire commented Mar 11, 2021 •

edited

acoulton commented Mar 11, 2021 •

edited

greg0ire commented Mar 12, 2021 •

edited

acoulton commented Mar 12, 2021 •

edited