Functional tests don't use SIMPLETEST_DB environmental variable as expected [#2987124]

Problem/Motivation

The site-under-test will use the database settings in settings.php and not those from the SIMPLETEST_DB. This is a major bug because you think you are testing against one database and in reality your testing against another. If you have no existing settings.php the correct database is used - which is fortunate because that's how DrupalCI is working.

Steps to reproduce:

Install Drupal using a MySQL database
Run SIMPLETEST_DB="pgsql://username:password@127.0.0.1/drupal8alt" sudo -u _www ./vendor/bin/phpunit -v -c core core/modules/system/tests/src/Functional/System/StatusTest.php --filter testStatusPage ensuring that browser output is set up correctly
View last browser output page generated by the test - you will see the site status report and the DB Driver will be MySQL.

Delete your settings.php and repeats steps to reproduce and you'll see that Postgres is now being used!??!?!?

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Comment	File	Size	Author
#12	2987124-12.patch	4.26 KB	alexpott
#12	8.7.x: PHP 7.1 & MySQL 5.7 Patch Failed to Apply 8.6.x: PHP 7 & MySQL 5.5 24,244 pass
#12	7-12-interdiff.txt	1.06 KB	alexpott
#7	2987124-7.patch	4.08 KB	alexpott
#7	8.7.x: PHP 7.1 & MySQL 5.7 24,248 pass PHP 7.1 & PostgreSQL 9.1 24,169 pass PHP 7.1 & SQLite 3.15 24,169 pass
#7	4-7-interdiff.txt	1.41 KB	alexpott
#4	2987124-4.patch	2.67 KB	alexpott
#4	8.7.x: PHP 7.1 & MySQL 5.7 24,056 pass, 185 fail PHP 7.1 & PostgreSQL 9.1 24,056 pass, 177 fail PHP 7.1 & SQLite 3.15 24,054 pass, 185 fail
#4	2987124-4.test-only.patch	1.89 KB	alexpott
#4	8.7.x: PHP 7.1 & MySQL 5.7 24,168 pass, 1 fail PHP 7.1 & PostgreSQL 9.1 24,168 pass, 1 fail PHP 7.1 & SQLite 3.15 24,168 pass, 1 fail
#3	2987124-3.patch	795 bytes	alexpott
#3	8.7.x: PHP 7.1 & MySQL 5.7 24,055 pass, 182 fail

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

20 July 2018 at 13:34

alexpott created an issue. See original summary.

Comment #2

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott commented 20 July 2018 at 13:39

Issue summary:

View changes

Comment #3

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott commented 20 July 2018 at 14:04

Status:

Active

» Needs review

File	Size
2987124-3.patch	795 bytes
8.7.x: PHP 7.1 & MySQL 5.7 24,055 pass, 182 fail

So fixing it is simple - and makes sense if you look at the \Drupal\Core\Database\Database::addConnectionInfo() code - it won't add anything if the connection is already there.

How to test?

Comment #4

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott commented 20 July 2018 at 14:25

File	Size
2987124-4.test-only.patch	1.89 KB
8.7.x: PHP 7.1 & MySQL 5.7 24,168 pass, 1 fail PHP 7.1 & PostgreSQL 9.1 24,168 pass, 1 fail PHP 7.1 & SQLite 3.15 24,168 pass, 1 fail
2987124-4.patch	2.67 KB
8.7.x: PHP 7.1 & MySQL 5.7 24,056 pass, 185 fail PHP 7.1 & PostgreSQL 9.1 24,056 pass, 177 fail PHP 7.1 & SQLite 3.15 24,054 pass, 185 fail

Given the importance of knowing that this works I think we should use mocking and reflection to get at the protected method.

Comment #5

20 July 2018 at 14:49

The last submitted patch, 3: 2987124-3.patch, failed testing. View results

Comment #6

20 July 2018 at 15:03

Status:

Needs review

» Needs work

The last submitted patch, 4: 2987124-4.patch, failed testing. View results

Comment #7

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott commented 20 July 2018 at 15:07

Status:

Needs work

» Needs review

File	Size
4-7-interdiff.txt	1.41 KB
2987124-7.patch	4.08 KB
8.7.x: PHP 7.1 & MySQL 5.7 24,248 pass PHP 7.1 & PostgreSQL 9.1 24,169 pass PHP 7.1 & SQLite 3.15 24,169 pass

Nice - our upgrade path tests only work because whilst we call changeDatabasePrefix() twice because we're not removing the db connection everything works. We shouldn't be calling this twice. In HEAD we do once from \Drupal\FunctionalTests\Update\UpdatePathTestBase::setUp() and then later in the same method we call prepareEnvironment() which also calls it.

Comment #8

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented 20 July 2018 at 17:08

+++ b/core/lib/Drupal/Core/Test/TestSetupTrait.php
@@ -158,6 +158,9 @@ protected function changeDatabasePrefix() {
       Database::addConnectionInfo('default', 'default', $database);

I wonder if it would be cleaner to use a 'test_run' database connection rather than 'default', and then call Database::setActiveConnection('test_run');.

What do you think?

Comment #9

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott commented 20 July 2018 at 17:32

@amateescu that feels like way too big a change. For example if we did that any call to \Drupal\Core\Database\Database::getConnectionInfo() in tests will need to cater for this.

Comment #10

mondrake

🇮🇹

CreditAttribution: mondrake commented 26 July 2018 at 09:22

Status:

Needs review

» Reviewed & tested by the community

4 files were hidden/shown/deleted

File	Size
2987124-3.patch	795 bytes
8.7.x: PHP 7.1 & MySQL 5.7 24,055 pass, 182 fail
2987124-4.test-only.patch	1.89 KB
8.7.x: PHP 7.1 & MySQL 5.7 24,168 pass, 1 fail PHP 7.1 & PostgreSQL 9.1 24,168 pass, 1 fail PHP 7.1 & SQLite 3.15 24,168 pass, 1 fail
2987124-4.patch	2.67 KB
8.7.x: PHP 7.1 & MySQL 5.7 24,056 pass, 185 fail PHP 7.1 & PostgreSQL 9.1 24,056 pass, 177 fail PHP 7.1 & SQLite 3.15 24,054 pass, 185 fail
4-7-interdiff.txt	1.41 KB

AFAICS this is good to go. It's a bit of an hidden issue, but I could figure out in TravisCI builds where I install Drupal before running tests that a) the issue was there (I install Drupal with a db driver, than change the db driver to run tests and, in case of failure of functional tests, then the backtrace was indicating the db driver in use was the installation one) and b) this patch forces the test installer to use the driver setup passed by the SIMPLETEST_DB variable.

RTBC

Comment #11

plach

he/him

Italian

Venezia

CreditAttribution: plach as a volunteer commented 7 August 2018 at 09:04

```
+++ b/core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBase.php
@@ -161,7 +161,15 @@ protected function setUp() {
+    $this->prepareEnvironment();
```
Given what ::prepareEnvironment() is doing, it might be even beneficial to move it here, as we will get a pristine environment and possibly better error logging during DB setup.

+1

+++ b/core/tests/Drupal/Tests/Core/Test/TestSetupTraitTest.php
@@ -0,0 +1,42 @@
+  public function testChangeDatabasePrefix() {

Missing phpdoc?

+++ b/core/tests/Drupal/Tests/Core/Test/TestSetupTraitTest.php
@@ -0,0 +1,42 @@
+    putenv('SIMPLETEST_DB=pgsql://user:pass@127.0.0.1/db');

I guess this would override valid environment values, however this shouldn't matter given that this is a unit test.

Comment #12

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott commented 7 August 2018 at 09:11

File	Size
7-12-interdiff.txt	1.06 KB
2987124-12.patch	4.26 KB
8.7.x: PHP 7.1 & MySQL 5.7 Patch Failed to Apply 8.6.x: PHP 7 & MySQL 5.5 24,244 pass

File

Size

7-12-interdiff.txt

1.06 KB

2987124-12.patch

4.26 KB

8.7.x:
PHP 7.1 & MySQL 5.7 Patch Failed to Apply

8.6.x:
PHP 7 & MySQL 5.5 24,244 pass

@plach thanks for the review. Patch attached improves the test commentary in light of points 2 and 3.

Comment #13

plach

he/him

Italian

Venezia

CreditAttribution: plach as a volunteer commented 7 August 2018 at 10:10

Version:

8.7.x-dev

» 8.6.x-dev

Committed bacd657 and pushed to 8.7.x. Thanks!

Comment #14

7 August 2018 at 10:10

plach committed bacd657 on 8.7.x

Issue #2987124 by alexpott, mondrake: Functional tests don't use...

Comment #15

8 August 2018 at 09:56

Status:

Reviewed & tested by the community

» Needs work

The last submitted patch, 12: 2987124-12.patch, failed testing. View results

Comment #16

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott commented 8 August 2018 at 11:12

Status:

Needs work

» Reviewed & tested by the community

Lol rtbc re-test.

Comment #17

catch

he/him

English

CreditAttribution: catch at Third and Grove commented 9 August 2018 at 11:42

+1 backporting this to 8.6.x

Comment #18

9 August 2018 at 13:16

plach committed e3458a7 on 8.6.x

Issue #2987124 by alexpott, mondrake: Functional tests don't use...

Comment #19

plach

he/him

Italian

Venezia

CreditAttribution: plach as a volunteer commented 9 August 2018 at 13:16

Status:

Reviewed & tested by the community

» Fixed

Committed e3458a7 and pushed to 8.6.x. Thanks!

Comment #20

neclimdul

He/Him

CreditAttribution: neclimdul commented 9 August 2018 at 16:44

Ooooooh I thought I saw some tests running in the wrong database a while back but it didn't cause enough problem for me to dig in to it. It suddenly makes sense, it was broken! Awesome catch, thanks!