Incorrect field assertions in AssertLegacyTrait

1. +++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
   @@ -262,12 +264,21 @@ protected function assertNoFieldByName($name, $value = '') {
   +   * @throws ElementNotFoundException
   

   Data types in docs need to be fully namespaced, see https://www.drupal.org/docs/develop/coding-standards/api-documentation-a...

2. +++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
   @@ -262,12 +264,21 @@ protected function assertNoFieldByName($name, $value = '') {
   +      throw new ElementNotFoundException($this->getSession()->getDriver(), 'form field', 'id|name|label|value', $field);
   

   we are searching by ID here, so the third parameter should be just "id", right?

3. +++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
   @@ -410,16 +421,24 @@ protected function assertNoLinkByHref($href) {
   +   * @throws ExpectationException
   

   FQDN again.

4. +++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
   @@ -410,16 +421,24 @@ protected function assertNoLinkByHref($href) {
   +    // Manual test instead of assertNull() to keep the thrown exception the
   +    // same.
   +    if ($field !== NULL && !isset($value)) {
   +      throw new ExpectationException(sprintf('Id "%s" appears on this page, but it should not.', $id), $this->getSession()->getDriver());
        }
   -    else {
   -      $this->assertSession()->fieldNotExists($id);
   +    // Manual test instead of assertEquals() to keep the thrown exception the
   +    // same.
   +    elseif ($field !== NULL && $value === $field->getValue()) {
   +      throw new ExpectationException(sprintf('Failed asserting that %s is not equal to %s', $field->getValue(), $value), $this->getSession()->getDriver());
   

   not super happy with that logic. we could have a first condition $field === NULL and return immediately. Because currently we are checking $field !== NULL twice.

Cool, just one minor thing:

+++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
@@ -410,16 +421,28 @@ protected function assertNoLinkByHref($href) {
+    // Manual test instead of assertNull() to keep the thrown exception the
+    // same.
+    if ($field === NULL) {
+      return;
     }

That comment does not really fit here. Should be something like "Return early if the field could not be found as expected."

Looks good now, thanks!

+++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
@@ -262,12 +264,21 @@ protected function assertNoFieldByName($name, $value = '') {
+    $field = $this->getSession()->getPage()->findById($id);

Isn't this too liberal and will find anything with this ID - not just fields?

The old simpletest code was doing

$xpath = '//textarea[@' . $attribute . '=:value]|//input[@' . $attribute . '=:value]|//select[@' . $attribute . '=:value]';

in \Drupal\simpletest\AssertContentTrait::constructFieldXpath()

Thanks, agreed with using xpath. Looks good!

+++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
@@ -410,16 +423,26 @@ protected function assertNoLinkByHref($href) {
   protected function assertNoFieldById($id, $value = '') {

The whole behaviour with $value here is really odd - but as far as I can we're duplicating the behaviour of Simpletest fwiw.

There's quite good testing around the $value path in assertNoFieldById() in \Drupal\FunctionalTests\BrowserTestBaseTest::testLegacyFieldAsserts. However what that is missing is any coverage of assertFieldById(). I think we should add that here. Specifically I think we should test for one of the reported bugs:

Buttons are ignored now, causing no results on a valid ID. They were not ignored in the WebTestBase and should therefore still work in this legacy trait.

This sounds right up my street...

Added a success-test and failure-test for both assertFieldById() and assertNoFieldById().

+++ b/core/tests/Drupal/FunctionalTests/BrowserTestBaseTest.php
@@ -222,6 +222,28 @@ public function testLegacyFieldAsserts() {
+    $this->assertFieldById('edit-save', NULL);
+    // Test that the assertion fails correctly if the field value is passed in
+    // rather than the id.
+    try {
+      $this->assertFieldById('Save', NULL);
+      $this->fail('The field with id of "Save" was found.');
+    }
+    catch (ExpectationException $e) {
+      $this->pass($e->getMessage());
+    }

So now we just need to add value testing for assertByFieldId() - we have it for the assertNoFieldById() case. And then we have full test coverage!

I was trying my hand in adding test for assertFieldById. I think similar code like the following assertNoFieldById code with assertFieldById will do.

    $this->assertNoFieldById('name');
    $this->assertNoFieldById('name', 'not the value');
    $this->assertNoFieldById('notexisting');
    $this->assertNoFieldById('notexisting', NULL);

    // Test that the assertion fails correctly if no value is passed in.
    try {
      $this->assertNoFieldById('description');
      $this->fail('The "description" field, with no value was not found.');
    }
    catch (ExpectationException $e) {
      $this->pass('The "description" field, with no value was found.');
    }

    // Test that the assertion fails correctly if a NULL value is passed in.
    try {
      $this->assertNoFieldById('name', NULL);
      $this->fail('The "name" field was not found.');
    }
    catch (ExpectationException $e) {
      $this->pass('The "name" field was found.');
    }

But I am unable to access the URL example.com/test-field-xpath over the browser. It is giving page not found.
How to access that path over browser so that I can inspect and find the id and value in the page.

I also tried to access all the drupalGet paths in the BrowserTestBaseTest file, but failed.
Thank you. I am new in writing code for BrowserTest.

@boaloysius to get to that path you need to enable the test_page_test module. Since it is a test module you need to add $settings['extension_discovery_scan_tests'] = TRUE; to your settings.php. To see what modules a test has enabled you need to look in the test class's $module property - normally near the top of the file. And also the classes it inherits from. (in this instance there aren't any in the parent classes). So this test has public static $modules = ['test_page_test', 'form_test', 'system_test'];.

Thank you @alexpott.

Currently in this patch we are testing for improper no-value and NULL. Should we add test for wrong value and wrong id?

+    // Test that the assertion fails correctly if no value is passed in.
+    try {
+      $this->assertFieldById('edit-name');
+      $this->fail('The "edit-name" field with no value was found.');
+    }
+    catch (ExpectationException $e) {
+      $this->pass('The "edit-name" field with no value was not found.');
+    }

What is wrong with this code?
This code checks edit-name field for default $value=''. But the default value of the edit-name field is 'Test name'. So it should throw an exception and pass no?

@boaloysius Your test was (almost) perfect, the problem was with the assertFieldById() method - the default $value parameter was wrong: it neither matched the documentation nor the WTB method it is replacing.

I have corrected the parameter, adjusted the exception that needs to be caught and added another assert for good measure!

BrowserTestBaseTest now passes locally, the only question is whether this parameter change breaks any other tests...

thank you Jo Fitzgerald

1. +++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
   @@ -262,12 +264,23 @@ protected function assertNoFieldByName($name, $value = '') {
       * @deprecated Scheduled for removal in Drupal 9.0.0.
       *   Use $this->assertSession()->fieldExists() or
       *   $this->assertSession()->fieldValueEquals() instead.
       */
   -  protected function assertFieldById($id, $value = NULL) {
   -    $this->assertFieldByName($id, $value);
   +  protected function assertFieldById($id, $value = '') {
   +    $xpath = $this->assertSession()->buildXPathQuery('//textarea[@id=:value]|//input[@id=:value]|//select[@id=:value]', [':value' => $id]);
   +    $field = $this->getSession()->getPage()->find('xpath', $xpath);
   +
   +    if (empty($field)) {
   +      throw new ElementNotFoundException($this->getSession()->getDriver(), 'form field', 'id', $field);
   +    }
   +
   +    if ($value !== NULL) {
   +      $this->assertEquals($value, $field->getValue());
   +    }
      }
   

   The deprecated documentation should be updated. There is something wrong asking to use some methods we don't find in the new test

2. +++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
   @@ -410,16 +423,26 @@ protected function assertNoLinkByHref($href) {
       * @deprecated Scheduled for removal in Drupal 9.0.0.
       *   Use $this->assertSession()->fieldNotExists() or
       *   $this->assertSession()->fieldValueNotEquals() instead.
       */
   

   Same that previous comment. @deprecated documentation is different from fix

3. +++ b/core/tests/Drupal/FunctionalTests/BrowserTestBaseTest.php
   @@ -184,13 +184,36 @@ public function testLegacyFieldAsserts() {
   +    }    ¶
   

   Remove white spaces from the end of the line.

Retests passed, back to RTBC.

Committed 4c01abe and pushed to 8.4.x. Thanks!
Committed 88747b7 and pushed to 8.3.x. Thanks!

Committed this to 8.3.x too because it is super important that assertions behave the same way.