Driver swapping on RSpec 2 #187

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
5 participants
@jeffkreeftmeijer
Contributor

jeffkreeftmeijer commented Nov 7, 2010

I built a really simple extension to Capybara called Swinger to allow quick driver swapping using RSpec's new metadata feature last week. You can read more about it in the article.

I wanted to just get it out there to use it myself and see if it needs any improvement before turning it into a patch for Capybara, so I released it as a gem first. After some small fixes, I turned it into a patch. What do you think? :)

@jnicklas

This comment has been minimized.

Show comment
Hide comment
@jnicklas

jnicklas Nov 8, 2010

Collaborator

Is monkey patching RSpec the only/right way of achieving this? Seems a bit potentially dangerous.

Collaborator

jnicklas commented Nov 8, 2010

Is monkey patching RSpec the only/right way of achieving this? Seems a bit potentially dangerous.

@jeffkreeftmeijer

This comment has been minimized.

Show comment
Hide comment
@jeffkreeftmeijer

jeffkreeftmeijer Nov 8, 2010

Contributor

I wanted to use RSpec's around filter to do something like this at first:

RSpec.configure do |c|
  c.around(:each) do |example|
    puts "set driver"
    example.run
    puts "reset driver"
  end
end

But since the example above is a proc and not an actual RSpec::Core::Example or RSpec::Core::ExampleGroup, I couldn't find a way to get the metadata to get the :driver.

Extending RSpec::Core::Example#run did allow me to get the metadata.

I'd love to know if anyone has a better way to solve this, though. :)

Contributor

jeffkreeftmeijer commented Nov 8, 2010

I wanted to use RSpec's around filter to do something like this at first:

RSpec.configure do |c|
  c.around(:each) do |example|
    puts "set driver"
    example.run
    puts "reset driver"
  end
end

But since the example above is a proc and not an actual RSpec::Core::Example or RSpec::Core::ExampleGroup, I couldn't find a way to get the metadata to get the :driver.

Extending RSpec::Core::Example#run did allow me to get the metadata.

I'd love to know if anyone has a better way to solve this, though. :)

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Nov 9, 2010

First let me put a disclaimer in place, I do not use RSpec, but I assume that whether or not it provides a ‘around-run’ hook has been investigated and the answer is ‘no’.

I personally think this kind of monkey-patching is safe enough, however I do understand your concern. This is what I would do:

  • accept the monkey-patch, for now
  • create a patch for RSpec adding a way to add a ‘around-run’ hook, which can then be discussed with the actual use case that’s in Capybara
  • remove monkey-patch from Capybara

Obviously the work should probably be done by Jeff, as he is the one using it :)

alloy commented Nov 9, 2010

First let me put a disclaimer in place, I do not use RSpec, but I assume that whether or not it provides a ‘around-run’ hook has been investigated and the answer is ‘no’.

I personally think this kind of monkey-patching is safe enough, however I do understand your concern. This is what I would do:

  • accept the monkey-patch, for now
  • create a patch for RSpec adding a way to add a ‘around-run’ hook, which can then be discussed with the actual use case that’s in Capybara
  • remove monkey-patch from Capybara

Obviously the work should probably be done by Jeff, as he is the one using it :)

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Nov 9, 2010

Ok, I tried to fix the list formatting, but I fail… :-/

alloy commented Nov 9, 2010

Ok, I tried to fix the list formatting, but I fail… :-/

@jnicklas

This comment has been minimized.

Show comment
Hide comment
@jnicklas

jnicklas Nov 9, 2010

Collaborator

Someone posted this on twitter: https://gist.github.com/669072

Seems like a much more elegant solution, unless I'm missing something? We could use this to enable both:

 it "foo", :driver => :akephalos
 it "foo", :js => true

From what I can see.

Collaborator

jnicklas commented Nov 9, 2010

Someone posted this on twitter: https://gist.github.com/669072

Seems like a much more elegant solution, unless I'm missing something? We could use this to enable both:

 it "foo", :driver => :akephalos
 it "foo", :js => true

From what I can see.

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Nov 9, 2010

If Swinger also only allows you to set a driver for a complete group, instead of per-example, then yes.

In your example you are using `it "foo", :driver => :akephalos', but that does not work if you add before and after hooks. I.e. they would run for each example in a group.

alloy commented Nov 9, 2010

If Swinger also only allows you to set a driver for a complete group, instead of per-example, then yes.

In your example you are using `it "foo", :driver => :akephalos', but that does not work if you add before and after hooks. I.e. they would run for each example in a group.

@jnicklas

This comment has been minimized.

Show comment
Hide comment
@jnicklas

jnicklas Nov 9, 2010

Collaborator

alloy: ye, but unless the example was tagged with the appropriate metadata they wouldn't actually do anything, no?

Collaborator

jnicklas commented Nov 9, 2010

alloy: ye, but unless the example was tagged with the appropriate metadata they wouldn't actually do anything, no?

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Nov 9, 2010

Ah yeah, you’re right, I looked to quick.

However, now that David has already picked it up, it’s probably better to do it that way, even if it internally would use the example from the gist. https://github.com/rspec/rspec-core/issues/issue/221

alloy commented Nov 9, 2010

Ah yeah, you’re right, I looked to quick.

However, now that David has already picked it up, it’s probably better to do it that way, even if it internally would use the example from the gist. https://github.com/rspec/rspec-core/issues/issue/221

@jeffkreeftmeijer

This comment has been minimized.

Show comment
Hide comment
@jeffkreeftmeijer

jeffkreeftmeijer Nov 9, 2010

Contributor

@jnicklas I'm not sure that will work, since I don't think it's able to access the example.metadata, which is the why Swinger can't use the around block: jnicklas#187 (comment)

@alloy Swinger allows you to set the driver per example and per group. :)

I'll send in a new pull request when RSpec has metadata in its around blocks: https://github.com/rspec/rspec-core/issues#issue/221 :)

Contributor

jeffkreeftmeijer commented Nov 9, 2010

@jnicklas I'm not sure that will work, since I don't think it's able to access the example.metadata, which is the why Swinger can't use the around block: jnicklas#187 (comment)

@alloy Swinger allows you to set the driver per example and per group. :)

I'll send in a new pull request when RSpec has metadata in its around blocks: https://github.com/rspec/rspec-core/issues#issue/221 :)

@unders

This comment has been minimized.

Show comment
Hide comment
@unders

unders Dec 10, 2010

Add RSpec support in Capybara itself, closed by aa46894

Just the basics of including Capybara and setting
up some metadata to switch between drivers.

unders commented Dec 10, 2010

Add RSpec support in Capybara itself, closed by aa46894

Just the basics of including Capybara and setting
up some metadata to switch between drivers.

@jnicklas

This comment has been minimized.

Show comment
Hide comment
@jnicklas

jnicklas Dec 10, 2010

Collaborator

So I just added some basic rspec support. It's pretty simple and doesn't freedom patch RSpec in any way. I'm pretty sure this should work as you intended. I've also merged and pushed the addition of using_driver, since it seemed just generally useful (though it isn't actually used by the rspec driver switching).

P.S.: The name on the commit is wrong, I forgot to switch my git info, I am the author.

Collaborator

jnicklas commented Dec 10, 2010

So I just added some basic rspec support. It's pretty simple and doesn't freedom patch RSpec in any way. I'm pretty sure this should work as you intended. I've also merged and pushed the addition of using_driver, since it seemed just generally useful (though it isn't actually used by the rspec driver switching).

P.S.: The name on the commit is wrong, I forgot to switch my git info, I am the author.

@cavalle

This comment has been minimized.

Show comment
Hide comment
@cavalle

cavalle Dec 11, 2010

Contributor

Good one. The only problem I see is that capybara/rspec will include capybara for any spec (i.e. in Rails, any model, controller or helper spec will include Capybara) which is not completely polite and might cause issues. In Steak, scenarios have the type metadata value set to acceptance, so you can include modules and configure hooks only for that type of specs.

Some discussion about this issue in Steak's mailing list: http://groups.google.com/group/steakrb/browse_thread/thread/28fa2b843e0239fd

Contributor

cavalle commented Dec 11, 2010

Good one. The only problem I see is that capybara/rspec will include capybara for any spec (i.e. in Rails, any model, controller or helper spec will include Capybara) which is not completely polite and might cause issues. In Steak, scenarios have the type metadata value set to acceptance, so you can include modules and configure hooks only for that type of specs.

Some discussion about this issue in Steak's mailing list: http://groups.google.com/group/steakrb/browse_thread/thread/28fa2b843e0239fd

@jnicklas

This comment has been minimized.

Show comment
Hide comment
@jnicklas

jnicklas Dec 11, 2010

Collaborator

Ahh, that seems like a good idea. Will have to fix that!

Collaborator

jnicklas commented Dec 11, 2010

Ahh, that seems like a good idea. Will have to fix that!

mcmire pushed a commit that referenced this pull request Apr 18, 2011

Anders Törnqvist
Add RSpec support in Capybara itself, closes #187
Just the basics of including Capybara and setting
up some metadata to switch between drivers.

This issue was closed.

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