-
-
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
Add ActionMailbox spec helpers and test type #2119
Add ActionMailbox spec helpers and test type #2119
Conversation
Adds the following helpers to example groups with `:type => :mailbox` * process(mail_or_attributes) - send mail directly to the mailbox under test for `process`ing. * receive_inbound_email(mail_or_attributes) - matcher for asserting whether incoming email would route to the mailbox under test. * have_been_delivered - matcher for asserting whether an incoming email object was delivered. * have_bounced - matcher for asserting whether an incoming email object has bounced. * have_failed - matcher for asserting whether an incoming email object has failed. Also adds an ActionMailbox test generator
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.
I'm going to work on landing some upstream changes to action_mailbox
to support easier testing - namely, exposing the "which mailbox does this route to" method and a supported test double for InboundEmail
- but wanted to get this out there for discussion. Main questions:
- Thoughts on the API exposed to tests? Anything you'd like to see added / changed?
- Does the feature toggling look reasonable? (Does travis not run on PRs into non-
master
branches?) - Any preference for how to handle
ActionMailbox::InboundEmail
? What level of integration do we want to provide there? Personally, I'd prefer for matchers likeprocess_inbound_email
not to have to write to the database.
FWIW, any kind of active_storage
support is probably going to have similar questions about ActiveStorage::Blob
s (and action_text
about ActionText::RichText
) so it'd be good to have a plan for handling those consistently.
|
||
if RSpec::Rails::FeatureCheck.has_action_mailbox? | ||
require 'action_mailbox/test_helper' | ||
extend ::ActionMailbox::TestHelper |
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.
I don't love this, but ActionMailbox
appears to expect to operate on a particular named ActionMailbox::InboundEmail < ActiveRecord::Base
, and I'd be worried that using some kind of double here might be brittle.
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.
I agree with you but why don't you like it?
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.
This is here mostly because ActionMailbox::InboundEmail.create_and_extract_message_id!
is doing a lot of work that I didn't want to duplicate here, but this feels like I'm importing implementation details, not clean abstractions.
My inclination here is to instead do something like
- define a lightweight e.g.
RSpec::Rails::ActionMailbox::MockInboundEmail
that we use internally inprocess_inbound_email
- add shared examples for
::ActionMailbox::InboundEmail
andMockInboundEmail
to make sure they are interchangeable - consider using
MockInboundEmail
for theprocess
helper (either by default or opt-in somehow?) - work on landing
MockInboundEmail
upstream
How does that sound to y'all?
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.
I love the idea of a MockInboundEmail
but I think it can be another PR?
|
||
def matches?(mailbox) | ||
@mailbox = mailbox | ||
@receiver = ApplicationMailbox.router.send(:match_to_mailbox, inbound_email) |
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.
actionmailbox
's public API does not expose a way to check which mailbox an email routes to that doesn't fully send the email to the mailbox for processing. expect(mailbox).to receive(:receive)
also seems fraught because of the method name there.
I'll open up a PR upstream to expose something like this in the public API. It seems like a generally useful thing for testing, and I hope won't be too hard to land. If it does, I'll update the implementation here.
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.
PR for that - rails/rails#36181
Thank you for this PR! It looks like overall the quality is pretty high :) I'll review this soon |
@@ -20,6 +20,10 @@ module Matchers | |||
require 'rspec/rails/matchers/relation_match_array' | |||
require 'rspec/rails/matchers/be_valid' | |||
require 'rspec/rails/matchers/have_http_status' | |||
|
|||
if RSpec::Rails::FeatureCheck.has_active_job? | |||
require 'rspec/rails/matchers/active_job' | |||
end |
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.
if we're going to add a blankline to our require block, let's add one here too.
# @private | ||
class MailboxGenerator < Base | ||
def create_mailbox_spec | ||
template 'mailbox_spec.rb.erb', File.join('spec/mailboxes', class_path, "#{file_name}_mailbox_spec.rb") |
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.
I know this is probably inconsistent with the rest of these files, but would you mind adding parentheses here, and then adding newlines on commas, to break this line apart so it's not so long?
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.
Updated this to be more consistent with the other generators that do have a line break here. Let me know if you meant for something like
template(
'mailbox_spec.rb.erb',
File.join(
'spec/mailboxes',
class_path,
"#{file_name}_mailbox_spec.rb"
)
)
instead though.
|
||
if RSpec::Rails::FeatureCheck.has_action_mailbox? | ||
require 'action_mailbox/test_helper' | ||
extend ::ActionMailbox::TestHelper |
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.
I agree with you but why don't you like it?
|
||
def failure_message | ||
"expected #{describe_inbound_email} to route to #{mailbox}".tap do |msg| | ||
if receiver |
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.
Nice :)
Thanks for your PR. I find it clear.
It seems ok for me.
It is ok for me. For the CI yes. I have open a PR for that #2120
For your first question. I re-read the doc of Action MailBox but didn't find things we should implement now.
Yes! |
The CI fail with missing documentation
https://travis-ci.org/rspec/rspec-rails/jobs/530437674 @jamesdabbs Do you think you can open a PR to fix this? :) |
Ok so the CI is failing for something else now. https://travis-ci.org/rspec/rspec-rails/jobs/530476234 Matcher should be dynamically loaded I think. Only on versions that have ActionMailbox. I merged #2120 so you could open a PR from updated |
|
||
module RSpec | ||
module Rails | ||
describe MailboxExampleGroup, :skip => !RSpec::Rails::FeatureCheck.has_active_job? do |
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.
this should check for has_action_mailbox?
instead of has_active_job?
..
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.
Thank you! Opened in #2123
class Inbox < ApplicationMailbox; end | ||
class Otherbox < ApplicationMailbox; end | ||
|
||
RSpec.describe "ActionMailbox matchers", :skip => !RSpec::Rails::FeatureCheck.has_active_job? do |
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.
same as above
* Add ActionMailbox spec helpers and test type Adds the following helpers to example groups with `:type => :mailbox` * process(mail_or_attributes) - send mail directly to the mailbox under test for `process`ing. * receive_inbound_email(mail_or_attributes) - matcher for asserting whether incoming email would route to the mailbox under test. * have_been_delivered - matcher for asserting whether an incoming email object was delivered. * have_bounced - matcher for asserting whether an incoming email object has bounced. * have_failed - matcher for asserting whether an incoming email object has failed. Also adds an ActionMailbox test generator * Add style changes from code review
* Add ActionMailbox spec helpers and test type Adds the following helpers to example groups with `:type => :mailbox` * process(mail_or_attributes) - send mail directly to the mailbox under test for `process`ing. * receive_inbound_email(mail_or_attributes) - matcher for asserting whether incoming email would route to the mailbox under test. * have_been_delivered - matcher for asserting whether an incoming email object was delivered. * have_bounced - matcher for asserting whether an incoming email object has bounced. * have_failed - matcher for asserting whether an incoming email object has failed. Also adds an ActionMailbox test generator * Add style changes from code review
Adds the following helpers to example groups with
:type => :mailbox
process(mail_or_attributes)
- send mail directly to the mailbox under test forprocess
ing.receive_inbound_email(mail_or_attributes)
- matcher for asserting whether incoming email would route to the mailbox under test.have_been_delivered
- matcher for asserting whether an incoming email object was delivered.have_bounced
- matcher for asserting whether an incoming email object has bounced.have_failed
- matcher for asserting whether an incoming email object has failed.Also adds an ActionMailbox test generator.