Allow only stubbed changes to `Spree::Config` in specs #3220

spaghetticode · 2019-06-07T11:40:23Z

By freezing Spree::Config.preference_store and allowing changes to Spree::Config settings only via RSpec stubs we ensure no global settings state change will leak outside the lifecycle of each spec example.

Also, stubbing changes to Spree::Config removes the necessity to reset these settings before each spec is run, which in the past encouraged the not-so-good practice of changing the global state and not returning it to the original values.

When writing tests, the preferred way to change Spree::Config settings is by using the helper method stub_spree_preferences:

stub_spree_preferences(currency: 'EUR', track_inventory_levels: true)

The helper method with_unfrozen_spree_preference_store can be used when for exceptional reasons one needs to use the unfrozen version of the preference_store. This helper method has the advantage of automatically returning the configuration state to the original value:

around do |example|
  with_unfrozen_spree_preference_store do
    example.run
  end
end

resolve #3219

Checklist:

I have followed Pull Request guidelines
I have added a detailed description into each commit message
I have added tests to cover this change

kennyadsl

Thanks @spaghetticode, this is fantastic work and I look forward to having it merged in core. I left a preliminary review with some questions, also I'd like to have your opinion on deprecating reset_spree_preferences at all so even stores and extensions will stop using it.

kennyadsl · 2019-06-10T08:48:20Z

core/spec/models/spree/promotion_handler/shipping_spec.rb

@@ -73,6 +73,8 @@ module PromotionHandler
 end

 context 'with a custom shipping action' do
+ let!(:default_actions) { Spree::Config.environment.promotions.shipping_actions }


Is it not possible to stub this one as well?

👍 yes it is, thanks for noticing it!

kennyadsl · 2019-06-10T08:51:07Z

frontend/spec/features/currency_spec.rb

@@ -14,7 +14,9 @@
 click_link "RoR Mug"
 click_button "Add To Cart"
 # Now that we have an order...
- Spree::Config[:currency] = "AUD"
- visit spree.root_path
+ with_unfrozen_spree_preference_store do


I think we should document in the commit message why it's not possible to stub this preference (mainly because is not entirely clear to me yet :P ).

I am also curious about this one.

Being an old regression spec I wanted to keep the code as similar as the original as possible in order to avoid making the spec irrelevant by overlooking some details, so I did not stubbed it in the beginning, but after running this spec against the original code for spree/spree#2340 I think this can be safely stubbed as well. See also the fix for issue 2340: spree/spree@58585fc

As far as I understand this spec basically ensured that this old line would find no order:

current_order = Spree::Order.find_by_id_and_currency(session[:order_id], current_currency, :include => :adjustments)

by changing the backend currency, thus making the line that followed (before the fix) raise an exception:

current_order.last_ip_address = ip_address

I think this spec is not much relevant anymore, as that part of the code is long gone, so I guess it may be safely removed. What do you think? Maybe in another PR to keep things clean and focused here.

kennyadsl · 2019-06-10T08:55:30Z

core/lib/spree/testing_support/preferences.rb

+ Spree::Config.preference_store = Spree::Config[:unfrozen_preference_store]
+ yield
+ ensure
+ reset_spree_preferences


I'm not sure to get why this reset_spree_preferences is needed. If we are replacing the preference store with the frozen one generated at the test suite boot we should have it in the same state without the need of a reset, right?

👍 Especially after my latest update, it's not needed anymore.

One thing worth noticing that surprised me is that reset_spree_preferences returns configuration values to the very default ones for the preference.

For example, Spree::Config.mails_from is returned to this default value instead of the one set in dummy_app.rb.

I think this is another good reason for avoiding to use it, or deprecate it as you already suggested.

jacobherrington

I really like this change @spaghetticode. I would just like to see a comment explaining the purpose of stub_spree_preferences. Great work! 👍

core/lib/spree/testing_support/preferences.rb

The method `#stub_spree_preferences` is the preferred way to change global preferences accessible via `Spree::Config`. The method stubs the provided preference calls without changing the actual preference, removing the need to return the preference value to its original.

In order to reuse the helper method `stub_spree_preferences` multiple times ( i.e. in a `before` block and then in a subsequent `it` block) it is necessary to keep previous stubs by not running the line: allow(Spree::Config).to receive(:[]).and_call_original if already executed once.

The method allows to temporarily change `Spree::Config.preference_store` to its original unfrozen values. Changes to the `Spree::Config` settings are reverted automatically at the end of the method execution. This method is required by some existing specs, where the conventional way to change Spree preferences by stubbing does not work or does not make sense. The unfrozen original `preference_store` is stored in `Spree::Config` itself for praticity. The unfrozen original includes changes made by application initializers, such as `dummy_app.rb` when running the test suite.

This is not needed anymore as changes to Spree::Config should only be via stubs.

The configuration preference store is now frozen in order to avoid global state changes that could affect other specs, when not reverted. The unfrozen original configuration is stored for later use in the config preference `unfrozen_preference_store`.

`Spree::Config.mails_from` is set in `dummy_app.rb`, but when using the method `reset_spree_preferences` that change is lost. In order to successfully test the method by veryfing that preference, it's necessary to execute `reset_spree_preferences` before checking the value for `mails_from`, so the before/after values are the same.

Spree::Config changes required by test setups are stubbed so the global configuration is never affected.

Spree::Config changes required test setups are stubbed so the global configuration is never affected.

spaghetticode · 2019-06-11T21:28:18Z

@kennyadsl I agree with you on deprecating reset_spree_preferences, so I've just added a commit for that.

jacobherrington

I like this change, this is a great idea. Well done @spaghetticode!

kennyadsl

Left the last comment, thanks Andrea!

kennyadsl · 2019-06-13T14:56:20Z

core/spec/lib/spree/core/testing_support/preferences_spec.rb

@@ -4,7 +4,23 @@

 RSpec.describe Spree::TestingSupport::Preferences do
 describe 'resetting the app configuration' do
+ around do |example|
+ with_unfrozen_spree_preference_store do
+ behavior = Spree::Deprecation.behavior


I think you should use Spree::Deprecation.silence {} instead here, right?

@kennyadsl 👍 that's much better, thank you!

Helper method `reset_spree_preferences` has been deprecated, as its usage contributed to the bad practice of changing Spree::Config preference values without returning them to the original values. Also, by using `reset_spree_preferences`, preference values set in application initializers such as `dummy_app.rb` are lost. As the method is now deprecated, specs that test it must temporarily change the deprecation behavior or the test suite on circleci will fail.

kennyadsl

👍thanks @spaghetticode, this is great!

spaghetticode force-pushed the spaghetticode/freeze-preferences branch from 2e6845d to f77f434 Compare June 10, 2019 07:26

kennyadsl reviewed Jun 10, 2019

View reviewed changes

spaghetticode force-pushed the spaghetticode/freeze-preferences branch from f77f434 to 916204a Compare June 10, 2019 14:17

jacobherrington reviewed Jun 10, 2019

View reviewed changes

core/lib/spree/testing_support/preferences.rb Show resolved Hide resolved

spaghetticode added 10 commits June 11, 2019 21:35

Do not reset Spree::Config when running specs

56a9ec3

This is not needed anymore as changes to Spree::Config should only be via stubs.

Stub Spree::Config changes in Core specs

dca7a9c

Spree::Config changes required by test setups are stubbed so the global configuration is never affected.

Stub Spree::Config changes in Frontend specs

54ffbde

Spree::Config changes required by test setups are stubbed so the global configuration is never affected.

Stub Spree::Config changes in API specs

4b27767

Spree::Config changes required by test setups are stubbed so the global configuration is never affected.

Stub Spree::Config changes in Backend specs

62d5c01

Spree::Config changes required test setups are stubbed so the global configuration is never affected.

spaghetticode force-pushed the spaghetticode/freeze-preferences branch 3 times, most recently from 2ad9eb4 to 364510a Compare June 11, 2019 21:06

jacobherrington approved these changes Jun 12, 2019

View reviewed changes

kennyadsl reviewed Jun 13, 2019

View reviewed changes

spaghetticode force-pushed the spaghetticode/freeze-preferences branch from 364510a to b3fe321 Compare June 13, 2019 15:08

kennyadsl approved these changes Jun 14, 2019

View reviewed changes

kennyadsl added the Important Change label Jun 17, 2019

kennyadsl merged commit d0c0616 into solidusio:master Jun 17, 2019

kennyadsl deleted the spaghetticode/freeze-preferences branch June 17, 2019 10:57

kennyadsl mentioned this pull request Jul 18, 2019

Freeze preferences for Backend, Frontend and Api specs as well #3275

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow only stubbed changes to `Spree::Config` in specs #3220

Allow only stubbed changes to `Spree::Config` in specs #3220

spaghetticode commented Jun 7, 2019

kennyadsl left a comment

kennyadsl Jun 10, 2019

spaghetticode Jun 10, 2019

kennyadsl Jun 10, 2019

jacobherrington Jun 10, 2019

spaghetticode Jun 11, 2019

kennyadsl Jun 10, 2019

spaghetticode Jun 10, 2019

jacobherrington left a comment

spaghetticode commented Jun 11, 2019

jacobherrington left a comment

kennyadsl left a comment

kennyadsl Jun 13, 2019

spaghetticode Jun 13, 2019

kennyadsl left a comment

Allow only stubbed changes to Spree::Config in specs #3220

Allow only stubbed changes to Spree::Config in specs #3220

Conversation

spaghetticode commented Jun 7, 2019

kennyadsl left a comment

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

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

jacobherrington left a comment

Choose a reason for hiding this comment

spaghetticode commented Jun 11, 2019

jacobherrington left a comment

Choose a reason for hiding this comment

kennyadsl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kennyadsl left a comment

Choose a reason for hiding this comment

Allow only stubbed changes to `Spree::Config` in specs #3220

Allow only stubbed changes to `Spree::Config` in specs #3220