Convert StandardTest to BrowserTestBase by introducing a AssertLegacyTrait and new assertions on the WebAssert

Here is a failing test.

Found the problem, we need to handle shut down callbacks same as TestBase does.

@dawehner: not sure, as long as we don't run into problems I think we can ignore it for kernel tests.

@jibran: good idea, although converting StandardTest requires porting tons of assertion functions. Not fully done with StandardTest, so this will fail.

Thanks jibran, but I think we should modify StandardTest as little as possible and provide maximum method compatibility on BrowserTestBase. That will make conversions easier since we don't have to change the actual test code as much (as first step).

I moved all the assertion helper methods to a trait.

StandardTest is now passing without test code changes, hooray!

Interdiff is against my previous patch, not jibran's.

Filed #2735691: Missing config schema for private files path

Turns out that BrowserTestBase is wrong in this case, not the config schema. Copied the code from WebTestBase and improved comments.

Private files were moved to settings.php in #2170235: file_private_path should be in $settings, like file_public_path, looks like that got lost when BrowserTestBase was created or we forgot to update.

Fixed use statements discovered when working on #2735199: Convert web tests to browser tests for help module.

1. +++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
   @@ -0,0 +1,182 @@
   + * @deprecated Scheduled for removal in Drupal 9.0.0. Use the wrapped methods
   + * instead.
   

   Should be indented 2 spaces on the second line. What are the wrapped methods you are referring to? I think the comment should be "Use methods of the WebAssert class instead, for example @code $this->assertSession()->statusCodeEquals(200); @endcode".

2. +++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
   @@ -0,0 +1,182 @@
   +  public function assertSession($name = NULL) {
   +    return new WebAssert($this->getSession($name));
   +  }
   

   this method is not deprecated, so it should not be on the legacy trait.

3. +++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
   @@ -0,0 +1,182 @@
   +   *   Use $assert->elementExists() instead.
   

   I think this should be $this->assertSession()->elementExists(), because people don't know where $assert is coming from.

4. +++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
   @@ -0,0 +1,182 @@
   +  protected function assertElementPresent($css_selector, $message = '') {
   

   $message is unused here and should be removed.

5. +++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
   @@ -0,0 +1,182 @@
   +  protected function assertElementNotPresent($css_selector, $message = '') {
   

   $message is unused here and should be removed.

6. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
   @@ -1529,4 +1608,153 @@ protected function cssSelectToXpath($selector, $html = TRUE, $prefix = 'descenda
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public static function assertEquals($expected, $actual, $message = '', $delta = 0.0, $maxDepth = 10, $canonicalize = FALSE, $ignoreCase = FALSE) {
   +    $expected = static::castSafeStrings($expected);
   +    $actual = static::castSafeStrings($actual);
   

   this should have a comment why we override the parent method. The more I think of it the more I don't like that we override assertEquals like that. It changes the meaning of the assert. Instead, I think we should fix all the invocations where we convert to string beforehand.

7. +++ b/core/tests/Drupal/Tests/WebAssert.php
   @@ -64,4 +66,72 @@ public function selectExists($select, TraversableElement $container = NULL) {
   +  /**
   +   * Asserts a condition.
   +   *
   +   * @param bool   $condition
   +   * @param string $message   Failure message
   +   *
   +   * @throws \Behat\Mink\Exception\ExpectationException
   +   *   when the condition is not fulfilled.
   +   */
   +  public function assert($condition, $message) {
   

   why is that overridden from the parent class? Please add a comment.

1. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
   @@ -1529,4 +1608,153 @@ protected function cssSelectToXpath($selector, $html = TRUE, $prefix = 'descenda
   +  protected function config($name) {
   +    return \Drupal::configFactory()->getEditable($name);
   +  }
   

   does not bleong on the legacy assert trait.

2. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
   --- a/core/tests/Drupal/Tests/WebAssert.php
   +++ b/core/tests/Drupal/Tests/WebAssert.php
   

   Let's keep this patch small by only adding the legacy assertion trait and not making any modifications to our Webassert class, we don't need it for this particular issue where we just fix StandardTest.

yes, I agree that we can expand WebAssert.php with useful helper methods. Thanks for your work on that and the idea! Can you move it to a separate issue?

I think this issue should introduce the assertion trait - and I like your name LegacyAssertTrait a lot more, because we should really be deprecating those assert methods and rely more on mink standard assertions. We don't have to maintain those, which means less code and less mistakes we can make.

Almost ready, this looks fluffy good!

+++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
@@ -0,0 +1,164 @@
+  protected function pass($message = '') {
+  }

pass() should not be empty, it should have an always true assertion. Otherwise PHPUnit will complain about a risky test if the test does not contain any other assertion.

I wonder if we should deprecate the usage of pass() as well. Since we are unsure and you did not deprecate this method let's move it to BrowserTestBase.

Great, another round of nitpicks now that we have to keep the assert() method.

1. +++ b/core/tests/Drupal/Tests/WebAssert.php
   @@ -64,4 +66,83 @@ public function selectExists($select, TraversableElement $container = NULL) {
   +   * The parent method is overridden because its a private method.
   

   "its" should be "it is"

2. +++ b/core/tests/Drupal/Tests/WebAssert.php
   @@ -64,4 +66,83 @@ public function selectExists($select, TraversableElement $container = NULL) {
   +   * @param bool $condition
   +   *   The condition
   +   * @param string $message
   +   *   The failure message
   

   @param description should end with dots.

3. +++ b/core/tests/Drupal/Tests/WebAssert.php
   @@ -64,4 +66,83 @@ public function selectExists($select, TraversableElement $container = NULL) {
   +   * @throws \Behat\Mink\Exception\ExpectationException
   +   *   when the condition is not fulfilled.
   

   Description should start with a capital letter.

+++ b/core/tests/Drupal/Tests/WebAssert.php
@@ -64,4 +66,83 @@ public function selectExists($select, TraversableElement $container = NULL) {
+   * @param string $message
+   *   The failure message

This is not a failure message, it is an assertion message. "The assertion message describing the condition."

Looks good, assuming the testbot agrees.

Removed the \Drupal usage in the new config() method, improved issue summary.

Just hiding some files.

We need AccountInterface because the following line should work:

$this->drupalLogin($this->rootUser);

$this->rootUser is created as UserSession in installDrupal().

And $this->drupalLogin($this->rootUser); usage was discovered in #2735199: Convert web tests to browser tests for help module and ported to this issue.