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

Replace ActiveJob::TestHelper with own module. #3543

Merged
merged 2 commits into from
Jun 23, 2020

Conversation

jessetilro
Copy link
Contributor

@jessetilro jessetilro commented Mar 8, 2020

Issue #3526.

Description
This is my first contribution. I decided to make an attempt to resolve this issue as it was flagged as "Good First Issue". Looking forward to your feedback!

The test code does no longer rely on ActiveJob::TestHelper.
The only method used from this module was perform_enqueued_jobs.
I implemented a method providing this functionality in an own testing helper module,
such that existing test cases do not have to be changed and remain readable.

Checklist:

@aldesantis
Copy link
Member

aldesantis commented Apr 19, 2020

@kennyadsl assigning you as a reviewer here because you made the original issue and I'm not 100% sure this is what you had in mind? Let us know!

Also, @jessetilro congrats on your first contribution and thanks! Looking forward to seeing you more.

@kennyadsl
Copy link
Member

@aldesantis the original issue was made by @filippoliverani. We discussed IRL about this and I think it's quite different from what we had in mind but I'm sure he is able to explain it better than me.

@filippoliverani
Copy link
Contributor

filippoliverani commented May 22, 2020

@kennyadsl The proposed solution in the issue was to stop calling #perform_enqueued_jobs by using :inline queue adapter by default. That would require to update some specs rewriting their expectations.

I think that this PR is good a first step in that direction, it has a moderate impact on code but effectively removes ActiveJob::TestHelper dependency 👍

api/spec/spec_helper.rb Outdated Show resolved Hide resolved
backend/spec/spec_helper.rb Outdated Show resolved Hide resolved
core/spec/rails_helper.rb Outdated Show resolved Hide resolved
@kennyadsl
Copy link
Member

@filippoliverani thanks for this review! I'm fine with this, @jessetilro can you please just fix the comments left in the review? Thanks again! 🙏

jessetilro pushed a commit to jessetilro/solidus that referenced this pull request May 22, 2020
Jesse Tilro added 2 commits May 22, 2020 22:05
Resolves issue solidusio#3526.

The test code does no longer rely on ActiveJob::TestHelper.
The only method used from this module was perform_enqueued_jobs.
I implemented a method providing this functionality in an own testing helper module,
such that existing test cases do not have to be changed and remain readable.
@jessetilro
Copy link
Contributor Author

@kennyadsl I fixed the comments in the review.

@aldesantis aldesantis merged commit f034e58 into solidusio:master Jun 23, 2020
aldesantis pushed a commit to aldesantis/solidus that referenced this pull request Dec 28, 2020
aldesantis pushed a commit to aldesantis/solidus that referenced this pull request Dec 28, 2020
mamhoff pushed a commit to mamhoff/solidus that referenced this pull request Feb 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:solidus_core Changes to the solidus_core gem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants