Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Allow shared_examples to be defined per ExampleGroup. #818

Merged
merged 17 commits into from May 15, 2013

Conversation

Projects
None yet
5 participants
Owner

JonRowe commented Mar 7, 2013

This is an attempt at implementing #792. It allows shared_examples to be defined per ExampleGroup in order for keys to be safely reused.

It was more challengeing than it appeared due to the need to:

  1. Inherit examples from outer context groups
  2. Allow examples to be defined in main!

The issues I can see with this attemp are:

Extra methods being defined on main and thus Object.
Having to refactor the specs to make it pass, as the specs often defined things within the context of a test, but used them on a different context (an example group normally), the old global behaviour allowed this but the new context specific behaviour didn't.

Some ideas I could refactor towards:
Try to pull the examples into a hash lookup in Registry
Redefine how these are mixed into main so that examplegroups has different behaviour to main

There is probably stuff I've not thought of to do with RSpec's nested context class system thing...

Thoughts?

@myronmarston myronmarston commented on an outdated diff Apr 7, 2013

features/example_groups/shared_examples.feature
@@ -220,3 +220,39 @@ Feature: shared examples
"""
1 example, 0 failures
"""
+ Scenario: Shared examples are specific to their context
+ Given a file named "context_specific_examples_spec.rb" with:
+ """
+ describe do
+ context "my context" do
+ shared_examples "is independant" do
+ specify { expect(subject).to eq "context" }
+ end
+
+ subject { "context" }
@myronmarston

myronmarston Apr 7, 2013

Owner

The way you've demonstrated independence here (and in the context below) is a little convoluted, as it requires understanding the interplay between subject and the shared group. I think a clearer way to demonstrate it is to define different examples with different doc strings in the two shared example groups -- and then run the specs with --format doc and verify that the documentation output is different as expected.

Thoughts?

@myronmarston myronmarston commented on an outdated diff Apr 7, 2013

features/example_groups/shared_examples.feature
@@ -220,3 +220,39 @@ Feature: shared examples
"""
1 example, 0 failures
"""
+ Scenario: Shared examples are specific to their context
+ Given a file named "context_specific_examples_spec.rb" with:
+ """
+ describe do
+ context "my context" do
+ shared_examples "is independant" do
+ specify { expect(subject).to eq "context" }
+ end
+
+ subject { "context" }
+
+ it_should_behave_like "is independant"
@myronmarston

myronmarston Apr 7, 2013

Owner

FWIW, I prefer it_behaves_like "is independent" over it_should_behave_like...thoughts?

@myronmarston myronmarston and 1 other commented on an outdated diff Apr 7, 2013

features/example_groups/shared_examples.feature
+ end
+ context "another context" do
+ shared_examples "is independant" do
+ specify { expect(subject).to eq "another context" }
+ end
+
+ subject { "another context" }
+
+ it_should_behave_like "is independant"
+ end
+ context do
+ begin
+ it_should_behave_like "is independant"
+ fail "Shared examples should be dependant on context"
+ rescue ArgumentError
+ end
@myronmarston

myronmarston Apr 7, 2013

Owner

This is kinda hard to follow. (And I'm not even sure if fail is available in the example group context...but of course the point here is that it doesn't get to that line). Instead, what do you think about doing this?

  • Put this context in a separate file.
  • Have two When...Then sections for this scenario:
    • The first will run the file containing the two contexts that define and use an example groups
    • The second will run the file containing the context that (wrongly) uses the context even though one isn't available. It can verify that it fails with an appropriate error.
@JonRowe

JonRowe Apr 7, 2013

Owner

FYI fail is a method of Kernel, it's available everywhere, fail 'message' is the same as doing raise 'message'

I'll look at separating this out into a separate scenario though.

@myronmarston myronmarston and 1 other commented on an outdated diff Apr 7, 2013

lib/rspec/core/shared_example_group.rb
@@ -29,7 +29,8 @@ module SharedExampleGroup
# @see ExampleGroup.include_examples
# @see ExampleGroup.include_context
def shared_examples *args, &block
- Registry.add_group(*args, &block)
+ context = self.is_a?(Class) ? self : self.class
@myronmarston

myronmarston Apr 7, 2013

Owner

I don't understand this ternary conditional -- can you explain it?

@JonRowe

JonRowe Apr 7, 2013

Owner

If you define a shared example group in the main object (e.g. in support files or at the top of spec files) then self is an instance of object, otherwise self is a class definition (which is how the nested describe/context works in general). This is normalising the context to be a class.

@myronmarston

myronmarston Apr 7, 2013

Owner

That makes sense, but I think it would be easier to follow the logic if main.shared_examples was defined slightly differently from RSpec::Core::ExampleGroup#shared_examples to take care of that case. That way, it's explicit which case is needed from which context (since the logic for that case is explicitly defined for that context). Does that make sense?

@myronmarston myronmarston commented on an outdated diff Apr 7, 2013

lib/rspec/core/shared_example_group.rb
@@ -29,7 +29,8 @@ module SharedExampleGroup
# @see ExampleGroup.include_examples
# @see ExampleGroup.include_context
def shared_examples *args, &block
- Registry.add_group(*args, &block)
+ context = self.is_a?(Class) ? self : self.class
+ Registry.add_group(context,*args, &block)
@myronmarston

myronmarston Apr 7, 2013

Owner

As I usually comment -- spaces between args here, please :).

@myronmarston myronmarston commented on an outdated diff Apr 7, 2013

lib/rspec/core/shared_example_group.rb
@@ -40,7 +41,15 @@ def shared_examples *args, &block
def share_as(name, &block)
RSpec.deprecate("Rspec::Core::SharedExampleGroup#share_as",
"RSpec::SharedContext or shared_examples")
- Registry.add_const(name, &block)
+ context = self.is_a?(Class) ? self : self.class
+ Registry.add_const(context,name, &block)
+ end
+
+ def shared_example_groups
+ ancestors[1..-1].inject(my_shared_example_groups) { |mine,other| mine.merge other.shared_example_groups }
+ end
+ def my_shared_example_groups
@myronmarston

myronmarston Apr 7, 2013

Owner

I like having blank lines between method definitions -- thoughts?

Owner

myronmarston commented Apr 7, 2013

@JonRowe -- thanks for taking a stab at this (and sorry for taking so long to review it!). I left some comments on the diff itself but I'm realizing there's a deeper issue we need to discuss first: I believe this is a breaking change as it currently exists. There's a common pattern that goes like this:

# spec/support/shared_examples/something.rb
shared_examples_for "something" do
end
# spec/unit/my_class_spec.rb
require 'support/shared_examples/something'

describe MyClass do
  it_behaves_like "something"
end

Unless I'm reading the diff wrong, it looks like this pattern will no longer work. Here's what I think we want:

  • If a shared example group is defined at the top level, it is available anywhere.
  • If a shared example group is defined within an example group, it is only available in that example group and child groups.

Note, however, that even this behavior would be a breaking change from what we have now. Up to know, folks have been able to define a shared example group anywhere (including within a deeply nested example group), and use it from anywhere. So I think we'll have to hold off until 3.0 for this, and find a good way to print a deprecation for the existing behavior when it is used in this fashion in 2.99.

Thoughts?

Owner

JonRowe commented Apr 7, 2013

I rebased this and updated some of the styling, I'll work on a few of the other things soon.

Owner

JonRowe commented Apr 7, 2013

@myronmarston Apparently soon means now, I've rewritten the feature so it's better worded than before, does it make more sense now?

Top level examples are preserved here, basically at any point you can call shared_examples defined in your current context or any parent context including the top level (Object#main).

What is a breaking chance is calling shared examples cross context. I personally think that will be niche enough to deploy pre 3, but I'd accept waiting until 3. I have no idea how to nicely deprecate that scenario for 2.99 though... Basically have to branch off this code and add in the old cold so if this can't find the examples we warn and then use the old definition? Fugly but would work...

@myronmarston myronmarston commented on an outdated diff Apr 7, 2013

features/example_groups/shared_examples.feature
+ context do
+ shared_examples "shared examples are isolated" do
+ specify { expect(true).to eq true }
+ end
+ end
+
+ context do
+ it_behaves_like "shared examples are isolated"
+ end
+ end
+ """
+ When I run `rspec isolated_shared_examples_spec.rb`
+ Then the output should contain:
+ """
+ Could not find shared examples "shared examples are isolated"
+ """
@myronmarston

myronmarston Apr 7, 2013

Owner

This is much better -- thanks!

@myronmarston myronmarston commented on the diff Apr 7, 2013

spec/rspec/core/formatters/base_text_formatter_spec.rb
@@ -110,7 +110,7 @@ def run_all_and_dump_failures
context 'for #share_examples_for' do
it 'outputs the name and location' do
- share_examples_for 'foo bar' do
+ group.share_examples_for 'foo bar' do
@myronmarston

myronmarston Apr 7, 2013

Owner

Why the need to change from share_examples_for to group.share_examples_for here and elsewhere? It suggests that you've made this much more of a breaking change...

@JonRowe

JonRowe Apr 7, 2013

Owner

This is down purely to not allowing not cross context scopes. The shared examples must therefore be declared and run from the same scope, which due to the way these spec's are written (they deliberately declare themselves outside of the current scope) looks a bit odd, because the spec's themselves are a bit odd.

@myronmarston myronmarston commented on the diff Apr 7, 2013

spec/rspec/core/world_spec.rb
world.reset
expect(world.example_groups).to be_empty
- expect(world.shared_example_groups).to be_empty
@myronmarston

myronmarston Apr 7, 2013

Owner

Seems like this should still clear the shared example groups defined at the top level, right? But not clear the ones defined within an example group.

Owner

myronmarston commented Apr 7, 2013

What is a breaking chance is calling shared examples cross context. I personally think that will be niche enough to deploy pre 3, but I'd accept waiting until 3. I have no idea how to nicely deprecate that scenario for 2.99 though... Basically have to branch off this code and add in the old cold so if this can't find the examples we warn and then use the old definition? Fugly but would work...

After releasing 2.13 and seeing some of the things users do that we didn't expect at all (e.g. using a let or subject from within before(:all)) I'm a little paranoid about this. I think we can add the feature while retaining backwards compatibility if we do this:

  • When a shared group is defined within an example group, store it at the top-level scope as well, but do so with some state on it that indicates that it was defined from within an example group.
  • When a shared example group is used cross context, it'll look up the ancestors chain and find it at the top level scope.
  • When an example group is overridden we usually warn, but if it is being overridden at the top level, and was defined from within an example group (as indicated by the state I mentioned above), allow it to be overriden w/o a warning (since that would usually be OK, and we're only setting it at the top level for backwards compatibility).
  • In 2.99, we can actually use the extra bit of state indicating a top level shared group was defined an example group to issue a warning if/when it is ever used -- and then in 3.0 we can stop storing it at the top level scope.

Would that work, you think?

Owner

JonRowe commented Apr 7, 2013

Ok, well I've had some ideas' how this could be reworked, if we create a registry for shared examples, we can record where they were defined from without defining them on specific classes/instances. We can then set the rules of how they are allowed to be accessed and pass out deprecation messages accordingly. We could also potentially make this configurable, and just set a hardline default in 3.

Owner

JonRowe commented Apr 10, 2013

This warns about deprecation when accessing shared_examples across a context, but will default to the specific context first, it would be easy to remove the deprecation and cross context behaviour at a later date.

Owner

JonRowe commented May 14, 2013

Is this good to merge? /cc @myronmarston @soulcutter

Owner

myronmarston commented May 14, 2013

I know I still had some concerns before. I'll try to make time to review this tomorrow and make sure I don't have any more outstanding concerns. Thanks for pinging me and for your patience!

@myronmarston myronmarston commented on an outdated diff May 15, 2013

lib/rspec/core/shared_example_group.rb
@@ -71,7 +95,7 @@ def add_group(*args, &block)
end
end
- def add_const(name, &block)
+ def add_const(source,name, &block)
@myronmarston

myronmarston May 15, 2013

Owner

A space would be nice between args here :).

@myronmarston myronmarston commented on an outdated diff May 15, 2013

lib/rspec/core/shared_example_group.rb
@@ -92,11 +116,23 @@ def self.included(kls)
end
shared_const = Object.const_set(name, mod)
- RSpec.world.shared_example_groups[shared_const] = block
+ add_shared_example_group source, shared_const, block
+ end
+
+ def shared_example_groups_for *sources
@myronmarston

myronmarston May 15, 2013

Owner

I know it's "seattle style" to not use parens in method defs, and I live in Seattle...but I really prefer the more common style of using parens in method defs (except in the case of no args). It's nice to have consistency within rspec's code base here.

@myronmarston myronmarston commented on an outdated diff May 15, 2013

lib/rspec/core/shared_example_group.rb
end
private
+ def add_shared_example_group source, key, block
@myronmarston

myronmarston May 15, 2013

Owner

Ditto here...parens would be nice for consistency.

@myronmarston myronmarston commented on an outdated diff May 15, 2013

lib/rspec/core/shared_example_group.rb
@@ -105,8 +141,8 @@ def raise_name_error
raise NameError, "The first argument (#{name}) to share_as must be a legal name for a constant not already in use."
end
- def warn_if_key_taken key, new_block
- return unless existing_block = example_block_for(key)
+ def warn_if_key_taken source, key, new_block
@myronmarston

myronmarston May 15, 2013

Owner

Ditto here...parents would be nice for consistency.

@myronmarston myronmarston commented on an outdated diff May 15, 2013

lib/rspec/core/shared_example_group.rb
@@ -121,8 +157,8 @@ def formatted_location block
block.source_location.join ":"
end
- def example_block_for key
- RSpec.world.shared_example_groups[key]
+ def example_block_for source, key
@myronmarston

myronmarston May 15, 2013

Owner

Ditto here...parents would be nice for consistency.

@myronmarston myronmarston commented on an outdated diff May 15, 2013

lib/rspec/core/shared_example_group/collection.rb
@@ -0,0 +1,41 @@
+module RSpec
+ module Core
+ module SharedExampleGroup
+ class Collection
+
+ def initialize sources, examples
@myronmarston

myronmarston May 15, 2013

Owner

Ditto here...parents would be nice for consistency.

@myronmarston myronmarston commented on an outdated diff May 15, 2013

lib/rspec/core/shared_example_group/collection.rb
+module RSpec
+ module Core
+ module SharedExampleGroup
+ class Collection
+
+ def initialize sources, examples
+ @sources, @examples = sources, examples
+ end
+
+ def [] key
+ fetch_examples(key) || warn_deprecation_and_fetch_anyway(key)
+ end
+
+ private
+
+ def fetch_examples key
@myronmarston

myronmarston May 15, 2013

Owner

Ditto here...parents would be nice for consistency.

@myronmarston myronmarston commented on an outdated diff May 15, 2013

lib/rspec/core/shared_example_group/collection.rb
+
+ def initialize sources, examples
+ @sources, @examples = sources, examples
+ end
+
+ def [] key
+ fetch_examples(key) || warn_deprecation_and_fetch_anyway(key)
+ end
+
+ private
+
+ def fetch_examples key
+ @examples[source_for key][key]
+ end
+
+ def source_for key
@myronmarston

myronmarston May 15, 2013

Owner

Ditto here...parents would be nice for consistency.

@myronmarston myronmarston commented on an outdated diff May 15, 2013

lib/rspec/core/shared_example_group/collection.rb
+
+ def [] key
+ fetch_examples(key) || warn_deprecation_and_fetch_anyway(key)
+ end
+
+ private
+
+ def fetch_examples key
+ @examples[source_for key][key]
+ end
+
+ def source_for key
+ @sources.reverse.find { |source| @examples[source].has_key? key }
+ end
+
+ def fetch_anyway key
@myronmarston

myronmarston May 15, 2013

Owner

Ditto here...parents would be nice for consistency.

@myronmarston myronmarston commented on an outdated diff May 15, 2013

lib/rspec/core/shared_example_group/collection.rb
+
+ private
+
+ def fetch_examples key
+ @examples[source_for key][key]
+ end
+
+ def source_for key
+ @sources.reverse.find { |source| @examples[source].has_key? key }
+ end
+
+ def fetch_anyway key
+ @examples.values.inject({},&:merge)[key]
+ end
+
+ def warn_deprecation_and_fetch_anyway key
@myronmarston

myronmarston May 15, 2013

Owner

Ditto here...parents would be nice for consistency.

@myronmarston myronmarston commented on an outdated diff May 15, 2013

lib/rspec/core/shared_example_group/collection.rb
@@ -0,0 +1,41 @@
+module RSpec
+ module Core
+ module SharedExampleGroup
+ class Collection
+
+ def initialize sources, examples
+ @sources, @examples = sources, examples
+ end
+
+ def [] key
@myronmarston

myronmarston May 15, 2013

Owner

Ditto here...parents would be nice for consistency.

@myronmarston myronmarston commented on the diff May 15, 2013

spec/rspec/core/shared_example_group/collection_spec.rb
+ #
+ let(:examples) do
+ Hash.new { |hash,k| hash[k] = Hash.new }.tap do |hash|
+ hash["main"] = { "top level group" => example_1 }
+ hash["nested 1"] = { "nested level one" => example_2 }
+ hash["nested 2"] = { "nested level two" => example_3 }
+ end
+ end
+ (1..3).each { |num| let("example_#{num}") { double "example #{num}" } }
+
+ context 'setup with one source, which is the top level' do
+
+ let(:collection) { Collection.new ['main'], examples }
+
+ it 'fetches examples from the top level' do
+ expect(collection['top level group']).to eq example_1
@myronmarston

myronmarston May 15, 2013

Owner

I think that to eq example_1 issues a warning (when run with warnings on) but to eq(example_1) doesn't. I'd like to get rspec warning free and keep it that way.

(I could be wrong about the warning; feel free to ignore this if so).

@JonRowe

JonRowe May 15, 2013

Owner

Running VERBOSE=true be rspec spec/rspec/core/shared_example_group/collection_spec.rb results in no warnings at all on 1.9.3 or 2.0.0, Ruby should only raise a warning for a parentesis-less arguments like this if it's ambiguous, or at least that's my understanding...

@myronmarston myronmarston commented on an outdated diff May 15, 2013

lib/rspec/core/shared_example_group/collection.rb
+ end
+
+ def source_for key
+ @sources.reverse.find { |source| @examples[source].has_key? key }
+ end
+
+ def fetch_anyway key
+ @examples.values.inject({},&:merge)[key]
+ end
+
+ def warn_deprecation_and_fetch_anyway key
+ if (example = fetch_anyway key)
+ RSpec.warn_deprecation <<-WARNING.gsub(/^ +\|/, '')
+ Accessing shared_examples defined across contexts is deprecated.
+ Please declare shared_examples within a shared context, or at the top level
+ WARNING
@myronmarston

myronmarston May 15, 2013

Owner
  • I noticed you used the .gsub(/^ +\|/, '') pattern I've used elsewhere, but the point of that pattern is to be able to indent the heredoc text here based on the surrounding code, while not having it that indented when the warning is printed. If you're going to do the gsub thing you should put some leading pipes to delimit the left border of the warning paragraph.
  • A period at the end of the second sentence would be nice :).
  • I've found that it's really helpful if these sorts of warnings can include a "called from: ..." line indicating where the user triggered this. Without such a line, it's hard to track down the source of the warning.

@myronmarston myronmarston commented on an outdated diff May 15, 2013

lib/rspec/core/shared_example_group.rb
@@ -105,8 +141,8 @@ def raise_name_error
raise NameError, "The first argument (#{name}) to share_as must be a legal name for a constant not already in use."
end
- def warn_if_key_taken key, new_block
- return unless existing_block = example_block_for(key)
+ def warn_if_key_taken source, key, new_block
+ return unless existing_block = example_block_for(source,key)
@myronmarston

myronmarston May 15, 2013

Owner

source, key would be nice :).

@myronmarston

myronmarston May 15, 2013

Owner

(Sometimes I feel bad about being anal about spacing...but I do think it helps readability!)

@myronmarston myronmarston commented on an outdated diff May 15, 2013

lib/rspec/core/shared_example_group/collection.rb
+ def [] key
+ fetch_examples(key) || warn_deprecation_and_fetch_anyway(key)
+ end
+
+ private
+
+ def fetch_examples key
+ @examples[source_for key][key]
+ end
+
+ def source_for key
+ @sources.reverse.find { |source| @examples[source].has_key? key }
+ end
+
+ def fetch_anyway key
+ @examples.values.inject({},&:merge)[key]
@myronmarston

myronmarston May 15, 2013

Owner

Clever :).

Also, {}, &:merge would read a bit nicer.

@myronmarston myronmarston commented on the diff May 15, 2013

lib/rspec/core/world.rb
@@ -23,7 +22,6 @@ def initialize(configuration=RSpec.configuration)
def reset
example_groups.clear
- shared_example_groups.clear
@myronmarston

myronmarston May 15, 2013

Owner

Hmm...removing this clear seems like a potentially breaking change. Do you think it should still clear out the top-level shared groups?

@JonRowe

JonRowe May 15, 2013

Owner

Restored

@myronmarston myronmarston commented on the diff May 15, 2013

lib/rspec/core/shared_example_group.rb
@@ -40,7 +40,31 @@ def shared_examples *args, &block
def share_as(name, &block)
RSpec.deprecate("Rspec::Core::SharedExampleGroup#share_as",
"RSpec::SharedContext or shared_examples")
- Registry.add_const(name, &block)
+ Registry.add_const(self, name, &block)
+ end
+
+ def shared_example_groups
+ Registry.shared_example_groups_for('main', *ancestors[0..-1])
@myronmarston

myronmarston May 15, 2013

Owner

I was hunting around trying to see if this handles getting a shared group from a parent context w/o a warning (since I think it should)....does this cause that to happen by passing the ancestors? I didn't see a spec specifically for that case -- would be good to have one since it's an important behavior, I think (or did I miss it?).

@JonRowe

JonRowe May 15, 2013

Owner

I've added one into the feature file...

Owner

myronmarston commented May 15, 2013

@JonRowe -- this is very well done. Nice work!

I did have some open questions above. I don't want to block this from being merged, though, so I'll trust your judgement on whether or not my questions above really need to be addressed or not. I'm just a drive by commenter on this one :).

Owner

myronmarston commented May 15, 2013

Oh yeah....a changelog entry would be nice as well :).

Owner

JonRowe commented May 15, 2013

@myronmarston I've made some changes as per your suggestions, when Travis comes back I'll merge this unless you object ;)

@samphippen samphippen commented on the diff May 15, 2013

lib/rspec/core/shared_example_group/collection.rb
+
+ def fetch_anyway(key)
+ @examples.values.inject({}, &:merge)[key]
+ end
+
+ def warn_deprecation_and_fetch_anyway(key)
+ if (example = fetch_anyway key)
+ RSpec.warn_deprecation <<-WARNING.gsub(/^ /, '')
+ Accessing shared_examples defined across contexts is deprecated.
+ Please declare shared_examples within a shared context, or at the top level.
+ This message was generated at: #{caller(0)[5 ]}
+ WARNING
+ example
+ end
+ end
+
@samphippen

samphippen May 15, 2013

Owner

@JonRowe there's a random line gap here, should we get rid of it? all the other ends are right next to each other in this file.

@JonRowe

JonRowe May 15, 2013

Owner

It's not random, it's separating the class/module definition from the methods, It looks weird to me without it, especially as these are private methods and thus indented inwards a level... But I'll remove it if I'm overruled...

@samphippen

samphippen May 15, 2013

Owner

I think it's totally fine if there's a reason. It just looked weird to me whilst scrolling.

@JonRowe JonRowe added a commit that referenced this pull request May 15, 2013

@JonRowe JonRowe Merge pull request #818 from JonRowe/context_aware_shared_examples
Allow shared_examples to be defined per ExampleGroup.
769e4d2

@JonRowe JonRowe merged commit 769e4d2 into rspec:master May 15, 2013

1 check passed

default The Travis CI build passed
Details

It doesn't look like this line is used and causes a warning when running rspec-core's tests. Is this an intentional addition?

Owner

JonRowe replied May 18, 2013

I believe so... Removed...

This implementation assumes that @shared_example_groups has already been initialized, whereas it might still be nil. I would suggest using the getter instead of directly accessing the instance variable, so the line should become like this:

def clear
  shared_example_groups.clear
end

This would ensure correct behavior regardless of state.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment