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

Enable partial doubles verification for RSpec (part 2) #3015

Conversation

cedum
Copy link
Contributor

@cedum cedum commented Dec 31, 2018

This is a follow up of #3005 and enables the partial double verification for RSpec also in Solidus API, Frontend and Backend.

@cedum cedum force-pushed the cedum/enable_verify_partial_doubles_p2 branch from f1413ef to 92e82c3 Compare December 31, 2018 12:01
Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

Thanks, just left a question to DRY a little bit the RSpec helper.

@@ -0,0 +1,16 @@
# frozen_string_literal: true

module PartialDoubleVerificationExampleGroup
Copy link
Member

Choose a reason for hiding this comment

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

Can't we share the support file introduced in core via #3005 ? Maybe moving the module in core/lib into the testing_support folder?

Copy link
Contributor

@aitbw aitbw left a comment

Choose a reason for hiding this comment

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

Other than @kennyadsl's comment to DRY this a little bit, LGTM! 💪

@cedum cedum force-pushed the cedum/enable_verify_partial_doubles_p2 branch from 92e82c3 to 0ec2707 Compare January 11, 2019 11:11
Allow mocking of `LineItem` user provided attributes through the `options`
parameter.
Allows mocking of URL helpers in the footer view tests.
When mocking an authenticated user in a Backend
or Frontend controller, use the
`try_spree_current_user` method, instead of
`spree_current_user`.
Disables RSpec partial double verification for Frontend checkout specs
depending on the Order state machine, which is not supported.
Mocking dynamically defined methods is not supported when having
`verify_partial_doubles` enabled.
@cedum cedum force-pushed the cedum/enable_verify_partial_doubles_p2 branch from 0ec2707 to ce85e17 Compare January 11, 2019 11:15
@cedum
Copy link
Contributor Author

cedum commented Jan 11, 2019

Pushed some changes that DRYes the partial double verification support file as per @kennyadsl suggestion.
I refactored also the module into a plain RSpec.configure block, mostly because I think there're no benefits to isolate it into a module (+ it was requiring ActiveSupport).
It made sense to me to extract also the c.verify_partial_doubles = true setting; in cases where a different setting is required in a single component, it can be easily overridden.
Wdyt?

Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

@cedum Thanks, I think your idea to keep the whole config in the same file is great.

Copy link
Contributor

@ericsaupe ericsaupe left a comment

Choose a reason for hiding this comment

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

Good changes, thanks!

Copy link
Contributor

@jacobherrington jacobherrington left a comment

Choose a reason for hiding this comment

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

Thanks for this @cedum! 🍰

Copy link
Member

@gmacdougall gmacdougall left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks for doing this!

@jacobherrington jacobherrington merged commit e6f648f into solidusio:master Jan 16, 2019
@cedum cedum deleted the cedum/enable_verify_partial_doubles_p2 branch July 31, 2020 11:36
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.

7 participants