Problem/Motivation

AssertLegacyTrait::assertFieldByName() is deprecated in drupal:8.2.0 and is removed from drupal:10.0.0. Use $this->assertSession()->fieldExists() or $this->assertSession()->buttonExists() or $this->assertSession()->fieldValueEquals() instead. See https://www.drupal.org/node/3129738

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#79 rawdiff_78-79.txt2.47 KBmondrake
#79 3139406-79.patch165.26 KBmondrake
#78 3139406-78.patch165.48 KBmondrake
#75 rawdiff_62-75.txt8.04 KBmondrake
#75 3139406-75.patch165.54 KBmondrake
#72 interdiff_69-72.txt919 bytesravi.shankar
#72 3139406-72.patch165.85 KBravi.shankar
#69 3139406-69.patch165.83 KBSuresh Prabhu Parkala
#62 interdiff_59-62.txt600 bytesSuresh Prabhu Parkala
#62 3139406-62.patch166.09 KBSuresh Prabhu Parkala
#60 rawdiff_56-59.txt7.13 KBmondrake
#59 3139406-59.patch166.16 KBpaulocs
#56 interdiff_54-56.txt1.43 KBmondrake
#56 3139406-56.patch165.8 KBmondrake
#54 interdiff_52-54.txt3.21 KBmondrake
#54 3139406-54.patch165.35 KBmondrake
#52 interdiff_49-52.txt3.37 KBmondrake
#52 3139406-52.patch165.38 KBmondrake
#49 rawdiff_46-49.txt22.61 KBmondrake
#49 3139406-49.patch165.34 KBmondrake
#46 interdiff.3139406.42-46.txt12.63 KBlongwave
#46 3139406-46.patch169.4 KBlongwave
#43 interdiff-40-42.txt9.44 KBjungle
#43 3139406-42.patch167.72 KBjungle
#40 interdiff.3139406.34-40.txt2.02 KBlongwave
#40 3139406-40.patch158.57 KBlongwave
#34 interdiff-29-34.txt4.72 KBHardik_Patel_12
#34 3139406-34.patch157.74 KBHardik_Patel_12
#29 interdiff_27-29.txt6.05 KBrahulrasgon
#29 3139406-29.patch159.61 KBrahulrasgon
#27 interdiff_19-27.txt3.8 KBmohrerao
#27 3139406-27.patch153.97 KBmohrerao
#20 3139406-19.patch157.45 KBrahulrasgon
#15 3139406-15.patch154.31 KBBunty Badgujar
#15 interdiff_13-15.txt6.68 KBBunty Badgujar
#13 3139406_interdiff_12-13.txt29.24 KBmohrerao
#13 3139406-13.patch154.04 KBmohrerao
#12 3139406_interdiff_some_more_fixes_10-12.txt4.51 KBmohrerao
#12 3139406-12.patch135.53 KBmohrerao
#10 interdiff_3139406_6-10.txt9.78 KBmohrerao
#10 3139406-10.patch136.42 KBmohrerao
#6 3139406-6.patch136.88 KBmohrerao
#5 3139406-5.patch137.3 KBmohrerao
#4 3139406-4.patch138.45 KBmohrerao
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mondrake created an issue. See original summary.

jungle’s picture

Issue tags: +Deprecated assertions

Adding the "Deprecated assertions" tag to add this into the kanban board https://contribkanban.com/board/Deprecatedassertions

mohrerao’s picture

Assigned: Unassigned » mohrerao

On it

mohrerao’s picture

Assigned: mohrerao » Unassigned
Status: Active » Needs work
FileSize
138.45 KB

Adding a draft patch. Still needs some work.

mohrerao’s picture

Status: Needs work » Needs review
FileSize
137.3 KB

Please ignore the previous patch as i generated it against 8.8.x. Adding new patch for the changes.

mohrerao’s picture

FileSize
136.88 KB

Apologies for all the noise. Please ignore 3139406-4.patch and 139406-5.patch.

Status: Needs review » Needs work

The last submitted patch, 6: 3139406-6.patch, failed testing. View results

ridhimaabrol24’s picture

Assigned: Unassigned » ridhimaabrol24
mohrerao’s picture

@ridhima, Im working on fixing the failed tests.

mohrerao’s picture

Fixed some of the failing tests. Still WIP

mohrerao’s picture

Assigned: ridhimaabrol24 » mohrerao
mohrerao’s picture

Still some of the tests are failing. Still WIP.

mohrerao’s picture

Adding patch for replacing AssertLegacyTrait::assertNoFieldByName.

mohrerao’s picture

Bunty Badgujar’s picture

Status: Needs work » Needs review
FileSize
6.68 KB
154.31 KB

Need re-roll also some test case failure fix from #13.

Description related to update

-    $entity_1->field_single->value = 0;
-    $entity_1->field_unlimited->value = 1;
+    $entity_1->field_single->value = 1;
+    $entity_1->field_unlimited->value = 2;
     $entity_1->save();

0 consider as null value.

-    $this->assertSession()->fieldExists('settings[handler_settings][sort][direction]');
+    $this->assertSession()->fieldNotExists('settings[handler_settings][sort][direction]');

Original code is $this->assertNoFieldByName('settings[handler_settings][sort][direction]');

Status: Needs review » Needs work

The last submitted patch, 15: 3139406-15.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Bunty Badgujar’s picture

Drupal\Tests\views\Functional\Plugin\ExposedFormTest::testExposedFilterPagination
Array to string conversion

There is open issue in un Mink fieldValueEquals does not support multiple select

Do we need seperate issue for this ?

rahulrasgon’s picture

Assigned: mohrerao » rahulrasgon
mohrerao’s picture

Status: Needs work » Needs review
FileSize
153.94 KB

Rerolled patch as the patch doesn't apply.

rahulrasgon’s picture

Assigned: rahulrasgon » Unassigned
FileSize
157.45 KB

Please review the patch.

Thanks

The last submitted patch, , failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Status: Needs review » Needs work

The last submitted patch, 20: 3139406-19.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

nitesh624’s picture

Assigned: Unassigned » nitesh624
nitesh624’s picture

Assigned: nitesh624 » Unassigned

Unassigning as I am bit busy in some other issue

naresh_bavaskar’s picture

Assigned: Unassigned » naresh_bavaskar
naresh_bavaskar’s picture

Assigned: naresh_bavaskar » Unassigned
mohrerao’s picture

Status: Needs work » Needs review
FileSize
153.97 KB
3.8 KB

@rahulrasgon, Please add interdiff after adding a patch. I reworked on p[atch in #19 and have fixed failing tests.

rahulrasgon’s picture

Assigned: Unassigned » rahulrasgon
Status: Needs review » Needs work
rahulrasgon’s picture

Assigned: rahulrasgon » Unassigned
Status: Needs work » Needs review
FileSize
159.61 KB
6.05 KB

All occurrences of $this->assertFieldByName() and $this->assertNotFieldByName() have been replaced.
The deprecation message suppression has been removed.
Added the @expectedDeprecation to the docblock of testFieldAssertsForTextfields, testFieldAssertsForButton and testFieldAssertsForCheckbox

Please review the patch.
Thanks

Status: Needs review » Needs work

The last submitted patch, 29: 3139406-29.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

narendra.rajwar27’s picture

Assigned: Unassigned » narendra.rajwar27
narendra.rajwar27’s picture

Assigned: narendra.rajwar27 » Unassigned

Un-assigning, not able to find cause for test case failure.
:(

Hardik_Patel_12’s picture

Assigned: Unassigned » Hardik_Patel_12

Working on it.

Hardik_Patel_12’s picture

Assigned: Hardik_Patel_12 » Unassigned
Status: Needs work » Needs review
FileSize
157.74 KB
4.72 KB

Kindly review a new patch.

Status: Needs review » Needs work

The last submitted patch, 34: 3139406-34.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

ravi.shankar’s picture

Assigned: Unassigned » ravi.shankar
ravi.shankar’s picture

Assigned: ravi.shankar » Unassigned
mondrake’s picture

Issue tags: +Needs reroll, +Needs tests

Needs reroll, deprecation tests, and completing removal of usages. Also where the $message argument is adding non-obvious context to the test, it should be added as an inline comment instead of removed.

mondrake’s picture

Issue tags: -Needs tests

Oops sorry, the deprecation test is present already.

longwave’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
158.57 KB
2.02 KB

Rerolled, fixed one more instance, fixed coding standards issues. The remaining instances are the legacy trait, legacy tests, or the identically-named method in the kernel test and trait.

Status: Needs review » Needs work

The last submitted patch, 40: 3139406-40.patch, failed testing. View results

jungle’s picture

Status: Needs work » Needs review

Fixing the test failed in #40. Changes made to BrowserTestBaseTest, key changes: 1) removed unnecessary try catch statements. 2) replacement/cleanup.

jungle’s picture

FileSize
167.72 KB
9.44 KB

Forgot attaching patch, sorry.

mondrake’s picture

Status: Needs review » Needs work
+++ b/core/modules/field/tests/src/Functional/NestedFormTest.php
@@ -88,8 +88,8 @@ public function testNestedFieldForm() {
-    $entity_1->field_single->value = 0;
-    $entity_1->field_unlimited->value = 1;
+    $entity_1->field_single->value = 1;
+    $entity_1->field_unlimited->value = 2;

why?

+++ b/core/modules/views_ui/tests/src/Functional/RowUITest.php
@@ -71,10 +71,10 @@ public function testRowUI() {
-    $this->assertFieldByName('row[type]', 'entity:node');
+    $this->assertSession()->fieldExists('row[type]');

We should use fieldValueEquals here, no?

Also:

  1. +++ b/core/modules/aggregator/tests/src/Functional/AggregatorAdminTest.php
    @@ -42,7 +40,7 @@ public function testSettingsPage() {
         foreach ($edit as $name => $value) {
    -      $this->assertFieldByName($name, $value, new FormattableMarkup('"@name" has correct default value.', ['@name' => $name]));
    +      $this->assertSession()->fieldValueEquals($name, $value);
         }
    

    The $message argument is adding non-obvious context to the test, it should be added as an inline comment instead of just being removed.

  2. +++ b/core/modules/block/tests/src/Functional/BlockUiTest.php
    @@ -277,20 +277,20 @@ public function testContextAwareBlocks() {
    -    $this->assertFieldByName('id', 'displaymessage', 'Block form uses raw machine name suggestion when no instance already exists.');
    +    $this->assertSession()->fieldValueEquals('id', 'displaymessage');
    

    same

  3. +++ b/core/modules/block/tests/src/Functional/BlockUiTest.php
    @@ -277,20 +277,20 @@ public function testContextAwareBlocks() {
    -    $this->assertFieldByName('id', 'displaymessage_2', 'Block form appends _2 to plugin-suggested machine name when an instance already exists.');
    +    $this->assertSession()->fieldValueEquals('id', 'displaymessage_2');
    

    same

  4. +++ b/core/modules/block/tests/src/Functional/BlockUiTest.php
    @@ -277,20 +277,20 @@ public function testContextAwareBlocks() {
    -    $this->assertFieldByName('id', 'displaymessage_3', 'Block form appends _3 to plugin-suggested machine name when two instances already exist.');
    +    $this->assertSession()->fieldValueEquals('id', 'displaymessage_3');
    

    same

  5. +++ b/core/modules/comment/tests/src/Functional/CommentAnonymousTest.php
    @@ -138,7 +138,7 @@ public function testAnonymous() {
    -    $this->assertFieldByName('uid', '', 'The author field is empty (i.e. anonymous) when editing the comment.');
    +    $this->assertSession()->fieldValueEquals('uid', '');
    

    same

  6. +++ b/core/modules/datetime/tests/src/Functional/DateTimeFieldTest.php
    @@ -697,7 +697,7 @@ public function testDefaultValue() {
    -      $this->assertFieldByName('default_value_input[default_date]', '', 'The relative default value is empty in instance settings page');
    +      $this->assertSession()->fieldValueEquals('default_value_input[default_date]', '');
    

    same

  7. +++ b/core/modules/datetime/tests/src/Functional/DateTimeFieldTest.php
    @@ -735,7 +735,7 @@ public function testDefaultValue() {
    -      $this->assertFieldByName('default_value_input[default_date]', '+90 days', 'The relative default value is displayed in instance settings page');
    +      $this->assertSession()->fieldValueEquals('default_value_input[default_date]', '+90 days');
    

    same

  8. +++ b/core/modules/datetime/tests/src/Functional/DateTimeFieldTest.php
    @@ -764,7 +764,7 @@ public function testDefaultValue() {
    -      $this->assertFieldByName('default_value_input[default_date]', '', 'The relative default value is empty in instance settings page');
    +      $this->assertSession()->fieldValueEquals('default_value_input[default_date]', '');
    

    same

  9. +++ b/core/modules/datetime_range/tests/src/Functional/DateRangeFieldTest.php
    @@ -1025,9 +1025,9 @@ public function testDefaultValue() {
    -    $this->assertFieldByName('default_value_input[default_date]', '', 'The relative start default value is empty in instance settings page');
    +    $this->assertSession()->fieldValueEquals('default_value_input[default_date]', '');
         $this->assertTrue($this->assertSession()->optionExists('edit-default-value-input-default-end-date-type', 'now')->isSelected());
    -    $this->assertFieldByName('default_value_input[default_end_date]', '', 'The relative end default value is empty in instance settings page');
    +    $this->assertSession()->fieldValueEquals('default_value_input[default_end_date]', '');
    

    same

  10. +++ b/core/modules/datetime_range/tests/src/Functional/DateRangeFieldTest.php
    @@ -1078,9 +1078,9 @@ public function testDefaultValue() {
    -    $this->assertFieldByName('default_value_input[default_date]', '+45 days', 'The relative default start value is displayed in instance settings page');
    +    $this->assertSession()->fieldValueEquals('default_value_input[default_date]', '+45 days');
         $this->assertTrue($this->assertSession()->optionExists('edit-default-value-input-default-end-date-type', 'relative')->isSelected());
    -    $this->assertFieldByName('default_value_input[default_end_date]', '+90 days', 'The relative default end value is displayed in instance settings page');
    +    $this->assertSession()->fieldValueEquals('default_value_input[default_end_date]', '+90 days');
    

    same

  11. +++ b/core/modules/datetime_range/tests/src/Functional/DateRangeFieldTest.php
    @@ -1111,9 +1111,9 @@ public function testDefaultValue() {
    -    $this->assertFieldByName('default_value_input[default_date]', '', 'The relative default start value is empty in instance settings page');
    +    $this->assertSession()->fieldValueEquals('default_value_input[default_date]', '');
         $this->assertTrue($this->assertSession()->optionExists('edit-default-value-input-default-end-date-type', '')->isSelected());
    -    $this->assertFieldByName('default_value_input[default_end_date]', '', 'The relative default end value is empty in instance settings page');
    +    $this->assertSession()->fieldValueEquals('default_value_input[default_end_date]', '');
    

    same

  12. +++ b/core/modules/field/tests/src/Functional/FormTest.php
    @@ -155,7 +155,7 @@ public function testFieldFormSingle() {
    -    $this->assertFieldByName("{$field_name}[0][value]", $value, 'Widget is displayed with the correct default value');
    +    $this->assertSession()->fieldValueEquals("{$field_name}[0][value]", $value);
    

    same

  13. +++ b/core/modules/field/tests/src/Functional/FormTest.php
    @@ -534,7 +534,7 @@ public function testFieldFormAccess() {
    -    $this->assertNoFieldByName("{$field_name_no_access}[0][value]", '', 'Widget is not displayed if field access is denied.');
    +    $this->assertSession()->fieldNotExists("{$field_name_no_access}[0][value]");
    

    same

  14. +++ b/core/modules/filter/tests/src/Functional/FilterAdminTest.php
    @@ -219,7 +219,7 @@ public function testFilterAdmin() {
    -    $this->assertFieldByName('filters[filter_html][settings][allowed_html]', "<a> <em> <strong> <cite> <code> <ul> <ol> <li> <dl> <dt> <dd> <quote>", 'Allowed HTML tag added.');
    +    $this->assertSession()->fieldValueEquals('filters[filter_html][settings][allowed_html]', "<a> <em> <strong> <cite> <code> <ul> <ol> <li> <dl> <dt> <dd> <quote>");
    

    same

  15. +++ b/core/modules/filter/tests/src/Functional/FilterAdminTest.php
    @@ -338,7 +338,7 @@ public function testFilterAdmin() {
    -    $this->assertFieldByName('filters[filter_html][settings][allowed_html]', $edit['filters[filter_html][settings][allowed_html]'], 'Changes reverted.');
    +    $this->assertSession()->fieldValueEquals('filters[filter_html][settings][allowed_html]', $edit['filters[filter_html][settings][allowed_html]']);
    
    @@ -347,7 +347,7 @@ public function testFilterAdmin() {
    -    $this->assertFieldByName('roles[' . RoleInterface::AUTHENTICATED_ID . ']', $edit['roles[' . RoleInterface::AUTHENTICATED_ID . ']'], 'Changes reverted.');
    +    $this->assertSession()->fieldValueEquals('roles[' . RoleInterface::AUTHENTICATED_ID . ']', $edit['roles[' . RoleInterface::AUTHENTICATED_ID . ']']);
    
    @@ -356,8 +356,8 @@ public function testFilterAdmin() {
    -    $this->assertFieldByName('filters[' . $second_filter . '][weight]', $edit['filters[' . $second_filter . '][weight]'], 'Changes reverted.');
    -    $this->assertFieldByName('filters[' . $first_filter . '][weight]', $edit['filters[' . $first_filter . '][weight]'], 'Changes reverted.');
    +    $this->assertSession()->fieldValueEquals('filters[' . $second_filter . '][weight]', $edit['filters[' . $second_filter . '][weight]']);
    +    $this->assertSession()->fieldValueEquals('filters[' . $first_filter . '][weight]', $edit['filters[' . $first_filter . '][weight]']);
    

    same

  16. +++ b/core/modules/language/tests/src/Functional/LanguageNegotiationInfoTest.php
    @@ -116,17 +116,17 @@ public function testInfoAlterations() {
    -    $this->assertNoFieldByName($form_field, NULL, 'Interface language negotiation method unavailable.');
    +    $this->assertSession()->fieldNotExists($form_field);
    ...
    -        $this->assertFieldByName($form_field, NULL, new FormattableMarkup('Type-specific test language negotiation method available for %type.', ['%type' => $type]));
    +        $this->assertSession()->fieldExists($form_field);
    ...
    -        $this->assertNoFieldByName($form_field, NULL, new FormattableMarkup('Type-specific test language negotiation method unavailable for %type.', ['%type' => $type]));
    +        $this->assertSession()->fieldNotExists($form_field);
    

    same

  17. +++ b/core/modules/menu_ui/tests/src/Functional/MenuUiTest.php
    @@ -520,25 +520,25 @@ public function testMenuQueryAndFragment() {
    -    $this->assertFieldByName('link[0][uri]', $path, 'Path is found with both query and fragment.');
    +    $this->assertSession()->fieldValueEquals('link[0][uri]', $path);
    

    same

  18. +++ b/core/modules/menu_ui/tests/src/Functional/MenuUiTest.php
    @@ -520,25 +520,25 @@ public function testMenuQueryAndFragment() {
    -    $this->assertFieldByName('link[0][uri]', $path, 'Path no longer has query or fragment.');
    +    $this->assertSession()->fieldValueEquals('link[0][uri]', $path);
    ...
    -    $this->assertFieldByName('link[0][uri]', $path, 'Path is found with both query and fragment.');
    +    $this->assertSession()->fieldValueEquals('link[0][uri]', $path);
    ...
    -    $this->assertFieldByName('link[0][uri]', $path, 'Path is found with both query and fragment.');
    +    $this->assertSession()->fieldValueEquals('link[0][uri]', $path);
    

    same

  19. +++ b/core/modules/node/tests/src/Functional/NodeEditFormTest.php
    @@ -283,7 +283,7 @@ protected function checkVariousAuthoredByValues(NodeInterface $node, $form_eleme
    -    $this->assertFieldByName($form_element_name, $expected, 'Authored by field displays the correct value for the anonymous user.');
    +    $this->assertSession()->fieldValueEquals($form_element_name, $expected);
    

    same

  20. +++ b/core/modules/views_ui/tests/src/Functional/RowUITest.php
    @@ -36,20 +36,20 @@ public function testRowUI() {
    -    $this->assertFieldByName('row_options[test_option]', NULL, 'Make sure the custom settings form from the test plugin appears.');
    +    $this->assertSession()->fieldExists('row_options[test_option]');
    

    same

  21. +++ b/core/modules/views_ui/tests/src/Functional/RowUITest.php
    @@ -36,20 +36,20 @@ public function testRowUI() {
    -    $this->assertFieldByName('row_options[test_option]', $random_name, 'Make sure the custom settings form field has the expected value stored.');
    +    $this->assertSession()->fieldValueEquals('row_options[test_option]', $random_name);
    

    same

  22. +++ b/core/modules/views_ui/tests/src/Functional/RowUITest.php
    @@ -63,7 +63,7 @@ public function testRowUI() {
    -    $this->assertFieldByName('row[type]', 'fields', 'Make sure that the fields got saved as used row plugin.');
    +    $this->assertSession()->fieldValueEquals('row[type]', 'fields');
    

    same

  23. +++ b/core/modules/views_ui/tests/src/Functional/StyleUITest.php
    @@ -35,20 +35,20 @@ public function testStyleUI() {
    -    $this->assertFieldByName('style[type]', 'default', 'The default style plugin selected in the UI should be unformatted list.');
    +    $this->assertSession()->fieldValueEquals('style[type]', 'default');
    ...
    -    $this->assertFieldByName('style_options[test_option]', NULL, 'Make sure the custom settings form from the test plugin appears.');
    +    $this->assertSession()->fieldExists('style_options[test_option]');
    ...
    -    $this->assertFieldByName('style_options[test_option]', $random_name, 'Make sure the custom settings form field has the expected value stored.');
    +    $this->assertSession()->fieldValueEquals('style_options[test_option]', $random_name);
    

    same

longwave’s picture

Assigned: Unassigned » longwave
+++ b/core/modules/field/tests/src/Functional/NestedFormTest.php
@@ -88,8 +88,8 @@ public function testNestedFieldForm() {
-    $entity_1->field_single->value = 0;
-    $entity_1->field_unlimited->value = 1;
+    $entity_1->field_single->value = 1;
+    $entity_1->field_unlimited->value = 2;

So reverting this fails with:

Behat\Mink\Exception\ExpectationException: The field "field_single[0][value]" value is "", but "0" expected.

Drilling into it, I think assertFieldByName() has a subtle bug as it only uses == for comparisons, and 0 == "". I guess the test field has another bug if 0 is not stored/output correctly, but I think that is out of scope to fix here.

Working on the rest.

longwave’s picture

Status: Needs work » Needs review
FileSize
169.4 KB
12.63 KB

Addressed most of #44. I disagree with #44.3, #44.4, #44.16b, #44.16c, #44.18 and #44.19 in that they have enough comments already. I also think #44.15 doesn't need comments either.

Regarding RowUITest I think the fieldExists check is useless as the drupalPostForm immediately afterwards also ensures the field exists, and really we don't care about the default value in this section of the test. I removed the line in question and added a subsequent comment instead.

longwave’s picture

Assigned: longwave » Unassigned
mondrake’s picture

Status: Needs review » Needs work
+++ b/core/modules/field/tests/src/Functional/String/StringFieldTest.php
@@ -90,8 +90,8 @@ public function _testTextfieldWidgets($field_type, $widget_type) {
-    $this->assertNoFieldByName("{$field_name}[0][format]", '1', 'Format selector is not displayed');
...
+    $this->assertSession()->fieldNotExists("{$field_name}[0][format]");

+++ b/core/tests/Drupal/FunctionalTests/BrowserTestBaseTest.php
@@ -390,40 +389,16 @@ public function testAssertField() {
    */
   public function testFieldAssertsForTextfields() {
     $this->drupalGet('test-field-xpath');
+    $assert_session = $this->assertSession();
 
-    // *** 1. fieldNotExists().
-    $this->assertSession()->fieldNotExists('invalid_name_and_id');
-
-    // Test that the assertion fails correctly when searching by name.
-    try {
-      $this->assertSession()->fieldNotExists('name');
-      $this->fail('The "name" field was not found based on name.');
-    }
-    catch (ExpectationException $e) {
-      // Expected exception; just continue testing.
-    }
-
-    // Test that the assertion fails correctly when searching by id.
-    try {
-      $this->assertSession()->fieldNotExists('edit-name');
-      $this->fail('The "name" field was not found based on id.');
-    }
-    catch (ExpectationException $e) {
-      // Expected exception; just continue testing.
-    }
-
-    // *** 2. fieldExists().
-    $this->assertSession()->fieldExists('name');
-    $this->assertSession()->fieldExists('edit-name');
-
-    // Test that the assertion fails correctly if the field does not exist.
-    try {
-      $this->assertSession()->fieldExists('invalid_name_and_id');
-      $this->fail('The "invalid_name_and_id" field was found.');
-    }
-    catch (ElementNotFoundException $e) {
-      // Expected exception; just continue testing.
-    }
+    $assert_session->fieldExists('name');
+    $assert_session->fieldExists('edit-name');
+    $assert_session->fieldValueEquals('name', 'Test name');
+    $assert_session->fieldValueNotEquals('name', 'not the value');
+    $assert_session->fieldExists('description');
+    $assert_session->fieldExists('edit-description');
+    $assert_session->fieldValueEquals('description', '');
+    $assert_session->fieldNotExists('non-existing-name-and-id');
 
     // *** 3. assertNoFieldById().
     $this->assertNoFieldById('name');
@@ -473,56 +448,6 @@ public function testFieldAssertsForTextfields() {

@@ -473,56 +448,6 @@ public function testFieldAssertsForTextfields() {
       // Expected exception; just continue testing.
     }
 
-    // *** 5. assertNoFieldByName().
-    $this->assertNoFieldByName('name');
-    $this->assertNoFieldByName('name', 'not the value');
-    $this->assertNoFieldByName('notexisting');
-    $this->assertNoFieldByName('notexisting', NULL);
-
-    // Test that the assertion fails correctly if no value is passed in.
-    try {
-      $this->assertNoFieldByName('description');
-      $this->fail('The "description" field, with no value was not found.');
-    }
-    catch (ExpectationException $e) {
-      // Expected exception; just continue testing.
-    }
-
-    // Test that the assertion fails correctly if a NULL value is passed in.
-    try {
-      $this->assertNoFieldByName('name', NULL);
-      $this->fail('The "name" field was not found.');
-    }
-    catch (ExpectationException $e) {
-      // Expected exception; just continue testing.
-    }
-
-    // *** 6. assertFieldByName().
-    $this->assertFieldByName('name');
-    $this->assertFieldByName('name', NULL);
-    $this->assertFieldByName('name', 'Test name');
-    $this->assertFieldByName('description');
-    $this->assertFieldByName('description', '');
-    $this->assertFieldByName('description', NULL);
-
-    // Test that the assertion fails correctly if given the wrong name.
-    try {
-      $this->assertFieldByName('non-existing-name');
-      $this->fail('The "non-existing-name" field was found.');
-    }
-    catch (ExpectationFailedException $e) {
-      // Expected exception; just continue testing.
-    }
-
-    // Test that the assertion fails correctly if given the wrong value.
-    try {
-      $this->assertFieldByName('name', 'not the value');
-      $this->fail('The "name" field with incorrect value was found.');
-    }
-    catch (ExpectationFailedException $e) {
-      // Expected exception; just continue testing.
-    }
-
     // Test that text areas can contain new lines.
     $this->assertFieldsByValue($this->xpath("//textarea[@id = 'edit-test-textarea-with-newline']"), "Test text with\nnewline");
   }
@@ -590,6 +515,13 @@ public function testFieldAssertsForOptions() {

@@ -590,6 +515,13 @@ public function testFieldAssertsForOptions() {
    */
   public function testFieldAssertsForButton() {
     $this->drupalGet('test-field-xpath');
+    $assert_session = $this->assertSession();
+
+    // Test that multiple fields with the same name are validated correctly.
+    $assert_session->buttonExists('duplicate_button');
+    $assert_session->buttonExists('Duplicate button 1');
+    $assert_session->buttonExists('Duplicate button 2');
+    $assert_session->buttonNotExists('Rabbit');
 
     $this->assertFieldById('edit-save', NULL);
     // Test that the assertion fails correctly if the field value is passed in
@@ -612,19 +544,6 @@ public function testFieldAssertsForButton() {

@@ -612,19 +544,6 @@ public function testFieldAssertsForButton() {
     catch (ExpectationException $e) {
       // Expected exception; just continue testing.
     }
-
-    // Test that multiple fields with the same name are validated correctly.
-    $this->assertFieldByName('duplicate_button', 'Duplicate button 1');
-    $this->assertFieldByName('duplicate_button', 'Duplicate button 2');
-    $this->assertNoFieldByName('duplicate_button', 'Rabbit');
-
-    try {
-      $this->assertNoFieldByName('duplicate_button', 'Duplicate button 2');
-      $this->fail('The "duplicate_button" field with the value Duplicate button 2 was not found.');
-    }
-    catch (ExpectationException $e) {
-      // Expected exception; just continue testing.
-    }
   }
 
   /**
@@ -645,44 +564,23 @@ public function testLegacyFieldAssertsForCheckbox() {

@@ -645,44 +564,23 @@ public function testLegacyFieldAssertsForCheckbox() {
    */
   public function testFieldAssertsForCheckbox() {
     $this->drupalGet('test-field-xpath');
+    $assert_session = $this->assertSession();
 
-    // Part 1 - Test by name.
-    // Test that checkboxes are found/not found correctly by name, when using
-    // TRUE or FALSE to match their 'checked' state.
-    $this->assertFieldByName('checkbox_enabled', TRUE);
-    $this->assertFieldByName('checkbox_disabled', FALSE);
-    $this->assertNoFieldByName('checkbox_enabled', FALSE);
-    $this->assertNoFieldByName('checkbox_disabled', TRUE);
-
-    // Test that checkboxes are found by name when using NULL to ignore the
-    // 'checked' state.
-    $this->assertFieldByName('checkbox_enabled', NULL);
-    $this->assertFieldByName('checkbox_disabled', NULL);
-
-    // Test that checkboxes are found by name when passing no second parameter.
-    $this->assertFieldByName('checkbox_enabled');
-    $this->assertFieldByName('checkbox_disabled');
-
-    // Test that we have legacy support.
-    $this->assertFieldByName('checkbox_enabled', '1');
-    $this->assertFieldByName('checkbox_disabled', '');
-
-    // Test that the assertion fails correctly when using NULL to ignore state.
-    try {
-      $this->assertNoFieldByName('checkbox_enabled', NULL);
-      $this->fail('The "checkbox_enabled" field was not found by name, using NULL value.');
-    }
-    catch (ExpectationException $e) {
-      // Expected exception; just continue testing.
-    }
+    // Part 1 - Test by Name.
+    $assert_session->fieldExists('checkbox_enabled');
+    $assert_session->fieldExists('checkbox_disabled');
+    $assert_session->fieldValueEquals('checkbox_enabled', TRUE);
+    $assert_session->fieldValueEquals('checkbox_disabled', FALSE);
+    $assert_session->fieldValueNotEquals('checkbox_enabled', FALSE);
+    $assert_session->fieldValueNotEquals('checkbox_disabled', TRUE);
 
     // Part 2 - Test by ID.
-    // Test that checkboxes are found/not found correctly by ID, when using
-    // TRUE or FALSE to match their 'checked' state.
-    $this->assertFieldById('edit-checkbox-enabled', TRUE);
-    $this->assertFieldById('edit-checkbox-disabled', FALSE);
-    $this->assertNoFieldById('edit-checkbox-enabled', FALSE);
-    $this->assertNoFieldById('edit-checkbox-disabled', TRUE);
+    $assert_session->fieldExists('edit-checkbox-enabled');
+    $assert_session->fieldExists('edit-checkbox-disabled');
+    $assert_session->fieldValueEquals('edit-checkbox-enabled', TRUE);
+    $assert_session->fieldValueEquals('edit-checkbox-disabled', FALSE);
+    $assert_session->fieldValueNotEquals('edit-checkbox-enabled', FALSE);
+    $assert_session->fieldValueNotEquals('edit-checkbox-disabled', TRUE);
 
     // Test that checkboxes are found by ID, when using NULL to ignore the
     // 'checked' state.
@@ -697,15 +595,6 @@ public function testFieldAssertsForCheckbox() {

@@ -697,15 +595,6 @@ public function testFieldAssertsForCheckbox() {
     $this->assertFieldById('edit-checkbox-enabled', '1');
     $this->assertFieldById('edit-checkbox-disabled', '');
 
-    // Test that the assertion fails correctly when using NULL to ignore state.
-    try {
-      $this->assertNoFieldById('edit-checkbox-disabled', NULL);
-      $this->fail('The "edit-checkbox-disabled" field was not found by ID, using NULL value.');
-    }
-    catch (ExpectationException $e) {
-      // Expected exception; just continue testing.
-    }
-
     // Part 3 - Test the specific 'checked' assertions.
     $this->assertSession()->checkboxChecked('edit-checkbox-enabled');
     $this->assertSession()->checkboxNotChecked('edit-checkbox-disabled');
@@ -918,4 +807,17 @@ public function testDeprecationHeaders() {

@@ -918,4 +807,17 @@ public function testDeprecationHeaders() {
     $this->assertCount(1, $test_deprecation_messages);
   }

Umm.. I think here we are removing too much - this will leave us only with positive tests that assertion works, but not with the negative tests that assertions end up in exceptions if they fail. Also, please let's avoid using a $assert_session variable and stick to $this->assertSession()->.. for each call. See #3027952-45: [Plan] Remove the usage of deprecated methods in tests and following. The rest is good for me now.

mondrake’s picture

Rerolled and addressed #48.

Status: Needs review » Needs work

The last submitted patch, 49: 3139406-49.patch, failed testing. View results

longwave’s picture

Shouldn't we just leave BrowserTestBaseTest alone except for tagging the tests as legacy and expecting deprecations? I don't think we need to test the underlying WebAssert methods, unless we don't trust Mink's own tests?

mondrake’s picture

Status: Needs work » Needs review
FileSize
165.38 KB
3.37 KB

#51 yes we should rely on Mink's own tests, but:

(a) we have some additional methods implemented in Drupal\Tests\WebAssert that do not have an explicit coverage (for example, buttonExists), only implicit through testing of the legacy method. If we mark them @legacy they'll be removed in later stage and we'd be left out of coverage.

(b) there are some specific cases tested like

+++ b/core/tests/Drupal/FunctionalTests/BrowserTestBaseTest.php
@@ -648,27 +646,23 @@ public function testFieldAssertsForCheckbox() {
     // Test that we have legacy support.
-    $this->assertFieldByName('checkbox_enabled', '1');
-    $this->assertFieldByName('checkbox_disabled', '');
+    $this->assertSession()->fieldValueEquals('checkbox-enabled', '1');
+    $this->assertSession()->fieldValueEquals('checkbox-disabled', '');

that need their own testing.

IMHO we should have one-to-one matching between the methods in WebAssert and tests in WebAssertTest (functional). I'm trying to add separate deprecation tests marked @legacy as we move along, and change the existing tests to the new WebAssert methods but keeping their structure. Once the merry-go-round of legacy AssertLegacyTrait usage removal is done, we could jump to #3159783: [meta] Add tests for the WebAssert class and properly move whatever is left and relevant in BrowserTestbaseTest to WebAssertTest.

That's my idea though - happy to see alternatives.

Fixing errors in #49.

Status: Needs review » Needs work

The last submitted patch, 52: 3139406-52.patch, failed testing. View results

mondrake’s picture

Status: Needs work » Needs review
FileSize
165.35 KB
3.21 KB

Status: Needs review » Needs work

The last submitted patch, 54: 3139406-54.patch, failed testing. View results

mondrake’s picture

Status: Needs work » Needs review
FileSize
165.8 KB
1.43 KB

Hopefully

mondrake’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
paulocs’s picture

Assigned: Unassigned » paulocs
paulocs’s picture

Assigned: paulocs » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
166.16 KB

Patch re-rolled.

mondrake’s picture

FileSize
7.13 KB
longwave’s picture

Status: Needs review » Needs work

I reviewed the entire patch with git diff --color-words and it is looking almost there, I don't see any issues except one.

+++ b/core/tests/Drupal/FunctionalTests/BrowserTestBaseTest.php
@@ -472,53 +472,29 @@ public function testFieldAssertsForTextfields() {
+    $this->assertSession()->fieldValueEquals('description', '');
+    $this->assertSession()->fieldValueEquals('description', NULL);

I don't see the point of the second assertion. The underlying code calls preg_quote() on the value and so both assertions are ultimately checking for an empty string as preg_quote(NULL) is also ''.

Suresh Prabhu Parkala’s picture

Status: Needs work » Needs review
FileSize
166.09 KB
600 bytes

Please review!

mondrake’s picture

#61 that's exactly the point of the test (this is the same in the removed hunk), I think... so that if the underlying code is for any reason changed, still its facing will have to treat NULL and empty string as the same.

longwave’s picture

Re #63, assertFieldByName() was subtly different. The original code was

    $this->assertFieldByName('description');
    $this->assertFieldByName('description', '');
    $this->assertFieldByName('description', NULL);

NULL (the default) doesn't check the value at all, it just checks that the field exists; from the assertFieldByXPath() docs:

   * @param string $value
   *   (optional) Value of the field to assert. You may pass in NULL (default)
   *   to skip checking the actual value, while still checking that the field
   *   exists.

fieldValueEquals() treats NULL differently - it explicitly checks the value is the empty string.

As this behaviour is removed I think #62 is OK, but will wait for your thoughts before marking RTBC.

mondrake’s picture

I see. Very subtle. That’s fine with #62, then, otherwise we’d be testing what Mink is testing itself. In any case, IMHO per #52 we should eventually refactor that test anyway later.

Thanks a lot for detailed review.

longwave’s picture

Status: Needs review » Reviewed & tested by the community

Great, let's get this in then!

mondrake’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll
Suresh Prabhu Parkala’s picture

Assigned: Unassigned » Suresh Prabhu Parkala
Suresh Prabhu Parkala’s picture

Assigned: Suresh Prabhu Parkala » Unassigned
Status: Needs work » Needs review
FileSize
165.83 KB

Re-rolled patch please review.

mondrake’s picture

@Suresh it would help if you added a raw diff i.e. the diff between the 2 patches - that is not easy to read but it helps identifying what was changed.

Status: Needs review » Needs work

The last submitted patch, 69: 3139406-69.patch, failed testing. View results

ravi.shankar’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
165.85 KB
919 bytes

Here I have tried to fixed failed tests and removed the needs reroll tag.

Looks like this: " $this->assertText('Name value: value changed by setValueForElement() in #element_validate', 'Form element value in $form_state was altered.'); " removed mistakenly instead of this " $this->assertFieldByName('name', '#value changed by #element_validate', 'Form element #value was altered.'); " in patch #69

So I have fixed the above.

Status: Needs review » Needs work

The last submitted patch, 72: 3139406-72.patch, failed testing. View results

mondrake’s picture

Assigned: Unassigned » mondrake

On this.

mondrake’s picture

Assigned: mondrake » Unassigned
mondrake’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC, thanks @Suresh @ravi.shankar for your work - unfortunately https://www.drupal.org/patch/reroll is not very helpful, I understand it can be difficult rerolling with merge conflicts - and it very much depends on your workflow and available tools. If I may, as a rule of thumb:

  1. start from a fresh git clone
  2. apply the latest patch with git apply, or git apply -3 in case git cannot resolve merge conflicts
  3. update manually the files that have unresolved conflicts
  4. use git add to mark files as you resolve the conficts
  5. use git diff --cached to get the new patch
mondrake’s picture

FileSize
165.48 KB

Reroll.

mondrake’s picture

FileSize
165.26 KB
2.47 KB

Reroll.

  • catch committed a14e48a on 9.1.x
    Issue #3139406 by mondrake, mohrerao, longwave, rahulrasgon, Suresh...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed a14e48a and pushed to 9.1.x. Thanks!

Status: Fixed » Closed (fixed)

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