Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#8 | 2554185-1-8.interdiff.txt | 1.06 KB | mikeker |
#8 | 2554185-8.patch | 1.64 KB | mikeker |
#2 | 2554185_1.patch | 1.61 KB | Mile23 |
Comments
Comment #2
Mile23Just skip rather than throw an exception.
Leave the exception if the environmental variable is present, but is a bad URL.
Comment #3
dawehnerDo you mind posting the before and after?
On top of that I'm curious why you haven't changed SIMPLETEST_BASE_URL yet.
Comment #4
Mile23Ah nice, it still applies. :-)
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:
After:
Comment #5
Mile23Add -v to get the error:
Comment #6
mikeker CreditAttribution: mikeker as a volunteer commented@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...
Comment #7
Mile23That is precisely the problem I want to fix. :-)
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.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.
Comment #8
mikeker CreditAttribution: mikeker as a volunteer commentedHa! Fair point.
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.
Comment #9
mikeker CreditAttribution: mikeker as a volunteer commentedI'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!
Comment #10
Mile23mikeker++
:-)
Comment #11
alexpottCommitted 2fdc2ea and pushed to 8.0.x. Thanks!