Problem/Motivation

This extract changes from #2747167: Convert first part of web tests of views_ui into its own patch.

Proposed resolution

There are many more, so let's not spend too much time here.

Remaining tasks

User interface changes

API changes

Data model changes

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

dawehner created an issue. See original summary.

dawehner’s picture

Title: Additional BC layers from WebTestBase to BrowserTestBase » Additional BC assertions from WebTestBase to BrowserTestBase
Status: Active » Needs review
FileSize
18.59 KB
klausi’s picture

Status: Needs review » Needs work

Yippee, great work!

  1. +++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
    @@ -181,12 +196,98 @@ protected function assertTitle($expected_title) {
    +   * @deprecated Scheduled for removal in Drupal 9.0.0.
    +   *   Use $this->assertSession()->fieldExists() or
    +   *   $this->assertSession()->fieldValueEquals() instead.
    +   */
    +  protected function assertNoFieldById($id, $value = '') {
    

    that does not really match. Did you mean fieldNotExists() and fieldValueNotEquals()?

  2. +++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
    @@ -181,12 +196,98 @@ protected function assertTitle($expected_title) {
    +  protected function assertUrl($path) {
    +    $path = "/$path";
    +    $this->assertSession()->addressEquals($path);
    +  }
    

    The $path prefixing is not necessary anymore, this is handled by our cleanUrl() override.

  3. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
    @@ -899,6 +919,11 @@ protected function submitForm(array $edit, $submit, $form_html_id = NULL) {
    +      // Provide support for 1, 0 for checkboxes instead of TRUE and FALSE.
    +      if (strpos($name, 'name[') === 0) {
    +        $value = (bool) $value;
    +      }
    

    I think we should not support that. People should really pass in TRUE/FALSE. Let's add a @todo to remove this and fix all the callers that pass a wrong integer later.

  4. +++ b/core/tests/Drupal/Tests/WebAssert.php
    @@ -215,6 +215,150 @@ public function linkExists($label, $index = 0, $message = '') {
    +   * @param string|\Drupal\Component\Render\MarkupInterface $label
    +   *   Text between the anchor tags.
    

    I really hate this two type anti pattern. We should only allow strings in the future. The caller is responsible to make it a string. In most tests it makes zero sense to call t() for asserting a link label. We should really clean up our test cases to never call t(), which is almost always wrong. That is also the reason why we have to mess up our method signatures everywhere with MarkupInterface. For this conversion effort I guess there is not much we can do about it. Sad klausi is sad.

  5. +++ b/core/tests/Drupal/Tests/WebAssert.php
    @@ -215,6 +215,150 @@ public function linkExists($label, $index = 0, $message = '') {
    +    if (!empty($links)) {
    +      throw new ExpectationException($message);
    +    }
    +    $this->assert(TRUE, $message);
    

    this can be shortened to just $this->assert(empty($links), $message);

  6. +++ b/core/tests/Drupal/Tests/WebAssert.php
    @@ -215,6 +215,150 @@ public function linkExists($label, $index = 0, $message = '') {
    +    if (!empty($links[$index])) {
    +      throw new ExpectationException($message);
    +    }
    +    else {
    +      $this->assert(TRUE, $message);
    +    }
    

    same here.

  7. +++ b/core/tests/Drupal/Tests/WebAssert.php
    @@ -215,6 +215,150 @@ public function linkExists($label, $index = 0, $message = '') {
    +  /**
    +   * Builds an XPath query.
    +   *
    +   * Builds an XPath query by replacing placeholders in the query by the value
    +   * of the arguments.
    +   *
    

    I have the feeling that this method is completely redundant and we just haven't found the mink equivalent yet :) Fine to keep it in the meantime, but let's keep looking.

dawehner’s picture

Status: Needs work » Needs review
FileSize
18.62 KB
3.94 KB

Thank you for your quick review klausi!

that does not really match. Did you mean fieldNotExists() and fieldValueNotEquals()?

Oh yeah, totally.

The $path prefixing is not necessary anymore, this is handled by our cleanUrl() override.

Good point.

I think we should not support that. People should really pass in TRUE/FALSE. Let's add a @todo to remove this and fix all the callers that pass a wrong integer later.

Well its 1/0 in the HTML representation.

I really hate this two type anti pattern. We should only allow strings in the future. The caller is responsible to make it a string. In most tests it makes zero sense to call t() for asserting a link label. We should really clean up our test cases to never call t(), which is almost always wrong. That is also the reason why we have to mess up our method signatures everywhere with MarkupInterface. For this conversion effort I guess there is not much we can do about it. Sad klausi is sad.

I totally agree. I'm a bit sad about klausi being sad :)

this can be shortened to just $this->assert(empty($links), $message);
same here.

Oh nice!

klausi’s picture

Status: Needs review » Needs work
  1. +++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
    @@ -181,12 +196,97 @@ protected function assertTitle($expected_title) {
    +   * Passes if a link with the specified label is found.
    +   *
    +   * An optional link index may be passed.
    +   *
    +   * @param string|\Drupal\Component\Render\MarkupInterface $label
    +   *   Text between the anchor tags.
    +   *
    +   * @deprecated Scheduled for removal in Drupal 9.0.0.
    +   *   Use $this->assertSession()->linkNotExists() instead.
    +   */
    +  protected function assertNoLink($label) {
    

    Comment is wrong. Should be "... is NOT found".

  2. +++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
    @@ -181,12 +196,97 @@ protected function assertTitle($expected_title) {
    +   * @deprecated Scheduled for removal in Drupal 9.0.0.
    +   *   Use $this->assertSession()->LinkByHref() instead.
    +   */
    

    "LinkByHref() should start lower case.

  3. +++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
    @@ -217,16 +317,44 @@ protected function assertNoOption($id, $option) {
    +   * Passes if the raw text IS NOT found escaped on the loaded page, fail otherwise.
    

    Over 80 characters. I think we can just remove the "fail otherwise" part.

  4. +++ b/core/tests/Drupal/Tests/WebAssert.php
    @@ -208,10 +208,140 @@ public function linkExists($label, $index = 0, $message = '') {
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function fieldValueEquals($field, $value, TraversableElement $container = NULL) {
    +    parent::fieldValueEquals($field, (string) $value, $container);
    +  }
    

    Let's not start with the (string) casting crap in our precious WebAssert class. We should do that in the legacy trait, but all future code should pass only strings because that is more correct.

  5. +++ b/core/tests/Drupal/Tests/WebAssert.php
    @@ -208,10 +208,140 @@ public function linkExists($label, $index = 0, $message = '') {
    +    // Cast MarkupInterface objects to string.
    +    $xpath = $this->buildXPathQuery('//a[contains(@href, :href)]', [':href' => $href]);
    +    $message = ($message ? $message : strtr('Link containing href %href found.', ['%href' => $href]));
    +    $links = $this->session->getPage()->findAll('xpath', $xpath);
    +    $this->assert(!empty($links[$index]), $message);
    

    Hm, you are not casting to string anywhere here, so the comment is wrong?

  6. +++ b/core/tests/Drupal/Tests/WebAssert.php
    @@ -208,10 +208,140 @@ public function linkExists($label, $index = 0, $message = '') {
    +    // Cast MarkupInterface objects to string.
    +    $xpath = $this->buildXPathQuery('//a[contains(@href, :href)]', [':href' => $href]);
    +    $message = ($message ? $message : strtr('Link containing href %href found.', ['%href' => $href]));
    +    $links = $this->session->getPage()->findAll('xpath', $xpath);
    +    $this->assert(empty($links[$index]), $message);
    

    same here?

  7. +++ b/core/tests/Drupal/Tests/WebAssert.php
    @@ -208,10 +208,140 @@ public function linkExists($label, $index = 0, $message = '') {
    +      // Cast MarkupInterface objects to string.
    +      if (is_object($value)) {
    +        $value = (string) $value;
    +      }
    

    and the casting again that I don't like. Can we get rid of it or does that cause a lot of fails?

dawehner’s picture

Status: Needs work » Needs review
FileSize
18.56 KB
4.19 KB

Thank you for your great review klausi.

and the casting again that I don't like. Can we get rid of it or does that cause a lot of fails?

Well, let's try it out

dawehner’s picture

klausi’s picture

Status: Needs review » Needs work

Good progress, let's make sure we keep our WebAssert class clean of the MarkupInterface conversion nightmare:

  1. +++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
    @@ -181,12 +196,97 @@ protected function assertTitle($expected_title) {
       /**
    +   * Passes if a link with the specified label is not found.
    +   *
    +   * An optional link index may be passed.
    +   *
    +   * @param string|\Drupal\Component\Render\MarkupInterface $label
    +   *   Text between the anchor tags.
    +   *
    +   * @deprecated Scheduled for removal in Drupal 9.0.0.
    +   *   Use $this->assertSession()->linkNotExists() instead.
    +   */
    +  protected function assertNoLink($label) {
    

    there is no optional link index parameter? I think that line should be removed.

  2. +++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
    @@ -181,12 +196,97 @@ protected function assertTitle($expected_title) {
    +   * @param \Drupal\Core\Url|string $path
    +   *   The expected system path or URL.
    

    If the method also accepts Url then $path must be converted before passing on to WebAssert.

  3. +++ b/core/tests/Drupal/Tests/WebAssert.php
    @@ -208,10 +208,135 @@ public function linkExists($label, $index = 0, $message = '') {
    +   * @param string|\Drupal\Component\Render\MarkupInterface $label
    +   *   Text between the anchor tags.
    

    let's keep our WebAssert class clean and only accept string here.

  4. +++ b/core/tests/Drupal/Tests/WebAssert.php
    @@ -208,10 +208,135 @@ public function linkExists($label, $index = 0, $message = '') {
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function fieldValueEquals($field, $value, TraversableElement $container = NULL) {
    +    parent::fieldValueEquals($field, $value, $container);
    +  }
    

    Why is that override needed if it only calls the parent method? Please add a comment.

  5. +++ b/core/tests/Drupal/Tests/WebAssert.php
    @@ -208,10 +208,135 @@ public function linkExists($label, $index = 0, $message = '') {
    +  public function linkByHrefNotExists($href, $index = 0, $message = '') {
    +    // Cast MarkupInterface objects to string.
    +    $xpath = $this->buildXPathQuery('//a[contains(@href, :href)]', [':href' => $href]);
    

    no, we are not casting anything to string here, so that comment should be removed.

  6. +++ b/core/tests/Drupal/Tests/WebAssert.php
    @@ -208,10 +208,135 @@ public function linkExists($label, $index = 0, $message = '') {
    +      // Cast MarkupInterface objects to string.
    +      if (is_object($value)) {
    +        throw new \InvalidArgumentException('Just pass in scalar values.');
    +      }
    

    Comment does not match what we are doing here. I think we should remove all the is_string() checking in this method. Just assume a string is passed in, if something goes wrong it is the callers fault.

dawehner’s picture

Status: Needs work » Needs review
FileSize
19.16 KB
5.24 KB

let's keep our WebAssert class clean and only accept string here.

Sure, let's fix linkExists as well.

Why is that override needed if it only calls the parent method? Please add a comment.

There is no reason :)

klausi’s picture

Status: Needs review » Needs work
+++ b/core/tests/Drupal/Tests/WebAssert.php
@@ -38,6 +39,9 @@ public function __construct(Session $session, $base_url = '') {
   protected function cleanUrl($url) {
+    if ($url instanceof Url) {
+      $url = $url->setAbsolute()->toString();
+    }

I think we should do that on the legacy trait. Our WebAssert is an extension of Mink's WebAssert, so I think we should conform to the data types of the parent class and only allow string. Same case as with MarkupInterface, I think we should not pollute our WebAssert class with this dual parameter type limbo.

If we think about the use case of addressEquals() (calling cleanUrl() down the line) then 95% of use cases will be to just call it with a string. Make sure I'm on the /node/1 path for example. Using Url is a special case and the caller should convert it to string.

dawehner’s picture

I think we should do that on the legacy trait. Our WebAssert is an extension of Mink's WebAssert, so I think we should conform to the data types of the parent class and only allow string. Same case as with MarkupInterface, I think we should not pollute our WebAssert class with this dual parameter type limbo.

The one thing I'm not sure about is the url. The common case/good approach is to use url objects maybe? We should make it not too hard to use that. Its hard to decided in some sane way.

klausi’s picture

Status: Needs work » Reviewed & tested by the community

I don't think the common case is to use Url, this looks weird:

$this->assertSession()->addressEquals(Url::formRoute('entity.node.preview', ['node_preview' => 1, 'view_mode_id' => 'full']));

which is just bad test code. This would be the right way to do it:

$this->assertSession()->addressEquals('node/preview/1/full');

A clear assertion that is a black box test of the actual test outcome - no Url abstraction layer testing in between.

Anyway, this is a philosophical detail we can continue to fight about another time. We should not hold up this issue because of that, otherwise looks RTBC to me.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: 2750941-9.patch, failed testing.

jibran’s picture

+++ b/core/tests/Drupal/Tests/BrowserTestBase.php
@@ -899,6 +919,12 @@ protected function submitForm(array $edit, $submit, $form_html_id = NULL) {
+      // Provide support for 1, 0 for checkboxes instead of TRUE and FALSE.
+      // @todo Get rid of supporting 1/0.
+      if (strpos($name, 'name[') === 0) {
+        $value = (bool) $value;
+      }

We can do this correctly after next line.

if (($type = $field->getAttribute('type')) && ($type == 'checkbox')) {
        $value = (bool) $value;
      }
klausi’s picture

Status: Needs work » Needs review
FileSize
19.1 KB
1.06 KB

Good idea, did a reroll and implemented that.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you @jibran. That's indeed much better

klausi’s picture

Status: Reviewed & tested by the community » Needs work

Just found out in #2737805: Convert web tests to browser tests for forum module that we cannot just remove the buildXPathQuery() method because it used by legacy tests. Should have a wrapper on AssertLegacyTrait.

klausi’s picture

Status: Needs work » Needs review
FileSize
24.16 KB
6.13 KB

Merged in requirements from forum test conversion.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Let's ensure to not great crazy, but those additions are great!

jibran’s picture

FileSize
529 bytes
24.38 KB

Moved following change from #2753179: Convert all color web tests to BrowserTestBase here..

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

Moved following change from #2753179: Convert all color web tests to BrowserTestBase here..

Well, this is sort of a break at this time, isn't it?

klausi’s picture

We introduced BlockCreationTrait only 2 weeks ago to BrowserTestBase, so I think it is fine to change the method name, which will help us a lot for conversions.

This is still RTBC IMO.

dawehner’s picture

Fair. Let's PLEASE stop adding stuff now.

klausi’s picture

Agreed, let's freeze this issue on the current changes and do anything else that might come up in follow-ups.

naveenvalecha’s picture

#23 +1 agreed Let's keep adding stuff in BrowserTestBase.php I have introduced couple of assertion method in contact module conversion. https://www.drupal.org/node/2742995#comment-11344165

  • catch committed c9fb29d on 8.2.x
    Issue #2750941 by dawehner, klausi, jibran: Additional BC assertions...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.2.x, thanks!

dawehner’s picture

Version: 8.2.x-dev » 8.1.x-dev
Status: Fixed » Reviewed & tested by the community

Thank you catch! Do you think this could be committed to 8.1.x as well? Many of the other conversions got committed as well.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Yep.

dawehner’s picture

catch commit faster than his shadow!

  • catch committed 8946087 on 8.1.x
    Issue #2750941 by dawehner, klausi, jibran: Additional BC assertions...
klausi’s picture

Version: 8.1.x-dev » 9.x-dev
Status: Fixed » Active

Thanks catch!

Changing this to active for the next round of missing legacy assert helpers, they need to be extracted from #2756059: Convert web tests to browser tests for language module for example.

klausi’s picture

Version: 9.x-dev » 8.2.x-dev

Wrong core version, sorry.

catch’s picture

Status: Active » Fixed

Let's please open a new issue for any new additions, otherwise the history gets confusing.

pfrenssen’s picture

public function buildXPathQuery($xpath, array $args = array()) {
  return $this->assertSession()->buildXPathQuery($xpath, $args);
}

This should be a protected method, like the other methods in this class. This is causing fatal errors in the tests for Organic Groups. Her is an example from build 140793354:

PHP Fatal error: Access level to Drupal\simpletest\AssertContentTrait::buildXPathQuery() must be public (as in class Drupal\Tests\BrowserTestBase) in /home/travis/build/amitaibu/og/og_ui/tests/src/Functional/BundleFormAlterTest.php on line 24

I'll make a followup to fix this.

pfrenssen’s picture

klausi’s picture

Status: Fixed » Closed (fixed)

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

klausi’s picture