Child to #2735005: Convert all Simpletest web tests to BrowserTestBase (or UnitTestBase/KernelTestBase)

Problem/Motivation

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#29 interdiff.txt533 bytesclaudiu.cristea
#29 2753179-29.patch2.5 KBclaudiu.cristea
#27 interdiff.txt3.3 KBclaudiu.cristea
#27 2753179-27.patch3.02 KBclaudiu.cristea
#21 interdiff.txt9.7 KBclaudiu.cristea
#21 2753179-21.patch6.33 KBclaudiu.cristea
#19 interdiff.txt3.42 KBclaudiu.cristea
#19 2753179-19.patch9.16 KBclaudiu.cristea
#17 interdiff.txt10.96 KBclaudiu.cristea
#17 2753179-17.patch6.45 KBclaudiu.cristea
#13 convert_all_color_web-2753179-13.patch8.62 KBjibran
#13 interdiff.txt4.73 KBjibran
#11 convert_all_color_web-2753179-11.patch8.45 KBjibran
#11 interdiff.txt2.51 KBjibran
#9 convert_all_color_web-2753179-9.patch10.7 KBjibran
#9 interdiff.txt928 bytesjibran
#6 convert_all_color_web-2753179-6.patch10.69 KBjibran
#6 interdiff.txt9.52 KBjibran
#2 convert_all_simpletest-2753179-2.patch6.56 KBjibran
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

jibran created an issue. See original summary.

jibran’s picture

Title: Convert all Simpletest web tests to BrowserTestBase (or UnitTestBase/KernelTestBase) » Convert all color web tests to BrowserTestBase
FileSize
6.56 KB
klausi’s picture

Status: Active » Needs work
  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.

jibran’s picture

Yeah I'm waiting for #2750941: Additional BC assertions from WebTestBase to BrowserTestBase to be committed.

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

How do we want to tackle this?

klausi’s picture

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

jibran’s picture

Status: Needs work » Needs review
FileSize
9.52 KB
10.69 KB

Re-roll after #2750941: Additional BC assertions from WebTestBase to BrowserTestBase. Added the helper methods as mentioned in #5. Fixed #3 as well.

klausi’s picture

Status: Needs review » Needs work
  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.

jibran’s picture

Thanks for the review.

  1. let's keep the conversion patches small. It is already done so I'm not going to revert it. If someone wants to then go for it.
  2. Yeah sure but imo the probability of getting this committed earlier then #2750941: Additional BC assertions from WebTestBase to BrowserTestBase is higher.
  3. Sure.
  4. Nice catch
  5. Sure.
jibran’s picture

Status: Needs work » Needs review
FileSize
928 bytes
10.7 KB
klausi’s picture

+++ 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.

jibran’s picture

Let's do 1) then

klausi’s picture

Status: Needs review » Needs work
  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?

jibran’s picture

Status: Needs work » Needs review
FileSize
4.73 KB
8.62 KB
  1. Moved to BTB.
  2. Moved to BTB.
  3. Done.
  4. Done.
  5. I'll create the new issue after the RTBC.
klausi’s picture

Status: Needs review » Needs work

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.

jibran’s picture

Thanks for another round.

  1. WTB is storing it for every request so I didn't change the flow but setting it on demand is a good idea.
  2. Yes exactly. Nice suggestion and a good catch.
  3. I copied it from WTB. We can surly drop it.
  4. Interesting, I was just trying to keep the changes in tests minimum but +1 on moving to WebAssert and I don' t think @dawehner would like it very much.
klausi’s picture

claudiu.cristea’s picture

FileSize
6.45 KB
10.96 KB
claudiu.cristea’s picture

Status: Needs work » Needs review
claudiu.cristea’s picture

FileSize
9.16 KB
3.42 KB

Removed also some messages.

klausi’s picture

Status: Needs review » Needs work
  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.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
6.33 KB
9.7 KB

OK

klausi’s picture

Status: Needs review » Postponed
claudiu.cristea’s picture

Status: Postponed » Needs review

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

klausi’s picture

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.

jibran’s picture

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

I won't mind this headache but I do mind postponing a complete issue on other incomplete issue.

@klausi if this is done then can we RTBC this. Thank you. :-)

klausi’s picture

Status: Needs review » Needs work

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

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
3.02 KB
3.3 KB
Eric_A’s picture

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?

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

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

xjm’s picture

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.)

  • xjm committed b561f51 on 8.2.x
    Issue #2753179 by claudiu.cristea, jibran, klausi: Convert all color web...
xjm’s picture

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.

claudiu.cristea’s picture

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.

  • xjm committed b561f51 on 8.3.x
    Issue #2753179 by claudiu.cristea, jibran, klausi: Convert all color web...

  • xjm committed b561f51 on 8.3.x
    Issue #2753179 by claudiu.cristea, jibran, klausi: Convert all color web...

Status: Fixed » Closed (fixed)

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