Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Status: Active » Needs review
FileSize
3.65 KB

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

ParisLiakos’s picture

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

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

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

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs review

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

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Cross post :)

damiankloip’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
434 bytes
4.08 KB

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.

neclimdul’s picture

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

neclimdul’s picture

FileSize
675 bytes

wrong interdiff...

dawehner’s picture

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.

Mile23’s picture

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.

tim.plunkett’s picture

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

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

That's in HEAD.

Mile23’s picture

Status: Needs work » Needs review
FileSize
14.43 KB
  public function testLoadInclude() {
    $this->assertFalse($this->moduleHandler->loadInclude('foo', 'inc'));
  }

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

neclimdul’s picture

FileSize
10.72 KB

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

ParisLiakos’s picture

Status: Needs review » Needs work

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

damiankloip’s picture

Status: Needs work » Needs review
FileSize
4.74 KB

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

Let's remove that.

dawehner’s picture

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.

neclimdul’s picture

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.

ParisLiakos’s picture

FileSize
911 bytes
4.61 KB

#15 accidentally reversed the assertFalse change from #12

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

neclimdul’s picture

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.

damiankloip’s picture

Status: Needs review » Reviewed & tested by the community

Yes please.

Mile23’s picture

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

alexpott’s picture

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

Patch no longer applies.

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll
FileSize
5.18 KB

Straight reroll, just a patch context conflict.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

dawehner’s picture

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