Problem/Motivation

The field assertions in AssertLegacyTrait do not have proper test coverage. This makes changing the behavior very difficult.

Proposed resolution

Add 100% coverage for the assertions.

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#11 2876357-11.patch11.88 KBmichielnugter
#11 interdiff-9-11.txt1.53 KBmichielnugter
#9 2876357-9.patch11.63 KBmichielnugter
#9 interdiff-2-9.txt2.6 KBmichielnugter
#2 2876357-2.patch10.64 KBmichielnugter
#2 interdiff.txt1.91 KBmichielnugter
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

michielnugter created an issue. See original summary.

michielnugter’s picture

michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commented
FileSize
interdiff.txt1.91 KB
2876357-2.patch10.64 KB

Patch is based on 2868019-47.patch. Interdiff is between these patches.

Thanks to @jonathan1055 for helping on the coverage. Don't forget to credit him on this issue!

michielnugter’s picture

michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commented
Title: AssertLegacyTrait field assertions not compatible with Simpletest assertions » Incomplete test coverage on AssertLegacyTrait field assertions
michielnugter’s picture

michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commented

Self/jonathan1055 review:

Still missing:
- Negative assertFieldChecked test coverage
- assertOptionSelected positive and negative coverage

Let's not add code that's out of scope for the issue:

  /**
   * Tests error functionality.
   */

Status: Needs review » Needs work

The last submitted patch, 2: 2876357-2.patch, failed testing.

jonathan1055’s picture

jonathan1055 CreditAttribution: jonathan1055 as a volunteer commented
Status: Needs work » Needs review

Good idea to split this off. I had thought about doing that before.
The test failure is nothing to do with our changes - seems core codebase has had a commit which fails the views tests.

[edit: I did not intend to set the status to 'needs review' it just stayed that way from before]

michielnugter’s picture

michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commented

Well, the fail wasn't related so a Review would be nice :)

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude as a volunteer and at Dx Experts commented
Status: Needs review » Needs work

Little more to add then the self review in #4, so

+++ b/core/tests/Drupal/FunctionalTests/BrowserTestBaseTest.php
@@ -267,6 +383,62 @@ public function testLegacyFieldAsserts() {
+    $this->assertFieldChecked('edit-checkbox-enabled');
+    $this->assertNoFieldChecked('edit-checkbox-disabled');

The expected fails for these two assertions. There is a test for a non-existent id, but more interesting would be a test using the other (existing) checkbox, so expected failing tests for:

$this->assertFieldChecked('edit-checkbox-disabled');
$this->assertNoFieldChecked('edit-checkbox-enabled');

and coverage for:
- assertOption
- assertOptionSelected

michielnugter’s picture

michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commented
Status: Needs work » Needs review
FileSize
interdiff-2-9.txt2.6 KB
2876357-9.patch11.63 KB

The expected fails for these two assertions. There is a test for a non-existent id, but more interesting would be a test using the other (existing) checkbox, so expected failing tests for:

Good one! Added it to the current patch. I think we have full coverage for the field related methods now, rest of the review comments have been addressed as well.

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude as a volunteer and at Dx Experts commented 
  1. +++ b/core/tests/Drupal/FunctionalTests/BrowserTestBaseTest.php
@@ -361,6 +367,16 @@
+      $this->assertOptionSelected('options', 4);

    same as for the checkboxes, could we use an option that does exist but isn't selected. 

  2. +++ b/core/tests/Drupal/FunctionalTests/BrowserTestBaseTest.php
@@ -437,7 +453,25 @@
+    // Test that the assertion fails correctly with non-existant field id.
...
+    // Test that the assertion fails correctly with non-existant field id.

    comments don't match the test.

michielnugter’s picture

michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commented
FileSize
interdiff-9-11.txt1.53 KB
2876357-11.patch11.88 KB

Good ones, really should start paying attention to the copy-paste comments ;)

Review processed, ready for review again.

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude as a volunteer and at Dx Experts commented
Status: Needs review » Reviewed & tested by the community

All green, all feedback addressed.

alexpott’s picture

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

Committed and pushed c6b88ca to 8.4.x and 6775d02 to 8.3.x. Thanks!

Really nice to see all this additional coverage.

  • alexpott committed c6b88ca on 8.4.x 
    Issue #2876357 by michielnugter, Lendude, jonathan1055: Incomplete test...

  • alexpott committed 6775d02 on 8.3.x 
    Issue #2876357 by michielnugter, Lendude, jonathan1055: Incomplete test...

Status: Fixed » Closed (fixed)

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