Converting XPath handling from Simpletest to PHPUnit browser tests

I have looked at the xpath conversions for the datetime module (those tests have other blockers for full BTB conversion), and some of the others.

I think the best thing to do is to discourage the use of the actual ->xpath() method, and instead encourage using some of the methods that will be inherited from Behat\Mink\WebAssert that can accept xpath expressions instead, eg the ->element* methods. This should make tests read better readable (programming more to the interface than the implementation). It would also allow us to convert some of these to css selectors, xpath selectors should just be used to check actual HTML element structure, not to find things that already have unique IDs on them.

I agree that using xpath-based assertions in tests is ugly compared to nice wrapper functions, but I've noticed that the Behat\Mink\WebAssert functions all seem to throw exceptions rather than returning a simple $this->fail($message), which has ramifications for the test results. I normally try to keep my tests from throwing exceptions and have normally tried to catch them and fail with better messages.

For instance, if I use $this->assertSession()->linkByHrefExists(), where a link does NOT exist by the passed href value, we end with error/exception that looks like:

There was 1 error:

1) Drupal\Tests\path\Functional\PathAliasTest::testDuplicateNodeAlias
Behat\Mink\Exception\ExpectationException: Link containing href #edit-path-0-alias found.

/Users/lowell/Sites/d8dot4test.local/core/tests/Drupal/Tests/WebAssert.php:393
/Users/lowell/Sites/d8dot4test.local/core/tests/Drupal/Tests/WebAssert.php:275
/Users/lowell/Sites/d8dot4test.local/core/modules/path/tests/src/Functional/PathAliasTest.php:374

FAILURES!
Tests: 4, Assertions: 61, Errors: 1.

I would far rather see a failure than error/exception.

When I ran it with an xpath-based assertion, for example using:
$this->assertTrue(!empty($this->xpath("//a[contains(@href,'#edit-path-0-alias')]")), 'Error link is to the URL alias field ID.');...
the test in question ended in a normal failure:

There was 1 failure:

1) Drupal\Tests\path\Functional\PathAliasTest::testDuplicateNodeAlias
Error link is to the URL alias field ID.
Failed asserting that false is true.

/Users/lowell/Sites/d8dot4test.local/core/tests/Drupal/KernelTests/AssertLegacyTrait.php:31
/Users/lowell/Sites/d8dot4test.local/core/modules/path/tests/src/Functional/PathAliasTest.php:374

When I looked at core/tests/Drupal/Tests/WebAssert.php to see if I could debug why the test was ending in an exception, it became clear that this is a design choice, but really not what I'd consider best practice. A test should end in failure when an assertion fails, IMHO, not in an error/exception. In the past, I've always tried to catch exceptions and when I saw tests ending in error, that meant there was something to fix in the testing system (i.e. not that the tests were indicating a failed assertion).

I noticed this when writing a test for #2851786: Path validation sets error on wrong element and thought it warranted some further discussion. (Maybe a new issue, perhaps, but this one at least seems relevant to the functionality provided by WebAssert.php)

@LoMo
Interesting comment! When I understand your comment correctly, phpunit has some special behaviour for failures (which are exceptions internally as well):

    public static function exceptionToString(Exception $e)
    {
        if ($e instanceof PHPUnit_Framework_SelfDescribing) {
            $buffer = $e->toString();

            if ($e instanceof PHPUnit_Framework_ExpectationFailedException && $e->getComparisonFailure()) {
                $buffer = $buffer . $e->getComparisonFailure()->getDiff();
            }

            if (!empty($buffer)) {
                $buffer = trim($buffer) . "\n";
            }
        } elseif ($e instanceof PHPUnit_Framework_Error) {
            $buffer = $e->getMessage() . "\n";
        } elseif ($e instanceof PHPUnit_Framework_ExceptionWrapper) {
            $buffer = $e->getClassname() . ': ' . $e->getMessage() . "\n";
        } else {
            $buffer = get_class($e) . ': ' . $e->getMessage() . "\n";
        }

        return $buffer;
    }

But in WebAssert we do use the following piece of code:

  /**
   * Asserts a condition.
   *
   * The parent method is overridden because it is a private method.
   *
   * @param bool $condition
   *   The condition.
   * @param string $message
   *   The success message.
   *
   * @throws \Behat\Mink\Exception\ExpectationException
   *   When the condition is not fulfilled.
   */
  public function assert($condition, $message) {
    if ($condition) {
      return;
    }

    throw new ExpectationException($message, $this->session->getDriver());
  }

Given that do you propose that in our assert function we should actually throw one of those phpunit failure exceptions like \PHPUnit_Framework_ExpectationFailedException ?

@dawehner Yes, I think you understand me and I think that might do it. If so, making that change should show the failed assertion under "failures" instead of under "errors".

@LoMo
Do you mind to open up a dedicated issue for that? The initial question: What should we do with all the xpaths is solved right now by basically converting them as we go.