Convert all color web tests to BrowserTestBase

1. +++ b/core/modules/color/test/src/Functional/ColorSafePreviewTest.php
   @@ -50,7 +50,7 @@ function testColorPreview() {
   -    $this->assertNoRaw('<script>alert("security filter test");</script>');
   +    $this->assertSession()->responseNotContains('<script>alert("security filter test");</script>');
   

   we should not change this in this conversion issue because assertNoRaw() will be added by #2750941: Additional BC assertions from WebTestBase to BrowserTestBase.

2. +++ b/core/modules/color/test/src/Functional/ColorTest.php
   @@ -202,8 +202,8 @@ function testOverrideAndResetScheme() {
   -    $this->assertNoRaw('files/color/bartik-', 'Make sure the color logo is not being used.');
   -    $this->assertRaw('bartik/logo.svg', 'Make sure the original bartik logo exists.');
   +    $this->assertSession()->responseNotContains('files/color/bartik-');
   +    $this->assertRaw('bartik/logo.svg');
   

   Same here, there will be assertNoRaw().

3. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
   @@ -43,7 +43,9 @@
   -  use BlockCreationTrait;
   +  use BlockCreationTrait {
   +    placeBlock as drupalPlaceBlock;
   +  }
   

   hm, that is kind of an API change. But I guess since we don't have placeBlock() for long this is OK to do.

As usual: create a deprecated method on AssertLegacyTrait and do the equivalent thing with WebAssert there.

1. +++ b/core/modules/color/test/src/Functional/ColorTest.php
   @@ -116,7 +116,7 @@ function _testColor($theme, $test_values) {
   -      $this->assertPattern('|' . file_url_transform_relative(file_create_url($stylesheet)) . '|', 'Make sure the color stylesheet is included in the content. (' . $theme . ')');
   +      $this->assertPattern('|' . file_url_transform_relative(file_create_url($stylesheet)) . '|');
   

   Let's not change this line if we don't have to. PHP will forgive us if we just pass one parameter too much. This can be cleaned up later, let's keep the conversion patches small.

   Same for the other places.

2. +++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
   @@ -204,6 +204,71 @@ protected function assertNoRaw($raw) {
      /**
   +   * Triggers a pass if the Perl regex pattern is found in the raw content.
   +   *
   +   * @param string $pattern
   +   *   Perl regex to look for including the regex delimiters.
   +   *
   +   * @deprecated Scheduled for removal in Drupal 9.0.0.
   +   *   Use $this->assertSession()->responseMatches() instead.
   +   */
   +  protected function assertPattern($pattern) {
   +    $this->assertSession()->responseMatches($pattern);
   +  }
   

   Can you extract this and other changes on AssertLegacyTrait and create a new patch in #2750941: Additional BC assertions from WebTestBase to BrowserTestBase?

3. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
   @@ -941,11 +955,15 @@ protected function submitForm(array $edit, $submit, $form_html_id = NULL) {
   +    // Store the drupalSettings JavaScript variable.
   +    $out = $this->getSession()->getPage()->getContent();
   +    $this->drupalSettings = [];
   +    if (preg_match('@<script type="application/json" data-drupal-selector="drupal-settings-json">([^<]*)</script>@', $out, $matches)) {
   +      $this->drupalSettings = Json::decode($matches[1]);
   +    }
   

   we should add a test case for BrowserTestBase for this. Can you open a new issue and do that?

4. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
   @@ -1613,6 +1631,30 @@ public function __sleep() {
   +   * @return array
   +   *   The drupalSettings value from the current page.
   +   */
   +  protected function getDrupalSettings() {
   +    return $this->drupalSettings;
   +  }
   

   This can return NULL if the variable has not been initialized. Should we initialize the class variable as array()?

5. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
   @@ -1613,6 +1631,30 @@ public function __sleep() {
   +   * @param array $settings
   +   *   The drupalSettings JavaScript variable.
   +   */
   +  protected function setDrupalSettings($settings) {
   

   type hint to array missing for the parameter.

+++ b/core/modules/color/test/src/Functional/ColorTest.php
@@ -116,7 +116,7 @@ function _testColor($theme, $test_values) {
-      $this->assertPattern('|' . file_url_transform_relative(file_create_url($stylesheet)) . '|', 'Make sure the color stylesheet is included in the content. (' . $theme . ')');
+      $this->assertPattern('|' . file_url_transform_relative(file_create_url($stylesheet)) . '|');

assertPattern() is deprecated, if we change the line we should do it properly. Two possibilities:
1) don't change the line.
2) change the line to $this->assertSession()->... which is not deprecated.

1. +++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
   @@ -204,6 +204,71 @@ protected function assertNoRaw($raw) {
   +  /**
   +   * Passes if the text is found ONLY ONCE on the text version of the page.
   +   *
   +   * The text version is the equivalent of what a user would see when viewing
   +   * through a web browser. In other words the HTML has been filtered out of
   +   * the contents.
   +   *
   +   * @param string|\Drupal\Component\Render\MarkupInterface $text
   +   *   Plain text to look for.
   +   */
   +  protected function assertUniqueText($text) {
   +    $this->assertUniqueTextHelper($text, TRUE);
   +  }
   

   All assert methods on AssertLegacyTrait should be deprecated. What should developers use instead? If we cannot find something good on Mink's WebAssert then we need to implemented this on our own WebAssert class.

2. +++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
   @@ -204,6 +204,71 @@ protected function assertNoRaw($raw) {
   +  /**
   +   * Passes if the text is found MORE THAN ONCE on the text version of the page.
   +   *
   +   * The text version is the equivalent of what a user would see when viewing
   +   * through a web browser. In other words the HTML has been filtered out of
   +   * the contents.
   +   *
   +   * @param string|\Drupal\Component\Render\MarkupInterface $text
   +   *   Plain text to look for.
   +   */
   +  protected function assertNoUniqueText($text) {
   

   same here.

3. +++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
   @@ -204,6 +204,71 @@ protected function assertNoRaw($raw) {
   +    if ($first_occurrence === FALSE) {
   +      $this->assertTrue(FALSE, $message);
   +    }
   

   Can be replaced with just $this->assertNotFalse($first_occurence, $message);

4. +++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
   @@ -204,6 +204,71 @@ protected function assertNoRaw($raw) {
   +    $this->assertTrue($be_unique == ($second_occurrence === FALSE), $message);
   

   let's always use "===" for consistency.

5. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
   @@ -941,11 +955,15 @@ protected function submitForm(array $edit, $submit, $form_html_id = NULL) {
   +    // Store the drupalSettings JavaScript variable.
   +    $out = $this->getSession()->getPage()->getContent();
   +    $this->drupalSettings = [];
   +    if (preg_match('@<script type="application/json" data-drupal-selector="drupal-settings-json">([^<]*)</script>@', $out, $matches)) {
   +      $this->drupalSettings = Json::decode($matches[1]);
   +    }
   

   any progress on the new issue that should introduce tests for this?

Thanks!

1. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
   @@ -637,7 +647,11 @@ protected function drupalGet($path, array $options = array()) {
   +    // Store the drupalSettings JavaScript variable.
   +    $this->drupalSettings = [];
   +    if (preg_match('@<script type="application/json" data-drupal-selector="drupal-settings-json">([^<]*)</script>@', $out, $matches)) {
   +      $this->drupalSettings = Json::decode($matches[1]);
   +    }
   

   Why do we even have to store that on every single drupalGet() request? We can just do that in a method on demand. So I think the getDrupalSettings() method can just do the regex extraction when needed.

2. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
   @@ -1613,6 +1631,30 @@ public function __sleep() {
      /**
   +   * Gets the value of drupalSettings for the currently-loaded page.
   +   *
   +   * The drupalSettings refers to the drupalSettings JavaScript variable.
   +   *
   +   * @return array
   +   *   The drupalSettings value from the current page.
   +   */
   +  protected function getDrupalSettings() {
   +    return $this->drupalSettings;
   +  }
   

   Not happy about the method naming, but I guess we want to keep the name for campatibility reasons? Also the short description should mention JavaScript to make it immediately clear.

   @return should say "The JSON decoded array of JavaScript drupalSettings from the current page."

3. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
   @@ -1613,6 +1631,30 @@ public function __sleep() {
   +  /**
   +   * Sets the value of drupalSettings for the currently-loaded page.
   +   *
   +   * The drupalSettings refers to the drupalSettings JavaScript variable.
   +   *
   +   * @param array $settings
   +   *   The drupalSettings JavaScript variable.
   +   */
   +  protected function setDrupalSettings(array $settings) {
   +    $this->drupalSettings = $settings;
   +  }
   

   why do we need the setter? What would be the use case to artificially set the javascript? I think this method should be removed.

4. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
   @@ -1780,6 +1822,56 @@ public static function assertEquals($expected, $actual, $message = '', $delta =
      /**
   +   * Passes if the text is found ONLY ONCE on the text version of the page.
   +   *
   +   * The text version is the equivalent of what a user would see when viewing
   +   * through a web browser. In other words the HTML has been filtered out of
   +   * the contents.
   +   *
   +   * @param string|\Drupal\Component\Render\MarkupInterface $text
   +   *   Plain text to look for.
   +   */
   +  protected function assertUniqueText($text) {
   +    $this->assertUniqueTextHelper($text, TRUE);
   +  }
   

   I think we should put all our assert methods on WebAssert and not mix some into BrowserTestBase and some on WebAssert.

Fine with me if you want to develop the test cases in this issue and then extract them later.

Extracted the JS settings method to #2759859: Implement getDrupalSettings() on BrowserTestBase for checking JS settings.

Removed also some messages.

1. +++ b/core/modules/color/tests/src/Functional/ColorTest.php
   @@ -108,7 +108,7 @@ function _testColor($theme, $test_values) {
   -    $this->assertUniqueText('Color set');
   +    $this->assertSession()->assertUniqueText('Color set');
   
   @@ -116,7 +116,7 @@ function _testColor($theme, $test_values) {
   -      $this->assertPattern('|' . file_url_transform_relative(file_create_url($stylesheet)) . '|', 'Make sure the color stylesheet is included in the content. (' . $theme . ')');
   +      $this->assertSession()->responseMatches('|' . file_url_transform_relative(file_create_url($stylesheet)) . '|');
   

   this line should not be changes, we should have assertPattern on the legacy trait.

2. +++ b/core/modules/color/tests/src/Functional/ColorTest.php
   @@ -108,7 +108,7 @@ function _testColor($theme, $test_values) {
   -    $this->assertUniqueText('Color set');
   +    $this->assertSession()->assertUniqueText('Color set');
   

   this line should not be changes, we should have assertUniqueText() on the legacy trait.

3. +++ b/core/tests/Drupal/Tests/WebAssert.php
   @@ -422,4 +422,55 @@ public function fieldDisabled($field, TraversableElement $container = NULL)  {
   +  public function assertUniqueText($text) {
   +    $this->assertUniqueTextHelper($text, TRUE);
   +  }
   

   we decided in #2759879: Additional assertions for WebAssert and AssertLegacyTrait, part 2 that we don't want to add this to our WebAssert.

I think we should not remove the messages with this patch, only makes it harder to review. The messages should be removed when the deprecated method calls are removed and WebAssert is used instead.

OK

Looks good! We need to get in #2759879: Additional assertions for WebAssert and AssertLegacyTrait, part 2 first.

Sorry, why postponing? Let's leave both in race. One will be committed first, the other will be rerolled accordingly.

The problem is that reviewers have to do double work now. Is the patch proposed here really doing the same thing as in the other issue? I need to compare both issues now to make sure the patch posted here really contains the thing proposed from over there. As soon as stuff is changed in the other issue, we need to fix stuff up here. But what if this gets to RTBC first and is accidentally committed while we change stuff in the other issue?

IMO this "race" just creates headaches, confusion and duplicate work for patch creators and reviewers.

As posted in #2759879: Additional assertions for WebAssert and AssertLegacyTrait, part 2 the logic in the new legacy assert methods need test coverage.

OK, this is after #2759879: Additional assertions for WebAssert and AssertLegacyTrait, part 2.

diff --git a/core/tests/Drupal/Tests/BrowserTestBase.php b/core/tests/Drupal/Tests/BrowserTestBase.php
index ce2721b..c045073 100644
--- a/core/tests/Drupal/Tests/BrowserTestBase.php
+++ b/core/tests/Drupal/Tests/BrowserTestBase.php
@@ -336,7 +336,7 @@ protected function initMink() {
   /**
    * Gets an instance of the default Mink driver.
    *
-   * @return Behat\Mink\Driver\DriverInterface
+   * @return \Behat\Mink\Driver\DriverInterface
    *   Instance of default Mink driver.
    *
    * @throws \InvalidArgumentException

Could we please keep any testing framework changes (8.1.x eligible?) out of this 8.2.x color.module issue?

ok.

Could we please keep any testing framework changes (8.1.x eligible?) out of this 8.2.x color.module issue?

Is documentation really a testing framework change? Well whatever

I've queued a heap of PHP7/Postgres test runs, to make sure this isn't impacted by the same problem as #2737805: Convert web tests to browser tests for forum module. (The forum test fails were by far the most disruptive recently, but I have also noticed similar fails in other BTB tests so I want to rule out that something similar might happen here.)

Okay, looks like this conversion does not introduce any fails at least above 3 Kelvin. Committed b561f51 and pushed to 8.2.x. Thanks!

Going forward, let's please stop doing individual module conversions and do larger chunks at once. See #2770921: Feb 21st: Convert chunk of WTB to BTB by just moving classes, changing use statements adding traits and https://www.drupal.org/core/scope#incomplete.

Hm... It's not possible for all. For example the Book conversion required to move one test as Javascript test #2757007: Convert all book web tests to BrowserTestBase.