Fix AssertLegacyTrait::assert(No)Text to handle html encoded strings (part 3)

+++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
@@ -64,6 +67,10 @@ protected function assertText($text) {
+      $text = html_entity_decode($text);

@@ -89,6 +99,10 @@ protected function assertNoText($text) {
+      $text = html_entity_decode($text);

+++ b/core/tests/Drupal/Tests/Core/Assert/AssertLegacyTraitTest.php
@@ -154,6 +157,58 @@ public function testAssertOptionSelectedFail() {
+   * @covers ::assertText
+   */
+  public function testAssertText() {
...
+  /**
+   * @covers ::assertNoText
+   */
+  public function testAssertNoText() {

It feels weird, this test seems to be mocking too much without testing the actual functionality, don't you think so?

Well, maybe mock just the actual HTTP client (guzzle) in this case and return a response object.

This bug exists in the stable branch.
Unfortunately the current patch will need a little forking because #2759879: Additional assertions for WebAssert and AssertLegacyTrait, part 2 wasn't pushed to 8.1.x.

Hm. I don't understand where is guzzle here.

@dawehner, any hints to get the major bug fixing moving again?
@claudiu.cristea, still working on this?

I think the approach of the patch is fine, just unit testing the trait methods. No need for any Guzzle responses testing, since the trait does not deal with Guzzle responses. Just one minor thing:

1. +++ b/core/tests/Drupal/Tests/Core/Assert/AssertLegacyTraitTest.php
   @@ -154,6 +157,58 @@ public function testAssertOptionSelectedFail() {
   +    $this->webAssert
   +      ->pageTextContains($sanitized)
   +      ->willThrow(new ResponseTextException('text not found', $this->getSession()));
   

   Instead of the willThrow() I think we should use ->shouldNotBeCalled(). So no need to set up the exception then.

2. +++ b/core/tests/Drupal/Tests/Core/Assert/AssertLegacyTraitTest.php
   @@ -154,6 +157,58 @@ public function testAssertOptionSelectedFail() {
   +    $this->webAssert
   +      ->pageTextNotContains($sanitized)
   +      ->willThrow(new ResponseTextException('text found', $this->getSession()));
   

   same here, should have ->shouldNotBeCalled().

Addressing my own feedback, removing some other not required mocking in the process.

+++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
@@ -89,6 +99,10 @@ protected function assertNoText($text) {
+      // Mink/BrowserKit uses PHP's DOMElement to get the value of an HTML tag.
+      // It will provide the value HTML entity decoded, so we need to do the
+      // same here for the text to actually match.
+      $text = html_entity_decode($text);

Great explanation!

I like the mocked text - but given the importance of this behaviour I think we should have a real test to prove the behaviour of mink. All the test currently does is prove what we pass it.

I think it is fine if we add a testLegacyAsserts() method to BrowserTestBaseTest. Same as I did in #2784537: Add legacy assertFieldByXPath()/assertNoFieldByXPath() method for browser tests.

@claudiu.cristea: I think you make a good point here and are right.

In Mink there is an important distinction to make:

$this->assertSession()->pageTextContains() is for inspecting the end user facing page text. All HTML tags are stripped in the page text, so you can never use this to test for actual script tags being there. Everything is HTML entity decoded, to represent the actual characters that the user sees.

Unfortunately Simpletest's assertText() works completely different. It uses the getTextContent() method which strips HTML tags with Xss::filter() and removes the whole section. It does not perform the HTML entity decoding.

So I think it is best to completely emulate Simpletest's behavior in assertText() and do the same Xss::filter() on the raw page content that you get from $this->getSession()->getPage()->getContent().

1. +++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
   @@ -89,11 +97,38 @@ protected function assertNoText($text) {
   +    $message = !$not_exists ? new FormattableMarkup('"@text" found', $args) : new FormattableMarkup('"@text" not found', $args);
   

   Could we remove this not(!) here? IMHO it just makes it a little harder to read.

2. +++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
   @@ -89,11 +97,38 @@ protected function assertNoText($text) {
   +
   +    $raw_content = $this->getSession()->getPage()->getContent();
   +    // Strip everything between the HEAD tags.
   +    $raw_content = preg_replace('@<head>(.+?)</head>@si', '', $raw_content);
   +    $page_text = Xss::filter($raw_content, []);
   +
   

   Maybe having some documentation why we are doing the head stripping + xss filtering would be nice.

3. +++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
   @@ -89,11 +97,38 @@ protected function assertNoText($text) {
   +    $actual = $not_exists == (strpos($page_text, (string) $text) === FALSE);
   +    $this->assertTrue($actual, $message);
   

   Could we use $this->assertEquals($not_exists, (strpos...)) instead?

Here we provide legacy assertions and we replicate the legacy behaviour. Simpletest assertText()/assertNoText() are returning a boolean but assertEquals() is returning a void. Our legacy method should return a boolean. That's why I computed the value in a variable and if I already have the variable why computing twice in assertEquals()?

Wow that's quite a level of detail ...

+++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
@@ -115,10 +115,12 @@ protected function assertNoText($text) {
+    // Retrieves the plain-text content from the current raw content by striping
+    // everything between the HEAD tags, well-forming all entities, removing
+    // dangerous protocols and filtering out all HTML tags.

This should contain more why, rather than, what, IMHO.

Here is an example.

assertText() tries to simulate what the user sees, given that it removes all text inside the head tags, removes inline JS etc., as they are not visible in a normal browser.

@claudiu.cristea
Yeah sorry I haven't put really a lot of time into phrasing the exact comment, I rather just wanted to give you an example of a WHY documentation in opposite to a WHAT documentation.

+++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
@@ -89,11 +97,41 @@ protected function assertNoText($text) {
+    $actual = $not_exists == (strpos($page_text, (string) $text) === FALSE);
+    $this->assertTrue($actual, $message);

This line is a big hard to read, but I cannot think of some really better solution, beside using assertEquals, which we probably don't want here?

Note: This needs a reroll against 8.3.x

Looks good, RTBC+1.

I'm not sure about the new approach taken in #22 / #23.

+++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
@@ -89,11 +97,41 @@ protected function assertNoText($text) {
+    $raw_content = preg_replace('@<head>(.+?)</head>@si', '', $raw_content);

This is wrong. It should be the body - otherwise this will include CSS file names (for example).

Ah yes you are right about the body - I just had interesting memories about an old issue.

+++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
@@ -57,6 +59,9 @@ protected function assertElementNotPresent($css_selector) {
    *   $this->assertSession()->responseContains() instead.
    */
   protected function assertText($text) {

@@ -82,6 +87,9 @@ protected function assertText($text) {
    *   $this->assertSession()->responseNotContains() instead.
    */
   protected function assertNoText($text) {

We need to update the @deprecated documentation to explain how to convert the assertions.

1. +++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
   @@ -53,10 +55,14 @@ protected function assertElementNotPresent($css_selector) {
   -   *   $this->assertSession()->responseContains() instead.
   
   @@ -78,10 +84,14 @@ protected function assertText($text) {
   -   *   $this->assertSession()->responseNotContains() instead.
   

   I'm pretty sure that the responseContains() is here because in Simpletests we occasionally set the content and then use this methods. But maybe @klausi might remember why this is here.

2. +++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
   @@ -78,10 +84,14 @@ protected function assertText($text) {
   +   *   exactly as human sees it in the browser.
   

   Missing a 'a'.

Of course one problem we have is that existing tests in contrib might have converted halfway... if still be using the assertText but have changed the text inline with the assumptions pageTextContains() makes. In which case committing this patch will break their tests. Tricky.

@claudiu.cristea contrib is tested against the latest branch of D8 - it has assertText since #2735045: Convert StandardTest to BrowserTestBase by introducing a AssertLegacyTrait and new assertions on the WebAssert landed... ie. 7th July this year.

@claudiu.cristea I think I agree - having the legacy methods behave the same way as WebTestBase is more important. Committing to 8.2.x as well since this is quite an important bug fix in the testing system that will help contrib convert to BrowserTestBase with the smallest amount of difficulty.

Committed and pushed d70e577 to 8.3.x and 95741c0 to 8.2.x. Thanks!

Thank you alex!