Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhedstrom’s picture

Status: Active » Needs review
FileSize
2.86 KB

Wow, this one was quite simple.

dawehner’s picture

Should we open a follow up to extend the test coverage or just do it here?

jhedstrom’s picture

Doing it here seems fine.

jhedstrom’s picture

This adds 100% code coverage.

dawehner’s picture

+++ b/core/tests/Drupal/Tests/Core/Routing/ControllerResolverTest.phpundefined
@@ -43,4 +43,67 @@ public function testContainerAware() {
+    $this->assertTrue($controller[0] instanceof MockControllerAsInterface, 'The wrong controller object was returned.');

+++ b/core/tests/Drupal/Tests/Core/Routing/MockControllerAsInterface.phpundefined
@@ -0,0 +1,31 @@
+  public static function create(ContainerInterface $container) {
+    return new self;

What about setting a variable, to ensure that create was really called?

jhedstrom’s picture

What about setting a variable, to ensure that create was really called?

Done and done.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you very much

Status: Reviewed & tested by the community » Needs work
Issue tags: -PHPUnit

The last submitted patch, system-controller-resolver-test-2042739-06.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
Issue tags: +PHPUnit
dawehner’s picture

Status: Needs review » Needs work

Wait, we also have to check the variable, don't we?

jhedstrom’s picture

Status: Needs work » Needs review
FileSize
7.43 KB
766 bytes

Yeah, I meant to add that check in the last patch.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you very much!

xjm’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/tests/Drupal/Tests/Core/Routing/ControllerResolverTest.phpundefined
@@ -0,0 +1,113 @@
+    $this->assertTrue($controller[0] instanceof MockController, 'The wrong controller object was returned.');
...
+    $this->assertTrue($controller[0] instanceof MockController, 'The wrong controller object was returned.');
...
+    $this->assertTrue($controller[0] instanceof MockControllerAsInterface, 'The wrong controller object was returned.');
...
+    $this->assertTrue($controller[0]->called, 'The controller was not properly instantiated.');

Afaik our test assertion messages should state the positive of what we're asserting ie. the success case not the fail. So in the final example here something like The controller's create method was called during instantiation should be the assertion text.

Also reading this patch makes me wonder if there is any way we can make more use of PHPUnit's mocking (http://phpunit.de/manual/3.7/en/test-doubles.html)?

dawehner’s picture

jhedstrom’s picture

We've been converting phpunit tests to display the failing message, as per https://drupal.org/node/2002190#comment-7494804 which references the PHPUnit docs:

Reports an error identified by $message if the two variables $expected and $actual are not equal.

jhedstrom’s picture

Status: Needs work » Needs review

re mocking, these tests are mostly covering discovery via class names, meaning mocking the objects would not easily work. Other unit tests that are being converted are starting to make use of mock objects where possible. Setting back to needs review, but can revisit mock objects if there are ideas on how to convert these.

ParisLiakos’s picture

This is great..only some nitpicks

  1. +++ b/core/tests/Drupal/Tests/Core/Routing/MockController.php
    @@ -2,10 +2,10 @@
    - * Contains Drupal\system\Tests\Routing\MockController.
    + * Contains Drupal\Tests\Core\Routing\MockController.
    
    +++ b/core/tests/Drupal/Tests/Core/Routing/MockControllerAsInterface.php
    @@ -0,0 +1,43 @@
    + * Contains Drupal\Tests\Core\Routing\MockControllerAsInterface.
    

    Drupal needs a backslash in front of it:
    Contains \Drupal\Tests\.....

  2. +++ b/core/tests/Drupal/Tests/Core/Routing/MockControllerAsInterface.php
    @@ -0,0 +1,43 @@
    +  public function run() {}
    

    Usually we have the closing bracket in new line even if the method is empty

jhedstrom’s picture

Usually we have the closing bracket in new line even if the method is empty

I wonder is it acceptable to not include the brackets at all? This is common in other standards for interface definitions:

public function run();

This is unclear from reading [#608152].

ParisLiakos’s picture

Status: Needs review » Needs work

i think, we only do this for interface through core so lets keep it consistent

bdone’s picture

Status: Needs work » Needs review
FileSize
1006 bytes
8 KB

trying to get my feet wet w/ writing phpunit tests...

here's a very simple re-roll, incorporating point 1 from from #18. i think the run method syntax (point 2) was already ok, but maybe i have have misunderstood the fix here.

bdone’s picture

Status: Needs review » Needs work

however, running this test produces the following fatal:

$ ./vendor/bin/phpunit
...
Fatal error: Interface 'Drupal\Core\Controller\ControllerInterface' not found in .../core/tests/Drupal/Tests/Core/Routing/MockControllerAsInterface.php on line 17
bdone’s picture

Status: Needs work » Needs review
FileSize
0 bytes
8.02 KB

here's a stab at fixing the fatal

bdone’s picture

FileSize
936 bytes

missing interdiff from #23

dawehner’s picture

  1. +++ b/core/tests/Drupal/Tests/Core/Routing/ControllerResolverTest.php
    @@ -0,0 +1,113 @@
    + * Contains \Drupal\Tests\Core\Routing\ControllerResolverTest
    ...
    +namespace Drupal\Tests\Core\Routing;
    

    Is there a reason why we place this in the routing directory but we actually test the ControllerResolver class which is part of the "Controller" directory?

  2. +++ b/core/tests/Drupal/Tests/Core/Routing/ControllerResolverTest.php
    @@ -0,0 +1,113 @@
    +/**
    + * Tests that the Drupal-extended ControllerResolver is functioning properly.
    + *
    + * @see \Drupal\Core\Controller\ControllerResolver
    + */
    +class ControllerResolverTest extends UnitTestCase {
    

    Let's add a @group Drupal

bdone’s picture

Status: Needs review » Needs work

thanks @dawehner. i started to update things, and noticed ControllerResolverTest within the \Drupal\Tests\Core\Controller namespace.

maybe some tests could be moved into \Drupal\Tests\Core\Controller\ControllerResolverTest, and \Drupal\Tests\Core\Routing\ControllerResolverTest could become routing specific tests?

maybe someone who is already familiar with these tests is more comfortable moving things around? seems there is still some more work needed here.

dawehner’s picture

I think we should just have one test living in controller ...

dawehner’s picture

Issue summary: View changes

Updated issue summary.

jhedstrom’s picture

dawehner’s picture

Status: Needs work » Needs review
FileSize
7.66 KB

I just merged together your patch with the existing test.

tim.plunkett’s picture

I somehow missed this issue and was just about to open a dupe. My patch doesn't build off of these, but it does give us complete coverage (excluding the logger parts).
Sorry for duplicating work.

dawehner’s picture

/me sighs ... but yeah your one is kind of nicer.

It is you who duplicated the work.

+++ b/core/tests/Drupal/Tests/Core/Controller/ControllerResolverTest.php
@@ -8,17 +8,21 @@
+ * @group Drupal
...
 class ControllerResolverTest extends UnitTestCase {

Let's also add routing or something like that.

dawehner’s picture

Let's just do that and get the awesome test in.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Let's just do that and get the awesome test in.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 32: controller_resolver-2042739..patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
jhedstrom’s picture

Status: Needs review » Reviewed & tested by the community

I think this is RTBC with passing tests.

xjm’s picture

tim.plunkett’s picture

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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