Comments

Status:Active» Needs review
StatusFileSize
new2.86 KB
PASSED: [[SimpleTest]]: [MySQL] 57,127 pass(es).
[ View ]

Wow, this one was quite simple.

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

Doing it here seems fine.

StatusFileSize
new6.96 KB
PASSED: [[SimpleTest]]: [MySQL] 57,532 pass(es).
[ View ]
new3.57 KB

This adds 100% code coverage.

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

StatusFileSize
new7.23 KB
PASSED: [[SimpleTest]]: [MySQL] 57,490 pass(es).
[ View ]
new1020 bytes

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

Done and done.

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.

Status:Needs work» Needs review
Issue tags:+phpunit

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new7.43 KB
PASSED: [[SimpleTest]]: [MySQL] 57,958 pass(es).
[ View ]
new766 bytes

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

Status:Needs review» Reviewed & tested by the community

Thank you very much!

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

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.

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.

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

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

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new1006 bytes
new8 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

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.

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

Status:Needs work» Needs review
StatusFileSize
new0 bytes
new8.02 KB
PASSED: [[SimpleTest]]: [MySQL] 59,416 pass(es).
[ View ]

here's a stab at fixing the fatal

StatusFileSize
new936 bytes

missing interdiff from #23

  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

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.

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

Issue summary:View changes

Updated issue summary.

Status:Needs work» Needs review
StatusFileSize
new7.66 KB
PASSED: [[SimpleTest]]: [MySQL] 58,048 pass(es).
[ View ]

I just merged together your patch with the existing test.

StatusFileSize
new11.27 KB
PASSED: [[SimpleTest]]: [MySQL] 59,052 pass(es).
[ View ]

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.

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

StatusFileSize
new737 bytes
new11.32 KB
PASSED: [[SimpleTest]]: [MySQL] 59,983 pass(es).
[ View ]

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

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.

Status:Needs work» Needs review

Status:Needs review» Reviewed & tested by the community

I think this is RTBC with passing tests.

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.