Introduce API for global configuration

[jferris]

Users were generally confused about where to configure things like
allowed URLs. Because they were reset in between each sessions, they
needed to be applied repeatedly in a before block.

This introduces an API for global configuration, which will be applied
for every session. It also deprecates the per-session configuration
methods, as those are less likely to be useful.

[mhoran]

I was surprised that configure works within an it block, but after reading through AppRunner (specifically, AppRunner.reset and AppRunner.included, I see how it does. Regardless, this is a bit confusing. However, this is a specific use case within our own specs, so I could be persuaded that it's all right.

[mhoran]

To elaborate: there's a lot of hidden magic here due to the fact that the driver is lazily initialized via the let block (with the driver itself hidden within AppRunner.) Previously some of that magic was exposed via explicit configuration directives. Now, AppRunner transparently resets the configuration hash between specs via an include block and then configures the driver on first request due to RSpec magic. Again, the straightforwardness of the test is nice -- but it certainly confused me on first glance.

[jferris]

Our specs have become sort of a DSL of their own on top of RSpec (driver_for_*, etc). I think they're generally in a pretty bad state (driver_spec.rb in particular). I can't think of a better way to mix configuration into the existing driver bootstrap methods, though.

I'd like to take a pass at reorganizing the specs in general. I think they do need to have a sort of specialty DSL because of the overhead in booting drivers, etc, but I'd like to at least make them more consistent.

However, I think all that has to take place outside this pull request.

[mhoran]

This looks great! I left a couple of comments in driver_spec.rb. Also, it'd be great if this were standardized in upstream Capybara. I believe the Poltergeist folks are tying to figure out how to configure their blacklist (and they're working on a whitelist, from what I understand.) Thoughts, @twalpole?

[twalpole]

I'm all for standardizing driver configuration - as long as it's something that allows for multiple driver use in a given set of tests. I haven't had a chance to read through this PR yet, but will get to it this weekend. Any suggestions for what Capybara should support are more than welcome.

[jferris]

@mhoran @twalpole

This is making it easier to support debug and the whitelist/blacklist methods, which aren't officially in the Capybara API. Currently, the most common approach to these options seems to be passing them to a custom driver registration block, but I've never enjoyed using that API.

I think there a few ways we could go from here:

• Decide the driver registration block is the best way to go for driver-specific options and use that in capybara-webkit.
• Try to introduce a new API in Capybara for setting driver-specific configuration.
• Try to move all these features into Capybara so that driver-specific config isn't necessary. This would be better in the long term, but I think having something in place for driver extensions is useful, because the Capybara API will always be out of sync with drivers.

[twalpole]

@jferris As you've basically stated, I think a combination of a new API in Capybara for setting driver-specific config and moving features into Capybara as soon as possible is the best solution. Do you have any suggestions for the new API?

[jferris]

@twalpole ideas for configuration:

Capybara.configure_driver :webkit do |config|
  config.debug = true
end

# OR

Capybara.configure do |config|
  config.driver :webkit do |webkit|
    webkit.debug = true
  end
end

I think I prefer the first example.

From a driver authoring perspective, it would be most convenient if the configuration object was passed to the driver registration block. Maybe we could extend register_driver to take an optional configuration object? Then we could do:

Capybara.register_driver(:webkit, Capybara::Webkit::Configuration.new) do |app, config|
  Capybara::Webkit::Driver.new(app, config)
end

[jferris]

@mhoran any thoughts on whether we should merge this? I could see going two ways:

• Just merge the driver changes so that you can globally configure. Users would have to use the register_driver approach and pass a hash of options. This lets us wait until Capybara's config API is resolved.
• Merge this as-is, and support Capybara's config API if/when one is released.

Thoughts? I'd love to move forward with the release and get to the point where we can stop worrying about Qt 4.

[twalpole]

@jferris Sorry - I completely missed your post from 4/24 - I'll try and get something into Capybara this weekend

[jferris]

@mhoran I found a few more settings lurking in the Browser class and moved them into the new Configuration object.

Part of my hope here is that we can eventually send this to the driver on initialization in a single command, as JSON, which I think would eliminate a bunch of cruft around config.

[twalpole]

@jferris Implementing the driver config stuff currently -- do you see a real advantage to the configuration info being a configuration block, as opposed to just a hash of configuration options?

[jferris]

Do you see a real advantage to the configuration info being a configuration block, as opposed to just a hash of configuration options?

I find it easier to manage the API for objects. You can do things like alias, deprecate, keep the defaults somewhere sane, and inspect the configuration. Hashes are always just hashes.

[mhoran]

👍 for a block over a hash. I think this leaves the API more open to driver developers as well, while standardizing the actual interface itself.

[jferris]

@mhoran any further thoughts on this? I think I'd like to go ahead with the current syntax. We can still support a standardized syntax if one becomes available. I think this is the only thing blocking 1.6, and then I can get started on 2.0, where we finally ditch a bunch of baggage.

[mhoran]

Let's go with this for now; I like it!

[mhoran]

Hey @jferris. I was approached by a coworker who just upgraded to 1.6.0 and was surprised by the new whitelist configuration mechanism. She was using tags to configure whitelisting on a per test basis. We talked it through, and it seems this is no longer possible (well, it is, but it's difficult and confusing).

Perhaps we should introduce another way to add one-off entries to the whitelist, and clear them out? I'm not sure what this should look like, given there was previous confusion around how to use the whitelist.

[jferris]

@mhoran What do you think about something like:

page.driver.reconfigure do |config|
   config.allow_url("x")
end

I'd be interested to know more about the use case. It seems like a smell that you'd need to allow different requests for different tests.

[mhoran]

After talking it through, it sounds like a smell. They were trying to optimize the whitelist and only allow URLs as required for certain tests. However, adding all the URLs to the global whitelist actually sped up the tests.

However, another interesting issue came to light. They'd actually registered a bunch of additional drivers: one for Puffing Billy, and another for an accessibility driver. They'd also overridden the default :webkit driver to provide options to the previous API. This meant when they moved over to the Capybara::Webkit.configure block, the configuration did not take effect.

Our default :webkit driver pulls in the Capybara::Webkit::Configuration via its #to_hash method. However, Those who have registered their own drivers will not have visibility into why their Capybara::Webkit.configuration block is not affecting the driver.

Perhaps we could extend Driver#initialize to raise a deprecation warning if a non-Configuration hash is provided, instead of the Configuration object. While it's not pretty, I'd like to make sure folks are aware of the changes instead of finding themselves frustrated and confused.

[jferris]

Perhaps we could extend Driver#initialize to raise a deprecation warning if a non-Configuration hash is provided, instead of the Configuration object.

Yes, I think this is a good idea. I have some ideas for cleaning up configuration in 2.0, and deprecating use of hashes for configuration will help there.

[jferris]

I opened #806 to track the configuration improvements.