Problem/Motivation

While working on #3083275: [meta] Update tests that rely on Classy to not rely on it anymore it has become clear that there are many assertions in Functionl/Functional JS tests related to status messages, and they are written many different ways. Most of them depend on Classy being the theme. For example:

$this->assertSession()->elementNotExists('xpath', '//div[contains(@class, "messages--error")]');

$xpath_err = '//div[contains(@class, "error")]';
    $this->assertNotEmpty($this->xpath($xpath_err), 'Enabling translation only for entity bundles generates a form error.');

$this->assertSession()->elementNotExists('xpath', '//div[contains(@class, "messages--status")]');

$this->assertSession()
          ->elementContains('css', '.messages--warning', node_get_type_label($node) . ' content items were skipped as they are under moderation and may not be directly published.');

And a couple examples from Functional JS tests where we wait for messages.

$assert_session->assertNoElementAfterWait('css', '.messages--warning');

// Check that the 'content has been updated' message status appears to confirm we left the editor.
    $assert_session->waitForElementVisible('css', 'messages messages--status');

Proposed resolution

Make assertions of Status Messages more consistent and manageable by introducing an AssertStatusMessageTrait that can be used in Functional/Function JS tests.

The methods should be:

  1. assertStatusMessageExists
  2. assertStatusMessageNotExists
  3. assertStatusMessageVisibleAfterWait
  4. assertNoStatusMessageAfterWait

each accepting optional $type and $message parameters.

  /**
   * Whatever this method does.
   *
   * @param string|null $type
   *   The optional message type: status, error, or warning.
   * @param string|null $message
   *   The optional message or partial message to assert.
   */

After getting the Trait into the codebase, the Trait can then be used to make #3083275: [meta] Update tests that rely on Classy to not rely on it anymore much smoother.

Another possibility, brought up in #28 is to add methods to WebAssert/JsWebAssert instead of making new traits.

Remaining tasks

Etc

User interface changes

None

API changes

New trait for tests, but essentially none

Data model changes

None

Release notes snippet

There's a new trait? Use it?

CommentFileSizeAuthor
#51 interdiff_38-51.txt9.85 KBdanflanagan8
#51 3268932-51.patch26.07 KBdanflanagan8
#38 interdiff_36-38.txt960 bytesdanflanagan8
#38 3268932-38.patch24.75 KBdanflanagan8
#36 interdiff_33-36.txt6.94 KBdanflanagan8
#36 3268932-36.patch24.76 KBdanflanagan8
#33 interdiff_29-33.txt24.76 KBdanflanagan8
#33 3268932-33.patch24.71 KBdanflanagan8
#32 interdiff_29-32.txt24.75 KBdanflanagan8
#32 3268932-32.patch24.7 KBdanflanagan8
#29 interdiff_27-29.txt28.38 KBdanflanagan8
#29 3268932-29.patch18.39 KBdanflanagan8
#27 interdiff_20-27.txt1.87 KBdanflanagan8
#27 3268932-27.patch19.36 KBdanflanagan8
#24 interdiff_22-24.txt2.35 KBdanflanagan8
#24 3268932-24-max-size-test-only.patch19.15 KBdanflanagan8
#22 interdiff_20-22.txt1.35 KBdanflanagan8
#22 3268932-22-max-size-test-only.patch43.1 KBdanflanagan8
#20 interdiff_19-20.txt4.62 KBdanflanagan8
#20 3268932-20.patch19.09 KBdanflanagan8
#19 interdiff_10-19.txt14.77 KBdanflanagan8
#19 3268932-19.patch14.48 KBdanflanagan8
#15 interdiff_10-15.txt13.19 KBdanflanagan8
#15 3268932-15.patch14.43 KBdanflanagan8
#10 interdiff_2-10.txt5.85 KBdanflanagan8
#10 3268932-10.patch4.4 KBdanflanagan8
#4 interdiff_2-4demo.txt7.14 KBdanflanagan8
#4 3268932-4-demonstration.patch10.64 KBdanflanagan8
#2 interdiff_2-2demo.txt7.12 KBdanflanagan8
#2 3268932-2.patch3.5 KBdanflanagan8
#2 3268932-2-demonstration.patch10.63 KBdanflanagan8
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

danflanagan8 created an issue. See original summary.

danflanagan8’s picture

Status: Active » Needs review
FileSize
10.63 KB
3.5 KB
7.12 KB

Here's a patch with the trait and another patch that modifies some existing tests to show how the trait works and kind of prove that it works.

danflanagan8’s picture

Assigned: danflanagan8 » Unassigned
danflanagan8’s picture

Let's try that again with a different typo. The typo is supposed to show the helpful exception.

Status: Needs review » Needs work

The last submitted patch, 4: 3268932-4-demonstration.patch, failed testing. View results

danflanagan8’s picture

Status: Needs work » Needs review

The patch in #2 is ready for review.

That fail in the demonstration was intentional, by the way:

1) Drupal\Tests\content_translation\Functional\ContentTranslationSettingsTest::testSettingsUI
InvalidArgumentException: The allowed status message types are 'status', 'error', 'warning'. Pass NULL or an empty string to match any status message type.

This happened when I called

$this->assertStatusMessageExists('not a valid status', 'Translation is not supported if language is always one of: Not specified, Not applicable');

I wanted to demonstrate that exception, which makes it harder for a typo to lead to a false negative. Although that would be more of an issue if asserting the message does NOT exist.

xjm’s picture

Great idea!

I'd suggest potentially splitting it into two traits, one that contains the BTB assertions, and a second trait that uses the first with the waitForElementVisible() calls (since those methods will not exist for non-BTB tests). Maybe AssertStatusMessageTrait and AssertJavaScriptStatusMessageTrait.

  1. +++ b/core/tests/Drupal/Tests/AssertStatusMessageTrait.php
    @@ -0,0 +1,104 @@
    +  protected function assertStatusMessageExists($type = NULL, $message = NULL) {
    ...
    +  protected function assertStatusMessageNotExists($type = NULL, $message = NULL) {
    ...
    +  protected function assertStatusMessageVisibleAfterWait($type = NULL, $message = NULL) {
    ...
    +  protected function assertNoStatusMessageAfterWait($type = NULL, $message = NULL) {
    ...
    +  protected function buildStatusMessageSelector($type, $message) {
    

    All these methods should have parameter and return typehints. Looks like the parameter typehints are generally ?string and all but the last method have void return.

  2. +++ b/core/tests/Drupal/Tests/AssertStatusMessageTrait.php
    @@ -0,0 +1,104 @@
    +      throw new \InvalidArgumentException("The allowed status message types are 'status', 'error', 'warning'. Pass NULL or an empty string to match any status message type.");
    

    Is there a particular reason to allow both NULL and empty string?

xjm’s picture

I wonder if we should also switch the order of the arguments. It seems like it would be less common to leave out asserting a specific message than it would be to leave out asserting whether it were a warning, error, etc.

danflanagan8’s picture

Assigned: Unassigned » danflanagan8
Status: Needs review » Needs work

Thanks for the review, @xjm!

I wonder if we should also switch the order of the arguments.

I think you're right. I'm not sure what the most common use case will be, but it would probably make sense to have the same order of arguments that MessengerInterface uses. From MessengerInterface:

public function addMessage($message, $type = self::TYPE_STATUS, $repeat = FALSE);

I was thinking it was the other way round. So we should do message first, then type.

I'd suggest potentially splitting it into two traits

+1 to two traits

Is there a particular reason to allow both NULL and empty string?

Not really. And certainly when we switch the order of arguments there's no need to be so overly accommodating.

Setting to NW so I can make those changes.

danflanagan8’s picture

Assigned: danflanagan8 » Unassigned
Status: Needs work » Needs review
FileSize
4.4 KB
5.85 KB

Here's a patch with the updates suggested by @xjm.

dww’s picture

Love this idea! Hope to review this tomorrow...

longwave’s picture

Status: Needs review » Needs work

Can we either add some explicit test cases that prove that each of these methods works, or convert some existing tests to use these new traits?

+++ b/core/tests/Drupal/Tests/AssertStatusMessageTrait.php
@@ -0,0 +1,86 @@
+      $selector .= ':contains("' . $message . '")';

Does $message need escaping in some way if it contains double quotes (or maybe other characters)?

danflanagan8’s picture

Does $message need escaping in some way if it contains double quotes (or maybe other characters)?

That's a good question and makes me think that explicit test coverage would be a good idea. That way we can force some edge cases.

larowlan’s picture

  1. +++ b/core/tests/Drupal/Tests/AssertStatusMessageTrait.php
    @@ -0,0 +1,86 @@
    +  protected function buildStatusMessageSelector(?string $message = NULL, ?string $type = NULL): string {
    +    $allowed_types = [
    +      'status',
    +      'error',
    +      'warning',
    +      NULL,
    +    ];
    

    it's a shame this isn't 10 only because we could use an Enum

  2. +++ b/core/tests/Drupal/Tests/AssertStatusMessageTrait.php
    @@ -0,0 +1,86 @@
    +    switch ($type) {
    +      case 'status':
    +        $selector .= ' div[aria-label="Status message"]';
    +        break;
    +
    +      case 'error':
    +        $selector .= ' div[aria-label="Error message"]';
    +        break;
    +
    +      case 'warning':
    +        $selector .= ' div[aria-label="Warning message"]';
    +    }
    +
    

    its a shame this isn't 10 only because we could use match

Agree we need some escaping on $message (and coverage for it)

Typically when we add these things, we try to include at least one usage of it. Should we do that here too?

danflanagan8’s picture

Status: Needs work » Needs review
FileSize
14.43 KB
13.19 KB

Here's a new patch that is really, really different because once I added test coverage I discovered a ton of bugs. As suggested in #13 using contains to select was broken by so much as a quote.

But then writing tests for the ajax side of things I learned several things about ajax messages which meant I mostly re-wrote the AssertJavascriptStatusMessageTrait so that it actually does what it says it does.

I'm really starting to think this works. What I'm much less confident about is where all the Traits and tests go. Nobody's complained about that yet, but I'm all ears.

Thanks for all the great feedback so far!

PS.

Typically when we add these things, we try to include at least one usage of it. Should we do that here too?

I have not addressed that yet. I'd be happy to do it though if that's in scope.

danflanagan8’s picture

I should also point out that I can't get escaping quotes to work when using :contains. Using addslashes($message) to try to escape quotes in the message resulted in:

1) Drupal\Tests\system\Functional\Bootstrap\DrupalMessengerServiceTest::testAssertStatusMessageTrait
Symfony\Component\CssSelector\Exception\SyntaxErrorException: Unclosed/invalid string at 41.

I guess :contains isn't too robust. Anyway, that's what I found. That's why I went in a different direction with the selector.

Status: Needs review » Needs work

The last submitted patch, 15: 3268932-15.patch, failed testing. View results

longwave’s picture

You could try building XPath based selectors instead and using WebAssert::buildXPathQuery() to build them, which will help with escaping.

danflanagan8’s picture

Status: Needs work » Needs review
FileSize
14.48 KB
14.77 KB

@longwave, you are right on the money with WebAssert::buildXPathQuery()!

So let's act like the patch in #15 never existed. It was a step in the wrong direction. Here's a new patch that uses xpath instead of css selectors. I'm putting an interdiff relative to #10 since we have agreed the patch in #15 never existed. ;)

danflanagan8’s picture

As suggested in #14 here's an update to the patch where each Trait is used to refactor an existing test. Each trait uses its positive and negative assertion, so it's pretty representative.

You'll have to forgive me if Drupal\Tests\file\FunctionalJavascript\MaximumFileSizeExceededUploadTest fails. I can't get it to complete on my local with or without my update. Hopefully it works because it certainly reads better with the new trait.

Status: Needs review » Needs work

The last submitted patch, 20: 3268932-20.patch, failed testing. View results

danflanagan8’s picture

Status: Needs work » Needs review
FileSize
43.1 KB
1.35 KB

Hmmm....most of those fails look random. I want to figure out what's going on with that MaxFileSize test. I'm going to just run that test. ONce again, it's not running at all locally for me.

Status: Needs review » Needs work

The last submitted patch, 22: 3268932-22-max-size-test-only.patch, failed testing. View results

danflanagan8’s picture

Status: Needs work » Needs review
FileSize
19.15 KB
2.35 KB

Hmmmm....this one reverts that test to its original state but then puts $this->assertNotEmpty() around a wait. Will this fail? Is that element not showing up? Let's find out!

Still running just the one test.

Status: Needs review » Needs work

The last submitted patch, 24: 3268932-24-max-size-test-only.patch, failed testing. View results

danflanagan8’s picture

Status: Needs work » Needs review

Well, there ya go. Looks like the test's existing message assertion is bogus. I guess this proves the need for this Trait!

Is that not an ajax message? I wonder if the page is reloading and it's actually a normal status message. I don't have the html output though since I can't run the test locally.

I'm setting back to NR because #19 still needs eyes.

And maybe someone has ideas about what's going on in the max file size test.

danflanagan8’s picture

I've said it before, but now I thin I have it! :)

(The interdiff here is relative to #20. #22 and #24 were experimental patches that ran one test.)

I had re-written that AssertJavascriptStatusMessageTrait as if ajax status messages always used MessageCommand. Turns out they almost NEVER use MessageCommand, and are instead almost always using a status_messages element. So I've updated the selector in AssertJavascriptStatusMessageTrait yet again such that:

   *The selector is designed to work with the Drupal.theme.message
   * template defined in message.js in addition to status-messages.html.twig
   * in the system module.

As for MaximumFileSizeExceededUploadTest, now the test should pass and it will actually be asserting the presence of a status message. This was an interesting case where I discovered bugs in two things at the same time. But now I think they're both fixed.

mondrake’s picture

Would it make sense to just add the methods to WebAssert?

And then call

$this->assertSession()->statusMessageExists
$this->assertSession()->statusMessageDoesNotExists
$this->assertSession()->statusMessageIsVisibleAfterWait
$this->assertSession()->statusMessageIsNotVisibleAfterWait

etc. and avoid proliferation of traits.

+++ b/core/modules/system/tests/modules/system_test/src/Controller/SystemTestController.php
@@ -146,6 +146,27 @@ public function messengerServiceTest() {
+  public function assertStatusMessageTraitTest() {

I had go over twice before I understood this was not a test method but a controller's callback, can we find a better name?

danflanagan8’s picture

Title: Create AssertStatusMessageTrait to allow more consistent tests » Add methods to assert status messages either to WebAssert or a new Trait
Issue summary: View changes
FileSize
18.39 KB
28.38 KB

Here's a patch that goes in @mondrake's suggested direction of adding methods to WebAssert/JsWebAssert instead of making new traits. I have updated the issue title to indicate the possible change in course.

I had go over twice before I understood this was not a test method but a controller's callback, can we find a better name?

^^ I've updated the language around this as well.

mondrake’s picture

Some typehints and method visibility things:

  1. +++ b/core/modules/system/tests/modules/system_test/src/Controller/SystemTestController.php
    @@ -146,6 +146,27 @@ public function messengerServiceTest() {
    +  public function statusMessagesForAssertions() {
    

    return typehint : string

  2. +++ b/core/modules/system/tests/src/Functional/Bootstrap/DrupalMessengerServiceTest.php
    @@ -61,4 +61,46 @@ public function testDrupalMessengerService() {
    +  public function testStatusMessageAssertions() {
    

    return typehint : void

  3. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/MessageCommandTest.php
    @@ -76,6 +76,55 @@ public function testMessageCommand() {
    +  public function testJsStatusMessageAssertions() {
    

    return typehint : void

  4. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
    @@ -523,4 +523,97 @@ public static function isExceptionNotClickable(Exception $exception): bool {
    +    $this->assert(!is_null($status_message_element), $failure_message);
    

    You can use assertNull()

  5. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
    @@ -523,4 +523,97 @@ public static function isExceptionNotClickable(Exception $exception): bool {
    +    $this->assert(is_null($status_message_element), $failure_message);
    

    same

  6. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
    @@ -523,4 +523,97 @@ public static function isExceptionNotClickable(Exception $exception): bool {
    +  public function buildJavascriptStatusMessageSelector(?string $message = NULL, ?string $type = NULL): string {
    

    can this be private?

  7. +++ b/core/tests/Drupal/Tests/WebAssert.php
    @@ -1125,4 +1125,94 @@ public function checkboxNotChecked($field, TraversableElement $container = NULL)
    +  public function buildStatusMessageSelector(?string $message = NULL, ?string $type = NULL): string {
    

    can this be protected?

Then, a couple of points I would have done differently. They could be seen as personal preferences so I'm not saying it should be done like this, just food for thought.

1)

+++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
@@ -523,4 +523,97 @@ public static function isExceptionNotClickable(Exception $exception): bool {
+  public function statusMessageVisibleAfterWait(?string $message = NULL, ?string $type = NULL): void {
...
+  public function statusMessageNotVisibleAfterWait(?string $message = NULL, ?string $type = NULL): void {
...
+  public function buildJavascriptStatusMessageSelector(?string $message = NULL, ?string $type = NULL): string {

+++ b/core/tests/Drupal/Tests/WebAssert.php
@@ -1125,4 +1125,94 @@ public function checkboxNotChecked($field, TraversableElement $container = NULL)
+  public function statusMessageExists(?string $message = NULL, ?string $type = NULL): void {
...
+  public function statusMessageNotExists(?string $message = NULL, ?string $type = NULL): void {
...
+  public function buildStatusMessageSelector(?string $message = NULL, ?string $type = NULL): string {

I'm not a big fan of allowing explicitly passing NULL to have different execution paths. Not anymore in PHP 8.1.
I would suggest a set of methods like

public function statusMessageContains(string $message, string $type = NULL): void {
public function statusMessageDoesNotContain(string $message, string $type = NULL): void {
public function statusMessageDoesNotExists(string $type = NULL): void {

IMHO this way is more clear what we are asserting (in the xpath constructor we use contains, so this is what we are doing). Similar in the JS equivalents

public function statusMessageContainsAfterWait(string $message, string $type = NULL): void {
public function statusMessageDoesNotContainAfterWait(string $message, string $type = NULL): void {
public function statusMessageDoesNotExistAfterWait(string $type = NULL): void {

2)

In PHPUnit world, an assertion, i.e. a specific condition checked by the test, 'passes' or 'fails'. If we have an error in the SUT that is not explicitly being checked, we have an 'error'. WebAssert is predominantly throwing exceptions when its assertions fail, but this is wrong IMHO.

For this reason, more recent additions to WebAssert throw a PHPUnit failure instead of an exception when the assertion fails, for example

  public function pageTextMatchesCount(int $count, string $regex, string $message = ''): void {
    $actual = preg_replace('/\s+/u', ' ', $this->session->getPage()->getText());
    $matches = preg_match_all($regex, $actual);
    if ($message === '') {
      $message = "Failed asserting that the page matches the pattern '$regex' $count time(s), $matches found.";
    }
    $constraint = new IsIdentical($count);
    Assert::assertThat($matches, $constraint, $message);
  }

In the patch here,

  1. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
    @@ -523,4 +523,97 @@ public static function isExceptionNotClickable(Exception $exception): bool {
    +  public function statusMessageNotVisibleAfterWait(?string $message = NULL, ?string $type = NULL): void {
    +    $selector = $this->buildJavascriptStatusMessageSelector($message, $type);
    +    $status_message_element = $this->waitForElementVisible('xpath', $selector);
    +    if ($message) {
    +      $failure_message = sprintf('A %s message containing the text "%s" appears on this page, but it should not.', $type, $message);
    +    }
    +    else {
    +      $failure_message = sprintf('A %s message appears on this page, but it should not.', $type);
    +    }
    +    $this->assert(is_null($status_message_element), $failure_message);
    +  }
    

    converting to PHPunit failure could be achieved by using assertNull (see above)

  2. +++ b/core/tests/Drupal/Tests/WebAssert.php
    @@ -1125,4 +1125,94 @@ public function checkboxNotChecked($field, TraversableElement $container = NULL)
    +  public function statusMessageExists(?string $message = NULL, ?string $type = NULL): void {
    +    $selector = $this->buildStatusMessageSelector($message, $type);
    +    $this->elementExists('xpath', $selector);
    +  }
    

    converting to PHPunit failure could be achieved by using try...catch

      public function statusMessageExists(?string $message = NULL, ?string $type = NULL): void {
        $selector = $this->buildStatusMessageSelector($message, $type);
        try {
          $this->elementExists('xpath', $selector);
        }
        catch (ExpectationException $e) {
          Assert::fail($e->getMessage());
        }
      }
    
mondrake’s picture

ops, sorry, we cannot use assertNull within WebAssert.

We should do
Assert::assertThat($actual, Assert::isNull(), $message);

danflanagan8’s picture

@mondrake, thanks for the extremely helpful review. That's a lot of stuff I did not know. I believe I have taken all of your suggestions into account in this new patch.

Now the public methods being added to WebAssert are:

  public function statusMessageExists(?string $type = NULL): void
  public function statusMessageNotExists(?string $type = NULL): void
  public function statusMessageContains(string $message, ?string $type = NULL): void
  public function statusMessageNotContains(string $message, ?string $type = NULL): void

And to JsWebAssert we are adding:

  public function statusMessageExistsAfterWait(?string $type = NULL): void
  public function statusMessageNotExistsAfterWait(?string $type = NULL): void
  public function statusMessageContainsAfterWait(string $message, ?string $type = NULL): void
  public function statusMessageNotContainsAfterWait(string $message, ?string $type = NULL): void

In addition to addressing your suggestions, I also added some assertions in the form or try/catch blocks to test that I could make each of these methods fail. That seemed like something that was missing in my attempt to have test coverage for these test methods. Maybe that's overkill. I'm happy to remove it!

danflanagan8’s picture

FileSize
24.71 KB
24.76 KB

Sorry. Here's a patch with cs problems fixed. The diff is still relative to #29. Let's act like #32 never happened!

mondrake’s picture

+++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
@@ -523,4 +526,137 @@ public static function isExceptionNotClickable(Exception $exception): bool {
+  public function statusMessageExistsAfterWait(?string $type = NULL): void {
...
+  public function statusMessageNotExistsAfterWait(?string $type = NULL): void {
...
+  public function statusMessageContainsAfterWait(string $message, ?string $type = NULL): void {
...
+  public function statusMessageNotContainsAfterWait(string $message, ?string $type = NULL): void {

+++ b/core/tests/Drupal/Tests/WebAssert.php
@@ -1125,4 +1125,136 @@ public function checkboxNotChecked($field, TraversableElement $container = NULL)
+  public function statusMessageNotExists(?string $type = NULL): void {
...
+  public function statusMessageContains(string $message, ?string $type = NULL): void {
...
+  public function statusMessageNotContains(string $message, ?string $type = NULL): void {

Getting there almost. Why ?string $type = NULL and not just string $type = NULL? That way if you pass something, it should be not-null; if you do not pass anything, it defaults to null. Or in other words, you can't (and don't need to) pass NULL explicitly.

danflanagan8’s picture

That way if you pass something, it should be not-null; if you do not pass anything, it defaults to null.

I didn't understand that. Thank you for the free course in php8! I'll update that.

danflanagan8’s picture

I think I have my argument types straightened out now. New patch here.

I also updated the exception message if a bad status message type gets passed. This one no longer suggests passing NULL.

mondrake’s picture

Title: Add methods to assert status messages either to WebAssert or a new Trait » Add methods to assert status messages to WebAssert
Status: Needs review » Needs work
Issue tags: +Needs change record

PHPStan in the D10 branch found an issue,

 ------ --------------------------------------------------------- 
  Line   tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php   
 ------ --------------------------------------------------------- 
  582    Call to sprintf contains 1 placeholder, 2 values given.  
 ------ --------------------------------------------------------- 

I'll be bold and set sail for the WebAssert addition solution, drop the trait-based one. We'll need a CR here. When this come back green tested on both 9 and 10 I'll RTBC

danflanagan8’s picture

Status: Needs work » Needs review
FileSize
24.75 KB
960 bytes

Darn it! Here's a fix for that sprintf boo boo. I canceled the test on #36 since I'm going to re-run it here.

mondrake’s picture

Status: Needs review » Reviewed & tested by the community

The D10 failure seems unrelated. This looks good to me. Do we need a follow up to make use of these methods more broadly?

Thanks @danflanagan8 for following my brain circles :)

danflanagan8’s picture

Thanks @danflanagan8 for following my brain circles :)

I'm the one who should thank you!

Do we need a follow up to make use of these methods more broadly?

I was planning on doing as many changes as possible as part of #3083275: [meta] Update tests that rely on Classy to not rely on it anymore, which has been going module-by-module decoupling tests from Classy. That would cover a lot of ground since many message assertion are relying on Classy markup.

That said, I'm open to other ideas.

danflanagan8’s picture

I created a change record and added some info and examples. I don't have much experience making change records and would welcome editorial advice!

mondrake’s picture

Issue tags: -Needs change record

CR done

dww’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record updates

Thanks for all the fabulous work and progress in here!

I started from reviewing the CR. I first spotted what seemed like a simple typo where the 2nd set of examples had the before with an elementNotExists() while the after says statusMessageExists(). I started to edit it to fix this myself. But then I looked more closely to the first example:

Before:
$this->assertSession()->elementNotExists('css', '.messages--warning');
After:
$this->assertSession()->statusMessageNotExists('warning');

$type is the 2nd arg and $message is the 1st. This example doesn’t work at all. We’re just checking there’s no status message with the message ‘error’.

But now that we can’t explicitly pass NULL as the first arg, I don’t think this API lets you do the kind of check for “there should be no error messages” that this example tries to demonstrate.

I don’t want to undo the suggestions or work from the last round of feedback, but I think this needs more work before it’s RTBC.

Thanks/sorry,
-Derek

dww’s picture

Sorry, that’s what I get fir trying to review things on my phone at 11:20pm. ;). That method only takes a type. There’s no $message at all. 😅 tee hee. API is probably fine. Maybe just the CR needs a few edits. 😉

I’ll look again after some sleep.

dww’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs change record updates

https://www.drupal.org/node/3270424/revisions/view/12588385/12589083

CR is now fine. API looks great. I didn't closely review the whole patch, but restoring status.

Thanks/sorry,
-Derek

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/system/tests/src/Functional/Bootstrap/DrupalMessengerServiceTest.php
    @@ -61,4 +62,91 @@ public function testDrupalMessengerService() {
    +    $this->assertTrue($expected_failure_occurred);
    ...
    +    $this->assertTrue($expected_failure_occurred);
    ...
    +    $this->assertTrue($expected_failure_occurred);
    ...
    +    $this->assertTrue($expected_failure_occurred);
    
    +++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/MessageCommandTest.php
    @@ -76,6 +77,94 @@ public function testMessageCommand() {
    +    $this->assertTrue($expected_failure_occurred);
    ...
    +    $this->assertTrue($expected_failure_occurred);
    ...
    +    $this->assertTrue($expected_failure_occurred);
    ...
    +    $this->assertTrue($expected_failure_occurred);
    

    assertTrue needs a message to have something useful in test logs.

  2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
    @@ -523,4 +526,137 @@ public static function isExceptionNotClickable(Exception $exception): bool {
    +  public function statusMessageExistsAfterWait(string $type = NULL): void {
    ...
    +  public function statusMessageNotExistsAfterWait(string $type = NULL): void {
    ...
    +  public function statusMessageContainsAfterWait(string $message, string $type = NULL): void {
    ...
    +  public function statusMessageNotContainsAfterWait(string $message, string $type = NULL): void {
    

    A wait should have a timeout. So callers can set it to longer or shorter.

  3. +++ b/core/tests/Drupal/Tests/WebAssert.php
    @@ -1125,4 +1125,136 @@ public function checkboxNotChecked($field, TraversableElement $container = NULL)
    +    $selector = $this->buildStatusMessageSelector('', $type);
    ...
    +    $selector = $this->buildStatusMessageSelector('', $type);
    

    I think this is why the $message argument is nullable on buildStatusMessageSelector() - so I think either we should change that or use NULL instead of ''. I think NULL is a more semantic why of saying we don't care about the message. Also in #34 @mondrake is wrong - https://3v4l.org/7saY2 you are still able to pass NULLs into a string $param = NULL parameter. string $param = NULL is the same as ?string $param = NULL.

  4. +++ b/core/tests/Drupal/Tests/WebAssert.php
    @@ -1125,4 +1125,136 @@ public function checkboxNotChecked($field, TraversableElement $container = NULL)
    +    try {
    +      $this->elementExists('xpath', $selector);
    +    }
    +    catch (ExpectationException $e) {
    +      Assert::fail($e->getMessage());
    +    }
    ...
    +    try {
    +      $this->elementNotExists('xpath', $selector);
    +    }
    +    catch (ExpectationException $e) {
    +      Assert::fail($e->getMessage());
    +    }
    ...
    +    try {
    +      $this->elementExists('xpath', $selector);
    +    }
    +    catch (ExpectationException $e) {
    +      Assert::fail($e->getMessage());
    +    }
    ...
    +    try {
    +      $this->elementNotExists('xpath', $selector);
    +    }
    +    catch (ExpectationException $e) {
    +      Assert::fail($e->getMessage());
    +    }
    

    Why are we why wrapping this in a try catch? We do plenty of $this->elementExists - it seems very unnecessary - PHP Unit will mark the test as failed and report the ExpectationException nicely without this additional work. Ah I see - this was recommended in #30. I'm really not sure that what @mondrake is proposing is necessary or even correct. There's no real difference if \PHPUnit\Framework\ExpectationFailedException is thrown or \Behat\Mink\Exception\ExpectationException. And if there is then PHPUnit is becoming an even less welcoming framework to build test suites on top of.

mondrake’s picture

#46.4 the only difference is that in PHPUnit CLI assertion failures are reported as an 'F', and other exceptions as an 'E'. Actually in DrupalCI a test ending in error ends up being reported twice, once with the PHPUnit CLI output, and once just flagging the fact that the PHPUnit CLI exited with an error.

And, yes, that's my personal preference since

an assertion, i.e. a specific condition checked by the test, 'passes' or 'fails'. If we have an error in the SUT that is not explicitly being checked, we have an 'error'.

mondrake’s picture

From https://phpunit.readthedocs.io/en/9.5/textui.html:

For each test run, the PHPUnit command-line tool prints one character to indicate progress:

. Printed when the test succeeds.

F Printed when an assertion fails while running the test method.

E Printed when an error occurs while running the test method.

and

PHPUnit distinguishes between failures and errors. A failure is a violated PHPUnit assertion such as a failing assertSame() call. An error is an unexpected exception or a PHP error. Sometimes this distinction proves useful since errors tend to be easier to fix than failures. If you have a big list of problems, it is best to tackle the errors first and see if you have any failures left when they are all fixed.

alexpott’s picture

Okay well I think that PHPUnit should have a way to register other exceptions as Assertion fails to make interoperability a bit simpler. But sure let's leave the try catches alone. Still need to fix #46.1, .2 and .3

mondrake’s picture

#49 an option could be to change WebAssert::assert() implementation

  public function assert($condition, $message) {
    if ($condition) {
      return;
    }

    throw new ExpectationException($message, $this->session->getDriver());
  }

to use PHPUnit's Assert::fail() instead of throwing Mink's ExpectationException. But that might lead to BC ripple effects?

danflanagan8’s picture

Here's a patch with 46.1, 46.2, and 46.3 addressed.

Adding the timeout also allowed me to shorten the waits on the "forced fails" being added in MessageCommandTest::testJsStatusMessageAssertions. That's an easy save of 36 seconds!

I'll make the necessary updates to the CR too. Thanks!

mondrake’s picture

Status: Needs review » Reviewed & tested by the community

Back to rtbc, the D10 failure is in head too.

mondrake’s picture

alexpott’s picture

Version: 9.4.x-dev » 9.3.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed and pushed e5e74fb998 to 10.0.x and 99b6843db1 to 9.4.x. Thanks!

alexpott’s picture

Let's see if this can be backported to 9.3.x as a test only change.

  • alexpott committed e5e74fb on 10.0.x
    Issue #3268932 by danflanagan8, mondrake, dww, xjm, longwave, alexpott,...

  • alexpott committed 99b6843 on 9.4.x
    Issue #3268932 by danflanagan8, mondrake, dww, xjm, longwave, alexpott,...
Wim Leers’s picture

Version: 9.3.x-dev » 9.4.x-dev
Status: Patch (to be ported) » Fixed

Given that 9.4.0-beta1 will be released today, marking this as Fixed and moving back to 9.4.x.

Status: Fixed » Closed (fixed)

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