Problem/Motivation

AjaxCommandsTest needs a general cleanup of coverage annotation.

The following problems exist:

Proposed resolution

  • Individual methods of AjaxCommandsTest test different classes, so they should be annotated as covering that class.
  • Change method mocks to reflect existing methods.
  • Set method mock expectations, so they will fail if the implementations change.
  • Remove docblock one-line summaries.

Remaining tasks

User interface changes

API changes

Original report by @daffie

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Unfrozen changes Unfrozen because it only improves automated testing and coverage accuracy.
CommentFileSizeAuthor
#8 interdiff.txt14.08 KBmile23
#8 2409655_8.patch15.04 KBmile23
#5 revised-Ajax-commands-test-2409655-5.patch7.41 KBzealfire

Comments

daffie’s picture

daffie commented
Parent issue: #2409647: Figure out if ContainerDerivativeDiscoveryDecoratorTest actually covers getDerivativeFetcher() »
Cogax’s picture

Cogax commented
Assigned: Unassigned » Cogax
Cogax’s picture

Cogax commented
Assigned: Cogax » Unassigned
zealfire’s picture

zealfire commented
Assigned: Unassigned » zealfire
zealfire’s picture

zealfire commented
Status: Active » Needs review
StatusFileSize
new revised-Ajax-commands-test-2409655-5.patch7.41 KB

Submitting a patch after reading the docs.Please review.
Thanks.

daffie’s picture

daffie commented
Status: Needs review » Needs work

The tricky part of this issue is that we are testing a group of classes and not one class with a number of methods.

+++ b/core/tests/Drupal/Tests/Core/Ajax/AjaxCommandsTest.php
@@ -31,14 +31,13 @@
- * Tests that each AJAX command object can be created and rendered.
- *
+ * @coversDefaultClass \Drupal\Core\Ajax\AjaxCommands

We are not testing one class. So this change does not apply.

+++ b/core/tests/Drupal/Tests/Core/Ajax/AjaxCommandsTest.php
@@ -31,14 +31,13 @@
+   * @covers ::addCssCommand

We are covering a whole class not just one method. So lets change this to "@covers \Drupal\Core\Ajax\AddCssCommand". The same for all the other @covers.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

+1 on #6.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
StatusFileSize
new 2409655_8.patch15.04 KB
new interdiff.txt14.08 KB

I did the following to AjaxCommandsTest:

  • Changed all test method annotation to @covers [className].
  • Set assertion calls to the standard format of expectation first.
  • Removed the fail notices, because PHPUnit gives you the data when you don't, and that's better.
  • Set up better expectations for the mocks in testOpenDialogCommand() and testOpenModalDialogCommand(). They were mocking a method which doesn't exist any more, without any expectations or return value.
  • Verified that the annotations were correct using #2415441: Automate finding @covers errors

Room for improvement:

  • Set the container to have a mocked service for rendering, and then avoid mocking in testOpenDialogCommand() and testOpenModalDialogCommand().
mile23’s picture

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

daffie commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

It all looks good to me.
It is about tests and documentation so it is allowed for beta-changes.
It is for me RTBC.
Good work @Mile23.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update

Can the issue title and summary be updated to reflect the current scope of the patch. Thanks.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Issue summary: View changes
Status: Needs work » Reviewed & tested by the community

Better?

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Title: AjaxCommandsTest misses @covers and coversDefaultClass documentation » General Cleanup of AjaxCommandsTest
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs issue summary update

Committed 226f002 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed 226f002 on 8.0.x 
    Issue #2409655 by Mile23, zealfire: General Cleanup of AjaxCommandsTest

Status: Fixed » Closed (fixed)

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