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. ;)
Comment | File | Size | Author |
---|---|---|---|
#36 | drupal_mail_send_test_8.patch | 8.89 KB | mr.baileys |
#32 | drupal_mail_send_test_7.patch | 8.97 KB | mr.baileys |
#28 | drupal_mail_send_test_5.patch | 5.31 KB | mr.baileys |
#26 | drupal_mail_send_test_4.patch | 5.84 KB | mr.baileys |
#20 | email_info_msg.jpg | 40.53 KB | mr.baileys |
Comments
Comment #1
ThiOz CreditAttribution: ThiOz commentedHi 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
Comment #2
ThiOz CreditAttribution: ThiOz commentedI forgot to change the status of this issue...
Comment #3
Dries CreditAttribution: Dries commented- Please don't glue words together to 'drupalmail'.
- Please add code comments.
- Please use proper capitalization.
Comment #4
ThiOz CreditAttribution: ThiOz commentedHi again ...
I made some changes and attached the new patch
Tataaaa
Comment #5
cwgordon7 CreditAttribution: cwgordon7 commentedI 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.
Comment #6
jhedstromThis variable_set is using the wrong file name:
it should set it to drupal_mail.inc
Comment #7
ThiOz CreditAttribution: ThiOz commentedHiya,
I changed the filename as requested by jhedstrom
Comment #8
cburschkaWhat is needed to review this? Running the thing in simpletest?
Comment #9
cwgordon7 CreditAttribution: cwgordon7 commentedPatch in #7 is malformed/makes no sense.
Comment #10
mr.baileysI 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.
Comment #11
Dries CreditAttribution: Dries commentedI 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?
Comment #12
alexanderpas CreditAttribution: alexanderpas commented/me agrees with dries, but what about assertMailEmptyBody() or assertMailNoBody()
Comment #13
mr.baileysI 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.
Comment #14
mr.baileysNew 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.
Comment #16
mr.baileysre-test
Comment #17
Dries CreditAttribution: Dries commentedThis 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.
Comment #18
mr.baileysYes, 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...
Comment #19
boombatower CreditAttribution: boombatower commentedTo be consistent with other assertion names it would seem that the methods should be:
For example:
Format:
Would it make more sense to make one function like assertMail() with optional parameters for subjects and body? More like
Comment #20
mr.baileysYes - I think it would.
New patch attached, which:
Comment #21
Dries CreditAttribution: Dries commentedThis looks good (and better) to me. Marking this RTBC -- will commit later after more people got a chance to review this.
Comment #22
Dries CreditAttribution: Dries commentedComment #23
boombatower CreditAttribution: boombatower commentedMay want to change the message from:
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)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.
Comment #24
boombatower CreditAttribution: boombatower commentedComment #25
Dries CreditAttribution: Dries commentedThanks for the feedback -- let's roll one more version then.
Comment #26
mr.baileysRe-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:
Comment #28
mr.baileysre-roll (required after #464714: Speed up the tests, which changed drupal_web_test_case.php, got commited)
Comment #29
chx CreditAttribution: chx commentedMake 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...?
Comment #30
mr.baileys@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?
Comment #31
mr.baileysRenamed 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:
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...
Comment #32
mr.baileysI 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.
Comment #33
Dries CreditAttribution: Dries commentedTo me, this looks ready, and it is still a favorite. Marking this RTBC so I can commit it later on.
Comment #34
chx CreditAttribution: chx commentedWhy 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.
Comment #35
chx CreditAttribution: chx commentedOpsie. Resetting title.
Comment #36
mr.baileysReplaced the flag inside drupalGetMails by a continue-statement (#34).
Comment #37
Dries CreditAttribution: Dries commentedHonestly, for me the $keep_message stuff was easier to grok -- either approach looks fine to me though.
Comment #38
chx CreditAttribution: chx commentedLets 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 :)
Comment #39
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks!