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

Files: 
CommentFileSizeAuthor
#9 simpletest-runner-phpunit-2105583-9-FAIL.patch1.05 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,413 pass(es).
[ View ]
#9 simpletest-runner-phpunit-2105583-9-PASS.patch459 bytestim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,810 pass(es).
[ View ]
#1 phpunit-2105583-1.patch434 bytesdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,397 pass(es).
[ View ]

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.