| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | simpletest.module |
| Category: | bug report |
| Priority: | critical |
| Assigned: | mr.baileys |
| Status: | closed (fixed) |
| Issue tags: | Favorite-of-Dries |
Issue Summary
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. ;)
Comments
#1
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
#2
I forgot to change the status of this issue...
#3
- Please don't glue words together to 'drupalmail'.
- Please add code comments.
- Please use proper capitalization.
#4
Hi again ...
I made some changes and attached the new patch
Tataaaa
#5
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.
#6
This variable_set is using the wrong file name:
<?php+ 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
#7
Hiya,
I changed the filename as requested by jhedstrom
#8
What is needed to review this? Running the thing in simpletest?
#9
Patch in #7 is malformed/makes no sense.
#10
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.
#11
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?
#12
/me agrees with dries, but what about assertMailEmptyBody() or assertMailNoBody()
#13
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:
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.
#14
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.
#15
The last submitted patch failed testing.
#16
re-test
#17
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.
#18
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...
#19
To be consistent with other assertion names it would seem that the methods should be:
For example:
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
<?phpfunction assertFieldByName($name, $value = '', $message = '')
?>
#20
Yes - I think it would.
New patch attached, which:
#21
This looks good (and better) to me. Marking this RTBC -- will commit later after more people got a chance to review this.
#22
#23
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()andtearDown()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)<?php
/**
* 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 bepublic static function getInfo().Otherwise this looks good. I am happy to see someone finally solved this.
#24
#25
Thanks for the feedback -- let's roll one more version then.
#26
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:
<?php$emails = $this->drupalGetMails(
array(
'to' => 'foobar@example.com',
'id' => 'drupal_mail_test'
)
);
?>
#27
The last submitted patch failed testing.
#28
re-roll (required after #464714: Speed up the tests, which changed drupal_web_test_case.php, got commited)
#29
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...?
#30
@chx: thanks for reviewing the patch!
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.
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)
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?
#31
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:
<?php// 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...
#32
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.
#33
To me, this looks ready, and it is still a favorite. Marking this RTBC so I can commit it later on.
#34
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.
#35
Opsie. Resetting title.
#36
Replaced the flag inside drupalGetMails by a continue-statement (#34).
#37
Honestly, for me the $keep_message stuff was easier to grok -- either approach looks fine to me though.
#38
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 :)
#39
Committed to CVS HEAD. Thanks!
#40
Automatically closed -- issue fixed for 2 weeks with no activity.