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.

Sign up for GitHub

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

Jump to bottom

Support expected counts #219

Merged
merged 6 commits into from
Apr 25, 2024
Merged

Support expected counts #219

merged 6 commits into from
Apr 25, 2024

Conversation

3v0k4
Copy link
Contributor

@3v0k4 3v0k4 commented Apr 23, 2024 •
edited
Loading

Add support for expected number of jobs to both enqueue_sidekiq_job and have_enqueued_sidekiq_job. Plus fixes the copy in some error messages.

See the updated README and the specs for details.

Happy to make edits if needed 🙂

@3v0k4 3v0k4 force-pushed the support-counts branch 5 times, most recently from f6c89e8 to f7016b0 Compare April 24, 2024 08:24
@3v0k4 3v0k4 marked this pull request as ready for review April 24, 2024 08:30
@wspurgin
Copy link
Owner

Nice! I've been wanting something like this after getting inspired by rspec-rails. I'll be a wee busy over the next couple weeks, but I'm excited for this and I'll review more in depth when I can!

wspurgin
wspurgin requested changes Apr 24, 2024
View reviewed changes
Copy link
Owner

@wspurgin wspurgin left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks a ton for this @3v0k4 🙌 I love it.

I have only a few minor changes to request (any comments marked with ⚠️).

Any comments made with 🎨 are just my suggestions but I won't hold up the PR if you choose not to do them or want to recommend something else.

All other comments are just me loving this work 😄 Thanks again!

lib/rspec/sidekiq/matchers/base.rb Show resolved Hide resolved
lib/rspec/sidekiq/matchers/base.rb Outdated Show resolved Hide resolved
lib/rspec/sidekiq/matchers/base.rb Show resolved Hide resolved
lib/rspec/sidekiq/matchers/base.rb Show resolved Hide resolved
spec/rspec/sidekiq/matchers/have_enqueued_sidekiq_job_spec.rb Show resolved Hide resolved
lib/rspec/sidekiq/matchers/base.rb Outdated Show resolved Hide resolved
lib/rspec/sidekiq/matchers/base.rb Outdated Show resolved Hide resolved
lib/rspec/sidekiq/matchers/base.rb Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
@3v0k4
Copy link
Contributor Author

3v0k4 commented Apr 25, 2024 •
edited
Loading

Loved the CR. It reminded me of flashtags.

I applied all the changes you requested (all great points) 🙏

@3v0k4 3v0k4 requested a review from wspurgin April 25, 2024 07:39
wspurgin
wspurgin approved these changes Apr 25, 2024
View reviewed changes
Copy link
Owner

@wspurgin wspurgin left a comment

Choose a reason for hiding this comment

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

🚀 Amazing work. Love having this feature. Thank you @3v0k4 I'll be sure this goes in with the 5.0 release 🙌

@wspurgin wspurgin merged commit 865e45c into wspurgin:main Apr 25, 2024
21 checks passed
@fmichaut-diff
Copy link

Hi @wspurgin when can we expect a release ? 😄
A quick 4.3 just for this change would be phenomenal if the 5.0 is not scheduled before a while

@wspurgin
Copy link
Owner

I was away for a few weeks, but not to worry @fmichaut-diff - 5.0 has now been released 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wspurgin wspurgin wspurgin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@3v0k4 @wspurgin @fmichaut-diff