Introduce Spree::Event's test interface to run only selected listeners

[waiting-for-dev]

Given the global nature of our event bus, we need a system to scope the
execution of a block to a selected list of subscribers. That's useful
for testing purposes, as we need to be sure that others subscribers are
not interfering with our expectations.

This commit introduces a Spree::Event.performing_only(listeners)
method. It takes a block during the execution of which only the provided
listeners are subscribed:

listener1 = Spree::Event.subscribe('foo') { do_something }
listener2 = Spree::Event.subscribe('foo') { do_something_else }

Spree::Event.performing_only(listener1) do
  Spree::Event.fire('foo') # only listener1 will run
end

Spree::Event.fire('foo') # both listener1 & listener2 will run

This behavior is only available for the new
Spree::Event::Adapters::Default adapter.

We only need that for testing purposes, so the method is made available
after calling Spree::Event.enable_test_interface. It prevents the main
Spree::Event API from being bloated and sends a more explicit message
to users.

We also add a Spree::Subscriber#listeners method, which returns the
set of generated listeners for a given subscriber module. It's called
automatically by Spree::Event.performing_only so that users can
directly specify that they only want the listeners for a given
subscriber module to be run. Spree::Subscriber#listeners accepts an
array of event names as arguments in case more fine-grained control is
required.

module EmailSubscriber
  include Spree::Event::Subscriber

  event_action :foo
  event_action :bar

  def foo(_event)
    do_something
  end

  def bar(_event)
    do_something_else
  end
end

Spree::Event.performing_only(EmailSubscriber) do
  Spree::Event.fire('foo') # both foo & bar methods will run
end

Spree::Event.performing_only(EmailSubscriber.listeners('foo')) do
  Spree::Event.fire('foo') # only foo method will run
end

A specialized Spree::Event.performing_nothing method calls
Spree::Event.performing_only with no listeners at all.

Checklist:

• I have followed Pull Request guidelines
• I have added a detailed description into each commit message
• I have updated Guides and README accordingly to this change (if needed)
• I have added tests to cover this change (if needed)
• I have attached screenshots to this PR for visual changes (if needed)

[adammathys]

Overall looks pretty reasonable to me.

[spaghetticode]

@waiting-for-dev thanks for this useful addition! I've left a few notes in regards to specs... the application code looks great!

[forkata]

What are your thoughts on having this as a stand-alone module (something like Spree::TestingSupport::Event) rather than extending the Spree::Event module? It doesn't seem like we are using anything other than default_adapter and the deprecation_handler both of which we have access to from outside the Spree::Event module.

[waiting-for-dev]

In my view, having those methods on Spree::Event is more convenient and communicate better. Otherwise, to use them, we should do something like:

RSpec.describe SomeClass do
  include Spree::TestingSupport::Event

  it 'does something' do
    # ...
    performing_only(listener) {}
    # ...
  end
end

We're already introducing a testing helper module for events in #4048. However, they are stubbing helpers, and the module includes RSpec matchers. My idea was to separate both kinds of testing strategies. Stubbing methods are indeed testing helpers. However, #performing_only is just an extension of the core event bus behavior (for instance, it has nothing to do with RSpec). We could have that method within Spree::Event itself, but we use ruby's flexibility to communicate intentions better. However, if you feel very strongly about it, we can revisit it.

[forkata]

Thanks for the explanation @waiting-for-dev, that makes sense as a motivation for the approach you have taken. I think the naming suggestion I used may have not been the best, as I wasn't suggesting we mix this "new" module into our test classes, but rather provide a different class from Spree::Event which has the #performing_only interface. I thought that may provide some more clarity over enhancing the behaviour of the existing class by calling enable_test_interface which isn't a reversible action. I don't feel strongly about this, so no need to change anything here 😄

[waiting-for-dev]

I see. Yeah, having them called directly from the test module is also clear. I think I reached a good middle ground, and now both systems are supported! So you can both:

Spree::Event.enable_test_interface
Spree::Event.performing_only(...) { ... }

or

Spree::Event::TestInterface.performing_only(...) { ... }

We still use the former on the doc examples, but we do support the latter (we mention it on the docs and have a test covering it). WDYT? 🙂

[forkata]

That sounds great, thanks @waiting-for-dev! This seems like a good compromise which shouldn't add any maintenance overhead 👍🏼

[kennyadsl]

Why this is needed in this PR?

[waiting-for-dev]

This PR adds other caller locations that have no direct access to the used adapter. It's easier to default it to the global adapter, instead of making the same operation from the caller each time. E.g.:

• solidus/core/lib/spree/event/subscriber_registry.rb

  Line 56 in 5881d86

  raise <<~MSG if Adapters::DeprecationHandler.legacy_adapter?

• solidus/core/spec/lib/spree/event/subscriber_spec.rb

  Line 95 in 5881d86

  unless Spree::Event::Adapters::DeprecationHandler.legacy_adapter?

• solidus/core/spec/lib/spree/event/test_interface_spec.rb

  Line 37 in 5881d86

  unless Spree::Event::Adapters::DeprecationHandler.legacy_adapter?

  
  

  solidus/core/spec/lib/spree/event/subscriber_registry_spec.rb

  Line 120 in 5881d86

  if Spree::Event::Adapters::DeprecationHandler.legacy_adapter?

[kennyadsl]

I don't understand why we need to store the old adapter, can you help me?

[forkata]

I believe this is necessary because Spree::Event.fire uses the default_adapter unless one is explicitly passed in the options here, so we have to replace that with one built by the with_listeners call for the duration of the tests, and then reset to the adapter instance which is not configured with a filter on the listeners.

[waiting-for-dev]

Exactly. The event bus is global. We change it for the duration of the performing_only block.

[kennyadsl]

Got it, I was confused because I wrongly interpreted old_adapter as legacy_adapter. What about changing it to adapter_in_use or something similar, instead?

[waiting-for-dev]

Oh, I see. Yeah, it can be confusing. I renamed it to adapter_in_use now 🙂

[kennyadsl]

Non blocking:

1. suggested to change a variable name
2. I would create two commits for this PR, moving in a separate one the part that adds the default adapter to methods signature, which is related but IMHO deserves some kind of specific documentation via its commit message.

[kennyadsl]

Got it, I was confused because I wrongly interpreted old_adapter as legacy_adapter. What about changing it to adapter_in_use or something similar, instead?

[waiting-for-dev]

@kennyadsl

1. suggested to change a variable name

Done!

2. I would create two commits for this PR, moving in a separate one the part that adds the default adapter to methods signature, which is related but IMHO deserves some kind of specific documentation via its commit message.

Unfortunately, that change belongs to the same unit of work. It'd leave the first commit in an inconsistent state (tests failing), so I'd prefer not to do that. However, I don't think it's that important, as we should remove all the deprecation logic once we remove the old adapter. WDYT? If you feel very strongly about it, we can try to find a way, though 🙂

[kennyadsl]

Thanks Marc!