-
-
Notifications
You must be signed in to change notification settings - Fork 1k
Fix parameter matching with mail delivery job and ActionMailer::MailDeliveryJob
#2516
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
Fix parameter matching with mail delivery job and ActionMailer::MailDeliveryJob
#2516
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please feel free to locally disable Metrics/ClassLength
for this class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job! Thank you for the contribution.
Thank you @fabn! Can we merge this now? I'd love to have this change. |
Please accept my apologies, I completely forgot about this PR @fabn |
Copy pasted from existing examples, I can change them |
@pirj Pushed a new commit to have different examples for each syntax |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Thanks for moving this forward.
A couple of small fixes/clarifications and it's good to go.
I've just run in to this when upgrading to Rails 6 myself - is there anything I can do to help get this over the line? |
Sure, just take a look at the PR, and make it work locally for Rails 5.2, 6.0 and 7.0. You can send a patch, or open a pr basing on what has been done, your choice. That would be very helpful as as 7.0 was just released.
|
@Haegin Please accept my apologies, I replied without looking. |
@pirj on the next week going to check this branch in the project. |
Just tested and seems it works in the following cases: expect {
expect {
request!
}.to have_enqueued_mail(LeadMailer, :user_created)
.with(
a_hash_including(
params: {
user: instance_of(User)
}
)
)
}.to have_enqueued_mail(DeviseMailer, :confirmation_instructions)
.with(
args: [
instance_of(User),
instance_of(String),
{}
]
) Now it looks like this: expect {
expect {
request!
}.to have_enqueued_mail(LeadMailer, :user_created)
.with(
user: instance_of(User)
)
}.to have_enqueued_mail(DeviseMailer, :confirmation_instructions)
.with(
instance_of(User),
instance_of(String),
{}
) Means it still works and I have to write less code. 👍 for the feature! 🚀 |
I had to make this change to make this change pass on both Rails 7 and Rails 6.0. Kindly appreciate your feedback. |
You're welcome |
This should fix #2351.
Maybe it's a breaking change for Rails 6 users, I don't know but it will allow users to easily upgrade to rails 6 without changing tons of specs.
Let me know if it's ok, it's my first PR in rspec.