Problem/Motivation

Trying to generate a coverage report, but I found @covers errors throughout Drupal\Tests\Core\Controller\AjaxControllerTest.

This file was last touched by #2364127: Merge AjaxResponseRenderer into AjaxController so I'm linking it here.

Proposed resolution

Fix the errors.

Remaining tasks

User interface changes

No.

API changes

No.

CommentFileSizeAuthor
#4 AjaxController.php_.html_.txt60.17 KByesct
#4 ajaxcoverage.png209.13 KByesct
#1 2366645_1.patch1.73 KBmile23

Comments

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
StatusFileSize
new 2366645_1.patch1.73 KB

Le patch.

Just changes @coversDefaultClass and @covers, plus function summary docs where appropriate.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Active » Needs review
yesct’s picture

yesct commented
Status: Needs review » Reviewed & tested by the community

I read the patch, read the tests, and the changes saying they are covering ::content seem reasonable.
I also checked out the commit hash from after #2364127: Merge AjaxResponseRenderer into AjaxController when those lines were added, and there is no methods renderContentIntoResponse or renderMainContent then (or now). so clearly the comments were wrong.

in general, to run test coverage, make sure xdebug is on, (maybe edit a php.ini and restart apache)
then
cd core
./vendor/phpunit/phpunit/phpunit --filter=AjaxControllerTest --coverage-html /tmp/report

before the patch, on head:
Trying to @cover or @use not existing method "\Drupal\Core\Controller\AjaxController::renderContentIntoResponse".

after the patch:
it gets further:

.........

Time: 34.83 seconds, Memory: 134.50Mb

OK (9 tests, 37 assertions)

Generating code coverage report in HTML format ...

(but after 9 minutes did not get to "done" and the report generated. but that might not be a problem with this patch and due to something else)
[edit: it took 10 minutes and did finish. :)]

I cant think of anything to improve this patch. so rtbc.

yesct’s picture

yesct commented
Issue summary: View changes
StatusFileSize
new ajaxcoverage.png209.13 KB
new AjaxController.php_.html_.txt60.17 KB

the coverage report:

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

Thanks for the review.

@YesCT: no methods renderContentIntoResponse or renderMainContent then (or now). so clearly the comments were wrong.

The proof is in the pudding... The tests only call content(), so therefore they can only @cover that method. eg:

    $this->assertSame($ajax_response, $this->ajaxController->content($request, $_content));

Also: Report generation can take a while for HTML, but you can also verify that it works correctly with --coverage-text. It's quicker, will fail on the same errors, and will show you abbreviated results in the console.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

My bad.

Patch looks good!

yesct’s picture

yesct commented

@Mile23 oh, thanks for the hint about leaving off the html to speed that up.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

This issue is an improvement to automated testing and there is unfrozen as per #2350615: [policy, no patch] What changes can be accepted during the Drupal 8 beta phase?. Committed 211fd40 and pushed to 8.0.x. Thanks!

  • alexpott committed 211fd40 on 8.0.x 
    Issue #2366645 by YesCT, Mile23: Drupal\Tests\Core\Controller\...

Status: Fixed » Closed (fixed)

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