Add methods to assert that a sequence of strings appears on the page in a given order

This looks like a nice addition. Sadly we are currently a bit stuck in the discussion what we want to add in our testing framework.

Note: We should have tests for our tests.

+++ b/core/tests/Drupal/Tests/WebAssert.php
@@ -501,4 +501,31 @@ public function hiddenFieldValueNotEquals($field, $value, TraversableElement $co
+      if (($pos = strpos($text, $item)) === FALSE) {
+        throw new ExpectationException("Cannot find '$item' in the page", $this->session->getDriver());
+      }
+      $strings[$pos] = $item;
...
+    if ($items !== array_values($strings)) {
+      throw new ExpectationException("Strings were not correctly ordered as: $ordered.", $this->session->getDriver());
+    }

Is there a reason you haven't used $this->assert() ?

Well we have \Drupal\Tests\Core\Assert\AssertLegacyTraitTest and \Drupal\FunctionalTests\BrowserTestBaseTest

+++ b/core/tests/Drupal/Tests/WebAssert.php
@@ -501,4 +501,31 @@ public function hiddenFieldValueNotEquals($field, $value, TraversableElement $co
+    if ($items !== array_values($strings)) {
+      throw new ExpectationException("Strings were not correctly ordered as: $ordered.", $this->session->getDriver());
+    }

This code is copied from #2757007: Convert all book web tests to BrowserTestBase and, yes, I agree that this should go as $this->assertNotSame().

On the other hand, I'm asking if we should not have 2 methods like this: one searching in page HTML, the other in page Text.

EDIT: That would be: orderInPageText() and orderInPageContent()

+++ b/core/tests/Drupal/Tests/WebAssert.php
@@ -501,4 +501,31 @@ public function hiddenFieldValueNotEquals($field, $value, TraversableElement $co
+    foreach ($items as $item) {
+      if (($pos = strpos($text, $item)) === FALSE) {
+        throw new ExpectationException("Cannot find '$item' in the page", $this->session->getDriver());
+      }
+      $strings[$pos] = $item;
+    }

Also here, I would collect all "not found" items and throw the exception after foreach() {...}, if case. This would be more helpful for the debugging.

This code is copied from #2757007: Convert all book web tests to BrowserTestBase and, yes, I agree that this should go as $this->assertNotSame().

Well, web asserts cannot access the usual assertions, just use $this->assert() though is fine. We are writing a new assertion here, so using "just"$this->assert is kinda fine.

@dawehner, how about this?

EDIT: Ignore the interdiff, it's a wrong patch.

.

+++ b/core/tests/Drupal/Tests/WebAssert.php
@@ -501,4 +501,62 @@ public function hiddenFieldValueNotEquals($field, $value, TraversableElement $co
+  public function orderInPageContent(array $items) {

I'd have expected this method to be named orderInPageHtml, so its clear what this is about. Content is quite a vague term to be honest.

OK, renamed. But I see often in the new phpunit framework the usage of Text for what a user sees in the browser and Content for page markup.

But I see often in the new phpunit framework the usage of Text for what a user sees in the browser and Content for page markup.

Oh do you have some examples? In that case it totally makes sense to keep content around.

@dawehner,

Oh do you have some examples? In that case it totally makes sense to keep content around.

This?

$this->getSession()->getPage()->getText();
$this->getSession()->getPage()->getContent();

But, anyway, "HTML" sounds more explicit for me, too.

Oh you are right, given that it feels a bit better to use content? Please decide what you think is better.

I don't have a strong opinion. I like more "html", because it tells better the story. On the other hand seems that "content" is more consistent, right?

Here's a version with "content". Committers are able to chose between #13 and #17 -- we always drop the big decisions on core committers :)))

Here's a version with "content". Committers are able to chose between #13 and #17 -- we always drop the big decisions on core committers :)))

Ha, good idea!

Nice addition to the test framework!

Not sure about the logic in orderInPageHelper() thought. Currently, if you search for the same string twice this will fail.

example:
$this->assertSession()->orderInPageContent(['item3', 'item3', 'item2']);
will fail, where I would expect it to pass.

Why not just loop through the strings and use the $pos of the previous string as an offset in strpos and just check === FALSE? Seems more straight forward to me.

Some other things:

1. +++ b/core/tests/Drupal/FunctionalTests/BrowserTestBaseTest.php
   @@ -289,4 +290,19 @@ public function testInstall() {
   +    $this->setExpectedException(ElementNotFoundException::class, "Cannot find item(s): 'item5', 'item8'.");
   

   This should probably use a try-catch/fail-pass pattern like in testLegacyFieldAsserts(), that will make it easier to add more cases to this test.

2. +++ b/core/tests/Drupal/Tests/WebAssert.php
   @@ -501,4 +501,68 @@ public function hiddenFieldValueNotEquals($field, $value, TraversableElement $co
   +  protected function orderInPageHelper(array $items, $string) {
   

   Why not make this public and rename to assertOrderInString or something? I can see a use case for testing the order of strings in something else then the full page.

Added new task: search for @todos.

Moved to 8.8.x as is about test framework.

The book titles are placed inside <input .../> tags, so we need to use the method that is searching markup, not in text.

1. +++ b/core/tests/Drupal/FunctionalTests/BrowserTestBaseTest.php
   @@ -776,4 +777,26 @@ public function testDeprecationHeaders() {
   +    // Check that passing non-existent search strings throws an exception.
   

   Does it also make sense to test the correct strings in the wrong order?

2. +++ b/core/tests/Drupal/Tests/WebAssert.php
   @@ -626,4 +626,78 @@ public function pageTextContainsOnce($text) {
   +      if (($pos = strpos($string, $substring, $offset)) === FALSE) {
   +        $not_found[] = "'$substring'";
   

   Now that it's using an offset, if the actual content is 'AB' and you expect ['B', 'A'] I think it will throw an ElementNotFoundException rather than failing on the order.

3. +++ b/core/tests/Drupal/Tests/WebAssert.php
   @@ -626,4 +626,78 @@ public function pageTextContainsOnce($text) {
   +    $this->assert($expected_order === $actual_order, $failure_message);
   

   I guess this is not using assertSame() to have a neat customised assertion message?

4. +++ b/core/tests/Drupal/Tests/WebAssert.php
   @@ -626,4 +626,78 @@ public function pageTextContainsOnce($text) {
   +  public function orderInPageContent(array $expected_order) {
   +    $this->orderInString($expected_order, $this->session->getPage()->getHtml());
   

   There was some talk from #12 about whether to call this orderInPageContent or orderInPageHtml but currently the assertion has Content in its name while it calls getHtml() (which returns the inner HTML apparently) rather than getContent(). It's an edge case, but I guess it makes more sense to use getContent() on the off-chance someone wants to assert on the attributes of the html element itself?!

5. +++ b/core/tests/Drupal/Tests/WebAssert.php
   @@ -626,4 +626,78 @@ public function pageTextContainsOnce($text) {
   +    catch (ExpectationException $exception) {
   +      // When checking the order in page text we throw ResponseTextException
   +      // rather than ExpectationException.
   +      throw new ResponseTextException($exception->getMessage(), $this->session->getDriver(), $exception);
   +    }
   

   Thanks for teaching me more about Drupal test exceptions (:

Thanks!

@AndyF, thank you for review.

#30.1: Added a test case.

#30.2: True. Fixed.

#30.3: I tried with assertSame() but because of offset the message is misleading. For instance it's possible to receive such an output:

Strings incorrectly ordered
Failed asserting that Array &0 (
    0 => 'item2'
) is identical to Array &0 (
    0 => 'item2'
    1 => 'item1'
    2 => 'item3'
).

and this might confuse the user as the actual order contains also item1 & item2. I went with a simple message.

#30.4: So, "content" or "html"?

#30.5: 👍

#2769825: Cannot swap tabledrag rows by dragging the lower over the upper row in tests is merged, we can now remove the @todo. However, I would wait to see it that goes also in 8.8.x because this also qualifies a as 8.8.x (because is an addition to the test framework).

#30.3: I tried with assertSame() but because of offset the message is misleading

Oh yeah, that's a very confusing message, +1.

#30.4: So, "content" or "html"?

I don't see a big difference, my vote goes for content just in case there's ever a need to assert order on the root html element itself, which seems a bit edge-case, but you know what they say about assumptions (:

1. +++ b/core/tests/Drupal/FunctionalTests/BrowserTestBaseTest.php
   @@ -776,4 +777,45 @@ public function testDeprecationHeaders() {
   +   * @see \Drupal\Tests\WebAssert::orderInString()
   +   * @see \Drupal\Tests\WebAssert::orderInPageContent()
   +   * @see \Drupal\Tests\WebAssert::orderInPageText()
   

   Does it make sense to use @covers?

2. +++ b/core/tests/Drupal/FunctionalTests/BrowserTestBaseTest.php
   @@ -776,4 +777,45 @@ public function testDeprecationHeaders() {
   +    // Check that passing existing substrings in the wrong order fails.
   +    try {
   +      $this->assertSession()->orderInString(['item2', 'item1', 'item3'], 'item1item3item2');
   +    }
   +    catch (ExpectationException $exception) {
   +      if ($exception->getMessage() !== 'Strings found but incorrectly ordered') {
   +        $this->fail();
   +      }
   +    }
   

   I don't think this is actually asserting the exception happens, just asserting the message if it does. I think \PHPUnit\Framework\TestCase::expectException() is the right tool here (and again below).

Thanks

Moving back to 8.9.x as #2769825: Cannot swap tabledrag rows by dragging the lower over the upper row in tests was not committed to 8.8.x.

#33.1: Yes, let's use @covers.

#33.2:

I don't think this is actually asserting the exception happens, just asserting the message if it does

I agree, it was an incomplete implementation. Now it asserts also that the exception is thrown and fails if not but still checks for the correct exception message.

I think \PHPUnit\Framework\TestCase::expectException() is the right tool here (and again below).

In this particular test we cannot use ::expectException() as you cannot have multiple expectException() in a single test. See https://thephp.cc/news/2016/02/questioning-phpunit-best-practices and https://stackoverflow.com/questions/1593834/how-do-i-test-for-multiple-e.... Please, also read comment #19.1.

As #2769825: Cannot swap tabledrag rows by dragging the lower over the upper row in tests has been committed I removed the custom implementation from the table drag test.

1. +++ b/core/tests/Drupal/FunctionalTests/BrowserTestBaseTest.php
   @@ -776,4 +777,43 @@ public function testDeprecationHeaders() {
   +    try {
   +      $this->assertSession()->orderInString(['item2', 'item1', 'item3'], 'item1item3item2');
   +      $this->fail('Expected ExpectationException exception has not been thrown.');
   +    }
   

   👍

2. +++ b/core/tests/Drupal/Tests/WebAssert.php
   @@ -626,4 +626,78 @@ public function pageTextContainsOnce($text) {
   +  public function orderInPageContent(array $expected_order) {
   +    $this->orderInString($expected_order, $this->session->getPage()->getHtml());
   

   I don't know how much we care, but there's still a mismatch between the name used in the WebAssert method and the call made to the page object. (Just in case I wasn't clear before, \Behat\Mink\Element\DocumentElement has both getContent() and getHtml()).

Apart from that one question about html/content it looks great to me, thanks.

There is a CS issue in orderInString method.
Edit: Never mind. I modified that file myself accidentally.

#35.1:

(...) but there's still a mismatch between the name used in the WebAssert method and the call made to the page object. (Just in case I wasn't clear before, \Behat\Mink\Element\DocumentElement has both getContent() and getHtml()).

@AndyF, Wow, finally I got the problem. I wasn't aware about the two methods. Inspecting both, I see they are different but are leading to the same results. So, I went with ::getContent().

#36: @Chi, I cannot find this CS warning, neither the error in ::orderInString().

👌 Looks good to me, thanks.

#36: @Chi, I cannot find this CS warning, neither the error in ::orderInString().

I'm also a bit lost as to where that might come from, I don't see any CS messages.

Seeing as this a) only adds new testing capabilities (which it tests) and b) there are two pre-existing tests that now use it and c) everything's green, I'm tentatively setting it to RTBC despite being just one, hope that's ok...

+++ b/core/tests/Drupal/Tests/WebAssert.php
@@ -626,4 +626,78 @@ public function pageTextContainsOnce($text) {
+  public function orderInPageContent(array $expected_order) {
...
+  public function orderInPageText(array $expected_order) {

orderInPageContent and orderInPageText is too close. This methods should be named like the ones in \Behat\Mink\WebAssert... so something like responseHasOrder() and pageTextHasOrder().

Also sometimes I guess I'm wondering why we don't use responseMatches and pageTextMatches and supply a regex.

Here I have made changes as suggested in comment #39.

The changes in #40 look good, thanks.

Thanks for working on this. These seem like they could be helpful API additions, although the fact that we've only found two tests to change makes me wonder if they're useful enough to be worth maintaining, given that we also have ongoing work to deprecate a lot of our various custom assertions in favor of PHPUnit-native stuff. Without spending too much time on it, could we identify other tests in core for which these assertions might be useful?

1. +++ b/core/modules/book/tests/src/FunctionalJavascript/BookJavascriptTest.php
   @@ -64,7 +63,7 @@ public function testBookOrdering() {
   -    $this->assertOrderInPage(['1st page', '2nd page']);
   +    $this->assertSession()->responseHasOrder(['1st page', '2nd page']);
   

   Is there a particular reason these are using responseHasOrder() rather than pageTextHasOrder()? The previous test was doing getHtml() rather than getText() but it still seems like it should work, no? Since (reading the text) these are the book titles, rather than anything internal to the markup? Edit: If I'm wrong about that, feel free to say so. Thanks!

2. +++ b/core/modules/system/tests/modules/test_page_test/src/Controller/Test.php
   

   This is out of scope, but: Worst fixture name ever.

3. +++ b/core/modules/system/tests/modules/test_page_test/src/Controller/Test.php
   @@ -168,4 +168,15 @@ public function deprecations() {
   +  /**
   +   * Renders a page with strings in a specific order.
   +   *
   +   * @return array
   +   *   A render array as expected by
   +   *   \Drupal\Core\Render\RendererInterface::render().
   +   */
   +  public function renderOrderInPage() {
   

   renderOrderInPage() is a weird name for this; that sounds like it's rendering the order of things in the page, rather than rendering a page with things in a particular order. Maybe renderPageWithOrderedContent()?

4. +++ b/core/tests/Drupal/Tests/WebAssert.php
   @@ -626,4 +626,78 @@ public function pageTextContainsOnce($text) {
   +   * Asserts that several strings are in a given order in the page content.
   

   This should probably say "in the response content".

5. +++ b/core/tests/Drupal/Tests/WebAssert.php
   @@ -626,4 +626,78 @@ public function pageTextContainsOnce($text) {
   +  public function responseHasOrder(array $expected_order) {
   

   responseHasOrder() is a little weird for the method name; makes me think it's about the order response headers were returned in, or something. Maybe responseContentHasOrder() would be a more clear name that would also still allude to the difference between pageTextContains() and responseContains()?

6. +++ b/core/tests/Drupal/Tests/WebAssert.php
   @@ -626,4 +626,78 @@ public function pageTextContainsOnce($text) {
   +   * Asserts that several strings are in a given order in the page text.
   

   Nit: "Several" in this one-line summary seems overly specific. It could be "two" or "dozens", neither of which are "several". (Or even one or zero, although neither of those would be particularly useful to make assertions about.) Let's say "multiple" instead of "several"?

   Also, maybe we should add tests that document the expected behavior with zero or one entries in the array?

7. +++ b/core/tests/Drupal/Tests/WebAssert.php
   @@ -626,4 +626,78 @@ public function pageTextContainsOnce($text) {
   +      // When checking the order in page text we throw ResponseTextException
   +      // rather than ExpectationException.
   

   OK, but why? (The comment should explain why.)

Thanks!

It'd be useful for #2932016: Entity names in the 'add view' form are unsorted for example.

+++ b/core/tests/Drupal/Tests/WebAssert.php
@@ -626,4 +626,78 @@ public function pageTextContainsOnce($text) {
+    $this->assert($expected_order === array_values($actual_order), 'Strings found but incorrectly ordered');

This should be $this->assertSame($expected_order, $actual_order, 'Strings found but incorrectly ordered') because then PHPUnit will then show you the incorrect order - which is helpful.

Hi @alexpott
Made changes as you suggested please review.

@xjm here's another use-case #3207047: Value & format scrambled when text_format element is in a subform . I've updated the IS with the occurrences I've found on a bird-eye check.

Let's start with a reroll against 9.2.x

This should make TestBot happy

This addresses #49

Keeping this on Needs work since all the points made by @xjm in #44 were not addressed yet.

Note: Expecting Failures from #3207086: [HEAD BROKEN] Consistent failure in MonthDatePluginTest

#3207086: [HEAD BROKEN] Consistent failure in MonthDatePluginTest was committed. Ordered retest.

Addressed:

#44.1: The text the test is looking for is actually in an <input type="text">-tag and isn't found when just searching for text. (Been there, tried that, no T-shirt)

#44.2: Agreed. (Both on worst name and out of scope)

#44.3: Renamed.

#44.4: Changed.

#44.5: Renamed.

#44.6.1: Comment changed.

#44.6.2: Added both requested tests.

#44.7: Not a clue TBH, changed it back to throwing a ExpectationException

Re #44.7, I had thought it was using ResponseTextException just because it's a specialization of ExpectationException that seems to fit pageTextHasOrder() (and not responseContentHasOrder()): Exception thrown when an expectation on the response text fails.

It seems consistent with \Drupal\Tests\WebAssert::pageTextContains() throwing a ResponseTextException while \Drupal\Tests\WebAssert::responseContains() just throws an ExpectationException.

@AndyF in #59:

Ah, that makes sense now.

Since I just love consistency, changed it back to ResponseTextException

Thanks for the explanation.

Grmbl...

On this line, the array passed the 80 chars

$this->assertSession()->orderInString(['item1', 'item2', 'item1', 'item1', 'item2'], 'item1.item2.item1.item1.item2');

On this please could you split the array in a second line? E.g:

$this->assertSession()->orderInString([
  'item1',
  'item2',
  'item1',
  'item1',
  'item2'
], 'item1.item2.item1.item1.item2');

It's easier to read code with long arrays

Thanks @RenatoG, well spotted.

Addressed in attached patch.

Used 2817657-66.patch as base for the MR against 9.3.x

- Rebased MR on 9.4.x-dev
- Merged latest commits from 9.4.x-dev

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Re-rolled the patch in #66 for Drupal 9.5 version

Re-rolled for 10.1.x

For 10.4

I think the signature should be more like ::assertStringOrder($items, $markup) - we can then get $markup from the page, but also from direct rendering or other sources. I needed something like this for #3546376: Use the 'yield' option instead of output buffering for twig rendering to support async rendering and found this issue.