Updated: Comment 0

Problem/Motivation

In #2096595: Make all PHPUnit tests pass with --strict we enabled strict mode. Sadly the time does not stop if you are on a breakpoint inside your debugger so you end up with just a single second to do all your debugging.
This is a really great initiative to speed up our development :p

Proposed resolution

Remove the strict tag but still try to add as many assertions as possible.

Remaining tasks

User interface changes

API changes

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
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.

Comments

dawehner’s picture

Status:Active» Needs review
StatusFileSize
new434 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,397 pass(es).
[ View ]

Removed the strict tag as this the only way to allow debugging of that stuff.

pwolanin’s picture

It 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).

dawehner’s picture

So, either we should do this, or anyone who wants to debug has to copy and edit the config file (or hack it temporarily).

I am sorry but this is the worst idea ever.

tim.plunkett’s picture

Status:Needs review» Reviewed & tested by the community

This 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.

dawehner’s picture

Well, Drupal 8 probably can't use phpunit 3.8 as it requires php 5.4

chx’s picture

Component:simpletest.module» other
Mile23’s picture

OK, 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

webchick’s picture

Status:Reviewed & tested by the community» Postponed

This 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?

tim.plunkett’s picture

Status:Postponed» Needs review
StatusFileSize
new459 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,810 pass(es).
[ View ]
new1.05 KB
PASSED: [[SimpleTest]]: [MySQL] 58,413 pass(es).
[ View ]

Actually, what about this?
Then testbot will run them as strict.

(The fail patch reverts one of the fixes to make it strict).

dawehner’s picture

Well, it is a problem if tests in the testbot behave different then on local systems.

Mile23’s picture

Yah, 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.

tim.plunkett’s picture

Status:Needs review» Postponed

Okay, back to postponed, in case PHPUnit itself is ever fixed.

tim.plunkett’s picture

PHPUnit fixed this https://github.com/sebastianbergmann/phpunit/commit/9101f51

But that isn't in stable yet and will be only in 3.4...

Mile23’s picture

Issue summary:View changes
Status:Postponed» Needs work

Drupal'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.

sun’s picture

Component:other» phpunit

PHPUnit 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:

  beStrictAboutTestsThatDoNotTestAnything="true"
  beStrictAboutOutputDuringTests="true"
  beStrictAboutTestSize="true"
  checkForUnintentionallyCoveredCode="false"
  forceCoversAnnotation="true"  [optional]
Mile23’s picture

@sun: 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.

Actually, 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

Mile23’s picture

Here 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.

neclimdul’s picture

Issue summary:View changes
Status:Needs work» Needs review
Related issues:+#2542486: Fix risky phpunit tests
StatusFileSize
new706 bytes
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 99,741 pass(es).
[ View ]

Oh 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

dawehner’s picture

  1. +++ b/core/phpunit.xml.dist
    @@ -1,6 +1,12 @@
    +         beStrictAboutTestsThatDoNotTestAnything="true"

    I really don't like that one. If you have a void function you have a void function, don't you?

  2. +++ b/core/phpunit.xml.dist
    @@ -1,6 +1,12 @@
    +         beStrictAboutOutputDuringTests="true"

    +1 about that

neclimdul’s picture

How 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:

<?php
if (is_null($value)) {
 
$this->fail($msg);
}
?>

instead of

<?php
$this
->assertNotNull($value, $msg);
?>
neclimdul’s picture

Note: None of these changes affects testbot because as Mile23 noted,

The main problem is that 'risky' doesn't equal 'fail,' which means no one will discover the errors of their ways.

To do that we'd need probably have update the test runner.

dawehner’s picture

Well, 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.

neclimdul’s picture

Mock 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.

dawehner’s picture

Mock commands are assertions so mock the backend.

OH I wasn't aware of that ...

neclimdul’s picture

Yeah, 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.

Mile23’s picture

Yes, setting an expectation, like:

$mock_thingie->expects($this->once())
  ->method('foo');

..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:

+++ b/core/phpunit.xml.dist
@@ -1,6 +1,12 @@
+         checkForUnintentionallyCoveredCode="true"
+         forceCoversAnnotation="true">

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. :-)

neclimdul’s picture

coverage 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:

Code Coverage will only be recorded for tests that use the @covers annotation documented in the section called “@covers”.

neclimdul’s picture

forceCoversAnnotation 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.

Mile23’s picture

Status:Needs review» Needs work

If 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 yes checkForUnintentionallyCoveredCode.

dawehner’s picture

Please keep in mind that with BrowserTestBase and KernelTestBaseTNG you will run more than one class, this is for sure.

Mile23’s picture

You 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.

neclimdul’s picture

Status:Needs work» Needs review
StatusFileSize
new667 bytes
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,668 pass(es).
[ View ]
new546 bytes
Mile23’s picture

Status:Needs review» Reviewed & tested by the community

Ossum.

Turn on the strict stuff, please.