-
-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
expect { }.to have_enqueued_mail().with() doesn't work with ActionMailer::MailDeliveryJob #2184
Comments
Hello @simmerz Thanks for your issue. Could you provide a reproduction script or new Rails project? Thanks |
Hi @benoittgt - here's a blank rails project recreating the issue: https://github.com/simmerz/rspec_mailer_bug |
@benoittgt I'm pretty certain this has to do with the class used, its just not detecting the mail jobs used in Rails 6. |
@JonRowe without the |
@simmerz Interesting, then its probably the format of the arguments used, as thats how it changes when you use |
Failure output:
|
There we go then, it does some new mangling of arguments, previously they were just verbatim. |
So that's something I should be fixing in my code, or should be handled in the |
Something that needs to be added to the params building code in the matcher when Rails 6 is being used with this new mailer class |
You could work around it for now with |
I don't have time to work on this myself right now, do you feel like tackling a PR or @benoittgt ? |
Like you've described, if I use the following, the test passes:
We need to handle someone creating a mail I'm not sure what the best way to tackle those would be. Any pointers appreciated and I'd be happy to submit a PR |
The tactic we've used before is to use the rails param parsing code itself to create the params we check against the queued ones. |
I just ran into the same issue (I think) and fixed it by changing
I'm not sure this is the right fix, and that it works in the other configuration, but it worked for me. |
Also, this requires changing
|
@mkamensky have you put it into a PR? If you fancy doing that, I'll test my codebase against your branch, to at least get some more validation on the method. |
Actually, I just tried running the specs, which failed, and also reread the comments here, and realised I probably don't understand how one is supposed to use this function. The example in the docs shows the following should pass: expect {
MyMailer.welcome(user).deliver_later
}.to have_enqueued_mail(MyMailer, :welcome).with(user) As far as I can tell, this does pass with my modification, and does not with the current code. On the other hand, the tests expect the following to pass: expect {
UnifiedMailer.with('foo' => 'bar').test_email.deliver_later
}.to have_enqueued_mail(UnifiedMailer, :test_email).with(
a_hash_including(:params => { 'foo' => 'bar' })
) and with my modification it does not. I don't know what |
I'd imagine its because you tried replacing the method / args for the new Rails 6 style, but thats not the only style of args thats in use.
|
Yes, that's what I thought, but I'm not familiar enough with the code to give a real solution here |
Hi, I try the following workaround and it works on my project: Ruby version: 2.6.5 RSpec::Rails::Matchers::HaveEnqueuedMail.define_method(:mailer_job) do
ActionMailer::Parameterized::DeliveryJob
end It seems that PR #2125 has resolved it but not released yet. |
@ybiquitous it should have been released as |
@JonRowe Oh, thank you! I'll try it. |
|
@simmerz is this still an issue for you? |
@simmerz Please feel free to reopen if you are still experiencing the issue. |
@pirj @JonRowe I still seem to be seeing an issue here with the unified mailer, though this could be me misunderstanding its use. The following test passes:
However, if i I strip out the |
Not at all:
The error I see when running RSpec is (ignore the object IDs - they're from different runs):
|
How does the part in |
Both are of the form:
|
Your code seems to be correct according to the guide, but I have not yet worked with parameterized mailers using a # classic
NotifierMailer.welcome(User.first).deliver_later
# parameterized
UserMailer.with(user: @user).welcome_email.deliver_later Can you please apply those changes: - BookingMailer.with(student: self).joining_instructions.deliver_later
+ BookingMailer.joining_instructions(student: self).deliver_later - BookingMailer.with(student: @student).joining_instructions.deliver_later
+ BookingMailer.joining_instructions(student: @student).deliver_later and see if the following works:
(notice I've also dropped If this passes as expected (and those two notations of sending mail have the same behavior), please file a new ticket. |
@pirj so using your notation above, the test fails: 1) StudentsController POST #resend_instructions queues an email to the student
Failure/Error:
expect { req }.to have_enqueued_mail(BookingMailer, :joining_instructions)
.with(student: student)
expected to enqueue BookingMailer.joining_instructions exactly 1 time with [{:student=>#<Student id: 7245, person_id: 5814, booking_id: 3155, aasm_state: nil, created_at: "2020-03-12 22:16:56", updated_at: "2020-03-12 22:16:56", instructions_sent_at: nil, event_id: 6945, course_id: 8099, course_delivery_id: 7591>}], but enqueued 0
Queued deliveries:
BookingMailer.joining_instructions with [{"args"=>[{"student"=>{"_aj_globalid"=>"gid://myapp/Student/7245"}, "_aj_symbol_keys"=>["student"]}], "_aj_symbol_keys"=>["args"]}] The controller does the following: BookingMailer.joining_instructions(student: @student).deliver_later |
I mean thats an issue with job serialisation not matching the test arguments, are the new matchers not serialising arguments? |
We don't have specs that would cover ActiveJob serialization, I'll double-check this @simmerz |
…nMailer::MailDeliveryJob as of Rails 6 See rspec/rspec-rails#2184
…nMailer::MailDeliveryJob as of Rails 6 See rspec/rspec-rails#2184
…nMailer::MailDeliveryJob as of Rails 6 See rspec/rspec-rails#2184
* Upgrade Ruby version to 2.7.1, and necessary related CloudFoundry buildpack changes * Fix deprecation warnings in Rspec suite * Install Rails 6 and run update script * Resolve conflict with multiple root paths defined * Fix ActiveStorage queue names * Fix for rspec-rails have_enqueued_mail matcher not working with ActionMailer::MailDeliveryJob as of Rails 6 See rspec/rspec-rails#2184 * Rubocop fixes * Fix for change to ActiveStorage attach interface * Fix preloading not working on decorated objects * Fix attachment blobs being destroyed before audit activity generated * Fix issue with updating test results comparing changes by reference to the modified object * Fix for test queue adapter being used by default in request specs * Set database adapter explicitly in production environment to avoid errors * Remove redundant framework defaults causing loader errors * Fix incorrect class name causing loader errors * Ensure all forms are submitted correctly rather than with AJAX * Bump govuk-design-system-rails to resolve deprecated usage warning * Fix for change to save behavior when model has an attached ActiveStorage::Blob
* Upgrade Ruby version to 2.7.1, and necessary related CloudFoundry buildpack changes * Fix deprecation warnings in Rspec suite * Install Rails 6 and run update script * Resolve conflict with multiple root paths defined * Fix ActiveStorage queue names * Fix for rspec-rails have_enqueued_mail matcher not working with ActionMailer::MailDeliveryJob as of Rails 6 See rspec/rspec-rails#2184 * Rubocop fixes * Fix for change to ActiveStorage attach interface * Fix preloading not working on decorated objects * Fix attachment blobs being destroyed before audit activity generated * Fix issue with updating test results comparing changes by reference to the modified object * Fix for test queue adapter being used by default in request specs * Set database adapter explicitly in production environment to avoid errors * Remove redundant framework defaults causing loader errors * Fix incorrect class name causing loader errors * Ensure all forms are submitted correctly rather than with AJAX * Bump govuk-design-system-rails to resolve deprecated usage warning * Fix for change to save behavior when model has an attached ActiveStorage::Blob
For the future researchers, this have been fixed in various PRs (e.g. #2566) and released in 5.1. |
What Ruby, Rails and RSpec versions are you using?
Ruby version: 2.5.1
Rails version: 6.0.0
RSpec version: branch 4-0-dev
Observed behaviour
Given an email enqueued with:
I would expect the following to pass:
It doesn't pass when setting the default delivery job to
ActionMailer::MailDeliveryJob
but does when using the legacyActionMailer::Parameterized::DeliveryJob
(ie.config.load_defaults 5.2
andRails.application.config.action_mailer.delivery_job = 'ActionMailer::MailDeliveryJob'
commented out inconfig/initializers/new_framework_defaults_6_0.rb
The listed enqueued mails shows the mail has been enqueued, but I cannot seem to access it with my test criteria.
The text was updated successfully, but these errors were encountered: