Follow-up from #2057905: [policy, no patch] Discuss the standards for phpunit based tests.

Mile23 points out our unit tests do not pass when the --strict flag is used.

Files: 
CommentFileSizeAuthor
#23 phpunit-2096595-23.patch5.18 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 59,430 pass(es).
[ View ]
#19 drupal8.simpletest-module.2096595-19.patch5.21 KBneclimdul
PASSED: [[SimpleTest]]: [MySQL] 58,430 pass(es).
[ View ]
#19 interdiff-2096595.txt617 bytesneclimdul
#18 2096595-18.patch4.61 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 58,735 pass(es).
[ View ]
#18 interdiff.txt911 bytesParisLiakos
#15 2096595-15.patch4.74 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 58,564 pass(es).
[ View ]
#13 interdiff.txt10.72 KBneclimdul
#12 drupal8.simpletest-module.2096595-13.patch14.43 KBMile23
PASSED: [[SimpleTest]]: [MySQL] 58,837 pass(es).
[ View ]
#8 interdiff.txt675 bytesneclimdul
#7 interdiff.txt2.67 KBneclimdul
#7 drupal8.simpletest-module.2096595-7.patch4.74 KBneclimdul
PASSED: [[SimpleTest]]: [MySQL] 59,030 pass(es).
[ View ]
#6 2096595-6.patch4.08 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 58,772 pass(es).
[ View ]
#6 interdiff-2096595-6.txt434 bytesdamiankloip
#1 phpunit-2096595-1.patch3.65 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,927 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new3.65 KB
PASSED: [[SimpleTest]]: [MySQL] 58,927 pass(es).
[ View ]

Two dataProvider methods were incorrectly named.
Refactored the other two to ensure there are always assertions.

nice!

+++ b/core/tests/Drupal/Tests/Core/Extension/ModuleHandlerUnitTest.php
@@ -34,9 +35,18 @@ function setUp() {
+      $notice_thrown = TRUE;
...
+    $this->assertFalse($notice_thrown, 'A notice was not thrown for a nonexistent module.');

we should use
@expectedException PHPUnit_Framework_Error_Notice

in the docblock here

Status:Needs review» Reviewed & tested by the community

or no..we need the reverse..which probably does not exist;)

Status:Reviewed & tested by the community» Needs review

There is no "expectedNoException", which is what we need here.

Status:Needs review» Reviewed & tested by the community

Cross post :)

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new434 bytes
new4.08 KB
PASSED: [[SimpleTest]]: [MySQL] 58,772 pass(es).
[ View ]

So we can enforce strict in the PHPUnit configuration too.. I think we should do that here too, as Tim's patch is fixing all the current shortcomings.

The rest of the changes to the tests are RTBC for me.

StatusFileSize
new4.74 KB
PASSED: [[SimpleTest]]: [MySQL] 59,030 pass(es).
[ View ]
new2.67 KB

One more thing. Tests failed locally for me and lead me to this. http://stackoverflow.com/a/10535787

MTimeProtectedFileStorageTest sleeps for 1 sec to trigger the mtime tests so we have to allow that tests to run in the next time bracket.

Otherwise looks great and I agree with using strict 100%.

StatusFileSize
new675 bytes

wrong interdiff...

There is no "expectedNoException", which is what we need here.

If there is an exception thrown and not catched nor expected the test will just fail as it is.

Status:Needs review» Needs work

More PHPUnit configuration concerns here: #2056607: /core/phpunit.xml.dist only finds top-level contrib modules

Strict makes us happy.

We'll need to do a big ol' run through of coding standards anyway. :-)

+++ b/core/tests/Drupal/Tests/Core/Extension/ModuleHandlerUnitTest.php
@@ -34,9 +35,18 @@ function setUp() {
+  public function testLoadInclude() {
+    $notice_thrown = FALSE;
+    try {
+      $this->moduleHandler->loadInclude('foo', 'inc');
+    }
+    catch (PHPUnit_Framework_Error_Notice $e) {
+      $notice_thrown = TRUE;
+    }
+    $this->assertFalse($notice_thrown, 'A notice was not thrown for a nonexistent module.');

Rename to something like testLoadIncludeFromNonExistantModule().

Say 'nonexistent' instead of 'foo'. :-)

You just want to test against the documented return value of loadInclude(). It should be FALSE if the file couldn't be found: http://drupalcode.org/project/drupal.git/blob/HEAD:/core/lib/Drupal/Core...

And like @dawehner said, it will automatically fail if an exception is thrown.

Yes, so then what is the assertion in the method?

Say 'nonexistent' instead of 'foo'. :-)

That's in HEAD.

Status:Needs work» Needs review
StatusFileSize
new14.43 KB
PASSED: [[SimpleTest]]: [MySQL] 58,837 pass(es).
[ View ]

  public function testLoadInclude() {
    $this->assertFalse($this->moduleHandler->loadInclude('foo', 'inc'));
  }

Also, I feel a need to write a module called foo.

StatusFileSize
new10.72 KB

Seems like there's some scope creep here? Here's the interdiff of #12

Status:Needs review» Needs work

the assertFalse change is sweet. but lets leave the DrupalTest.php file out, or move it to another issue

Status:Needs work» Needs review
StatusFileSize
new4.74 KB
PASSED: [[SimpleTest]]: [MySQL] 58,564 pass(es).
[ View ]

I think that was accidental, it's the patch from #2089787: Unit test the \Drupal class :)

Let's remove that.

On the one hand I really like to let the tests pass strict-mode.
On the other hand this though comes with a cost: it might be a little bit more annoying for people to write tests.

I think the little changes we had to do here show its not that much of a cost. We where almost doing it anyway by accident. The only weird case is a weird test.

StatusFileSize
new911 bytes
new4.61 KB
PASSED: [[SimpleTest]]: [MySQL] 58,735 pass(es).
[ View ]

#15 accidentally reversed the assertFalse change from #12

i was a bit sceptic with this change too, but i agree with #17

StatusFileSize
new617 bytes
new5.21 KB
PASSED: [[SimpleTest]]: [MySQL] 58,430 pass(es).
[ View ]

Ran the tests again before RTBC'ing and found another TimoutException. Locks waits for 1 second in one of its test. Not sure why these don't always fail but the fix is easy.

Status:Needs review» Reviewed & tested by the community

Yes please.

Re #15: Yup, my bad. Sorry. I should have made an interdiff, at which point I would have caught it.

@dahwenher: "On the other hand this though comes with a cost: it might be a little bit more annoying for people to write tests."

It's certainly annoying to try and read a test that doesn't make explicit assertions. I'd rather unit tests be specific and considered. We could get really disciplined and require @covers... :-)

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

Patch no longer applies.

Status:Needs work» Reviewed & tested by the community
Issue tags:-Needs reroll
StatusFileSize
new5.18 KB
PASSED: [[SimpleTest]]: [MySQL] 59,430 pass(es).
[ View ]

Straight reroll, just a patch context conflict.

Status:Reviewed & tested by the community» Fixed

Committed 1402c66 and pushed to 8.x. Thanks!

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