Fix assert_select_email to work on non-multipart emails as well as converting the Mail::Body to a string to prevent errors. #2499

Merged
merged 1 commit into from Sep 7, 2011

2 participants

@akaspick

Fix assert_select_email to work on non-multipart emails as well as converting the Mail::Body to a string to prevent errors.

assert_select_email didn't work at all on emails that weren't multipart. Emails with one text/html part were simply skipped and nothing was tested at all. The original test for assert_select_email also didn't report any errors because it didn't perform any of the inner assert_select tests.

assert_select_email now works on both single-part and multipart emails.

This patch also fixes the body passed to the HTML::Document.new which expects a string. The body is a Mail::Body class which threw this error if passed...

NoMethodError: undefined method `encoding_aware?' for <div><p>foo</p><p>bar</p></div>:Mail::Body

Converting the body to a string with to_s fixes that issue.

Please backport to Rails 3-0 stable as well.

@akaspick akaspick fix assert_select_email to work on non-multipart emails as well as co…
…nverting the Mail::Body to a string to prevent errors.
60d358b
@jonleighton
Ruby on Rails member

Hiya, I am a bit confused about what you are fixing:

  • You say "assert_select_email didn't work at all on emails that weren't multipart", but the test you have written relates to a multipart email
  • Regarding "converting the Mail::Body to a string to prevent errors", how does this error occur? What does the backtrace look like when it happens? Is it covered by a test?

Thanks

@akaspick

Sorry for the confusion, the diff doesn't make things clear, hence the longish description. assert_email_select didn't actually test anything previously for non-multipart emails because the inner loop was never run because delivery.parts is always empty for non-multipart. Because the inner code was never run, it looked like the previous tests were valid.

Because assert_email_select didn't really do anything (because it is broken), the existing tests (for non-multipart) passed anyway. The diff adds tests for multipart emails, but the fix also makes the tests that already existed (for non-multipart) actually run correctly now; the inner loop now has a "body" to perform assert_select against.

Basically, there are tests for multipart and non-multipart. The former tests already existed, but didn't really test anything because the tests weren't actually working correctly. The latter tests were added in the diff. The fix I've made now allows the former tests (that already exist) to run correctly... they now assert_select against the email body.

The error fixed with to_s only appeared with multipart emails, which there were no tests for previously. The tests I added test multipart emails which all pass because of the to_s fix. If to_s is removed the multipart email tests will fail.

It's a bit confusing because everything previously didn't do anything, so I've made what already existed work properly again which in the test code didn't require any new changes for non-multipart... the prior tests work fine now with this diff applied.

Does that clear it up?

@jonleighton jonleighton merged commit 16f1ce4 into rails:master Sep 7, 2011
@jonleighton
Ruby on Rails member

Great, thanks for the explanation!

If you would like to backport to 3-1-stable with a CHANGELOG entry, I am happy to apply this there too.

@akaspick

Created PR #2918 for backporting to 3-0-stable. Thanks.

@akaspick

PR #2926 now also created to backport to 3-1-stable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment