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
Phpunit has some functionality for raising flags for tests that might cause problems. "Risky" tests. This was previously strict mode which cased some problems but the more recent versions of phpunit provide more fine grained control. Such flags can help identify problematic tests before they cause problems.
Proposed resolution
Enable some sane strict flags but don't enforce any failures yet.
Remaining tasks
none.
User interface changes
none.
API changes
none.
Beta phase evaluation
Issue category | Bug because we decide to be strict but we are not. |
---|---|
Issue priority | Major because this was a regression and we decided to be strict |
Unfrozen changes | Tests |
Disruption | Should be limited. Most phpunit tests do a good job already and we don't seem to be adding risky tests. |
Comment | File | Size | Author |
---|---|---|---|
#54 | add_some_sane-2105583-54.patch | 1.3 KB | neclimdul |
#54 | 2105583-54.interdiff.txt | 587 bytes | neclimdul |
Comments
Comment #1
dawehnerRemoved the strict tag as this the only way to allow debugging of that stuff.
Comment #2
pwolanin CreditAttribution: pwolanin commentedIt seems that phpunit allows you to add --strict on the command line, but no way to suppress a strict flag in the config. So, either we should do this, or anyone who wants to debug has to copy and edit the config file (or hack it temporarily).
Comment #3
dawehnerI am sorry but this is the worst idea ever.
Comment #4
tim.plunkettThis is a bummer, but the correct thing to do.
After this is committed, can it be set back to postponed? We can fix this for real once https://github.com/sebastianbergmann/phpunit/issues/914 happens.
Comment #5
dawehnerWell, Drupal 8 probably can't use phpunit 3.8 as it requires php 5.4
Comment #6
chx CreditAttribution: chx commentedComment #7
Mile23OK, so how about have 'needs to pass --strict' as a coding standard, set the testbots to use --strict, and remove it from phpunit.xml.dist?
#2057905: [policy, no patch] Discuss the standards for phpunit based tests
Comment #8
webchickThis bug report made me laugh out loud. Thanks for that. :)
Committed and pushed to 8.x. Thanks! Marking postponed, per #4.
I didn't quite follow #7 though? So we can restore this functionality in a different way?
Comment #9
tim.plunkettActually, what about this?
Then testbot will run them as strict.
(The fail patch reverts one of the fixes to make it strict).
Comment #10
dawehnerWell, it is a problem if tests in the testbot behave different then on local systems.
Comment #11
Mile23Yah, it's better to have consistency.
So: Farewell, --strict. It was nice knowing you. But maybe still have a PHPUnit coding standard about always using an assertion, for clarity, and also because you can't make a fail message without it.
Comment #12
tim.plunkettOkay, back to postponed, in case PHPUnit itself is ever fixed.
Comment #13
tim.plunkettPHPUnit fixed this https://github.com/sebastianbergmann/phpunit/commit/9101f51
But that isn't in stable yet and will be only in 3.4...
Comment #14
Mile23Drupal's composer.json has us up to PHPUnit 4.1.*.
Is strict a problem or not at this point, for debugging?
Could other more granular settings be used? As per: http://phpunit.de/manual/4.1/en/strict-mode.html
Over in #2298667: Code review for report generation we have some degree of contention on this issue, and really, that's not where we should be deciding it, since this issue exists.
Comment #15
sunPHPUnit 4's new strict mode applies different code coverage rules than before, which is not really documented yet. Sebastian Bergmann's recent speaker slide-deck mentions it, but doesn't explain it. This blog post attempts an explanation; short version:
Unless every function called by the tested code is specified in
@covers
, a test will be marked as "risky". — That's a very explicit level of code coverage reporting, which may be suitable for 100% stable code that isn't supposed to be changed anymore, but for Drupal, that's too much.I recommend to use the following, granular sub-options instead, to enable all aspects of
--strict
but explicitly disable the new "risky" behavior:Comment #16
Mile23Actually, it's not hard to fix those things, and what we're talking about here is which kind of errors should fail in the testbot.
If we're past the days when strict kills the debug process, then let's start catching errors in tests during CI, instead of after the fact like this: https://www.drupal.org/comment/8980283
Comment #17
Mile23Here are the strict rules for PHPUnit: https://phpunit.de/manual/4.1/en/strict-mode.html
Here are the rules for coverage: https://phpunit.de/manual/4.1/en/appendixes.configuration.html
I would argue that checkForUnintentionallyCoveredCode should be TRUE, rather than false as @sun suggests, because it only really affects coverage reports and that only seems to interest metrics nerds like myself.
But the rest will go a long way towards helping maintain the quality of the unit tests through the testbot, so that we can at least run a coverage report to figure out what to test next.
The main problem is that 'risky' doesn't equal 'fail,' which means no one will discover the errors of their ways.
Comment #18
neclimdulOh hey, look at this guy still sitting here. #2542486: Fix risky phpunit tests to cleanup "risky" tests this didn't catch since it never got finished.
And a patch. I added beStrictAboutChangesToGlobalState because it should be easy enough to be good about that now and checkForUnintentionallyCoveredCode true because i agree with Mile23
Comment #19
dawehnerI really don't like that one. If you have a void function you have a void function, don't you?
+1 about that
Comment #20
neclimdulHow would you have a void function for a test? The only one that sort of did this in our current suite was
SpecialAttributesRouteSubscriberTest::testOnRouteBuildingValidVariables()
because it asserts that an event handler doesn't throw a PHP warning when called with some valid values. There isn't an explict method for this in phpUnit but because phpUnit throws an exception in its error handler you can just assert that code is run after the method call. Everything else was basically doing something like:instead of
Comment #21
neclimdulNote: None of these changes affects testbot because as Mile23 noted,
To do that we'd need probably have update the test runner.
Comment #22
dawehnerWell, let's assume your method returns NULL always but you want to test the interaction with some storage backend. Testing a
::set()
method is a common example for that.Comment #23
neclimdulMock commands are assertions so mock the backend. I assume we already test a lot of set methods?
For things that don't use mocks generally there is either a connected get method you use to confirm set worked which you can assert or there is some behavior you have to test which implies an assertion on that behavior.
Comment #24
dawehnerOH I wasn't aware of that ...
Comment #25
neclimdulYeah, there are likely many current tests that are missing ->assertX methods but are covered for this by their mocks. This is a feature of phpUnit and by design specifically for this sort of case.
Comment #26
Mile23Yes, setting an expectation, like:
..counts as an assertion.
If you say
$mock->expects($this->any());
I'm pretty sure it doesn't count as an assertion.Also, if you have a void function you can say
assertNull()
, which asserts that the function is still void as expected.Review:
I'm generally of the opinion that stricter=better, but have you tried to create a coverage report with these settings?
./vendor/bin/phpunit --coverage-html ../../coverage
is an easy way.We'd probably need a follow-up to fix all the @covers errors. There are a lot. :-)
Comment #27
neclimdulcoverage worked. checkForUnintentionallyCoveredCode I actually check in my on CI so I would have caught that previously. I've made a couple patches on and off. ;)
And yeah... forceCoversAnnotation doesn't make sense for what they actually does:
Comment #28
neclimdulforceCoversAnnotation is sort of a double edged sword btw. Because we don't really use coverage a whole lot as a community the report is sort of in a weird place and I'm ok with this going either way.
On one hand, it can be really helpful to have it turned off and see what code should have had @covers entries. The downside being you get giant reports of every thing that didn't have a coverage tag and used a service. This is sometimes so large you have to inspect the page HTML to see all the entries.
On the other hand it is really handy to see only the tests explicitly covering the code you are looking at. Its terse and clear. The downside is that we aren't good about marking coverage so lots of code looks to have low coverage. @see previous hand for figuring out what's actually covering code.
Comment #29
Mile23If we'd been disciplined about it from the start, we'd want
forceCoversAnnotation
, but since we're haven't, we don't. :-)It can be useful to look at a coverage report and see which tests touch specific lines of code, even if it's because they have lazy multiple
@covers
, or don't have@uses
.Really that's the only way to see such things, since 'risky' from
checkForUnintentionallyCoveredCode
isn't a fail. And we can't fix them if we don't see them. In fact, it's the only way to decide if it's important.So: No
forceCoversAnnotation
but yescheckForUnintentionallyCoveredCode
.Comment #30
dawehnerPlease keep in mind that with BrowserTestBase and KernelTestBaseTNG you will run more than one class, this is for sure.
Comment #31
Mile23You mean functional tests, right? Yes. Different from unit tests. Functional tests should be marked as
@coversNothing
, because you're not testing behavior at the method or line-of-code level, you're testing behavior at a systems-interaction level.Comment #32
neclimdulComment #33
Mile23Ossum.
Turn on the strict stuff, please.
Comment #34
alexpottAre we sure that this is not going to create random fails depending on machine performance. Basically I'm asking are all our tests sized correctly?
Comment #35
neclimdulI don't have a good way to confirm that it wont but I'm fairly confident. The only tests at this point that even have @medium have it because of sleep calls. I don't know about going forward but the thing we should probably keep an eye out for causing random failures would tests doing lots of file operations. Scanning all of core or something.
I will say that I added the --enforce-time-limit option to my coverage reports. It runs in a vm sitting on a 6 year old vm server with a bunch of other development vms running and with xdebug and xdebug's coverage engine running it takes a moderate amount of time. It has not had any problem with the current sizing of tests.
Comment #36
alexpottSo we should have had a new issue after #8 because the rtbc patch is doing exactly the opposite of the issue title and summary. However that's all history.
The issue summary needs updating (and should record the potted history) and we need a new title.
Comment #37
neclimdulComment #39
dawehnerDoesn't that requires some pointless assertNull(someVoidFunction());
So this enables a time limit?
+1 if you can explicit opt out.
Comment #40
neclimdulphpunit/php-invoker
library which we currently dont. Removed it for now.An important point that may address all of your points though, all of these flags now only mark the test as risky. This is mostly just additional information helpful to developers manually testing and maybe someday in drupalci.
Comment #41
dawehnerAh I see, so in those cases it should at least have a
$this->assertNull($return);
.Good point!
I'm a bit confused about this particular statement ... won't this cause issues all over the place with our kernel tests? Did you actually ran all tests having xdebug enabled for code coverage?
Comment #42
neclimdulLost response...
Yes, run it on every commit. Yes, most of our tests treat everything as coverage of everything it touches. Only a couple people writing tests are very strict about their covers flags and there are _lots_ of old tests without them so this flags quite a bit even in our unit tests. However, it only does so when running coverage reports. Even with xdebug enabled, unless you're running one of the coverage options, its completely ignored.
Comment #43
neclimdulWow, so i run all the same options as command line arguments but somehow that's not behaving the same and there is actually an error in HEAD that popped up last month. Working on figuring out what's going on.
Comment #44
dawehner@neclimdul
So do you think we would one day add code coverage reports on the testbot?
Comment #45
neclimdulHere's the bug. #2597510: Simpletest unit test is failing to assert success case correctly.
Comment #46
neclimdul@dawehner I think it might be useful but it would be an incremental thing. We're not treating anything Risky as a failure yet so even then it would just have a bunch of R's in the runner.
Here's what things look like now.
Comment #47
Mile23I added the patch in #40 and did
./vendor/bin/phpunit -c core -v
, but I don't see Rs in those places. I got this (plus the verbose info):The error was
SimpletestPhpunitRunCommandTest
, reported above.Risky:
The skipped tests dealt with SIMPLETEST_DB because I forgot to supply that. :-)
Comment #48
Mile23Added this: #2597644: ComposerIntegrationTest::testAllModulesReplaced() doesn't test anything.
Comment #49
Mile23Re: #41:
If the test is whether something happened without throwing an exception, then that's kind of reasonable. But it also might leave us with a false pass, which is worse than a strict fail.
Comment #50
neclimdulanother #2597844: EntityManagerTest doesn't assert everything it thinks it does
Comment #51
neclimdullast one. #2597860: UpdateRegistryTest tests missing assertions.
Comment #52
neclimdulPatch rerolled with fix for #2597510: Simpletest unit test is failing to assert success case correctly. seems like since this patch is changing the behavior we can go ahead and fix it here I think.
Comment #53
neclimdulI'm going to backtrack on my previous opinion only slightly. checkForUnintentionallyCoveredCode is going to need to be false false for now. But only because it causes leaky problems with our current UnitTestCase implementation. As it is now, we need to add
to ever single unit test to make it work because methods on these classes are called in the setUp method and will be treated as unintentionally covered code. That's not reasonable so adding a follow up to sort this out.
Comment #54
neclimdulderp, didn't finish the todo comment.
Comment #55
Mile23Actually, it would be cool to avoid anything in a base test class that requires
setUp()
. We don't want to carry dependencies with us that we don't need. IIRC many or our tests are based on\PHPUnit_Framework_TestCase
for this reason, and the testing system can deal with that.But yah, definitely in a follow-up.
Also, I approve.
Comment #56
alexpottThis is a sensible improvement to PHPUnit testing. Committed adfca62 and pushed to 8.0.x and 8.1.x. Thanks!
Comment #59
klausiWe have now some risky tests appearing in Rules and PHPunit tells us that the global variable 'loader' is not the same after the test run for BanIpTest. Executed with:
Any hints where and how a global 'loader' variable is manipulated in Drupal phpunit tests appreciated!
Comment #60
chx CreditAttribution: chx at Smartsheet commentedcore/tests/bootstrap.php
has$loader = require __DIR__ . '/../../autoload.php';
in the global scope. Latercore/tests/Drupal/KernelTests/KernelTestBase.php
utilizes it$this->classLoader = $GLOBALS['loader'];
.Comment #61
neclimdulthis isn't using KTB but something similar sounds likely.
Comment #62
chx CreditAttribution: chx at Smartsheet commentedThe bootstrap is not KTB specific
core/phpunit.xml.dist:<phpunit bootstrap="tests/bootstrap.php" colors="true"
Comment #63
neclimdulSo what wasn't clear to me was when it got modified. But, looking at the error and how the error is reported I think has a clue.
The differing string is a hash of the object. So something inside the class loader is changing. Chx asked a important question of how this ever passed because the class loader should always be changing but I'm guessing a lot of class loading happens in some hidden step and everything just "worked." But, somehow in this contrib test something is different my guess is it has to do with traits because the only thing in the test is a call to a method on a trait but I'm not 100% on that.
Either way, having the class loader stored in a global variable is problematic for asserting global state. My preference would be to follow up and put the loader logic in a function and take it out of the global scope. I've tested this and it solves the problem.
Comment #64
Mile23BanIPTest
extendsRulesIntegrationTestBase
, which does its own stuff with namespaces, which is kind of interesting, in that it might be testing against classes which can't be resolved in a specific subset of namespaces.But I'd wager that at some point this isolated namespace gets sent off to be global in some way.
Or maybe not.
run-tests.sh
does the same trick as well. It saves the results of the autoload scan in a global so that it can be re-used if it's still there.We could modify
tests/bootstrap.php
and rename the global to$test_runner_autoloader_dont_overload_this_absurdly_long_name
or something...In an ideal world, everything would be properly injected.
Comment #65
neclimdulFilled follow up #2631478: bootstrap.php loader can show as changed global state
Comment #67
Wim LeersFix for #2597510: Simpletest unit test is failing to assert success case correctly. was included in #52, closed that as a duplicate.