Problem/Motivation

The /core/lib/Drupal/Core/Test/AssertMailTrait.php() uses the result of the preg_match() function in assertTrue(). However preg_match() function returns 1, 0 or FALSE.

protected function assertMailPattern($field_name, $regex, $message = '', $group = 'Other') {
  $mails = $this->getMails();
  $mail = end($mails);
  $regex_found = preg_match("/$regex/", $mail[$field_name]);
  if (!$message) {
    $message = new FormattableMarkup('Expected text found in @field of email message: "@expected".', ['@field' => $field_name, '@expected' => $regex]);
  }
  return $this->assertTrue($regex_found, $message, $group);
}

Proposed resolution

Use data type casting to boolean.

CommentFileSizeAuthor
#21 interdiff_17-21.txt1.3 KBmunish.kumar
#21 3082859-21.patch1.6 KBmunish.kumar
#17 interdiff_2-17.txt1.08 KBmunish.kumar
#17 3082859-17.patch1.89 KBmunish.kumar
#2 3082859-2.patch678 bytesKrzysztof Domański
#2 3082859-test-only-pass.patch1.47 KBKrzysztof Domański
#2 3082859-test-only-failure.patch1.46 KBKrzysztof Domański
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Krzysztof Domański created an issue. See original summary.

Krzysztof Domański’s picture

Krzysztof Domański
Location Poland
CreditAttribution: Krzysztof Domański at abventor commented
Status: Active » Needs review
FileSize
3082859-test-only-failure.patch1.46 KB
3082859-test-only-pass.patch1.47 KB
3082859-2.patch678 bytes

Note that the assertMailPattern is not used anywhere so I added them to UserAdminTest.php in test only.

The last submitted patch, 2: 3082859-test-only-failure.patch, failed testing. View results

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

samuel.mortenson’s picture

samuel.mortenson
he/him
Primary language English
CreditAttribution: samuel.mortenson commented
Issue tags: +D9 readiness, +D9 upgrade path

This is breaking tests for me that pass in D8 but not in D9 ref #3121551: Confirm D9 tests pass

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Kristen Pol’s picture

Kristen Pol
she/her
Primary language English
Location Santa Cruz, CA, USA
CreditAttribution: Kristen Pol at Hook 42 commented
Issue tags: -D9 readiness, -D9 upgrade path +Drupal 9 compatibility

Fixing outdated tags.

Kristen Pol’s picture

Kristen Pol
she/her
Primary language English
Location Santa Cruz, CA, USA
CreditAttribution: Kristen Pol at Hook 42 commented
Issue tags: -Drupal 9 compatibility

Whoops. The "Drupal 9 compatibility" tag is for contributed projects and not for core. Not sure if "Drupal 9 readiness" should be added since this is already marked for Drupal 9 so leaving it off. Sorry for the noise.

samuel.mortenson’s picture

samuel.mortenson
he/him
Primary language English
CreditAttribution: samuel.mortenson commented
Status: Needs review » Reviewed & tested by the community

I had to override this method to port my module to D9, this change is pretty simple so I'm marking as RTBC.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue tags: +Needs followup

So custom assertions returning a value and that value being meaningful is a legacy SimpleTest thing. PHPUnit fails the test as soon as an assertion fails, and PHPUnit 8 specifically specifies a void typehint as the return value for assertions.

I think what we should do here is actually deprecate there being a return value somehow. Contrib tests should not be relying on return values fmor assertions. This also came up in [edit, wrong link, looking for the right one].

That said, this assertion is clearly more broken ATM than just legacy, so followup?

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented

Definitely +1 on #10!

samuel.mortenson’s picture

samuel.mortenson
he/him
Primary language English
CreditAttribution: samuel.mortenson commented

The return behavior of this method is not related to the bug, it's the assertion itself.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Re: #12 Right, sorry for not being clear, the method is obvs. bugged ATM regardless of the return. It's just that returning true is also legacy.

Which makes it occur to me that we also need test coverage (not for the return value, but that it passes when it should).

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

In #10, #3131900: Refactor assertions that assign return values to variables is the actual issue I had in mind. See also #3138078: [D9.3 beta - w/c Nov 8, 2021] Add a 'void' return typehint to custom assert* methods.

Also this patch will conflict with #3129006: Modernize Drupal\KernelTests\AssertContentTrait but it's a one-line conflict to resolve manually for that patch, so not terrible either way. No it doesn't; it just conflicts with the different scope I had in my head.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue tags: +Novice

Looks like there is already core/tests/Drupal/KernelTests/Core/Test/AssertMailTraitTest.php for this, so adding test coverage for this could be a good novice task. Thanks!

shaktik’s picture

shaktik
he/him
Primary language Hindi
Location New Delhi
CreditAttribution: shaktik as a volunteer and at Srijan | A Material+ Company for Drupal India Association commented
Assigned: Unassigned » shaktik
munish.kumar’s picture

munish.kumar CreditAttribution: munish.kumar as a volunteer and at Srijan | A Material+ Company for Drupal India Association commented
Status: Needs work » Needs review
FileSize
3082859-17.patch1.89 KB
interdiff_2-17.txt1.08 KB

Hi @xjm,

Please review the patch as this is the first time I am writing the test cases, So please help me out with this. I have also gone through the Add a 'void' return typehint to custom assert* methods related issues. I have added a patch with the test cases only and not refactoring the return statement in /core/lib/Drupal/Core/Test/AssertMailTrait.php() Which will fix in other issues.

shaktik’s picture

shaktik
he/him
Primary language Hindi
Location New Delhi
CreditAttribution: shaktik as a volunteer and at Srijan | A Material+ Company for Drupal India Association commented
Assigned: shaktik » Unassigned

Unassigned myself, Thanks @munish.kumar to fix this.

Krzysztof Domański’s picture

Krzysztof Domański
Location Poland
CreditAttribution: Krzysztof Domański at abventor commented
Status: Needs review » Needs work

@munish.kumar tests looks good. I have some suggestions for improvement.

1. Can we add a more complex regular expression here?

+    $this->assertMailPattern('to', substr($message['to'], 10));

For example "^.{32}@example\.com$"

2. Let's add tests for several parameters (e.g. "subject", "body", "to").

$subject = $this->randomString(64);
$body = $this->randomString(128);
$message = [
  'id' => 'drupal_mail_test',
  'headers' => ['Content-type' => 'text/html'],
  'subject' => $subject,
  'to' => 'foobar@example.com',
  'body' => $body,
];

$this->assertMailPattern('subject', "^.{64}$");
$this->assertMailPattern('to', "[a-z]{6}@example\.com$");
$this->assertMailPattern('body', "^.{128}$");
munish.kumar’s picture

munish.kumar CreditAttribution: munish.kumar as a volunteer and at Srijan | A Material+ Company for Drupal India Association commented
Assigned: Unassigned » munish.kumar
munish.kumar’s picture

munish.kumar CreditAttribution: munish.kumar as a volunteer and at Srijan | A Material+ Company for Drupal India Association commented
Assigned: munish.kumar » Unassigned
Status: Needs work » Needs review
FileSize
3082859-21.patch1.6 KB
interdiff_17-21.txt1.3 KB

Hi @Krzysztof Domański, Thanks for your suggestions, I have made some improvements in test cases as mentioned in #19. Please review.

Krzysztof Domański’s picture

Krzysztof Domański
Location Poland
CreditAttribution: Krzysztof Domański at abventor commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs followup, -Needs tests, -Novice

Tests looks good. Back to RTBC (#9). Follow-up has been created (#13, #14).

  • catch committed b829de6 on 9.1.x 
    Issue #3082859 by Krzysztof Domański, munish.kumar, xjm, samuel....
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed b829de6 and pushed to 9.1.x. Thanks!

Status: Fixed » Closed (fixed)

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