Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Task to convert Drupal\system\Tests\Routing\ControllerResolverTest
to phpunit.
Comment | File | Size | Author |
---|---|---|---|
#32 | controller_resolver-2042739..patch | 11.32 KB | dawehner |
#32 | interdiff.txt | 737 bytes | dawehner |
#30 | controller-resolver-2042739-30.patch | 11.27 KB | tim.plunkett |
Comments
Comment #1
jhedstromWow, this one was quite simple.
Comment #2
dawehnerShould we open a follow up to extend the test coverage or just do it here?
Comment #3
jhedstromDoing it here seems fine.
Comment #4
jhedstromThis adds 100% code coverage.
Comment #5
dawehnerWhat about setting a variable, to ensure that create was really called?
Comment #6
jhedstromDone and done.
Comment #7
dawehnerThank you very much
Comment #9
dawehner#6: system-controller-resolver-test-2042739-06.patch queued for re-testing.
Comment #10
dawehnerWait, we also have to check the variable, don't we?
Comment #11
jhedstromYeah, I meant to add that check in the last patch.
Comment #12
dawehnerThank you very much!
Comment #13
xjm#11: system-controller-resolver-test-2042739-11.patch queued for re-testing.
Comment #14
alexpottAfaik 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)?
Comment #15
dawehnerOpened a policy issue: #2057905: [policy, no patch] Discuss the standards for phpunit based tests
Comment #16
jhedstromWe've been converting phpunit tests to display the failing message, as per https://drupal.org/node/2002190#comment-7494804 which references the PHPUnit docs:
Comment #17
jhedstromre 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.
Comment #18
ParisLiakos CreditAttribution: ParisLiakos commentedThis is great..only some nitpicks
Drupal needs a backslash in front of it:
Contains \Drupal\Tests\.....
Usually we have the closing bracket in new line even if the method is empty
Comment #19
jhedstromI 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].
Comment #20
ParisLiakos CreditAttribution: ParisLiakos commentedi think, we only do this for interface through core so lets keep it consistent
Comment #21
bdone CreditAttribution: bdone commentedtrying 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.
Comment #22
bdone CreditAttribution: bdone commentedhowever, running this test produces the following fatal:
Comment #23
bdone CreditAttribution: bdone commentedhere's a stab at fixing the fatal
Comment #24
bdone CreditAttribution: bdone commentedmissing interdiff from #23
Comment #25
dawehnerIs 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?
Let's add a @group Drupal
Comment #26
bdone CreditAttribution: bdone commentedthanks @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.
Comment #27
dawehnerI think we should just have one test living in controller ...
Comment #27.0
dawehnerUpdated issue summary.
Comment #28
jhedstromComment #29
dawehnerI just merged together your patch with the existing test.
Comment #30
tim.plunkettI 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.
Comment #31
dawehner/me sighs ... but yeah your one is kind of nicer.
It is you who duplicated the work.
Let's also add routing or something like that.
Comment #32
dawehnerLet's just do that and get the awesome test in.
Comment #33
dawehnerLet's just do that and get the awesome test in.
Comment #35
dawehner32: controller_resolver-2042739..patch queued for re-testing.
Comment #36
jhedstromI think this is RTBC with passing tests.
Comment #37
xjm32: controller_resolver-2042739..patch queued for re-testing.
Comment #38
tim.plunkett32: controller_resolver-2042739..patch queued for re-testing.
Comment #39
webchickCommitted and pushed to 8.x. Thanks!