Add ActionMailbox spec helpers and test type #2119

jamesdabbs-procore · 2019-05-04T16:19:57Z

Adds the following helpers to example groups with :type => :mailbox

process(mail_or_attributes) - send mail directly to the mailbox under test for processing.
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.

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

jamesdabbs-procore

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 like process_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::Blobs (and action_text about ActionText::RichText) so it'd be good to have a plan for handling those consistently.

jamesdabbs-procore · 2019-05-04T16:25:09Z

lib/rspec/rails/example/mailbox_example_group.rb

+
+      if RSpec::Rails::FeatureCheck.has_action_mailbox?
+        require 'action_mailbox/test_helper'
+        extend ::ActionMailbox::TestHelper


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.

I agree with you but why don't you like it?

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 in process_inbound_email

add shared examples for ::ActionMailbox::InboundEmail and MockInboundEmail to make sure they are interchangeable

consider using MockInboundEmail for the process helper (either by default or opt-in somehow?)

work on landing MockInboundEmail upstream

How does that sound to y'all?

I love the idea of a MockInboundEmail but I think it can be another PR?

jamesdabbs-procore · 2019-05-04T16:31:17Z

lib/rspec/rails/matchers/action_mailbox.rb

+
+          def matches?(mailbox)
+            @mailbox  = mailbox
+            @receiver = ApplicationMailbox.router.send(:match_to_mailbox, inbound_email)


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.

PR for that - rails/rails#36181

fables-tales · 2019-05-04T17:02:19Z

Thank you for this PR! It looks like overall the quality is pretty high :)

I'll review this soon

fables-tales · 2019-05-04T17:37:39Z

lib/rspec/rails/matchers.rb

@@ -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


if we're going to add a blankline to our require block, let's add one here too.

fables-tales · 2019-05-04T17:38:12Z

lib/generators/rspec/mailbox/mailbox_generator.rb

+    # @private
+    class MailboxGenerator < Base
+      def create_mailbox_spec
+        template 'mailbox_spec.rb.erb', File.join('spec/mailboxes', class_path, "#{file_name}_mailbox_spec.rb")


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?

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.

benoittgt · 2019-05-05T13:09:25Z

lib/rspec/rails/example/mailbox_example_group.rb

+
+      if RSpec::Rails::FeatureCheck.has_action_mailbox?
+        require 'action_mailbox/test_helper'
+        extend ::ActionMailbox::TestHelper


I agree with you but why don't you like it?

benoittgt · 2019-05-05T13:11:57Z

lib/rspec/rails/matchers/action_mailbox.rb

+
+          def failure_message
+            "expected #{describe_inbound_email} to route to #{mailbox}".tap do |msg|
+              if receiver


benoittgt · 2019-05-05T13:55:16Z

Thanks for your PR. I find it clear.

Thoughts on the API exposed to tests? Anything you'd like to see added / changed?

It seems ok for me.

Does the feature toggling look reasonable? (Does travis not run on PRs into non-master branches?)

It is ok for me. For the CI yes. I have open a PR for that #2120

Any preference for how to handle ActionMailbox::InboundEmail? What level of integration do we want to provide there? Personally, I'd prefer for matchers like process_inbound_email not to have to write to the database.

For your first question. I re-read the doc of Action MailBox but didn't find things we should implement now.

I'd prefer for matchers like process_inbound_email not to have to write to the database.

Yes!

benoittgt · 2019-05-09T20:18:00Z

The CI fail with missing documentation

Undocumented Objects:
(in file: lib/rspec/rails/example/mailbox_example_group.rb)
RSpec::Rails::MailboxExampleGroup.create_inbound_email
RSpec::Rails::MailboxExampleGroup#have_bounced
RSpec::Rails::MailboxExampleGroup#have_failed
Missing documentation coverage (currently at 97.84%)
The command "script/run_build 2>&1" exited with 1.

https://travis-ci.org/rspec/rspec-rails/jobs/530437674

@jamesdabbs Do you think you can open a PR to fix this? :)

benoittgt · 2019-05-09T21:23:58Z

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 4-0-dev and CI should build your PR. It will make the fix easier to validate before merging it. ;)

ignatiusreza · 2019-05-09T23:27:17Z

spec/rspec/rails/example/mailbox_example_group_spec.rb

+
+module RSpec
+  module Rails
+    describe MailboxExampleGroup, :skip => !RSpec::Rails::FeatureCheck.has_active_job? do


this should check for has_action_mailbox? instead of has_active_job?..

Thank you! Opened in #2123

ignatiusreza · 2019-05-09T23:27:35Z

spec/rspec/rails/matchers/action_mailbox_spec.rb

+class Inbox < ApplicationMailbox; end
+class Otherbox < ApplicationMailbox; end
+
+RSpec.describe "ActionMailbox matchers", :skip => !RSpec::Rails::FeatureCheck.has_active_job? do


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

jamesdabbs-procore commented May 4, 2019

View reviewed changes

fables-tales requested changes May 4, 2019

View reviewed changes

benoittgt reviewed May 5, 2019

View reviewed changes

Add style changes from code review

6826be3

fables-tales approved these changes May 9, 2019

View reviewed changes

fables-tales merged commit 8840963 into rspec:4-0-dev May 9, 2019

jamesdabbs mentioned this pull request May 9, 2019

Add documentation to recently added ActionMailbox methods #2122

Merged

ignatiusreza reviewed May 9, 2019

View reviewed changes

jamesdabbs mentioned this pull request May 10, 2019

Fix ActionMailbox feature checks #2123

Merged

benoittgt mentioned this pull request May 17, 2019

Long running 4.0 development branch #2071

Merged

benoittgt added a commit that referenced this pull request May 18, 2019

Changelog for #2119, #2113, #2125

0b877c5

benoittgt added a commit that referenced this pull request Aug 21, 2019

Changelog for #2119, #2113, #2125

df4ce36

benoittgt mentioned this pull request Aug 21, 2019

How to test ActionText components? #2154

Open

benoittgt added a commit to benoittgt/rspec-rails that referenced this pull request Aug 24, 2019

Changelog for rspec#2119, rspec#2113, rspec#2125

8b5f08d

This was referenced May 18, 2020

rspec-rails 4.0 release blog post rspec/rspec.github.io#143

Merged

Add documentation for ActionMailbox #2341

Closed

dependabot bot mentioned this pull request Mar 8, 2021

Bump rspec-rails from 3.9.1 to 4.1.0 garyzhuge/rails_6_docker#88

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ActionMailbox spec helpers and test type #2119

Add ActionMailbox spec helpers and test type #2119

jamesdabbs-procore commented May 4, 2019 •

edited

jamesdabbs-procore left a comment

jamesdabbs-procore May 4, 2019

benoittgt May 5, 2019 •

edited

jamesdabbs May 5, 2019 •

edited

benoittgt May 6, 2019

jamesdabbs-procore May 4, 2019

jamesdabbs May 4, 2019

fables-tales commented May 4, 2019

fables-tales May 4, 2019

fables-tales May 4, 2019

jamesdabbs May 5, 2019 •

edited

benoittgt May 5, 2019 •

edited

benoittgt May 5, 2019

benoittgt commented May 5, 2019

benoittgt commented May 9, 2019

benoittgt commented May 9, 2019 •

edited

ignatiusreza May 9, 2019

jamesdabbs May 10, 2019

ignatiusreza May 9, 2019

Add ActionMailbox spec helpers and test type #2119

Add ActionMailbox spec helpers and test type #2119

Conversation

jamesdabbs-procore commented May 4, 2019 • edited

jamesdabbs-procore left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benoittgt May 5, 2019 • edited

Choose a reason for hiding this comment

jamesdabbs May 5, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fables-tales commented May 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesdabbs May 5, 2019 • edited

Choose a reason for hiding this comment

benoittgt May 5, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benoittgt commented May 5, 2019

benoittgt commented May 9, 2019

benoittgt commented May 9, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesdabbs-procore commented May 4, 2019 •

edited

benoittgt May 5, 2019 •

edited

jamesdabbs May 5, 2019 •

edited

jamesdabbs May 5, 2019 •

edited

benoittgt May 5, 2019 •

edited

benoittgt commented May 9, 2019 •

edited