We need to test Drupal core's mail-sending capabilities.

This can be a bit tricky to test, however I think I have worked it all out, and here's how to do it:

- In your test case, variable_set('smtp_library', 'relative/path/to/test/case').
- In the same file as your test case, define the function drupal_mail_wrapper($message).
- Create another wrapper function. Use it to statically store your test case for access by the drupal_mail_wrapper($message) function.
- Cause a mail to be sent in your test case.
- Access the test case from your drupal_mail_wrapper() function, and write your assertions directly to it from within that function (which should be called instead of mail() from within drupal_mail().

Note that it might be nice to have the ability to test mail messages directly from DrupalWebTestCase (e.g. assertMail()). So, the DrupalWebTestCase API should probably be updated for mail handling/asserting, and then the test case can use the extended DrupalWebTestCase API and not have to worry about too much on its own. This would also make the work here reusable.

This is kind of a complex one, so don't take it if it's your first one or anything. ;)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ThiOz’s picture

FileSize
1.83 KB

Hi Guys,

I wrote a simple test for the drupal_mail_send function, using a simple wrapper method... needless to say this code needs the utmost scrutiny

cheers Rob / ThiOz

ThiOz’s picture

Status: Active » Needs review

I forgot to change the status of this issue...

Dries’s picture

Status: Needs review » Needs work

- Please don't glue words together to 'drupalmail'.
- Please add code comments.
- Please use proper capitalization.

ThiOz’s picture

Status: Needs work » Needs review
FileSize
2.38 KB

Hi again ...

I made some changes and attached the new patch

Tataaaa

cwgordon7’s picture

I would really like it if we could make this awesome work reusable by adding clean API functions to DrupalWebTestCase; that way, more tests for sending mail could be written in the future. That can be in a follow up patch if necessary, however.

jhedstrom’s picture

Status: Needs review » Needs work

This variable_set is using the wrong file name:

+  function testMailSend() {
+    // Set the smtp_library variable and get it again.
+    variable_set('smtp_library', drupal_get_path('module', 'simpletest') . '/tests/drupalmail.inc');

it should set it to drupal_mail.inc

ThiOz’s picture

Status: Needs work » Needs review
FileSize
1.9 KB

Hiya,

I changed the filename as requested by jhedstrom

cburschka’s picture

What is needed to review this? Running the thing in simpletest?

cwgordon7’s picture

Status: Needs review » Needs work

Patch in #7 is malformed/makes no sense.

mr.baileys’s picture

Component: tests » base system
Assigned: Unassigned » mr.baileys
Status: Needs work » Needs review
FileSize
4.81 KB

I tried reworking the patch from #4 but ended up completely rewriting this.

This patch extends drupal_web_test_case.php and adds three new assertions (available to all tests):
- assertMail (assert that an email with a given subject was sent)
- assertMailText (assert that a given piece of text appears in the body of an email)
- assertMailNoText (assert that a given piece of text does not appear in the body of an email)

Currently, the assertions work on the most recently sent email. I'm not sure if (and how) we should address multiple emails sent during the same test case.

Dries’s picture

I looked at this patch and I think we can improve the API.

assertMail() should probably be called assertMailSubject(), and assertMailText() assertMailBody()?

Do we really need assertMailNoText()? If so, I'd also consider renaming it to assertMailBodyNoText()?

Anyway, it feels nice if you can tell what the assert applies too. The suggested changes make it so.

Thoughts?

alexanderpas’s picture

/me agrees with dries, but what about assertMailEmptyBody() or assertMailNoBody()

mr.baileys’s picture

Status: Needs review » Needs work

Do we really need assertMailNoText()? If so, I'd also consider renaming it to assertMailBodyNoText()?

I think we do, as it allows us to test, for example, that an outgoing email does not contain a plain-text password or that all tokens have been replaced in the body of an email (no lingering '!username').

@alexanderpas:

/me agrees with dries, but what about assertMailEmptyBody() or assertMailNoBody()

the assertMailNoText tests for the absence of a piece of text within the email body, not for a missing or empty body. I'm not sure we should add such an assertion, especially since assertMailText would fail on an empty body anyway.

I'll upload a new patch later today to improve API naming as per suggestions.

mr.baileys’s picture

Component: base system » simpletest.module
Status: Needs work » Needs review
FileSize
4.84 KB

New patch with changed function names:

- assertMailSubject
- assertMailBodyText
- assertMailBodyNoText
These last 2 mimic the assertText and assertNoText assertions already present for HTML pages.

Also moved this to the simpletest queue since this patch adds assertions.

Status: Needs review » Needs work

The last submitted patch failed testing.

mr.baileys’s picture

Status: Needs work » Needs review

re-test

Dries’s picture

This looks good to me but it would be nice to get a review from someone else too.

Two things:

1. Instead of writing a dummy DrupalMailTestCase, I think it would be a lot more useful to test the mails sent by the user module and/or the contact module.

2. In documentation and UIs, we write e-mail instead of email. I think it would be good to clean that up a bit.

mr.baileys’s picture

Status: Needs review » Needs work

Instead of writing a dummy DrupalMailTestCase, I think it would be a lot more useful to test the mails sent by the user module and/or the contact module.

Yes, it would be more useful :) As it stands, this patch just adds the assertions for other tests to use (and there are a number of 'mail' test issues lingering), and it tests the drupal_mail_wrapper functionality itself. As soon as this lands, we should be able to make some progress on testing mails sent from other modules.

back to NW for the comments in #18. I'm also not entirely sure about the assertions themselves. I'm going to take a look at some of the other tests for emails and see what the best API would be...

boombatower’s picture

To be consistent with other assertion names it would seem that the methods should be:

  • assertMailSubject
  • assertMailBodyText
  • assertNoMailBodyText

For example:

  • assertText
  • assertNoText
  • assertUniqueText
  • assertNoUniqueText (not assertUniqueNoText)
  • assertPattern
  • assertNoPattern

Format:

assert[item]
assertNo[item]

Would it make more sense to make one function like assertMail() with optional parameters for subjects and body? More like

function assertFieldByName($name, $value = '', $message = '')
mr.baileys’s picture

Status: Needs work » Needs review
FileSize
5.07 KB
40.53 KB

Would it make more sense to make one function like assertMail() with optional parameters for subjects and body?

Yes - I think it would.

New patch attached, which:

  1. Only has one assertion (assertMail), which can be used to test any of the properties of the most recently sent e-mail;
  2. Added a drupalGetMails-function to DrupalWebTestCase, which will return an array of all e-mail messages sent during the current test case. The result can then be used in those cases where assertMail doesn't cut it. It's modeled on drupalGetContent.
  3. Added an informative message (similar to the HTTP response messages) that shows the number of e-mails sent during a test case. See screenshot.
  4. Reworked the way sent e-mails are tracked. The logic behind my initial attempt to store these in a static member of DrupalWebTestCase was flawed: E-mails sent during a drupalGet/drupalPost would get lost since they occured in a different process. Now using a variable (simpletest_emails) and variable_get/variable_set to keep track of the e-mails, and confirmed that this works for both unit tests (drupal_mail called from DrupalWebTestCase) as well as functional tests (e-mails sent during a drupalGet/drupalPost call)
Dries’s picture

This looks good (and better) to me. Marking this RTBC -- will commit later after more people got a chance to review this.

Dries’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Favorite-of-Dries
boombatower’s picture

May want to change the message from:

!count e-mail was sent during this test case.

We general refer to each class as a "test case" DruaplWebTestCase, BlockTestCase. setUp() and tearDown() run every test method or "test". This maybe something we want to nail down the terminology on, but I would change it from "test case" to "test". (minor)

/**
 * Wrapper function to override the default mail handler function.
 *
 * @param array $message
 * @return
 */
function drupal_mail_wrapper($message) {
  $captured_emails = variable_get('simpletest_emails', array());
  $captured_emails[] = $message;
  variable_set('simpletest_emails', $captured_emails);

  return TRUE;
}

Documentation should be filled in for @return and as of now we do not put the parameter type in the documentation as on @param.

Also please leave a blank line at the end of file.

Please add the // $Id$ line to the top of the .test file.

getInfo() should be public static function getInfo().

Otherwise this looks good. I am happy to see someone finally solved this.

boombatower’s picture

Status: Reviewed & tested by the community » Needs work
Dries’s picture

Thanks for the feedback -- let's roll one more version then.

mr.baileys’s picture

Status: Needs work » Needs review
FileSize
5.84 KB

Re-rolled with the comments from #23.

One change to the API: I looked at some of the issues in the queue about tests and e-mails: whenever more than one e-mail is sent during drupalGet/drupalPost, the tests can't use assertMail (since this works on the last e-mail, and functional tests should not assume that e-mails are sent in a specific order). They have to rely on drupalGetMails to find what they're after.

To make it easier to write tests, I've added $filter to drupalGetMails, which lets you restrict the e-mails returned by drupalGetMails to just those you want, for example:

$emails = $this->drupalGetMails(
  array(
    'to' => 'foobar@example.com', 
    'id' => 'drupal_mail_test'
  )
);

Status: Needs review » Needs work

The last submitted patch failed testing.

mr.baileys’s picture

Status: Needs work » Needs review
FileSize
5.31 KB

re-roll (required after #464714: Speed up the tests, which changed drupal_web_test_case.php, got commited)

chx’s picture

Status: Needs review » Needs work

Make the wrapper a class method instead of a function -- array($this, 'mailWrapper'). Instead of a variable_get/set you can change the the global $conf . I recommend saving and restoring the whole global as it is, it's pointless to save the different keys and restore 'em one by one. This class method could store captured mails in a class property instead of variable_get/set. With all this... why do you need more than new the unit test...?

mr.baileys’s picture

@chx: thanks for reviewing the patch!

Make the wrapper a class method instead of a function -- array($this, 'mailWrapper').

I don't think this is possible (or my brain doesn't understand what you mean :) ): setting the mail library is done by setting the smtp_library variable to the file where Drupal can find a function called "drupal_mail_wrapper". Since Drupal looks for that specific function name, we can't point it to a static class method unfortunately (unless something like #321771: Make drupal_mail_wrapper a hook. happens).

We could slim down the current wrapper (drupal_mail_wrapper) function and make it point to a static class method that contains the actual logic if you feel that makes more sense? I didn't because I tried to make as little changes as possible to drupal_web_test_case.

Instead of a variable_get/set you can change the the global $conf. I recommend saving and restoring the whole global as it is, it's pointless to save the different keys and restore 'em one by one. This class method could store captured mails in a class property instead of variable_get/set.

What options are there to store persistent data across simpletest tests besides variable_get/set? I started out with a static property in DrupalWebTestCase to store captured e-mails, but this failed because the drupalGet/drupalPost calls run in different processes, and the results were lost.

I tried using $conf instead of the var_get/set as per your suggestion, but I run into the same problem (mails are captured, but the ones captured during a drupalGet/Post disappear into a big black hole)

With all this... why do you need more than new the unit test...?

Do you mean why the drupalGetMails($filter) function is necessary when we are exposing the captured emails via a class property?

I think it certainly serves a function: one of the tests I was looking into was one for the contact module: suppose you want to test the "auto-reply" + "send me a copy" options on the contact form. In this case, one submit will send out three different e-mails. With something like drupalGetMails, the test can request just the e-mails that were sent to a certain user, or just the e-mails that have a given id, subject, etc. I think if we leave it out, a lot of tests might have to write the filtering logic themselves resulting in duplication, no?

mr.baileys’s picture

Title: TestingParty08: Sending Drupal mail » Capture e-mails sent during tests and add e-mail assertions / API

Renamed to clear up some confusion. The goal of this issue is to come up with a framework/API that will make it easier to write tests involving e-mails.

Having an API/assertions to test e-mails would help resolve a number of unresolved TestingParty issues like:

Additionally, there is a number of assertions for messages ending in "...sent to your e-mail address." which would probably benefit from some extra assertMail assertions, and some tests currently use work-arounds to avoid testing the e-mails themselves, for example the "reset password" functionality is currently tested like this:

    // Login using password reset page.
    $url = user_pass_reset_url($user);
    sleep(1); // TODO Find better way.
    $this->drupalGet($url);
    $this->assertText(t('This login can be used only once.'), t('Login can be used only once.'));

A proper functional test would visit the password reset page, fire the e-mail, then extract the link and visit the one-time login page.

After talking to chx on IRC, we've decided to get rid of the unit test (since this will be tested by tests using the mail assertion anyway)

Will upload a rerolled patch soon...

mr.baileys’s picture

Status: Needs work » Needs review
FileSize
8.97 KB

After talking to chx on IRC, we've decided to get rid of the unit test (since this will be tested by tests using the mail assertion anyway)

I misunderstood: chx pointed out that the *existing* test did not add much and was irrelevant. A more thorough test to actually test the newly added API does make sense though, so this patch does a better job of verifying that the added logic actually works. I moved the test to the SimpleTest group, since we are now testing the API/framework itself and there is no need to do this in a separate test file.

Additionally, at one point the "x e-mails sent during this test." got removed from the patch. Added it back in.

Dries’s picture

Status: Needs review » Reviewed & tested by the community

To me, this looks ready, and it is still a favorite. Marking this RTBC so I can commit it later on.

chx’s picture

Title: Capture e-mails sent during tests and add e-mail assertions / API » Add mail capture and assert to SimpleTest
Status: Reviewed & tested by the community » Needs work

Why the rush? There is a new function which could be simplified by using continue 2 instead of a flag variable. Other than that, yes, it's ready.

chx’s picture

Title: Add mail capture and assert to SimpleTest » Capture e-mails sent during tests and add e-mail assertions / API

Opsie. Resetting title.

mr.baileys’s picture

Status: Needs work » Needs review
FileSize
8.89 KB

Replaced the flag inside drupalGetMails by a continue-statement (#34).

Dries’s picture

Honestly, for me the $keep_message stuff was easier to grok -- either approach looks fine to me though.

chx’s picture

Status: Needs review » Reviewed & tested by the community

Lets get this in so we can finish off the other three. Its funny how none of us attacked this fearing it'd be hard. Nice job :)

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks!

Status: Fixed » Closed (fixed)
Issue tags: -Favorite-of-Dries

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