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

Consider changing the semantics of shared example group metadata #1790

Closed
myronmarston opened this issue Nov 24, 2014 · 5 comments
Closed

Consider changing the semantics of shared example group metadata #1790

myronmarston opened this issue Nov 24, 2014 · 5 comments
Milestone

Comments

@myronmarston
Copy link
Member

Currently, you can tag a shared example group definition with some metadata:

RSpec.shared_context "DB support", :db do
  # ...
end

...and that will cause the shared example group to be automatically included in matching example groups:

RSpec.describe MyModelThatUsesTheDB, :db do
  # ...
end

This works fine (and was originally my idea, IIRC), but I've realized a few problems with it:

  • The first argument to shared_context (the shared group name) is superfluous. It feels a bit like "what's this argument for again?" (Note that you could still use it with include_context to include the group manually, but it's a bit odd to mix-and-match the approaches).
  • Some users have expressed surprised that the metadata is treated "special" here and isn't simply applied to the shared example group like it is applied to a normal example group.
  • It makes it impossible to attach some metadata to a shared example group that will be automatically applied to including example groups. While no user has asked for this, I've occasionally wanted to temporarily add :focus, :skip or :pending metadata to a shared example group so that that behavior applies to all inclusions of the shared group.
  • There's no obvious way to make a shared example group get auto-included in every example group (e.g. for a global before hook or let you want to make available everywhere...). You could do something like RSpec.shared_context "...", :description => //, since // will match the description of every example group, but that's pretty indirect and non-obvious.
  • It doesn't gel well with the scoping behavior of shared contexts (see Auto-included shared contexts are not subject to the same scoping rules that manually included ones are #1762).
  • It's inconsistent with how module inclusion works (e.g. config.include mod, *metadata).

I think we can rectify all of these with a few simple changes:

  • Add a new config.include_context API that is similar to config.include but is for shared contexts. This would solve the "no obvious way to auto-include a shared group everywhere" issue mentioned above and provide a sister API to config.include for consistency.
  • Stop using the metadata passed to shared_context as a means to determine which example groups should have the context auto-included, instead allowing it the metadata to get applied to including example groups.

The first change is a new API and could be made in any 3.x release. The latter is a breaking change and either has to wait until RSpec 4 or we have to add a config option. I lean towards the latter -- something like config.use_shared_example_group_metadata_for_auto_inclusion = true (although that is a bit ambiguous -- the config.include_context API would still use it for auto-inclusion -- maybe someone has a better idea?)

Thoughts?

/cc @rspec/rspec

@myronmarston
Copy link
Member Author

One other thing -- if we did this, I don't think we'd have to do anything to address #1762 as this would take care of that issue as well. config.include_context "shared group name" would obviously validate that "shared group name" is a top-level shared example group that can be included anywhere.

@JonRowe
Copy link
Member

JonRowe commented Nov 25, 2014

  • Add a new config.include_context API that is similar to config.include but is for shared contexts. This would solve the "no obvious way to auto-include a shared group everywhere" issue mentioned above and provide a sister API to config.include for consistency.

I like this idea, I use config.include stuff a lot and this would just be one more nicety in defining it.

  • Stop using the metadata passed to shared_context as a means to determine which example groups should have the context auto-included, instead allowing it the metadata to get applied to including example groups.

This is a breaking change and either has to wait until RSpec 4 or we have to add a config option. I lean towards the latter -- something like config.use_shared_example_group_metadata_for_auto_inclusion = true (although that is a bit ambiguous -- the config.include_context API would still use it for auto-inclusion -- maybe someone has a better idea?)

I'm fine with this, it's similar to how we changed the metadata symbol means true stuff in RSpec 2 -> 3. How about make the config option a "behaviour" rather than a on / off flag? config.shared_context_metadata_behaviour = :included_in_examples or config.shared_context_metadata_behaviour = :apply_to_examples

@myronmarston
Copy link
Member Author

One update to this idea: we could consider making config.include support shared group names in addition to module references, rather than introducing a new config.include_context API.

@xaviershay
Copy link
Member

One update to this idea: we could consider making config.include support shared group names in addition to module references, rather than introducing a new config.include_context API.

Then it's switching behaviour based on type of the argument? I'm slightly negative on that.

This is a breaking change and either has to wait until RSpec 4 or we have to add a config option.

I lean towards the former. This doesn't seem important enough to justify introducing more config, RSpec 4 is in theory not that far away (... any reason we shouldn't just call master 4 and start working on it?), and there would be a way to transition to the new behaviour with include_context.

@myronmarston
Copy link
Member Author

Then it's switching behaviour based on type of the argument? I'm slightly negative on that.

I wouldn't consider it switching behavior. Ultimately it's including a module -- either a manually defined and referenced one from the user, or a RSpec::Core::SharedExampleGroupModule created for the user when they defined the shared example group and referenced via the group name.

myronmarston added a commit that referenced this issue Jun 4, 2016
This simplifies things, makes things more consistent,
and prepares us to be able to better handle metadata
as per discussion in #1790.
myronmarston added a commit that referenced this issue Jun 5, 2016
- Takes care of preserving metadata inheritance.
- Takes care of re-applying filtered config items
  like module inclusions and hooks.

This is necessary for #1790.
myronmarston added a commit that referenced this issue Jun 5, 2016
Previously, it always triggered auto-inclusion based on
matching metadata. The option allows you to opt-in to having
it add the metadata to included groups and examples instead.

- Closes #1790 (this is the last thing necessary for it).
- Addresses #1762.
- Addresses user confusion reported in:
  - rspec/rspec-rails#1241
  - rspec/rspec-rails#1579
myronmarston added a commit that referenced this issue Jun 5, 2016
Previously, it always triggered auto-inclusion based on
matching metadata. The option allows you to opt-in to having
it add the metadata to included groups and examples instead.

- Closes #1790 (this is the last thing necessary for it).
- Addresses #1762.
- Addresses user confusion reported in:
  - rspec/rspec-rails#1241
  - rspec/rspec-rails#1579
myronmarston added a commit that referenced this issue Jun 5, 2016
Previously, it always triggered auto-inclusion based on
matching metadata. The option allows you to opt-in to having
it add the metadata to included groups and examples instead.

- Closes #1790 (this is the last thing necessary for it).
- Addresses #1762.
- Addresses user confusion reported in:
  - rspec/rspec-rails#1241
  - rspec/rspec-rails#1579
myronmarston added a commit that referenced this issue Jun 5, 2016
Previously, it always triggered auto-inclusion based on
matching metadata. The option allows you to opt-in to having
it add the metadata to included groups and examples instead.

- Closes #1790 (this is the last thing necessary for it).
- Addresses #1762.
- Addresses user confusion reported in:
  - rspec/rspec-rails#1241
  - rspec/rspec-rails#1579
myronmarston added a commit that referenced this issue Jun 5, 2016
Previously, it always triggered auto-inclusion based on
matching metadata. The option allows you to opt-in to having
it add the metadata to included groups and examples instead.

- Closes #1790 (this is the last thing necessary for it).
- Addresses #1762.
- Addresses user confusion reported in:
  - rspec/rspec-rails#1241
  - rspec/rspec-rails#1579
myronmarston added a commit that referenced this issue Jun 5, 2016
- Takes care of preserving metadata inheritance.
- Takes care of re-applying filtered config items
  like module inclusions and hooks.

This is necessary for #1790.
MatheusRich pushed a commit to MatheusRich/rspec-core that referenced this issue Oct 30, 2020
This simplifies things, makes things more consistent,
and prepares us to be able to better handle metadata
as per discussion in rspec#1790.
MatheusRich pushed a commit to MatheusRich/rspec-core that referenced this issue Oct 30, 2020
- Takes care of preserving metadata inheritance.
- Takes care of re-applying filtered config items
  like module inclusions and hooks.

This is necessary for rspec#1790.
MatheusRich pushed a commit to MatheusRich/rspec-core that referenced this issue Oct 30, 2020
Previously, it always triggered auto-inclusion based on
matching metadata. The option allows you to opt-in to having
it add the metadata to included groups and examples instead.

- Closes rspec#1790 (this is the last thing necessary for it).
- Addresses rspec#1762.
- Addresses user confusion reported in:
  - rspec/rspec-rails#1241
  - rspec/rspec-rails#1579
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

No branches or pull requests

3 participants