Skip to content
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

Remove redundant test from message_delivery_test #47893

Conversation

shivamsinghchahar
Copy link
Contributor

Using test macro instead of regular method definition to make the tests uniform.

@zzak
Copy link
Member

zzak commented May 21, 2023

You can argue it's a cosmetic change, but I do see the value in consistency! 🤔

@zzak zzak reopened this May 21, 2023
Comment on lines 60 to 63
test "should enqueue and run correctly in activejob" do
@mail.deliver_later!
assert_equal 1, ActionMailer::Base.deliveries.size
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could just remove this test instead. It looks like the described behavior is thoroughly covered by the next two tests, right?

test "should enqueue the email with :deliver_now delivery method" do
assert_performed_with(job: ActionMailer::MailDeliveryJob, args: ["DelayedMailer", "test_message", "deliver_now", args: [1, 2, 3]]) do
@mail.deliver_later
end
end
test "should enqueue the email with :deliver_now! delivery method" do
assert_performed_with(job: ActionMailer::MailDeliveryJob, args: ["DelayedMailer", "test_message", "deliver_now!", args: [1, 2, 3]]) do
@mail.deliver_later!
end
end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jonathanhefner Yes, that makes sense. I can do it. Do you want me to raise another PR for this or should I change the title of this one?

zzak referenced this pull request May 22, 2023
@shivamsinghchahar shivamsinghchahar force-pushed the use-test-macro-message-delivery-test branch from dc8b476 to f30fc95 Compare May 22, 2023 14:07
@shivamsinghchahar shivamsinghchahar changed the title message_delivery_test use test macro to make the test definitions uniform Remove redundant test from message_delivery_test May 22, 2023
@jonathanhefner jonathanhefner merged commit 56edc94 into rails:main May 22, 2023
9 checks passed
@jonathanhefner
Copy link
Member

Thank you, @shivamsinghchahar! ✂️

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

Successfully merging this pull request may close these issues.

None yet

3 participants