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

Update method name in HaveEnqueuedMail (job_match? to job_matches?) #2793

Merged
merged 1 commit into from
Sep 5, 2024
Merged

Update method name in HaveEnqueuedMail (job_match? to job_matches?) #2793

merged 1 commit into from
Sep 5, 2024

Conversation

davidrunger
Copy link
Contributor

This change fixes a bug wherein a spec that expects no mail to have been enqueued will fail (falsely claiming that a mail was enqueued) if any non-mail job is enqueued, even if indeed no mail job is actually enqueued.

This bug was introduced in ab6e6e8 / #2780 , which was first included in the 7.0.0 release.

In that change, the RSpec::Rails::Matchers::ActiveJob::Base job_match? instance method was renamed to job_matches?. The problem is that RSpec::Rails::Matchers::HaveEnqueuedMail, which inherits from RSpec::Rails::Matchers::ActiveJob::Base (via
RSpec::Rails::Matchers::ActiveJob::HaveEnqueuedJob), intends to override that method, which it does by defining its own job_match? instance method. However, because this job_match? method name is no longer the same as the (now so called) job_matches? method that it intends to override, the job_match? method of
RSpec::Rails::Matchers::HaveEnqueuedMail is no longer having the intended override effect on the behavior of the matcher, producing the aforementioned bug.

This change resolves the issue by updating the method name in the RSpec::Rails::Matchers::HaveEnqueuedMail subclass from job_match? to job_matches?, reflecting that renaming that was done in the parent class in ab6e6e8 .

Comment on lines -308 to -311
non_mailer_job = Class.new(ActiveJob::Base) do
def perform; end
def self.name; "NonMailerJob"; end
end
Copy link
Contributor Author

@davidrunger davidrunger Sep 4, 2024

Choose a reason for hiding this comment

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

(Since now there are two specs in this file (i.e. the spec being modified here, and the new spec that I added above) that have occasion to refer to a non-mailer job class, I am extracting this previously anonymous/local job class up to an actual class constant defined in the setup at the top of this file. The downside of this is that the global Ruby namespace in specs is now polluted by an additional constant, NonMailerJob, but personally I think that's worthwhile, for the upside of any specs that need to reference such a job being able conveniently to do so.)

Comment on lines +102 to +103
it "passes when negated with 0 arguments and a non-mailer job is enqueued" do
expect { NonMailerJob.perform_later }.not_to have_enqueued_email
Copy link
Contributor Author

Choose a reason for hiding this comment

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

(This new spec fails without the job_match? => job_matches? rename done in this PR.)

@davidrunger
Copy link
Contributor Author

davidrunger commented Sep 4, 2024

For an earlier approved build (here) before I force-pushed a minor change or two, two matrix items failed:

  1. Ruby: 3.3, Rails: ~> 7.2.0
  2. Ruby: 3.3, Rails: ~> 7.1.0

However, these are also failing on main (e.g. as can be seen in what is currently the most recent build for main, here), so I don't think that those failures are related to or caused by this PR, and I expect that those will fail again, if another build is approved to run.

@davidrunger
Copy link
Contributor Author

For an earlier approved build (here) before I force-pushed a minor change or two, two matrix items failed:

  1. Ruby: 3.3, Rails: ~> 7.2.0
  2. Ruby: 3.3, Rails: ~> 7.1.0

However, these are also failing on main (e.g. as can be seen in what is currently the most recent build for main, here), so I don't think that those failures are related to or caused by this PR, and I expect that those will fail again, if another build is approved to run.

I am pretty sure that these test failures are a result of the fact that Ruby 3.3.5 (released yesterday) is moving ostruct (for OpenStruct) from the standard library to a gem and that it now prints a warning about this. This aligns with the fact that only matrix builds for Ruby 3.3 are failing, whereas matrix builds for lower Ruby versions are passing.

Also, I think that this has been somehow fixed in Rails's main branch (I think maybe mostly via this PR?, which removes usage of OpenStruct and requires of ostruct), which explains why the Ruby: 3.3, Rails: main build is passing and only the Rails ~> 7.2.0 and ~> 7.1.0 builds are failing (and only for Ruby 3.3, which is currently resolving to Ruby 3.3.5).

@davidrunger
Copy link
Contributor Author

Jon Rowe has put up one possible fix for the failing specs on main here #2794 , and I've put up an alternative possible fix here #2796 .

@JonRowe
Copy link
Member

JonRowe commented Sep 4, 2024

Good catch can you rebase this please

Verified

This commit was signed with the committer’s verified signature.
jeevatkm Jeevanandam M.
This change fixes a bug wherein a spec that expects no mail to have been
enqueued will fail (falsely claiming that a mail _was_ enqueued) if any
_non-mail_ job is enqueued, even if indeed no _mail_ job is actually
enqueued.

This bug was introduced in ab6e6e8 / #2780 , which was first included
in the 7.0.0 release.

In that change, the `RSpec::Rails::Matchers::ActiveJob::Base`
`job_match?` instance method was renamed to `job_matches?`. The problem
is that `RSpec::Rails::Matchers::HaveEnqueuedMail`, which inherits from
`RSpec::Rails::Matchers::ActiveJob::Base` (via
`RSpec::Rails::Matchers::ActiveJob::HaveEnqueuedJob`), intends to
override that method, which it does by defining its own `job_match?`
instance method. However, because this `job_match?` method name is no
longer the same as the (now so called) `job_matches?` method that it
intends to override, the `job_match?` method of
`RSpec::Rails::Matchers::HaveEnqueuedMail` is no longer having the
intended override effect on the behavior of the matcher, producing the
aforementioned bug.

This change resolves the issue by updating the method name in the
`RSpec::Rails::Matchers::HaveEnqueuedMail` subclass from `job_match?` to
`job_matches?`, reflecting that renaming that was done in the parent
class in ab6e6e8 .
@davidrunger
Copy link
Contributor Author

@JonRowe Thank you for fixing the failing specs on main, and thank you for taking a look at this PR.

I just rebased with latest main and force-pushed.

@JonRowe JonRowe merged commit 387fe76 into rspec:main Sep 5, 2024
14 of 17 checks passed
@JonRowe
Copy link
Member

JonRowe commented Sep 5, 2024

Thanks, merged despite build failure as that has been fixed in #2798

JonRowe added a commit that referenced this pull request Sep 5, 2024

Verified

This commit was signed with the committer’s verified signature.
jeevatkm Jeevanandam M.
…ethod-name

Update method name in HaveEnqueuedMail (job_match? to job_matches?)
@davidrunger davidrunger deleted the update-have-enqueued-mail-method-name branch September 5, 2024 17:09
@JonRowe
Copy link
Member

JonRowe commented Nov 9, 2024

Released in 7.0.2 and 7.1.0

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

Successfully merging this pull request may close these issues.

None yet

2 participants