Reconsider shared_context_metadata_behavior

[pirj]

Background

We have had a problem with metadata defined on spec-local shared example groups (shared_examples / shared_examples_for / shared_context) causing them to be included in completely unrelated example group reported in [1]/[2]. Some more information in #1762.

In #1790 (ed2d59c), shared_context_metadata_behavior with an non-default :apply_to_host_groups value. 3589ab5 added it to the project initializer along with a note that it will become the default and only option in RSpec 4.

:trigger_inclusion remained the default setting, though, for SemVer reasons.

In a nutshell:

`:trigger_inclusion`: shared context will be implicitly included in any groups (or examples) that have matching metadata.
`:apply_to_host_groups`: the metadata will be inherited by the metadata hash of all host groups and examples.

Usage

Personally, I've never seen :apply_to_host_groups being used the way it was designed for. On the other hand, I've seen some projects use :trigger_inclusion for globally-defined shared example groups/contexts.
But it's a sample of one. Let's check around.

As a sandbox for new rubocop-rspec cops, I've assembled a list of most starred Ruby projects that use RSpec, real-world-rspec, ~35 projects total. Out of those 35 (it includes RSpec repos, too!), 7 use :apply_to_host_groups in their spec helpers:

24pullrequests/ administrate/ Homebrew/ camaleon-cms/ canvas-lms/ capistrano/ capybara/ cartodb/ chatwoot/ chef/ diaspora/ discourse/ locomotivecms/ errbit/ fat_free_crm/ forem/ gitlabhq/ hound/ huginn/ lobsters/ loomio/ mastodon/ open-source-billing/ publify/ puppet/ radiant/ refinerycms/ rspec-core/ rspec-expectations/ rspec-mocks/ rspec-rails/ rubocop/ rubytoolbox/ sharetribe/ solidus/ spree/

engine/spec/spec_helper.rb|48| 10:  config.shared_context_metadata_behavior = :apply_to_host_groups
rspec-rails/spec/spec_helper.rb|56| 10:  config.shared_context_metadata_behavior = :apply_to_host_groups
lobsters/spec/spec_helper.rb|45| 10:  config.shared_context_metadata_behavior = :apply_to_host_groups
rubytoolbox/spec/spec_helper.rb|49| 10:  config.shared_context_metadata_behavior = :apply_to_host_groups
forem/spec/spec_helper.rb|58| 10:  config.shared_context_metadata_behavior = :apply_to_host_groups
gitlabhq/qa/spec/spec_helper.rb|56| 10:  config.shared_context_metadata_behavior = :apply_to_host_groups
chatwoot/spec/spec_helper.rb|16| 10:  config.shared_context_metadata_behavior = :apply_to_host_groups

And one in lib:

capybara/lib/capybara/spec/spec_helper.rb|19| 16:        config.shared_context_metadata_behavior = :apply_to_host_groups

Others, since they don't have this setting and the default is :trigger_inclusion, either don't use shared example groups metadata, or rely on triggering inclusion.

Let's take a look at usages. Open this spoiler to see **all** 40 shared groups/contexts with metadata (out of ~3000 total shared groups)

rspec-core/spec/rspec/core/metadata_spec.rb|317| 42:              RSpec.shared_examples_for("some shared behavior", :include_it => true) do
puppet/spec/shared_contexts/digests.rb|16| 1:shared_context('with supported digest algorithms', :uses_checksums => true) do
puppet/spec/shared_contexts/digests.rb|27| 1:shared_context("when digest_algorithm is set to sha256", :digest_algorithm => 'sha256') do
puppet/spec/shared_contexts/digests.rb|42| 1:shared_context("when digest_algorithm is set to md5", :digest_algorithm => 'md5') do
puppet/spec/shared_contexts/digests.rb|57| 1:shared_context("when digest_algorithm is set to sha512", :digest_algorithm => 'sha512') do
puppet/spec/shared_contexts/digests.rb|72| 1:shared_context("when digest_algorithm is set to sha384", :digest_algorithm => 'sha384') do
puppet/spec/shared_contexts/digests.rb|87| 1:shared_context("when digest_algorithm is set to sha224", :digest_algorithm => 'sha224') do
diaspora/spec/support/gon.rb|3| 1:shared_context :gon do
diaspora/spec/spec_helper.rb|163| 1:shared_context suppress_csrf_verification: :none do
rubocop/lib/rubocop/rspec/shared_contexts.rb|5| 7:RSpec.shared_context 'isolated environment', :isolated_environment do
rubocop/lib/rubocop/rspec/shared_contexts.rb|43| 7:RSpec.shared_context 'maintain registry', :restore_registry do
rubocop/lib/rubocop/rspec/shared_contexts.rb|56| 7:RSpec.shared_context 'config', :config do # rubocop:disable Metrics/BlockLength
rubocop/lib/rubocop/rspec/shared_contexts.rb|114| 7:RSpec.shared_context 'mock console output' do
rubocop/lib/rubocop/rspec/shared_contexts.rb|126| 7:RSpec.shared_context 'ruby 2.4', :ruby24 do
rubocop/lib/rubocop/rspec/shared_contexts.rb|130| 7:RSpec.shared_context 'ruby 2.5', :ruby25 do
rubocop/lib/rubocop/rspec/shared_contexts.rb|134| 7:RSpec.shared_context 'ruby 2.6', :ruby26 do
rubocop/lib/rubocop/rspec/shared_contexts.rb|138| 7:RSpec.shared_context 'ruby 2.7', :ruby27 do
rubocop/lib/rubocop/rspec/shared_contexts.rb|142| 7:RSpec.shared_context 'ruby 3.0', :ruby30 do
brew/Library/Homebrew/test/support/helper/spec/shared_context/homebrew_cask.rb|34| 7:RSpec.shared_context "Homebrew Cask", :needs_macos do
chef/spec/support/shared/functional/securable_resource.rb|78| 1:shared_context "use Windows permissions", :windows_only do
canvas-lms/spec/lib/turnitin/turnitin_spec_helper.rb|22| 7:RSpec.shared_context "shared_tii_lti", :shared_context => :metadata do
canvas-lms/spec/lib/turnitin/turnitin_spec_helper.rb|22| 41:RSpec.shared_context "shared_tii_lti", :shared_context => :metadata do
canvas-lms/spec/lti2_course_spec_helper.rb|22| 7:RSpec.shared_context "lti2_course_spec_helper", :shared_context => :metadata do
canvas-lms/spec/lti2_course_spec_helper.rb|22| 50:RSpec.shared_context "lti2_course_spec_helper", :shared_context => :metadata do
canvas-lms/spec/plagiarism_platform_spec_helper.rb|22| 7:RSpec.shared_context "plagiarism_platform", :shared_context => :metadata do
canvas-lms/spec/plagiarism_platform_spec_helper.rb|22| 46:RSpec.shared_context "plagiarism_platform", :shared_context => :metadata do
canvas-lms/spec/lti2_spec_helper.rb|22| 7:RSpec.shared_context "lti2_spec_helper", :shared_context => :metadata do
canvas-lms/spec/lti2_spec_helper.rb|22| 43:RSpec.shared_context "lti2_spec_helper", :shared_context => :metadata do
canvas-lms/spec/lti_1_3_tool_configuration_spec_helper.rb|22| 7:RSpec.shared_context "lti_1_3_tool_configuration_spec_helper", shared_context: :metadata do
canvas-lms/spec/lti_1_3_tool_configuration_spec_helper.rb|22| 64:RSpec.shared_context "lti_1_3_tool_configuration_spec_helper", shared_context: :metadata do
canvas-lms/spec/lti_1_3_spec_helper.rb|23| 7:RSpec.shared_context "lti_1_3_spec_helper", shared_context: :metadata do
canvas-lms/spec/lti_1_3_spec_helper.rb|23| 45:RSpec.shared_context "lti_1_3_spec_helper", shared_context: :metadata do
canvas-lms/spec/apis/lti/lti2_api_spec_helper.rb|24| 7:RSpec.shared_context "lti2_api_spec_helper", :shared_context => :metadata do
canvas-lms/spec/apis/lti/lti2_api_spec_helper.rb|24| 47:RSpec.shared_context "lti2_api_spec_helper", :shared_context => :metadata do
gitlabhq/spec/lib/gitlab/git/merge_base_spec.rb|11| 3:  shared_context 'existing refs with a merge base', :existing_refs do
gitlabhq/spec/lib/gitlab/git/merge_base_spec.rb|17| 3:  shared_context 'when passing a missing ref', :missing_ref do
gitlabhq/spec/lib/gitlab/git/merge_base_spec.rb|23| 3:  shared_context 'when passing refs that do not have a common ancestor', :no_common_ancestor do
gitlabhq/spec/lib/gitlab/ci/config/entry/retry_spec.rb|8| 3:  shared_context 'when retry value is a numeric', :numeric do
gitlabhq/spec/lib/gitlab/ci/config/entry/retry_spec.rb|13| 3:  shared_context 'when retry value is a hash', :hash do
rspec-expectations/spec/spec_helper.rb|72| 7:RSpec.shared_context "with #should enabled", :uses_should do
rspec-expectations/spec/spec_helper.rb|99| 7:RSpec.shared_context "with #should exclusively enabled", :uses_only_should do
rspec-expectations/spec/spec_helper.rb|120| 7:RSpec.shared_context "with warn_about_potential_false_positives set to false", :warn_about_potential_false_positives do

If we make an intersection with the previous list (rspec-rails, lobsters, rubytoolbox, forem, gitlabhq, chatwoot), it turns out that no project uses :apply_to_host_groups.
gitlabhq might be a bit confusing, they actually have two spec helpers, gitlabhq/qa/spec/spec_helper.rb and gitlabhq/spec/spec_helper.rb.
And they do use :trigger_inclusion:

  shared_context 'existing refs with a merge base', :existing_refs do
    let(:refs) do
      %w(304d257dcb821665ab5110318fc58a007bd104ed 0031876facac3f2b2702a0e53a26e89939a42209)
    end
  end

  describe '#sha' do
    context 'when the refs exist', :existing_refs do

in their specs.

We use it, too. rspec-expectations:

RSpec.shared_context "with warn_about_potential_false_positives set to false", :warn_about_potential_false_positives do
  original_value = RSpec::Expectations.configuration.warn_about_potential_false_positives?

  after(:context)  { RSpec::Expectations.configuration.warn_about_potential_false_positives = original_value }
end

Preliminary conclusion: those popular Ruby projects that use RSpec who configured shared_context_metadata_behavior to :apply_to_host_groups did this blindlessly and never used it.

Semantic

We have two ways of inclusing shared groups. include_context/include_examples and it_behaves_like. The latter creates a nested group.

It doesn't make much sense to apply metadata that is defined for an implicitly created nested group.

On the other hand, just like with several lets defined in different included contexts, metadata, if applied from different included contexts, has a chance to override one another, and we don't print a warning for this case, leaving space for confusion.

If we happen to remove :trigger_inclusion, what would we recommend to replace it? E.g. for globally-defined shared groups:

For globally-defined shared groups this works:

RSpec.shared_examples 'it is odd' do
  it { is_expected.to be_odd }
end

RSpec.configure do |config|
  config.include_context 'it is odd', :odd
end

RSpec.describe do
  context 'odd', :odd do
    subject { 1 }
  end
end

However, why is it include_context?
Well, we only have include_context on our Configuration object. There's not include_examples or it_behaves_like there.

More common and contrived usage:

config.include_context "example guest user", :type => :request

But, what to promise in return for locally-defined shared groups?
Sometimes, it's quite unweildy to call include_context/it_behaves_like, and implicit inclusion via matching metadata is useful to make things dry. At a cost of a little magic. Which RSpec metadata is all about anyway.

Unpopular opinion

No doubt that this option has solved the issue with the inclusion of shared examples defined in a completely unrelated scope.

However, was there a good reason to "apply metadata to host group"? Is it used? Is it useful? Isn't it confusing?

Proposal

I suggest:

• keep shared_context_metadata_behavior option along with its default :trigger_inclusion value
• remove notions of deprecations

... release 4.0

• fix the problem with incorrect inclusions (proof of concept)
• change the project initializer, remove deprecation note, and use :trigger_inclusion as the default
• deprecate :apply_to_host_groups

If we keep :trigger_inclusion the default, the behaviour won't change for the majority. Less tickets.

Doubts

It may require a significant overhaul to fix the inclusion, up to the point where it makes it barely possible.
I still hope the reality won't shatter my youthful overly-optimistic dreams, and it's doable.

[pirj]

I'd love to summon @myronmarston as the driving force behind :apply_to_host_groups.

[myronmarston]

Thanks for writing this up @pirj and summoning me to comment :).

I still think that implicit inclusion of shared example groups via metadata still has problems. #1790 outlines the problems I saw with it when I wrote up that issue. Overall, the implicit inclusion behavior feels inconsistent with the rest of RSpec, IMO; consider that all other ways that RSpec metadata is leveraged you configure it in an RSpec.configure block. Having 1 RSpec feature use metadata in an implicit way (without requiring users to explicitly configure it in RSpec.configure) is something that I expect to be confusing to users who have used RSpec metadata but are not aware of the implicit inclusion behavior. Both rspec-rails#1579 and rspec-rails#1241 are examples of this: users tagged their shared example groups with metadata, expecting it to act like metadata on a normal group acts (e.g. triggering inclusion of config.include'd modules, being available for filtering, etc) and were surprised to see that's not how it behaved. In fact, they were so surprised by it that they reported it as a bug even though it was working as designed. That suggests to me that it was a poor design to begin with: if it truly aligned with the rest of RSpec it wouldn't lead users to report it as a bug.

As a sandbox for new rubocop-rspec cops, I've assembled a list of most starred Ruby projects that use RSpec, real-world-rspec, ~35 projects total. Out of those 35 (it includes RSpec repos, too!), 7 use :apply_to_host_groups in their spec helpers:

The exact semantic of tagging a shared example group with metadata is something that I'd expect 99% of RSpec users to never think about and the confusing situation the newer behavior solves are relatively rare....so I'm not surprised by that at all. TBH, I suspect :apply_to_host_groups is probably primarily used by projects that generated (or re-generated) their spec_helper.rb after we changed the generated spec_helper.rb config code in 3589ab5.

For users who want to trigger shared example group inclusion based on metadata, the config.include_context API is provided which, IMO, is simpler, more consistent, and more explicit that doing it automatically for users because they tagged a shared group and a normal group with the same metadata. AFAIK, there's nothing that :trigger_inclusion provides that can't be accomplished by users using config.include_context explicitly. (If there is something that can't be accomplished that way, please let me know!). OTOH, if a user wants to tag their shared example group with some metadata, and apply it everywhere the shared group is included...I don't think there's an alternate mechanism for this. (Actually, maybe config.define_derived_metadata could be used to simulate this, but IMO that feels like a "work around" for a lacking feature).

Personally, I've never seen :apply_to_host_groups being used the way it was designed for.

Here's how I've used it:

• I've temporarily tagged a shared example group with :focus, :pending, or :skipped so that I can focus on (or filter out) specs that depend on a specific shared example group. A shared example group generally means there is some cross-cutting concern that applies to all including groups, and being able to run just those specs (or exclude those specs) can be quite useful. It's worth noting that this usage of the feature would not generally show up in the committed code for any projects, because :focus, :skip and :pending are usually temporary changes that are not committed (particularly :focus).
• It can be quite useful as a way to express dependencies on other test harness code. For example, consider a project that conditionally wraps examples in a DB transaction when they are tagged with :db. (This could be accomplished by defining a DB support shared example group with an appropriate around hook, and then use config.include_context "DB support", :db). Now imagine that the developers want to define a new shared example group named logged in as admin that defines a before hook that creates an admin user in the database and logs in as that user using rack-test or whatever. Since this new shared example group interacts with the database in a before hook, it has a dependency on the DB support shared example group. IMO, the cleanest way to express this dependency is to tag the logged in as admin shared example group with :db so that the DB support is automatically applied when logged in as an admin is included in a host group. You could do include_context 'DB support' in the logged in as an admin example group to apply the DB transactions automatically, but if the "normal" way you manage that everywhere else is through a :db tag it's weird to not be able to do that here.

We use it, too. rspec-expectations:

LOL :). I'm not too surprised by that; we haven't historically upgraded RSpec to all of the latest "standard" config settings, etc, particularly if the issues the new config settings address don't happen within RSpec's tests. It's easy enough to change the rspec-expectations metadata-triggered inclusion to use config.include_context, right?

Preliminary conclusion: those popular Ruby projects that use RSpec who configured shared_context_metadata_behavior to :apply_to_host_groups did this blindlessly and never used it.

I'd honestly be surprised if there are more than a handful of users who have thought about how exactly they want shared example group metadata to behave, to then go and set the config option to the behavior they want. I imagine nearly all usages of :apply_to_host_groups in config is due to the generation of it via rspec --init.

I don't think that's necessarily an argument against :apply_to_host_groups, though; as I see it, half of the benefit to :apply_to_host_groups is that it disables the counterintuitive, implicit legacy auto-inclusion behavior. The other half of the benefit is that it enables a new behavior that is more in keeping (IMO, YMMV, of course) with the overall design of RSpec. A project that configures :apply_to_host_groups and then never actually tags shared groups with metadata can still be benefiting from the option; there's still potentially confusing behavior that they are avoiding. For example, if a developer temporarily tags a shared group with :focus, intending to focus it...it's beneficial that RSpec isn't instead including the shared group in other groups that are being :focused on.

It doesn't make much sense to apply metadata that is defined for an implicitly created nested group.

I think :apply_to_host_groups still provides beneficial behavior for this case, even though you're right that applying the metadata to the host group doesn't really apply per-se here. Consider the case of a group of shared examples that all depend on the database (in a project that tags such examples with :db to wrap them in DB transactions). It's useful for users to be able to tag their shared example group with :db to express the dependency on the database (and to automatically wrap all the contained examples in DB transactions). While it_behaves_like will not merge the :db metadata into the metadata of a host group, it's still nice to be able to tag the shared group with :db and have it applied to the examples in it. The legacy :trigger_inclusion behavior precludes this possibility because when you do shared_examples_for "common metadata support", :db, the :db tag is used to auto-include this shared group in other :db-tagged groups.

This is basically the crux of why I think :apply_to_host_groups is preferable: we have a way to include shared example groups based on metadata using config.include_context (which nicely mirrors config.include for modules...), but if we implicitly use shared group metadata to trigger inclusion, it prevents users from being able to tag their shared examples with common metadata at the group level. And if we use shared group metadata in that fashion, then IMO the natural extension of that for include_context/include_examples (when the inclusion isn't nested) is to merge the metadata w/ the host group's metadata. (But YMMV; that's just my mental model of how RSpec metadata works).

However, why is it include_context? Well, we only have include_context on our Configuration object. There's not include_examples or it_behaves_like there.

That's by design. IMO, it would be surprising (and weird) to automatically include some shared examples in many example groups based on contexts. A user could look at the spec file, see 4 it blocks (suggesting running the file will run 4 examples), run it, and be surprised to see 20 examples run (e.g. due to 16 shared example being included). IMO, examples are a primary thing that must be seen and understood to understand how the test suite is structured, and how it works. Shared test harness code (e.g. helper methods, lets, or an around hook that manages a DB transaction...) does not have the same need to be visible. In fact, I tend to use shared example groups (and include_context) specifically to hide unimportant details from adding noise to tons of specs. For example, for a cross-cutting concern like log capture, or DB transactions, I don't want each test that has those concerns to have the noise of visible code to manage those things. Being able to tag the group with :db or :capture_logs and letting config.include_context apply the shared code for me, results in specs with higher signal-to-noise ratio. It's not the same with include_examples/it_behaves_like, which are not intended to be used for the same sorts of common cross-cutting concerns that are nice to handle automatically.

Anyhow, that's my two cents :). Since I'm no longer involved in maintaining RSpec, do not feel obligated to give my opinion undue weight.

[pirj]

Thanks a lot for such a detailed and insightful answer, @myronmarston!

it's beneficial that RSpec isn't instead including the shared group in other groups that are being :focused on

That indeed would be an apotheosis of confusion.

My doubts are dispelled, I'm going to proceed with making :apply_to_host_groups the default and only option.
I'll keep the ticket open, will link PRs to it.

[myronmarston]

My doubts are dispelled, I'm going to proceed with making :apply_to_host_groups the default and only option.

It might be worth removing the config option entirely, given the plan is to not support any other behavior. Or maybe for backwards compatibility it could be defined but ignored (although that could be confusing if the user configured the legacy behavior...). I don't know if y'all are planning on releasing an RSpec 3.99 (like we did with 2.99) to provide upgrade warnings, but if you go that route 3.99 could retain the option and warn appropriately and 4.0 could not have the option at all perhaps.

[JonRowe]

Thanks for the detailed reply @myronmarston 😂 I'm closing this because I agree, and judging from Phil's PRs he has been convinced also.