Problem/Motivation

Some reading: https://phpunit.de/manual/current/en/incomplete-and-skipped-tests.html#i...

So the super-duper jaw-dropping feat of #2304461: KernelTestBaseTNG™ introduced a new behavior to PHPUnit based tests in Drupal: If you don't specify a SIMPLETEST_DB environment variable, some tests will fail.

We're not testing whether the dependency is present, we're testing whether the code works, so therefore the test should skip rather than throw an exception.

Proposed resolution

Modify the tests to skip if the environmental variable isn't present.

Remaining tasks

User interface changes

API changes

Data model changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Mile23 created an issue. See original summary.

Mile23’s picture

Status: Active » Needs review
FileSize
1.61 KB

Just skip rather than throw an exception.

Leave the exception if the environmental variable is present, but is a bad URL.

dawehner’s picture

Do you mind posting the before and after?

On top of that I'm curious why you haven't changed SIMPLETEST_BASE_URL yet.

Mile23’s picture

Ah nice, it still applies. :-)

On top of that I'm curious why you haven't changed SIMPLETEST_BASE_URL yet.

Not sure why I need to. The problem is that tests shouldn't fail when an environmental variable is missing, because we're testing code, not environmental variables.

Before:

[...]
19) Drupal\Tests\system\Kernel\PhpStorage\PhpStorageFactoryTest::testGetOverride
InvalidArgumentException: There is no database connection so no tests can be run. You must provide a SIMPLETEST_DB environment variable, like "sqlite://localhost//tmp/test.sqlite", to run PHPUnit based functional tests outside of run-tests.sh.

/Users/paul/pj2/drupal/core/tests/Drupal/KernelTests/KernelTestBase.php:417
/Users/paul/pj2/drupal/core/tests/Drupal/KernelTests/KernelTestBase.php:287
/Users/paul/pj2/drupal/core/tests/Drupal/KernelTests/KernelTestBase.php:221
/Users/paul/pj2/drupal/core/modules/system/tests/src/Kernel/PhpStorage/PhpStorageFactoryTest.php:29

FAILURES!
Tests: 8591, Assertions: 16929, Errors: 19, Skipped: 4.
Grant:core paul$ 

After:

[...]
............................................................. 8113 / 8593 ( 94%)
............................................................. 8174 / 8593 ( 95%)
............................................................. 8235 / 8593 ( 95%)
............................................................. 8296 / 8593 ( 96%)
............................................................. 8357 / 8593 ( 97%)
............................................................. 8418 / 8593 ( 97%)
............................................................. 8479 / 8593 ( 98%)
............................................................. 8540 / 8593 ( 99%)
..............................SSSSSSSSSSSSSSSSSSSSSSS

Time: 1.69 minutes, Memory: 307.00Mb

OK, but incomplete, skipped, or risky tests!
Tests: 8591, Assertions: 16929, Skipped: 23.
Grant:core paul$ 
Mile23’s picture

Add -v to get the error:

$ ./vendor/bin/phpunit --filter PhpStorageFactoryTest -v
PHPUnit 4.8.6 by Sebastian Bergmann and contributors.

Runtime:	PHP 5.5.26
Configuration:	/Users/paul/pj2/drupal/core/phpunit.xml.dist

EEE

Time: 5.95 seconds, Memory: 126.25Mb

There were 3 errors:

1) Drupal\Tests\system\Kernel\PhpStorage\PhpStorageFactoryTest::testGetNoSettings
InvalidArgumentException: There is no database connection so no tests can be run. You must provide a SIMPLETEST_DB environment variable, like "sqlite://localhost//tmp/test.sqlite", to run PHPUnit based functional tests outside of run-tests.sh.

/Users/paul/pj2/drupal/core/tests/Drupal/KernelTests/KernelTestBase.php:417
/Users/paul/pj2/drupal/core/tests/Drupal/KernelTests/KernelTestBase.php:287
/Users/paul/pj2/drupal/core/tests/Drupal/KernelTests/KernelTestBase.php:221
/Users/paul/pj2/drupal/core/modules/system/tests/src/Kernel/PhpStorage/PhpStorageFactoryTest.php:29

2) Drupal\Tests\system\Kernel\PhpStorage\PhpStorageFactoryTest::testGetDefault
InvalidArgumentException: There is no database connection so no tests can be run. You must provide a SIMPLETEST_DB environment variable, like "sqlite://localhost//tmp/test.sqlite", to run PHPUnit based functional tests outside of run-tests.sh.

/Users/paul/pj2/drupal/core/tests/Drupal/KernelTests/KernelTestBase.php:417
/Users/paul/pj2/drupal/core/tests/Drupal/KernelTests/KernelTestBase.php:287
/Users/paul/pj2/drupal/core/tests/Drupal/KernelTests/KernelTestBase.php:221
/Users/paul/pj2/drupal/core/modules/system/tests/src/Kernel/PhpStorage/PhpStorageFactoryTest.php:29

3) Drupal\Tests\system\Kernel\PhpStorage\PhpStorageFactoryTest::testGetOverride
InvalidArgumentException: There is no database connection so no tests can be run. You must provide a SIMPLETEST_DB environment variable, like "sqlite://localhost//tmp/test.sqlite", to run PHPUnit based functional tests outside of run-tests.sh.

/Users/paul/pj2/drupal/core/tests/Drupal/KernelTests/KernelTestBase.php:417
/Users/paul/pj2/drupal/core/tests/Drupal/KernelTests/KernelTestBase.php:287
/Users/paul/pj2/drupal/core/tests/Drupal/KernelTests/KernelTestBase.php:221
/Users/paul/pj2/drupal/core/modules/system/tests/src/Kernel/PhpStorage/PhpStorageFactoryTest.php:29

FAILURES!
Tests: 3, Assertions: 0, Errors: 3.
mikeker’s picture

@Mile23, I agree with you on principle that we should test code, not environment. But having a database is pretty much a given for Drupal dev work. Is it a good idea to (somewhat) hide the fact that a user's environment is not setup to correctly test Drupal core? I can see novice contributors submitting patches that they thought passed (ie: nothing failed) when, instead, they just skipped the relevant tests.

We *should* change the error message to point to a d.o doc page since "sqlite://localhost//tmp/test.sqlite" only works for those with sqlite setup on their localhost like that...

Mile23’s picture

But having a database is pretty much a given for Drupal dev work.

That is precisely the problem I want to fix. :-)

Is it a good idea to (somewhat) hide the fact that a user's environment is not setup to correctly test Drupal core?

This doesn't do that. It shows you that the tests were skipped. I think it's better to educate people as to what that means rather than have them figure out why their passing tests fail.

For instance, if I make a module and it has unit tests, I don't need a database to test it. PHPUnit will falsely *fail* my tests even though they pass. I have to use --group or --filter or whatever, or provide an unneeded database.

So basically, everyone has to learn how to do things, including adding -v to their test so it fails skipped ones.

We *should* change the error message to point to a d.o doc page since "sqlite://localhost//tmp/test.sqlite" only works for those with sqlite setup on their localhost like that...

Yes, that's a step in the right direction. However, if you fail the tests because there is *no* sqlite setup then you are testing the wrong thing.

What should really happen here is that we use the kernel's environment parameter properly, but I think D8 can't deal with that quite yet.

mikeker’s picture

That is precisely the problem I want to fix. :-)

Ha! Fair point.

For instance, if I make a module and it has unit tests, I don't need a database to test it. PHPUnit will falsely *fail* my tests even though they pass. I have to use --group or --filter or whatever, or provide an unneeded database.

That's the most persuasive argument to me. Though technically it doesn't fail *your* tests, the overall test run fails because *other* tests fail. But decoupling is a good.

I've started a skipped-tests section of the phpunit docs page and added it to the skipped test message in this patch.

mikeker’s picture

Status: Needs review » Reviewed & tested by the community

I'm comfortable marking this RTBC since I only changed the message string. Also, this is very similar to #2478247: SIMPLETEST_BASE_URL is an environmental requirement which should not fail tests which was fixed, so we should skip instead of error just for consistency!

Mile23’s picture

mikeker++

:-)

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 2fdc2ea and pushed to 8.0.x. Thanks!

  • alexpott committed 2fdc2ea on 8.0.x
    Issue #2554185 by mikeker, Mile23: Tests should skip if they don't have...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.