yield current running example to it/example and before/after hooks #666

Merged
merged 7 commits into from Jun 26, 2013

Conversation

Projects
None yet
8 participants
@dchelimsky
Member

dchelimsky commented Aug 27, 2012

Let's yield current running example to it/example and before/after hooks instead of exposing it via the implicit example getter.

@myronmarston, @alindeman comments, please

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Aug 27, 2012

This pull request passes (merged 478a8e4 into 4f101b3).

This pull request passes (merged 478a8e4 into 4f101b3).

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Aug 27, 2012

This pull request fails (merged 0861172 into 4f101b3).

This pull request fails (merged 0861172 into 4f101b3).

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Aug 27, 2012

Member

Interesting idea. I assume this is in response to #663?

I like the fact that this creates fewer potential naming conflicts (especially since example could reasonably be used as a method name by end users), but I do think this potentially makes things more cumbersome. Consider some code like this:

module SomeHelperMethods
  def some_helper_method
    if example.metadata[:blah]
      do_something
    end
  end
end

RSpec.configure do |c|
  c.include SomeHelperMethods
end

In this highly contrived and totally generic example, the helper method is able to directly access the example metadata in a conditional. With your proposed change, this would no longer be possible -- all helper methods that need access to the example would have to accept (and be passed) an example argument. That feels more cumbersome than the current way that it works -- particularly when you have a helper method that calls a helper method that calls a helper method that needs access to the example. All the intermediary helper methods would need to accept (and pass on) the example arg, and then the example itself would have to pass it in.

So I guess you could say I'm on the fence about this.

BTW, if we do make this change, how many other method names are we "squatting" (for lack of a better term)? Are there likely to be other methods that could have similar conflicts?

Member

myronmarston commented Aug 27, 2012

Interesting idea. I assume this is in response to #663?

I like the fact that this creates fewer potential naming conflicts (especially since example could reasonably be used as a method name by end users), but I do think this potentially makes things more cumbersome. Consider some code like this:

module SomeHelperMethods
  def some_helper_method
    if example.metadata[:blah]
      do_something
    end
  end
end

RSpec.configure do |c|
  c.include SomeHelperMethods
end

In this highly contrived and totally generic example, the helper method is able to directly access the example metadata in a conditional. With your proposed change, this would no longer be possible -- all helper methods that need access to the example would have to accept (and be passed) an example argument. That feels more cumbersome than the current way that it works -- particularly when you have a helper method that calls a helper method that calls a helper method that needs access to the example. All the intermediary helper methods would need to accept (and pass on) the example arg, and then the example itself would have to pass it in.

So I guess you could say I'm on the fence about this.

BTW, if we do make this change, how many other method names are we "squatting" (for lack of a better term)? Are there likely to be other methods that could have similar conflicts?

@dchelimsky

This comment has been minimized.

Show comment
Hide comment
@dchelimsky

dchelimsky Aug 27, 2012

Member

What if we hung example on RSpec? e.g.

module SomeHelperMethods
  def some_helper_method
    if RSpec.example.metadata[:blah]
      do_something
    end
  end
end

RSpec.configure do |c|
  c.include SomeHelperMethods
end

That makes it available for the edge case. Another option would be to offer a config option:

RSpec.configure do |c|
  c.expose_current_running_example_as :rspec_example
end

or some such.

The only other instance methods on ExampleGroup are running_example (deprecated predecessor to example), described_class (which, arguably, should be treated the same way) and instance_eval_with_rescue, which would be easy enough to refactor out (only used internally).

Member

dchelimsky commented Aug 27, 2012

What if we hung example on RSpec? e.g.

module SomeHelperMethods
  def some_helper_method
    if RSpec.example.metadata[:blah]
      do_something
    end
  end
end

RSpec.configure do |c|
  c.include SomeHelperMethods
end

That makes it available for the edge case. Another option would be to offer a config option:

RSpec.configure do |c|
  c.expose_current_running_example_as :rspec_example
end

or some such.

The only other instance methods on ExampleGroup are running_example (deprecated predecessor to example), described_class (which, arguably, should be treated the same way) and instance_eval_with_rescue, which would be easy enough to refactor out (only used internally).

@dchelimsky

This comment has been minimized.

Show comment
Hide comment
@dchelimsky

dchelimsky Aug 27, 2012

Member

Doing this w/ described_class, btw, would hurt :)

Member

dchelimsky commented Aug 27, 2012

Doing this w/ described_class, btw, would hurt :)

@dchelimsky

This comment has been minimized.

Show comment
Hide comment
@dchelimsky

dchelimsky Aug 27, 2012

Member

What if we don't deprecate these methods for rspec-2.x, but "soft deprecate" them (i.e. a deprecation notice tells users how to turn it off - requires some infrastructure).

BTW - this is something I've wanted for a long time. I actually did it in response to a reaction I had working on slides for a preso I'm doing next week. This happened a lot w/ The RSpec Book - I'm writing about something and think "wow, that's lame," and fix it :) Perhaps it was subconsciously on my mind due to #633 as well, but that's not what I was thinking of.

In regard to #633, however, part of this commit points at the solution if we don't expose the block arg or deprecate example: always reference an ivar internally instead of accessing the example through the method, or hand the method internally on RSpec. WDYT about doing that regardless?

Member

dchelimsky commented Aug 27, 2012

What if we don't deprecate these methods for rspec-2.x, but "soft deprecate" them (i.e. a deprecation notice tells users how to turn it off - requires some infrastructure).

BTW - this is something I've wanted for a long time. I actually did it in response to a reaction I had working on slides for a preso I'm doing next week. This happened a lot w/ The RSpec Book - I'm writing about something and think "wow, that's lame," and fix it :) Perhaps it was subconsciously on my mind due to #633 as well, but that's not what I was thinking of.

In regard to #633, however, part of this commit points at the solution if we don't expose the block arg or deprecate example: always reference an ivar internally instead of accessing the example through the method, or hand the method internally on RSpec. WDYT about doing that regardless?

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Aug 27, 2012

Member

What if we hung example on RSpec?

I'm OK with that idea, although I think it should be current_example, because that's a lot more self-descriptive.

I also realized there's a fairly trivial way for end-users to add an example helper method to there example groups if they need it:

module ExampleHelper
  extend RSpec::Core::SharedContext
  attr_reader :example
  before { |ex| @example = ex }
end

RSpec.configure do |c|
  c.include ExampleHelper
end

Given the simplicity of that, maybe we should just direct users who need this towards doing that? It feels like overkill to add a bunch of infrastructure for a fairly simple deprecation like this.

Anyhow, one other thing I thought of...I think this is a potentially breaking change for anyone who uses lambdas for hooks or examples, e.g.:

def before_hook_for_foo(some_arg)
  lambda do
    # do something
  end
end

before &before_hook_for_foo(:bar)

Lambda semantics dictate that it raises an error if the number of args don't match (in contrast, proc semantics -- which are used for blocks -- do not raise an error if the number of args don't match). With the new example arg passed to the lambda, it'll blow up, I think.

In regard to #633, however, part of this commit points at the solution if we don't expose the block arg or deprecate example: always reference an ivar internally instead of accessing the example through the method, or hand the method internally on RSpec. WDYT about doing that regardless?

I think that's a good idea, regardless.

Member

myronmarston commented Aug 27, 2012

What if we hung example on RSpec?

I'm OK with that idea, although I think it should be current_example, because that's a lot more self-descriptive.

I also realized there's a fairly trivial way for end-users to add an example helper method to there example groups if they need it:

module ExampleHelper
  extend RSpec::Core::SharedContext
  attr_reader :example
  before { |ex| @example = ex }
end

RSpec.configure do |c|
  c.include ExampleHelper
end

Given the simplicity of that, maybe we should just direct users who need this towards doing that? It feels like overkill to add a bunch of infrastructure for a fairly simple deprecation like this.

Anyhow, one other thing I thought of...I think this is a potentially breaking change for anyone who uses lambdas for hooks or examples, e.g.:

def before_hook_for_foo(some_arg)
  lambda do
    # do something
  end
end

before &before_hook_for_foo(:bar)

Lambda semantics dictate that it raises an error if the number of args don't match (in contrast, proc semantics -- which are used for blocks -- do not raise an error if the number of args don't match). With the new example arg passed to the lambda, it'll blow up, I think.

In regard to #633, however, part of this commit points at the solution if we don't expose the block arg or deprecate example: always reference an ivar internally instead of accessing the example through the method, or hand the method internally on RSpec. WDYT about doing that regardless?

I think that's a good idea, regardless.

@samwgoldman

This comment has been minimized.

Show comment
Hide comment
@samwgoldman

samwgoldman Aug 27, 2012

Contributor

I do use example in the manner described by @myronmarston, but only ever to get at the current example's metadata. I wonder if there isn't some nicer way to expose metadata

Contributor

samwgoldman commented Aug 27, 2012

I do use example in the manner described by @myronmarston, but only ever to get at the current example's metadata. I wonder if there isn't some nicer way to expose metadata

@patmaddox

This comment has been minimized.

Show comment
Hide comment
@patmaddox

patmaddox Sep 11, 2012

Contributor

I personally like the block syntax & explicitly passing the example to the helper. Perhaps it's verbose because all helper methods will need it, but I like that the example is an explicit argument. It means the helper has a bit less knowledge of RSpec's internals. Granted you know it's in a module that's mixed into RSpec so the whole point is to extend it...but I figure anything we can do to have an explicit API is good.

Contributor

patmaddox commented Sep 11, 2012

I personally like the block syntax & explicitly passing the example to the helper. Perhaps it's verbose because all helper methods will need it, but I like that the example is an explicit argument. It means the helper has a bit less knowledge of RSpec's internals. Granted you know it's in a module that's mixed into RSpec so the whole point is to extend it...but I figure anything we can do to have an explicit API is good.

@JonRowe

This comment has been minimized.

Show comment
Hide comment
Member

JonRowe commented Mar 31, 2013

What @totallymike said... /cc @myronmarston

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Mar 31, 2013

Member

I need to think about it some more, but I'd say the earliest we would make this change is 3.0.

Member

myronmarston commented Mar 31, 2013

I need to think about it some more, but I'd say the earliest we would make this change is 3.0.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Jun 18, 2013

Member

This has sat for a while but I still like it and would like to get it in RSpec 3. FWIW :).

Member

myronmarston commented Jun 18, 2013

This has sat for a while but I still like it and would like to get it in RSpec 3. FWIW :).

@dchelimsky

This comment has been minimized.

Show comment
Hide comment
@dchelimsky

dchelimsky Jun 19, 2013

Member

It needs to be rebased. Working on that now.

Member

dchelimsky commented Jun 19, 2013

It needs to be rebased. Working on that now.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jun 19, 2013

Coverage Status

Coverage increased (+0%) when pulling 45d0e2b on yield-example into dee12fc on master.

Coverage Status

Coverage increased (+0%) when pulling 45d0e2b on yield-example into dee12fc on master.

@dchelimsky

This comment has been minimized.

Show comment
Hide comment
@dchelimsky

dchelimsky Jun 19, 2013

Member

I rebased this off master, force pushed, and added another commit. I'm pretty happy with it as/is but welcome feedback /cc @myronmarston @alindeman @JonRowe @soulcutter @samphippen

Member

dchelimsky commented Jun 19, 2013

I rebased this off master, force pushed, and added another commit. I'm pretty happy with it as/is but welcome feedback /cc @myronmarston @alindeman @JonRowe @soulcutter @samphippen

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jun 19, 2013

Coverage Status

Coverage increased (+0%) when pulling 7e0a9c3 on yield-example into dee12fc on master.

Coverage Status

Coverage increased (+0%) when pulling 7e0a9c3 on yield-example into dee12fc on master.

lib/rspec/core/example_group.rb
+ # end
+ #
+ # example "does something" do |ex|
+ # # ex is a wrapper for the current running example

This comment has been minimized.

@myronmarston

myronmarston Jun 19, 2013

Member

I thought ex was the current example object? In what sense is it a wrapper of the current example rather than than the current example itself?

@myronmarston

myronmarston Jun 19, 2013

Member

I thought ex was the current example object? In what sense is it a wrapper of the current example rather than than the current example itself?

This comment has been minimized.

@dchelimsky

dchelimsky Jun 19, 2013

Member

You're correct. I think at one point I had a wrapper in mind (a la example and group proxies for rspec-1 formatters), but ended up just with the example.

@dchelimsky

dchelimsky Jun 19, 2013

Member

You're correct. I think at one point I had a wrapper in mind (a la example and group proxies for rspec-1 formatters), but ended up just with the example.

lib/rspec/core/example_group.rb
+ # example "does something" do
+ # end
+ #
+ # example "does something", :with => 'addtional metadata' do

This comment has been minimized.

@myronmarston

myronmarston Jun 19, 2013

Member

s/addtional/additional/

@myronmarston

myronmarston Jun 19, 2013

Member

s/addtional/additional/

+ def example
+ RSpec.deprecate("example", :replacement => "a block argument")
+ @_current_rspec_example
+ end

This comment has been minimized.

@myronmarston

myronmarston Jun 19, 2013

Member

I was originally thinking we'd remove this method in 3.0 and than add a deprecation like this to 2.99. I'm open to keeping it deprecated in 3.0, though, especially if you think it's important. Generally, for most breaking changes like this I'm in favor of deprecating in 2.99 and removing in 3.0, except in a few circumstances:

  • It could trip up newcomers who are using an old tutorial
  • The API is likely to be used by an extension gem, which the user may not have direct control over and can't easily update during the 2.x -> 2.99 -> 3.0 migration process.

Do you think either of these apply here?

@myronmarston

myronmarston Jun 19, 2013

Member

I was originally thinking we'd remove this method in 3.0 and than add a deprecation like this to 2.99. I'm open to keeping it deprecated in 3.0, though, especially if you think it's important. Generally, for most breaking changes like this I'm in favor of deprecating in 2.99 and removing in 3.0, except in a few circumstances:

  • It could trip up newcomers who are using an old tutorial
  • The API is likely to be used by an extension gem, which the user may not have direct control over and can't easily update during the 2.x -> 2.99 -> 3.0 migration process.

Do you think either of these apply here?

This comment has been minimized.

@dchelimsky

dchelimsky Jun 19, 2013

Member

I agree that's the right path - just wasn't thinking clearly that master is 3.0. So let's merge this (when we do) as/is to both 2-14-maintenance and master and I'll add another commit to master to remove the deprecated methods. Fair?

@dchelimsky

dchelimsky Jun 19, 2013

Member

I agree that's the right path - just wasn't thinking clearly that master is 3.0. So let's merge this (when we do) as/is to both 2-14-maintenance and master and I'll add another commit to master to remove the deprecated methods. Fair?

This comment has been minimized.

@myronmarston

myronmarston Jun 19, 2013

Member

Sounds fair...although we need this in 2-99-maintenance, not 2-14-maintenance. Unless you want this to go out in 2.14? I think just being deprecated in 2.99 is sufficient, given that it's not very common to access the example.

@myronmarston

myronmarston Jun 19, 2013

Member

Sounds fair...although we need this in 2-99-maintenance, not 2-14-maintenance. Unless you want this to go out in 2.14? I think just being deprecated in 2.99 is sufficient, given that it's not very common to access the example.

This comment has been minimized.

@myronmarston

myronmarston Jun 19, 2013

Member

So if we're going to remove the #example and #running_example, is there a way we can remove @_current_rspec_example as well? Or do we still need the instance variable?

@myronmarston

myronmarston Jun 19, 2013

Member

So if we're going to remove the #example and #running_example, is there a way we can remove @_current_rspec_example as well? Or do we still need the instance variable?

This comment has been minimized.

@dchelimsky

dchelimsky Jun 19, 2013

Member

We still need to access it internally - just not expose it via a method. The assumption here is that it won't conflict with a @_current_rspec_example instance var in your app's examples :)

@dchelimsky

dchelimsky Jun 19, 2013

Member

We still need to access it internally - just not expose it via a method. The assumption here is that it won't conflict with a @_current_rspec_example instance var in your app's examples :)

This comment has been minimized.

@myronmarston

myronmarston Jun 19, 2013

Member

We still need to access it internally - just not expose it via a method. The assumption here is that it won't conflict with a @_current_rspec_example instance var in your app's examples :)

Random idea: what if we stored the current example in a thread local? That would remove the need for the ivar. If/when we ever wanted to make a multi-threaded runner, that would be automatically thread safe in a way that instance variables aren't. We could then expose it from RSpec.current_example (which I think was one of your earlier ideas), which can be handy for when a helper method needs to access the example metadata, and we could remove the example= method in this class. I don't know how likely it is that users would have an example= helper method, but it's not outside the realm of possibility.

re: 2.14 v 2.99 v 3.0. We can definitely do 2.99 (w/ deprecations) and 3.0 (with removal), but it feels odd to add a new feature to 2.99 and only give people 2.99 on to use it. WDYT?

I don't consider this a new feature. I consider it an API change. The current example has always been available; this just changes the means users use to access it.

@myronmarston

myronmarston Jun 19, 2013

Member

We still need to access it internally - just not expose it via a method. The assumption here is that it won't conflict with a @_current_rspec_example instance var in your app's examples :)

Random idea: what if we stored the current example in a thread local? That would remove the need for the ivar. If/when we ever wanted to make a multi-threaded runner, that would be automatically thread safe in a way that instance variables aren't. We could then expose it from RSpec.current_example (which I think was one of your earlier ideas), which can be handy for when a helper method needs to access the example metadata, and we could remove the example= method in this class. I don't know how likely it is that users would have an example= helper method, but it's not outside the realm of possibility.

re: 2.14 v 2.99 v 3.0. We can definitely do 2.99 (w/ deprecations) and 3.0 (with removal), but it feels odd to add a new feature to 2.99 and only give people 2.99 on to use it. WDYT?

I don't consider this a new feature. I consider it an API change. The current example has always been available; this just changes the means users use to access it.

This comment has been minimized.

@dchelimsky

dchelimsky Jun 19, 2013

Member

What's your concern about an instance variable here? It's got a name that is very unlikely to cause a conflict. We can always move to a thread local when we want to make a multi-threaded runner, which will likely need other changes as well.

I agree re: API change v new feature. 2.99 and 3.0 it is.

@dchelimsky

dchelimsky Jun 19, 2013

Member

What's your concern about an instance variable here? It's got a name that is very unlikely to cause a conflict. We can always move to a thread local when we want to make a multi-threaded runner, which will likely need other changes as well.

I agree re: API change v new feature. 2.99 and 3.0 it is.

This comment has been minimized.

@myronmarston

myronmarston Jun 19, 2013

Member

What's your concern about an instance variable here? It's got a name that is very unlikely to cause a conflict. We can always move to a thread local when we want to make a multi-threaded runner, which will likely need other changes as well.

I don't have a specific concern. It's just the anal part of me thinking about the fact that the goal here was to stop /reduce polluting the user's namespace, and we're still doing so -- just with something that's less likely to conflict.

@myronmarston

myronmarston Jun 19, 2013

Member

What's your concern about an instance variable here? It's got a name that is very unlikely to cause a conflict. We can always move to a thread local when we want to make a multi-threaded runner, which will likely need other changes as well.

I don't have a specific concern. It's just the anal part of me thinking about the fact that the goal here was to stop /reduce polluting the user's namespace, and we're still doing so -- just with something that's less likely to conflict.

This comment has been minimized.

@dchelimsky

dchelimsky Jun 19, 2013

Member

I think thread local is overkill for this, and if we expose RSpec.current_example it will get used. I'd recommend keeping the ivar for now. Can always adjust later if a holistic approach emerges.

@dchelimsky

dchelimsky Jun 19, 2013

Member

I think thread local is overkill for this, and if we expose RSpec.current_example it will get used. I'd recommend keeping the ivar for now. Can always adjust later if a holistic approach emerges.

This comment has been minimized.

@myronmarston

myronmarston Jun 19, 2013

Member

I'm fine with that.

@myronmarston

myronmarston Jun 19, 2013

Member

I'm fine with that.

+
+ def example=(current_example)
+ @_current_rspec_example = current_example
+ end

This comment has been minimized.

@myronmarston

myronmarston Jun 19, 2013

Member

Is this being called anywhere? I've scrolled down this diff 3 times looking for where it is being called but can't find it. I figure it must be called somewhere given that other things are relying on @_current_rspec_example...

@myronmarston

myronmarston Jun 19, 2013

Member

Is this being called anywhere? I've scrolled down this diff 3 times looking for where it is being called but can't find it. I figure it must be called somewhere given that other things are relying on @_current_rspec_example...

This comment has been minimized.

@dchelimsky

dchelimsky Jun 19, 2013

Member

Example#run

@dchelimsky

dchelimsky Jun 19, 2013

Member

Example#run

This comment has been minimized.

@myronmarston

myronmarston Jun 19, 2013

Member

Thanks, I missed the fact that example= already existed via the attr_accessor declaration. I was thinking this was a new method and didn't see anything in the diff that was calling it.

@myronmarston

myronmarston Jun 19, 2013

Member

Thanks, I missed the fact that example= already existed via the attr_accessor declaration. I was thinking this was a new method and didn't see anything in the diff that was calling it.

- raise unless example
- example.set_exception(e, context)
+ raise unless @_current_rspec_example
+ @_current_rspec_example.set_exception(e, context)

This comment has been minimized.

@myronmarston

myronmarston Jun 19, 2013

Member

Given that this method accepts an example argument, it would seem cleaner to use that rather than the instance variable. Any reason to prefer the instance variable over the local?

@myronmarston

myronmarston Jun 19, 2013

Member

Given that this method accepts an example argument, it would seem cleaner to use that rather than the instance variable. Any reason to prefer the instance variable over the local?

This comment has been minimized.

@dchelimsky

dchelimsky Jun 19, 2013

Member

I changed it and got an error. Looks like this is a hack to ensure different behavior for an after(:all) block. Looking into a cleaner, more expressive fix.

@dchelimsky

dchelimsky Jun 19, 2013

Member

I changed it and got an error. Looks like this is a hack to ensure different behavior for an after(:all) block. Looking into a cleaner, more expressive fix.

@@ -76,7 +76,7 @@ def pending_fixed?; true; end
# # ...
# end
def pending(*args)
- return self.class.before(:each) { pending(*args) } unless example
+ return self.class.before(:each) { pending(*args) } unless @_current_rspec_example

This comment has been minimized.

@myronmarston

myronmarston Jun 19, 2013

Member

This has nothing to do with your PR since it was already this way...but under which circumstances would this be called w/o a current example? I can't think of a case where this early return would actually happen.

@myronmarston

myronmarston Jun 19, 2013

Member

This has nothing to do with your PR since it was already this way...but under which circumstances would this be called w/o a current example? I can't think of a case where this early return would actually happen.

This comment has been minimized.

@dchelimsky

dchelimsky Jun 19, 2013

Member

It's for the before(:all) { pending() } case. Happy to consider eliminating support for that, but it works today.

@dchelimsky

dchelimsky Jun 19, 2013

Member

It's for the before(:all) { pending() } case. Happy to consider eliminating support for that, but it works today.

This comment has been minimized.

@myronmarston

myronmarston Jun 19, 2013

Member

Gotcha. It's fine as is (a refactoring to that would really be a separate issue). Mostly I was just trying to understand what this case was here for.

@myronmarston

myronmarston Jun 19, 2013

Member

Gotcha. It's fine as is (a refactoring to that would really be a separate issue). Mostly I was just trying to understand what this case was here for.

- :replacement => "example")
- example
+ RSpec.deprecate("running_example", :replacement => "a block argument")
+ @_current_rspec_example
end

This comment has been minimized.

@myronmarston

myronmarston Jun 19, 2013

Member

I noticed there's no spec (that I can find, anyway) for either #example or #running_example, to show that it continues to return the example object and that they print a deprecation warning.

Do you think that's worth adding?

@myronmarston

myronmarston Jun 19, 2013

Member

I noticed there's no spec (that I can find, anyway) for either #example or #running_example, to show that it continues to return the example object and that they print a deprecation warning.

Do you think that's worth adding?

This comment has been minimized.

@dchelimsky

dchelimsky Jun 19, 2013

Member

Added in ed5a3be

This comment has been minimized.

@myronmarston

myronmarston Jun 19, 2013

Member

Thanks!

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Jun 19, 2013

Member

Looking good. There are probably some cukes that we should edit so as to use this new style as well.

Member

myronmarston commented Jun 19, 2013

Looking good. There are probably some cukes that we should edit so as to use this new style as well.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jun 19, 2013

Coverage Status

Coverage increased (+0%) when pulling 6e02be3 on yield-example into dee12fc on master.

Coverage Status

Coverage increased (+0%) when pulling 6e02be3 on yield-example into dee12fc on master.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Jun 19, 2013

Member

Do you think that we should yield the example from let and subject blocks as well?

I've seen let used like so:

shared_context "user helpers" do
  let(:user) { User.find(example.metadata.fetch(:user_id)) }
end

describe "Accessing the API as an admin", user_id: 123 do
  include_context "user helpers"
end

...and for this to continue to be possible the example needs to be yielded to let, I think:

shared_context "user helpers" do
  let(:user) { |ex| User.find(ex.metadata.fetch(:user_id)) }
end
Member

myronmarston commented Jun 19, 2013

Do you think that we should yield the example from let and subject blocks as well?

I've seen let used like so:

shared_context "user helpers" do
  let(:user) { User.find(example.metadata.fetch(:user_id)) }
end

describe "Accessing the API as an admin", user_id: 123 do
  include_context "user helpers"
end

...and for this to continue to be possible the example needs to be yielded to let, I think:

shared_context "user helpers" do
  let(:user) { |ex| User.find(ex.metadata.fetch(:user_id)) }
end
@dchelimsky

This comment has been minimized.

Show comment
Hide comment
@dchelimsky

dchelimsky Jun 19, 2013

Member

Do you think that we should yield the example from let and subject blocks as well?

Yes. I hate to think of it being used :), but at least it's consistent. Will add.

Member

dchelimsky commented Jun 19, 2013

Do you think that we should yield the example from let and subject blocks as well?

Yes. I hate to think of it being used :), but at least it's consistent. Will add.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Jun 19, 2013

Member

Yes. I hate to think of it being used :), but at least it's consistent. Will add.

I agree that it could easily be mis-used and get out of hand, but I don't think it's too bad if used sparingly. But yeah -- consistency is my main reason for asking here.

Member

myronmarston commented Jun 19, 2013

Yes. I hate to think of it being used :), but at least it's consistent. Will add.

I agree that it could easily be mis-used and get out of hand, but I don't think it's too bad if used sparingly. But yeah -- consistency is my main reason for asking here.

@dchelimsky

This comment has been minimized.

Show comment
Hide comment
@dchelimsky

dchelimsky Jun 19, 2013

Member

Do you think that we should yield the example from let and subject blocks as well?

Yes. I hate to think of it being used :), but at least it's consistent. Will add.

Not so fast. The support for using super complicates this a bit. Because the block passed to let is now passed to define_method, adding block args to the block effectively defines parameters on the resulting method. Not sure there's a way to support exposing the example to a let or subject block and super, return etc at the same time . WDYT?

Member

dchelimsky commented Jun 19, 2013

Do you think that we should yield the example from let and subject blocks as well?

Yes. I hate to think of it being used :), but at least it's consistent. Will add.

Not so fast. The support for using super complicates this a bit. Because the block passed to let is now passed to define_method, adding block args to the block effectively defines parameters on the resulting method. Not sure there's a way to support exposing the example to a let or subject block and super, return etc at the same time . WDYT?

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Jun 19, 2013

Member

Not so fast. The support for using super complicates this a bit. Because the block passed to let is now passed to define_method, adding block args to the block effectively defines parameters on the resulting method. Not sure there's a way to support exposing the example to a let or subject block and super, return etc at the same time . WDYT?

It feels a bit like a hack, but you could do this, I think:

        def let(name, &block)
          # We have to pass the block directly to `define_method` to
          # allow it to use method constructs like `super` and `return`.
          raise "#let or #subject called without a block" if block.nil?
          MemoizedHelpers.module_for(self).send(:define_method, name, &block)

          # Apply the memoization. The method has been defined in an ancestor
          # module so we can use `super` here to get the value.
          define_method(name) do
            __memoized.fetch(name) do |k|
              arity = self.class.superclass.instance_method(name).arity
              args = arity.zero? ? [] : [@_current_rspec_example]
              __memoized[k] = super(*args, &nil)
            end
          end
        end

Basically, check the arity of the superclass method.

Member

myronmarston commented Jun 19, 2013

Not so fast. The support for using super complicates this a bit. Because the block passed to let is now passed to define_method, adding block args to the block effectively defines parameters on the resulting method. Not sure there's a way to support exposing the example to a let or subject block and super, return etc at the same time . WDYT?

It feels a bit like a hack, but you could do this, I think:

        def let(name, &block)
          # We have to pass the block directly to `define_method` to
          # allow it to use method constructs like `super` and `return`.
          raise "#let or #subject called without a block" if block.nil?
          MemoizedHelpers.module_for(self).send(:define_method, name, &block)

          # Apply the memoization. The method has been defined in an ancestor
          # module so we can use `super` here to get the value.
          define_method(name) do
            __memoized.fetch(name) do |k|
              arity = self.class.superclass.instance_method(name).arity
              args = arity.zero? ? [] : [@_current_rspec_example]
              __memoized[k] = super(*args, &nil)
            end
          end
        end

Basically, check the arity of the superclass method.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jun 19, 2013

Coverage Status

Coverage decreased (-0%) when pulling 4f3f66a on yield-example into dee12fc on master.

Coverage Status

Coverage decreased (-0%) when pulling 4f3f66a on yield-example into dee12fc on master.

@dchelimsky

This comment has been minimized.

Show comment
Hide comment
@dchelimsky

dchelimsky Jun 19, 2013

Member

OK - I added support for yielding the example to let and subject. It does check the arity, but directly on the block. This avoids some errors I saw when checking the superclass, and it also feels cleaner since we have access to the block directly. Let me know what you think.

Member

dchelimsky commented Jun 19, 2013

OK - I added support for yielding the example to let and subject. It does check the arity, but directly on the block. This avoids some errors I saw when checking the superclass, and it also feels cleaner since we have access to the block directly. Let me know what you think.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Jun 19, 2013

Member

OK - I added support for yielding the example to let and subject. It does check the arity, but directly on the block. This avoids some errors I saw when checking the superclass, and it also feels cleaner since we have access to the block directly. Let me know what you think.

👍 Much better than my solution! It also potentially performs better; rather than checking the conditional for each example that calls the let-defined method, it runs the check only once, when the let is defined.

Member

myronmarston commented Jun 19, 2013

OK - I added support for yielding the example to let and subject. It does check the arity, but directly on the block. This avoids some errors I saw when checking the superclass, and it also feels cleaner since we have access to the block directly. Let me know what you think.

👍 Much better than my solution! It also potentially performs better; rather than checking the conditional for each example that calls the let-defined method, it runs the check only once, when the let is defined.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Jul 1, 2013

Member

On a side note, I just realized that the fact that subject and let yield example helps make those constructs' relationship with examples more clear and helps show why it's nonsensical to access those constructs from before(:all). That's a nice side benefit :).

Member

myronmarston commented Jul 1, 2013

On a side note, I just realized that the fact that subject and let yield example helps make those constructs' relationship with examples more clear and helps show why it's nonsensical to access those constructs from before(:all). That's a nice side benefit :).

@dchelimsky

This comment has been minimized.

Show comment
Hide comment
@dchelimsky

dchelimsky Jul 1, 2013

Member
let(:_default_file_to_render) do |example|
  example.example_group.top_level_description
end

❤️ ❤️ ❤️

Member

dchelimsky commented Jul 1, 2013

let(:_default_file_to_render) do |example|
  example.example_group.top_level_description
end

❤️ ❤️ ❤️

@alindeman

This comment has been minimized.

Show comment
Hide comment
@alindeman

alindeman Jul 2, 2013

Contributor

Perfect, thank you :)

Contributor

alindeman commented Jul 2, 2013

Perfect, thank you :)

@alindeman

This comment has been minimized.

Show comment
Hide comment
@alindeman

alindeman Jul 4, 2013

Contributor

OK, it looks like some libraries like capybara also use example in hooks. See capybara/rspec.rb.

What should we recommend to the capybara folks? Is it possible to be compatible with both RSpec 2 and 3 there?

Contributor

alindeman commented Jul 4, 2013

OK, it looks like some libraries like capybara also use example in hooks. See capybara/rspec.rb.

What should we recommend to the capybara folks? Is it possible to be compatible with both RSpec 2 and 3 there?

@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe Jul 4, 2013

Member

I think a fix similar to yours would work no?

let(:example) {  |current_example| current_example ? current_example : super }

or

let(:_current_example) { |current_example| current_example ? current_example : example }
Member

JonRowe commented Jul 4, 2013

I think a fix similar to yours would work no?

let(:example) {  |current_example| current_example ? current_example : super }

or

let(:_current_example) { |current_example| current_example ? current_example : example }
@alindeman

This comment has been minimized.

Show comment
Hide comment
@alindeman

alindeman Jul 4, 2013

Contributor

Yielding the example is new in 2.14, right? How do we maintain compatibility with versions before that and RSpec 3 (where example is gone entirely)?

Contributor

alindeman commented Jul 4, 2013

Yielding the example is new in 2.14, right? How do we maintain compatibility with versions before that and RSpec 3 (where example is gone entirely)?

@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe Jul 4, 2013

Member

Yielding the example will be new in 3, I don't think it's in even 2.14.0rc1... Hence the conditional, I've just tried this on 2.14 and it's a bit more like this:

let(:current_example) do |*args|
  yielded_example, *rest = args
  yielded_example ? yielded_example : example
end
Member

JonRowe commented Jul 4, 2013

Yielding the example will be new in 3, I don't think it's in even 2.14.0rc1... Hence the conditional, I've just tried this on 2.14 and it's a bit more like this:

let(:current_example) do |*args|
  yielded_example, *rest = args
  yielded_example ? yielded_example : example
end
@alindeman

This comment has been minimized.

Show comment
Hide comment
@alindeman

alindeman Jul 4, 2013

Contributor

Is that what we recommend library authors (like capybara) do? That's pretty rough :/ I'll sleep on this, but I think we need something better.

Contributor

alindeman commented Jul 4, 2013

Is that what we recommend library authors (like capybara) do? That's pretty rough :/ I'll sleep on this, but I think we need something better.

@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe Jul 4, 2013

Member

It's a work around they could use for supporting multiple major versions, I think this is a conflict they will have to decide how to resolve, there's a couple of strategies that I can see...

  1. they can use ugly work arounds temporarily
  2. they can pick different integration code depending on rspec versions
  3. they can drop support for 2.x

the later would be best for us by encouraging adoption of 3.x ;)

Whilst we should be sympathetic and help, we shouldn't let 3rd party libraries drive our decisions either.

Member

JonRowe commented Jul 4, 2013

It's a work around they could use for supporting multiple major versions, I think this is a conflict they will have to decide how to resolve, there's a couple of strategies that I can see...

  1. they can use ugly work arounds temporarily
  2. they can pick different integration code depending on rspec versions
  3. they can drop support for 2.x

the later would be best for us by encouraging adoption of 3.x ;)

Whilst we should be sympathetic and help, we shouldn't let 3rd party libraries drive our decisions either.

@dchelimsky

This comment has been minimized.

Show comment
Hide comment
@dchelimsky

dchelimsky Jul 4, 2013

Member

Yielding the example is new in 2.14, right? How do we maintain compatibility with versions before that and RSpec 3 (where example is gone entirely)?

How about an opt-in via config like config.expose_example_as_example or some such? That would at least offer end-users a way of using RSpec 3 with libs that haven't moved forward yet.

Whilst we should be sympathetic and help, we shouldn't let 3rd party libraries drive our decisions either.
3) they can drop support for 2.x
the later would be best for us by encouraging adoption of 3.x ;)

And it would do just as good a job of encouraging, if not forcing, people to stop using RSpec. It's one thing to nudge people forward, and another to cut them off at the knees. We have to allow for the fact that it will take some time for 3rd party libs to adjust, and do what we can to make this all seamless for end-users. In fact - I'm beginning to think we should deprecate example but leave it in RSpec-3.

Member

dchelimsky commented Jul 4, 2013

Yielding the example is new in 2.14, right? How do we maintain compatibility with versions before that and RSpec 3 (where example is gone entirely)?

How about an opt-in via config like config.expose_example_as_example or some such? That would at least offer end-users a way of using RSpec 3 with libs that haven't moved forward yet.

Whilst we should be sympathetic and help, we shouldn't let 3rd party libraries drive our decisions either.
3) they can drop support for 2.x
the later would be best for us by encouraging adoption of 3.x ;)

And it would do just as good a job of encouraging, if not forcing, people to stop using RSpec. It's one thing to nudge people forward, and another to cut them off at the knees. We have to allow for the fact that it will take some time for 3rd party libs to adjust, and do what we can to make this all seamless for end-users. In fact - I'm beginning to think we should deprecate example but leave it in RSpec-3.

@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe Jul 4, 2013

Member

I'm more than prepared to go and lend a hand to things like capybara to help them gain RSpec 3.x support, I think in most cases the changes won't be that extreme, there are probably going to be more gems out there version locked to 2.x with a ~> that will cause issues than soft loaded gems like Capybara.

2.99 should print deprecation warnings for this, and we are planning to have that around for a significant time to help people get ready for the change, so I don't feel it's necessary to leave things like this around deprecated... we can help people upgrade for the change, we can document things they can do and suggested solutions, I hardly think this will be cutting people off at the knees.

I do however apologise for the language in my comment, it was more humour than perhaps it was taken.

Member

JonRowe commented Jul 4, 2013

I'm more than prepared to go and lend a hand to things like capybara to help them gain RSpec 3.x support, I think in most cases the changes won't be that extreme, there are probably going to be more gems out there version locked to 2.x with a ~> that will cause issues than soft loaded gems like Capybara.

2.99 should print deprecation warnings for this, and we are planning to have that around for a significant time to help people get ready for the change, so I don't feel it's necessary to leave things like this around deprecated... we can help people upgrade for the change, we can document things they can do and suggested solutions, I hardly think this will be cutting people off at the knees.

I do however apologise for the language in my comment, it was more humour than perhaps it was taken.

@dchelimsky

This comment has been minimized.

Show comment
Hide comment
@dchelimsky

dchelimsky Jul 4, 2013

Member

I'm more than prepared to go and lend a hand to things like capybara to help them gain RSpec 3.x support

That's great.

2.99 should print deprecation warnings for this, and we are planning to have that around for a significant time to help people get ready for the change

I think it's the combination of 2.99 and 3.0.0.beta/rc that needs to be around for a while before 3.0.0 final goes out so lib maintainers and end-users alike can see the deprecation warnings and validate that their resulting changes work with the new version. We've talked about this before but haven't really defined what "a significant time" means. I think we need some means of validating that enough people (definition TBD) are successfully using 3.0 pre-releases with a sufficiently wide variety of 3rd party gems (definition TBD) in play. Surveys? A user-sourced compatibility chart? An opt-in stats report that rspec-3.0.0 pre-releases can send to a server so we can collect data like gem versions, which we can use to publish a compatibility chart? Other ideas?

Member

dchelimsky commented Jul 4, 2013

I'm more than prepared to go and lend a hand to things like capybara to help them gain RSpec 3.x support

That's great.

2.99 should print deprecation warnings for this, and we are planning to have that around for a significant time to help people get ready for the change

I think it's the combination of 2.99 and 3.0.0.beta/rc that needs to be around for a while before 3.0.0 final goes out so lib maintainers and end-users alike can see the deprecation warnings and validate that their resulting changes work with the new version. We've talked about this before but haven't really defined what "a significant time" means. I think we need some means of validating that enough people (definition TBD) are successfully using 3.0 pre-releases with a sufficiently wide variety of 3rd party gems (definition TBD) in play. Surveys? A user-sourced compatibility chart? An opt-in stats report that rspec-3.0.0 pre-releases can send to a server so we can collect data like gem versions, which we can use to publish a compatibility chart? Other ideas?

@alindeman

This comment has been minimized.

Show comment
Hide comment
@alindeman

alindeman Jul 4, 2013

Contributor

What if we did expose RSpec.current_example as mention earlier? That way capybara and others could do something like:

current_example = RSpec.respond_to?(:current_example) ? RSpec.current_example : example

(the check could also be written in such a way that it'd only be performed once for performance reasons)

... and maintain compatibility? Or is there a compelling reason to not expose it like that?

Contributor

alindeman commented Jul 4, 2013

What if we did expose RSpec.current_example as mention earlier? That way capybara and others could do something like:

current_example = RSpec.respond_to?(:current_example) ? RSpec.current_example : example

(the check could also be written in such a way that it'd only be performed once for performance reasons)

... and maintain compatibility? Or is there a compelling reason to not expose it like that?

@alindeman

This comment has been minimized.

Show comment
Hide comment
@alindeman

alindeman Jul 4, 2013

Contributor

RSpec.current_example could be exposed publicly, but we wouldn't necessary document it anywhere other than the API docs. It'd be for library authors, and users writing examples in their apps would be encouraged to use the block argument.

Thoughts?

Contributor

alindeman commented Jul 4, 2013

RSpec.current_example could be exposed publicly, but we wouldn't necessary document it anywhere other than the API docs. It'd be for library authors, and users writing examples in their apps would be encouraged to use the block argument.

Thoughts?

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Jul 4, 2013

Member

FWIW, I think the best way to encourage adoption of 3.x is to make the upgrade as simple and painless as possible, both for users and library authors, so there's no reason not to upgrade. That's why we're putting significant effort into 2.99, so that users have a very clear upgrade path, with explicit instructions about what's changing and how it affects them, rather than just a changelog they have to read to figure out how it applies to them.

In fact - I'm beginning to think we should deprecate example but leave it in RSpec-3.

I'm 👎 on this idea, but certainly could be convinced otherwise once we get more community feedback. One of the main wins of this PR was reducing the amount of namespace squatting we're doing, and if we leave it in place deprecated, we lose that benefit until RSpec 4...which is a long time to wait. I'd first like to see if we can't ease the transition in other ways.

RSpec.current_example could be exposed publicly, but we wouldn't necessary document it anywhere other than the API docs. It'd be for library authors, and users writing examples in their apps would be encouraged to use the block argument.

Thoughts?

👍 I like this.

Member

myronmarston commented Jul 4, 2013

FWIW, I think the best way to encourage adoption of 3.x is to make the upgrade as simple and painless as possible, both for users and library authors, so there's no reason not to upgrade. That's why we're putting significant effort into 2.99, so that users have a very clear upgrade path, with explicit instructions about what's changing and how it affects them, rather than just a changelog they have to read to figure out how it applies to them.

In fact - I'm beginning to think we should deprecate example but leave it in RSpec-3.

I'm 👎 on this idea, but certainly could be convinced otherwise once we get more community feedback. One of the main wins of this PR was reducing the amount of namespace squatting we're doing, and if we leave it in place deprecated, we lose that benefit until RSpec 4...which is a long time to wait. I'd first like to see if we can't ease the transition in other ways.

RSpec.current_example could be exposed publicly, but we wouldn't necessary document it anywhere other than the API docs. It'd be for library authors, and users writing examples in their apps would be encouraged to use the block argument.

Thoughts?

👍 I like this.

@dchelimsky

This comment has been minimized.

Show comment
Hide comment
@dchelimsky

dchelimsky Jul 4, 2013

Member

RSpec.current_example could be exposed publicly, but we wouldn't necessary document it anywhere other than the API docs. It'd be for library authors, and users writing examples in their apps would be encouraged to use the block argument.

Thoughts?
I like this.

That's backwards to me. If a lib author is going to make a change, we want him/her to follow the path forward and use the yielded example object, not propagate the dependency on example or running_example methods. Exposing the example and running_example methods helps end-users who are using libs that haven't been upgraded yet. They'd still be encouraged (via docs) to use the block arg in their own spec suites.

Member

dchelimsky commented Jul 4, 2013

RSpec.current_example could be exposed publicly, but we wouldn't necessary document it anywhere other than the API docs. It'd be for library authors, and users writing examples in their apps would be encouraged to use the block argument.

Thoughts?
I like this.

That's backwards to me. If a lib author is going to make a change, we want him/her to follow the path forward and use the yielded example object, not propagate the dependency on example or running_example methods. Exposing the example and running_example methods helps end-users who are using libs that haven't been upgraded yet. They'd still be encouraged (via docs) to use the block arg in their own spec suites.

@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe Jul 5, 2013

Member

Thought, could we deprecate the direct calls to example now before we put out 2.14? That'd give even more notice in this case? I'm with @dchelimsky on the exposing RSpec.current_example is a bit of a backwards step...

Member

JonRowe commented Jul 5, 2013

Thought, could we deprecate the direct calls to example now before we put out 2.14? That'd give even more notice in this case? I'm with @dchelimsky on the exposing RSpec.current_example is a bit of a backwards step...

@dchelimsky

This comment has been minimized.

Show comment
Hide comment
@dchelimsky

dchelimsky Jul 5, 2013

Member

I'm with @dchelimsky on the exposing RSpec.current_example is a bit of a backwards step...

Except that's not what I intended :)

I was saying that RSpec.current_example would serve end users, not lib authors, which is the opposite of what @alindeman had proposed. I'm actually in favor of including it to support end-users and don't view it as a step back.

Member

dchelimsky commented Jul 5, 2013

I'm with @dchelimsky on the exposing RSpec.current_example is a bit of a backwards step...

Except that's not what I intended :)

I was saying that RSpec.current_example would serve end users, not lib authors, which is the opposite of what @alindeman had proposed. I'm actually in favor of including it to support end-users and don't view it as a step back.

@alindeman

This comment has been minimized.

Show comment
Hide comment
@alindeman

alindeman Jul 5, 2013

Contributor

I'm a tad confused, sorry. Why would end users want RSpec.current_example?

I'm nervous right now because rspec-rails' test suite on master is badly red due to capybara using example in a before and after hook. And I don't know how to fix it without making a breaking change that forces capybara to release a new version than only supports RSpec >= 2.14 and 3.x (where this change is merged) ... or necessitate adding some conditional logic against ::RSpec:Core::VERSION

... and this makes me suspect there are many other libraries that are now broken against RSpec master and will need to release a new major version if we don't provide some easier way to support both RSpec 2 and 3.

I agree that sometimes we need to break compatibility in a major version release, but I feel like we should be conservative about it and only do it when there's a really compelling reason and no great way to support a simple unifying API.

My thought was that RSpec.current_example could be that API. But maybe there's something better or something I'm missing?

In my opinion, even if you think this change is positive, it's simultaneously small yet far-reaching: I think we should add, document, and maybe help (via pull requests) gems that integrate with RSpec to support both 2.x and 3.x.

Thoughts?

Contributor

alindeman commented Jul 5, 2013

I'm a tad confused, sorry. Why would end users want RSpec.current_example?

I'm nervous right now because rspec-rails' test suite on master is badly red due to capybara using example in a before and after hook. And I don't know how to fix it without making a breaking change that forces capybara to release a new version than only supports RSpec >= 2.14 and 3.x (where this change is merged) ... or necessitate adding some conditional logic against ::RSpec:Core::VERSION

... and this makes me suspect there are many other libraries that are now broken against RSpec master and will need to release a new major version if we don't provide some easier way to support both RSpec 2 and 3.

I agree that sometimes we need to break compatibility in a major version release, but I feel like we should be conservative about it and only do it when there's a really compelling reason and no great way to support a simple unifying API.

My thought was that RSpec.current_example could be that API. But maybe there's something better or something I'm missing?

In my opinion, even if you think this change is positive, it's simultaneously small yet far-reaching: I think we should add, document, and maybe help (via pull requests) gems that integrate with RSpec to support both 2.x and 3.x.

Thoughts?

@dchelimsky

This comment has been minimized.

Show comment
Hide comment
@dchelimsky

dchelimsky Jul 5, 2013

Member

I'm a tad confused, sorry. Why would end users want RSpec.current_example?

They wouldn't except to protect against lib authors who are slow to upgrade.

Today there is some set of end users (possibly a set of 0, but I doubt it) who use example in their apps. When they upgrade to 2.99 they'll get deprecation warnings, they'll be able to use the yielded example instead, and they'll be ready for 3.0. Hooray!

Similarly there is some set of lib authors who use example in their libs. For the ones who update their libs to support RSpec 3 (with or without our help) they'll do the same thing: upgrade to 2.99, see deprecation warnings and use the yielded example instead (or use let to define a method that yields the example as @myronmarston described as a potential solution for rspec-rails). Once they release those changes their users can upgrade to RSpec 3 as well. Hooray!

The problem case is the end user who uses lib xyz, and lib xyz does not get updated, even though we've submitted pull requests and bought the author a bottle of Octomore (which is not particularly inexpensive, but oh, so delicious!). That end user can not upgrade to RSpec 3 without:

a) bagging lib xyz. Boo!
b) using RSpec.current_example to expose example so lib xyz works even though lib xyz's author didn't update it yet. Hooray!

Make sense?

That solves the Capybara issue, btw, without imposing anything on Capybara in the short run. It just makes the end user do a little extra work until the Capybara issue is properly solved within Capybara.

Member

dchelimsky commented Jul 5, 2013

I'm a tad confused, sorry. Why would end users want RSpec.current_example?

They wouldn't except to protect against lib authors who are slow to upgrade.

Today there is some set of end users (possibly a set of 0, but I doubt it) who use example in their apps. When they upgrade to 2.99 they'll get deprecation warnings, they'll be able to use the yielded example instead, and they'll be ready for 3.0. Hooray!

Similarly there is some set of lib authors who use example in their libs. For the ones who update their libs to support RSpec 3 (with or without our help) they'll do the same thing: upgrade to 2.99, see deprecation warnings and use the yielded example instead (or use let to define a method that yields the example as @myronmarston described as a potential solution for rspec-rails). Once they release those changes their users can upgrade to RSpec 3 as well. Hooray!

The problem case is the end user who uses lib xyz, and lib xyz does not get updated, even though we've submitted pull requests and bought the author a bottle of Octomore (which is not particularly inexpensive, but oh, so delicious!). That end user can not upgrade to RSpec 3 without:

a) bagging lib xyz. Boo!
b) using RSpec.current_example to expose example so lib xyz works even though lib xyz's author didn't update it yet. Hooray!

Make sense?

That solves the Capybara issue, btw, without imposing anything on Capybara in the short run. It just makes the end user do a little extra work until the Capybara issue is properly solved within Capybara.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Jul 5, 2013

Member

That solves the Capybara issue, btw, without imposing anything on Capybara in the short run. It just makes the end user do a little extra work until the Capybara issue is properly solved within Capybara.

This sounds good in theory, but the part that confuses me (and may be confuses Andy) is this:

using RSpec.current_example to expose example so lib xyz works even though lib xyz's author didn't update it yet. Hooray!

I don't see how an end user using RSpec.current_example will solve the issue of lib xyz using RSpec::ExampleGroup#example, which will raise a NoMethodError in RSpec 3. Users can use RSpec.current_example all they want to get access to the current example, but in lib xyz, as long as it calls the removed example method, it will still cause a NoMethodError to get raised.

Member

myronmarston commented Jul 5, 2013

That solves the Capybara issue, btw, without imposing anything on Capybara in the short run. It just makes the end user do a little extra work until the Capybara issue is properly solved within Capybara.

This sounds good in theory, but the part that confuses me (and may be confuses Andy) is this:

using RSpec.current_example to expose example so lib xyz works even though lib xyz's author didn't update it yet. Hooray!

I don't see how an end user using RSpec.current_example will solve the issue of lib xyz using RSpec::ExampleGroup#example, which will raise a NoMethodError in RSpec 3. Users can use RSpec.current_example all they want to get access to the current example, but in lib xyz, as long as it calls the removed example method, it will still cause a NoMethodError to get raised.

@dchelimsky

This comment has been minimized.

Show comment
Hide comment
@dchelimsky

dchelimsky Jul 5, 2013

Member

Aha! And now I see the disconnect. Buried deep within this thread I proposed something like this:

RSpec.configure do |c|
  c.expose_current_running_example_as :example
end

And I mistook RSpec.current_example for that. So, backtracking ...

  1. I think we should include the expose_current_running_example_as config option to support the end user who uses a lib that has not upgraded to.
  2. I'm OK with or without RSpec.current_example
Member

dchelimsky commented Jul 5, 2013

Aha! And now I see the disconnect. Buried deep within this thread I proposed something like this:

RSpec.configure do |c|
  c.expose_current_running_example_as :example
end

And I mistook RSpec.current_example for that. So, backtracking ...

  1. I think we should include the expose_current_running_example_as config option to support the end user who uses a lib that has not upgraded to.
  2. I'm OK with or without RSpec.current_example
@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Jul 5, 2013

Member

I think we should include the expose_current_running_example_as config option to support the end user who uses a lib that has not upgraded to.

👍

I'm OK with or without RSpec.current_example

I think RSpec.current_example is potentially useful as a means to help libraries remain compatible with RSpec 2.x and 3.0, but it's not super important.

Member

myronmarston commented Jul 5, 2013

I think we should include the expose_current_running_example_as config option to support the end user who uses a lib that has not upgraded to.

👍

I'm OK with or without RSpec.current_example

I think RSpec.current_example is potentially useful as a means to help libraries remain compatible with RSpec 2.x and 3.0, but it's not super important.

@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe Jul 5, 2013

Member

I'd rather

RSpec.configure do |c|
  c.expose_current_running_example_as :example
end

than

RSpec.current_example

(just my 2¢)

Member

JonRowe commented Jul 5, 2013

I'd rather

RSpec.configure do |c|
  c.expose_current_running_example_as :example
end

than

RSpec.current_example

(just my 2¢)

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Jul 7, 2013

Member

I'd rather

RSpec.configure do |c|
 c.expose_current_running_example_as :example
end

than

RSpec.current_example

(just my 2¢)

They have completely different purposes. It's not an either/or.

  • expose_current_running_example_as :example config option is meant to support users who are using gems that depend on the RSpec 2 API. It's not meant for library authors to use.
  • RSpec.current_example is an idea @alindeman suggested as a possible means to assist library authors in making their code work on RSpec 2 and RSpec 3. It's not meant for end users to use.
Member

myronmarston commented Jul 7, 2013

I'd rather

RSpec.configure do |c|
 c.expose_current_running_example_as :example
end

than

RSpec.current_example

(just my 2¢)

They have completely different purposes. It's not an either/or.

  • expose_current_running_example_as :example config option is meant to support users who are using gems that depend on the RSpec 2 API. It's not meant for library authors to use.
  • RSpec.current_example is an idea @alindeman suggested as a possible means to assist library authors in making their code work on RSpec 2 and RSpec 3. It's not meant for end users to use.

myronmarston added a commit that referenced this pull request Jul 7, 2013

myronmarston added a commit that referenced this pull request Jul 7, 2013

@alindeman

This comment has been minimized.

Show comment
Hide comment
@alindeman

alindeman Jul 7, 2013

Contributor

I'm still really confused. This is not about Capybara using RSpec for its own test suite; instead, the problem is that Capybara integrates with RSpec ... adding methods and hooks to example groups for users to drive web browsers.

When you bring in Capybara to your application or library with require 'capybara/rspec', Capybara adds hooks to your own specs that use example.

The issue is how we make those hooks compatible with both RSpec <= 2.14 and 3, without creating a rift in Capybara itself (where Capybara has to release a version that's only compatible with RSpec >= 2.14, 3). If I were a maintainer of Capybara, I'd be pretty annoyed if RSpec made me do this, since my integration with RSpec is somewhat incidental to my gem's purpose.

I think RSpec.current_example is a way to do this, but I'm open to other ideas. What do you think?

Contributor

alindeman commented Jul 7, 2013

I'm still really confused. This is not about Capybara using RSpec for its own test suite; instead, the problem is that Capybara integrates with RSpec ... adding methods and hooks to example groups for users to drive web browsers.

When you bring in Capybara to your application or library with require 'capybara/rspec', Capybara adds hooks to your own specs that use example.

The issue is how we make those hooks compatible with both RSpec <= 2.14 and 3, without creating a rift in Capybara itself (where Capybara has to release a version that's only compatible with RSpec >= 2.14, 3). If I were a maintainer of Capybara, I'd be pretty annoyed if RSpec made me do this, since my integration with RSpec is somewhat incidental to my gem's purpose.

I think RSpec.current_example is a way to do this, but I'm open to other ideas. What do you think?

@dchelimsky

This comment has been minimized.

Show comment
Hide comment
@dchelimsky

dchelimsky Jul 7, 2013

Member

The issue is how we make those hooks compatible with both RSpec <= 2.14 and 3, without creating a rift in Capybara itself

@alindeman I don't think that the addition of RSpec.current_example is going to save 3rd party libs that use example from having to make changes to function with both rspec 2 and 3. They'll still have to something like:

if RSpec.respond_to?(:current_example)
  # use RSpec.current_example
else
  # use the local example method
end

Right?

Member

dchelimsky commented Jul 7, 2013

The issue is how we make those hooks compatible with both RSpec <= 2.14 and 3, without creating a rift in Capybara itself

@alindeman I don't think that the addition of RSpec.current_example is going to save 3rd party libs that use example from having to make changes to function with both rspec 2 and 3. They'll still have to something like:

if RSpec.respond_to?(:current_example)
  # use RSpec.current_example
else
  # use the local example method
end

Right?

@alindeman

This comment has been minimized.

Show comment
Hide comment
@alindeman

alindeman Jul 7, 2013

Contributor

Right?

Yep, right. But at least there's an option, even if it's a conditional.

If we keep it as it is right now, there's nothing that doesn't feel very fragile and ugly (as far as I can tell anyway). I don't like this because there are a lot of gems that integrate optionally with RSpec (so you don't get rubygems backing up the version constraint) and because many gems integrate with RSpec as a convenience for users, even when that's not a core feature. Making a breaking change like this and giving no good option to support both versions feels like we're not being a very good citizen.

Contributor

alindeman commented Jul 7, 2013

Right?

Yep, right. But at least there's an option, even if it's a conditional.

If we keep it as it is right now, there's nothing that doesn't feel very fragile and ugly (as far as I can tell anyway). I don't like this because there are a lot of gems that integrate optionally with RSpec (so you don't get rubygems backing up the version constraint) and because many gems integrate with RSpec as a convenience for users, even when that's not a core feature. Making a breaking change like this and giving no good option to support both versions feels like we're not being a very good citizen.

@dchelimsky

This comment has been minimized.

Show comment
Hide comment
@dchelimsky

dchelimsky Jul 7, 2013

Member

Yep, right. But at least there's an option, even if it's a conditional.
If we keep it as it is right now, there's nothing that doesn't feel very fragile and ugly (as far as I can tell anyway).

I agree we don't have a better alternative at the moment. I read your concern about Capybara to mean that you thought there was a way that Capy wouldn't need any changes at all to properly support rspec-3.

I don't like this because there are a lot of gems that integrate optionally with RSpec (so you don't get rubygems backing up the version constraint) and because many gems integrate with RSpec as a convenience for users, even when that's not a core feature. Making a breaking change like this and giving no good option to support both versions feels like we're not being a very good citizen.

Agree with all of that ^^ as well, so I think we're on the same page at this point re: RSpec.current_example: we should add it unless we can come up with a better option for lib authors. That is a separate issue from end-users who use libs that haven't adapted, which could be resolved with config.expose_current_example_as :xxx, which should be documented as an interim measure. You on board w/ that @alindeman?

Member

dchelimsky commented Jul 7, 2013

Yep, right. But at least there's an option, even if it's a conditional.
If we keep it as it is right now, there's nothing that doesn't feel very fragile and ugly (as far as I can tell anyway).

I agree we don't have a better alternative at the moment. I read your concern about Capybara to mean that you thought there was a way that Capy wouldn't need any changes at all to properly support rspec-3.

I don't like this because there are a lot of gems that integrate optionally with RSpec (so you don't get rubygems backing up the version constraint) and because many gems integrate with RSpec as a convenience for users, even when that's not a core feature. Making a breaking change like this and giving no good option to support both versions feels like we're not being a very good citizen.

Agree with all of that ^^ as well, so I think we're on the same page at this point re: RSpec.current_example: we should add it unless we can come up with a better option for lib authors. That is a separate issue from end-users who use libs that haven't adapted, which could be resolved with config.expose_current_example_as :xxx, which should be documented as an interim measure. You on board w/ that @alindeman?

@alindeman

This comment has been minimized.

Show comment
Hide comment
@alindeman

alindeman Jul 7, 2013

Contributor

Agree with all of that ^^ as well, so I think we're on the same page at this point re: RSpec.current_example: we should add it unless we can come up with a better option for lib authors. That is a separate issue from end-users who use libs that haven't adapted, which could be resolved with config.expose_current_example_as :xxx, which should be documented as an interim measure. You on board w/ that @alindeman?

Yes, these two things sound good to me :) I'm sorry for all the misunderstandings throughout this thread.

I'll make a PR for RSpec.current_example shortly. I'll open an issue for expose_current_example_as to make sure it doesn't get lost too.

Contributor

alindeman commented Jul 7, 2013

Agree with all of that ^^ as well, so I think we're on the same page at this point re: RSpec.current_example: we should add it unless we can come up with a better option for lib authors. That is a separate issue from end-users who use libs that haven't adapted, which could be resolved with config.expose_current_example_as :xxx, which should be documented as an interim measure. You on board w/ that @alindeman?

Yes, these two things sound good to me :) I'm sorry for all the misunderstandings throughout this thread.

I'll make a PR for RSpec.current_example shortly. I'll open an issue for expose_current_example_as to make sure it doesn't get lost too.

@yujinakayama yujinakayama referenced this pull request in yujinakayama/transpec Nov 13, 2013

Closed

Support conversion of RSpec::Core::ExampleGroup#example #23

shin1ohno added a commit to shin1ohno/autodoc that referenced this pull request Dec 19, 2013

Make Rspec 3 compliant
As of RSpec 3, we can't call example in test.
see discussion: rspec/rspec-core#666

@shin1ohno shin1ohno referenced this pull request in r7kamura/autodoc Dec 19, 2013

Closed

Make Rspec 3 compliant #7

yuya373 pushed a commit to yuya373/autodoc that referenced this pull request Oct 24, 2014

Make Rspec 3 compliant
As of RSpec 3, we can't call example in test.
see discussion: rspec/rspec-core#666

@junaruga junaruga referenced this pull request in teamcapybara/xpath May 24, 2016

Closed

Update to RSpec 3 #73

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