Add matcher support to `assert_enqueued_email_with` #46626

maxim · 2022-12-01T17:44:31Z

Motivation / Background

Currently assert_enqueued_with allows you to pass procs for matching args.

However, assert_enqueued_email_with doesn't let you do that for args and params.

I propose that we should.

For example, if you are passing a generated token to an email, you would have to mock/stub your way to verify that it looks right in job args. Or you have to reach for the lower level assert_enqueued_with helper. With this change you could simply do something like this:

assert_enqueued_email_with UserMailer, :verification, params: -> p { p[:token] =~ /\w+/ }

Fixes #46621.

Detail

Since assert_enqueued_email_with tries to be clever about figuring out whether args are params, I decided to leave that logic completely intact (to preserve current behavior), and add another conditional branch which only kicks in if either args or params are a proc. In such a case, it never acts clever about args/params, and simply passes down a lambda that fails if any job args don't match, and succeeds if they do.

One unrelated question: why are we currently overwriting the passed-in params? Is that a bug?

Checklist

Before submitting the PR make sure the following are checked:

This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
Tests are added or updated if you fix a bug or add a feature.
CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.
CI is passing.

swanson · 2023-05-10T14:21:17Z

It would be nice if this could support skipping the arg/param checking if nothing is provided. I have often wanted to test that the expected Mailer method was enqueued, but I don't particularly care to check the args/params (this is especially annoying if you use named parameters for mailer methods as you have to match the string-keyed hash).

assert_enqueued_email_with ManagerMailer, :bounced_email do
  ManagerMailer.bounced_email(
    plan: plan,
    to_user: user,
    subject: message[:subject],
    error: description
  ).deliver_later
end
``

maxim · 2023-05-10T15:32:30Z

@swanson Wouldn't assert_enqueued_emails take care of that? Or are you worried that you aren't confirming the email type, just that it was 1 email?

I do like the idea regardless, and wouldn't mind adding support for this.

jonathanhefner

This seems like a helpful addition. 👍

jonathanhefner · 2023-05-10T17:12:53Z

actionmailer/lib/action_mailer/test_helper.rb

+      expected_args = if args.is_a?(Proc) || params.is_a?(Proc)
+        -> (job_args) {
+          # Ordered from the least to the most heavy checks.
+          return false if [mailer.to_s, method.to_s, "deliver_now"] != job_args[0..2]
+          return false if !params.is_a?(Proc) && params != job_args[3][:params]
+          return false if !args.is_a?(Proc) && Array(args) != job_args[3][:args]
+          return false if args.is_a?(Proc) && !args.(job_args[3][:args])
+          return false if params.is_a?(Proc) && !params.(job_args[3][:params])
+
+          true
+        }
      else
-        [mailer.to_s, method.to_s, "deliver_now", args: Array(args)]
+        if args.is_a?(Hash)
+          [mailer.to_s, method.to_s, "deliver_now", params: args, args: []]
+        elsif params.present?
+          [mailer.to_s, method.to_s, "deliver_now", params: params, args: Array(args)]
+        else
+          [mailer.to_s, method.to_s, "deliver_now", args: Array(args)]
+        end
      end


This code was a bit hard for me to follow. I've pushed a commit that unifies the logic. Let me know what you think. If you like it, you or I can rebase and squash the commits.

maxim

@jonathanhefner Thanks for the review. Your version is much smaller, which is always a win. 👍🏻 However, it raised a couple of questions.

actionmailer/lib/action_mailer/test_helper.rb

maxim · 2023-05-10T18:32:11Z

actionmailer/lib/action_mailer/test_helper.rb

-      expected_args = if args.is_a?(Proc) || params.is_a?(Proc)
-        -> (job_args) {
-          # Ordered from the least to the most heavy checks.
-          return false if [mailer.to_s, method.to_s, "deliver_now"] != job_args[0..2]
-          return false if !params.is_a?(Proc) && params != job_args[3][:params]
-          return false if !args.is_a?(Proc) && Array(args) != job_args[3][:args]
-          return false if args.is_a?(Proc) && !args.(job_args[3][:args])
-          return false if params.is_a?(Proc) && !params.(job_args[3][:params])
+      params, args = args, nil if args.is_a?(Hash)
+      args = Array(args) unless args.is_a?(Proc)


Wouldn't we want to support a situation when both args and params are passed in, even if args is a Hash?

The original implementation didn't seem to support it, so my PR was proposing to carefully fix it (I thought may've been a bug) alongside adding support for procs, but without breaking existing tests. The bug fix would only kick in if either args or params is a proc (something that wasn't supported before). It would switch to the "new mode" of testing. In the "new mode" if you put hash in either of them, it gets tested.

Your simplification makes it such that if you try passing args: { foo: 'bar' }, params: ->(_){ … }, then expected params are overwritten with expected args and get compared against actual params, so the original expected params (the proc) is dropped. Additionally, expected args become [], so actual args are still being compared, but against an empty array. Is that desirable? Or am I misreading this? 🤔

P.S. I think this is why I ended up writing it that way. 😬 I seem to remember my original attempt was much shorter, but didn't handle this.

Wouldn't we want to support a situation when both expected args and params are passed in, even if args is a Hash?

The original implementation didn't seem to support it, so my PR was proposing to carefully fix it (I thought may've been a bug) alongside adding support for procs

I have not dug into why the original code was written that way, but it feels like a separate concern, so I think it would be better to address it in another PR.

The bug fix would only kick in if either args or params is a proc (something that wasn't supported before). It would switch to the "new mode" of testing.

That seems unintuitive to me. I think if we change the behavior, we should change it for all types of arguments.

Part of the reason I factored params = ... and args = ... out from the comparison logic is to make that kind of change easier, as well as implementing the suggestion from #46626 (comment). (But, again, as a separate PR with a dedicated discussion.)

I have not dug into why the original code was written that way, but it feels like a separate concern, so I think it would be better to address it in another PR. […] I think if we change the behavior, we should change it for all types of arguments.

I guess depends how you think about it.

Option 1: Add new feature "correctly", then fix old feature as a breaking change.
Option 2: Add new feature "brokenly", but consistently with old feature, then fix both of them with a breaking change for both.

I honestly prefer option 1, even though it feels a bit unintuitive. It minimizes unintended behavior.

And since this doesn't seem urgent, we also have these:

Option 3: dig into old feature, fix it, then add this new feature
Option 4: fix them all at once

Since I'm not part of the Rails team, I can only provide reasoning/feedback, but ultimately respect whatever decision you make. Let me know if you want to go option 2, and I can help with the rebase/squash.

The :params kwarg was introduced in #45752. The lesser precedence of :params vs :args seems to be intentional. Code that specifies :params is expected to specify :args as an array (if at all).

I think that is a reasonable expectation, so I would not call it "broken". Though I agree it also isn't ideal. #45752 mentions possibly deprecating the old behavior. I have submitted #48194 to do so.

Gotcha, yeah I was calling it "broken" for brevity, definitely a nuanced case. Nice, very efficient work. 🎉👍🏻

swanson · 2023-05-11T00:46:46Z

@swanson Wouldn't assert_enqueued_emails take care of that? Or are you worried that you aren't confirming the email type, just that it was 1 email?

I do like the idea regardless, and wouldn't mind adding support for this.

Yes, I usually just end up with assert_enqueued_emails but I would prefer to check that the actual mailer is called!

Support args/params proc matchers in `assert_enqueued_email_with` to make it behave more like `assert_enqueued_with`. Fix rails#46621. Co-authored-by: Jonathan Hefner <jonathan@hefner.pro>

jonathanhefner · 2023-05-11T18:08:23Z

I rebased and squashed, and submitted #48194 as a follow-up (see #46626 (comment)).

Thank you, @maxim! 📬

rails-bot bot added the actionmailer label Dec 1, 2022

maxim force-pushed the assert_enqueued_email_with_matchers branch from 7ddc566 to 5a9d6b9 Compare December 1, 2022 17:46

maxim mentioned this pull request Dec 2, 2022

Should assert_enqueued_email_with accept args/params matchers? #46621

Closed

maxim changed the title ~~Add matcher support to assert_enqueued_email_with~~ Add matcher support to assert_enqueued_email_with Dec 5, 2022

jonathanhefner reviewed May 10, 2023

View reviewed changes

maxim commented May 10, 2023

View reviewed changes

Add matcher support to assert_enqueued_email_with

96f19e9

Support args/params proc matchers in `assert_enqueued_email_with` to make it behave more like `assert_enqueued_with`. Fix rails#46621. Co-authored-by: Jonathan Hefner <jonathan@hefner.pro>

jonathanhefner force-pushed the assert_enqueued_email_with_matchers branch from a6e7157 to 96f19e9 Compare May 11, 2023 17:41

jonathanhefner merged commit cb1073e into rails:main May 11, 2023
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add matcher support to `assert_enqueued_email_with` #46626

Add matcher support to `assert_enqueued_email_with` #46626

maxim commented Dec 1, 2022 •

edited

swanson commented May 10, 2023

maxim commented May 10, 2023 •

edited

jonathanhefner left a comment

jonathanhefner May 10, 2023

maxim left a comment •

edited

maxim May 10, 2023 •

edited

jonathanhefner May 10, 2023

maxim May 10, 2023 •

edited

jonathanhefner May 11, 2023

maxim May 11, 2023

swanson commented May 11, 2023

jonathanhefner commented May 11, 2023

Add matcher support to assert_enqueued_email_with #46626

Add matcher support to assert_enqueued_email_with #46626

Conversation

maxim commented Dec 1, 2022 • edited

Motivation / Background

Detail

Checklist

swanson commented May 10, 2023

maxim commented May 10, 2023 • edited

jonathanhefner left a comment

Choose a reason for hiding this comment

jonathanhefner May 10, 2023

Choose a reason for hiding this comment

maxim left a comment • edited

Choose a reason for hiding this comment

maxim May 10, 2023 • edited

Choose a reason for hiding this comment

jonathanhefner May 10, 2023

Choose a reason for hiding this comment

maxim May 10, 2023 • edited

Choose a reason for hiding this comment

jonathanhefner May 11, 2023

Choose a reason for hiding this comment

maxim May 11, 2023

Choose a reason for hiding this comment

swanson commented May 11, 2023

jonathanhefner commented May 11, 2023

Add matcher support to `assert_enqueued_email_with` #46626

Add matcher support to `assert_enqueued_email_with` #46626

maxim commented Dec 1, 2022 •

edited

maxim commented May 10, 2023 •

edited

maxim left a comment •

edited

maxim May 10, 2023 •

edited

maxim May 10, 2023 •

edited