Add Capybara 2.0 support

[brutuscat]

See: #136 Capybara 2.0 support

Things changed:

• Server#url is no more. It's a Session responsibility and happens transparently from the Driver's point of view.
• Session is responsible for starting the server
• Use Capybara::SpecHelper.run_specs to run validate all specs with the driver
• render is now save_screenshot (for a failing spec)
• set_cookie needed a domain in specs eg: :domain => '127.0.0.1' (for a failing spec)
• Normalized whitespace stripping much like capybara-webkit is doing (for a failing spec)

All but one capybara's spec are passing:

• HTML5 Multiple File Upload: This is from the capybara specs and also didn't found proper documentation in phantomjs that would lead me into having this fixed...

@jonleighton I'm calling for help here to fix multiple file uploads

[mhenrixon]

Thanks man, you got a whole lot further than I did! Really looking forward to getting the last spec fixed. I might give it a try this weekend if work situation is willing.

[brutuscat]

@mhenrixon no problem ;) Please give it a try to confirm everything still works as expected

[pjg]

Good work!

[jonleighton]

Great job.

Regarding multiple-file upload, I assume this test actually tries to upload two files with one file picker? Unfortunately that's not supported by PhantomJS yet. I thought it was supported in master but it doesn't look like it. So I will probably need to work on adding it to PhantomJS (or somebody here is welcome to have a go, but you'll need to mess around with Qt and C++).

In the meantime, is it possible to ignore the test / mark as pending for now? I don't know if the new stuff in Capybara 2.0 permits this.

[jonleighton]

Also, let's not remove support for calling render. This just adds annoying work for people who are upgrading. Let's just alias save_screenshot to render.

[brutuscat]

@jonleighton I was trying to turn it off, but because those specs are not tagged, currently we can't. We need capybara to tag those specs for us and then we can add :skip => [:the_tag] when calling #run_specs

[brutuscat]

Rebased with aliased save_screenshot and a couple of small details

[timonv]

Awesome!

[carlosantoniodasilva]

I think I'd rather stick with ~> 2.0 here, wdyt?

[carlosantoniodasilva]

Nice @brutuscat 👍

[timonv]

@carlosantoniodasilva Capybara 2.0 had some quirks that can be a bit uncanny. See diff here teamcapybara/capybara@2.0.0...2.0.1

[carlosantoniodasilva]

@timonv Ok, got it, but any bundle update would give you 2.0.1 anyway. It's just that I don't feel safe using >=, because that opens the doors for any version :).

[timonv]

Can't disagree on that, +1 on making it ~>

[brutuscat]

There you go @carlosantoniodasilva
I use it >= because I took webkit's driver as a model. But yes I guess all of us are going to get 2.0.1 anyway ;)

[carlosantoniodasilva]

@brutuscat ❤️

Lets wait @jonleighton thoughts now :)

[m4tm4t]

work great here, good job

[mhoran]

There were slight API changes between 2.0.0 and 2.0.1, which is why capybara-webkit locks at 2.0.1. It's technically incorrect for the gemfile to be compatible with 2.0.0 because of those API changes. It probably doesn't matter much, except for those using the #source/#html methods, or node equality.

[jonleighton]

can't we do this:

s.add_dependency "capybara", "~> 2.0", ">= 2.0.1"

?

[brutuscat]

It looks like we can http://rubygems.rubyforge.org/rubygems-update/Gem/Specification.html#method-i-add_runtime_dependency

EDIT

I see you are doing it with faye-websocket

[jonleighton]

It's a nitpick, but please insert a newline above the start of the list to be consistent (and the same for the "Features" bit below)

[jonleighton]

Also, I consider Capybara 2 support to be a feature, not a bug fix ;)

[jonleighton]

PhantomJS 1.8 will be release around 20th December. I am going to try and see if I can implement support for multiple file upload and get it in the release.

[carlosantoniodasilva]

Awesome news :)

[mhoran]

@jonleighton, not sure if it's of much help given the architectural differences between PhantomJS and capybara-webkit, but you can see how I implemented multiple file upload here: thoughtbot/capybara-webkit@ea2f785.

[jonleighton]

@mhoran thanks, it's a useful demonstration of how to use QtWebKit's crazy API ;)

[jonleighton]

Multiple file upload implemented in ariya/phantomjs#357.

[jonleighton]

The patch is now merged into phantomjs and so will be supported by 1.8.

@brutuscat, if you are keen to update your PR you could compile phantomjs master and get it passing. or we can just wait until 1.8 is out.

[brutuscat]

@jonleighton I have no idea what to do actually. I guess it would be better to wait till 1.8 my env is kind of broken and I couldn't compile it so...

Let me know what I should do to push this ;)

[jonleighton]

@brutuscat ok, let's wait for 1.8 to be out. it's only a few days away.

[rb2k]

Aaaand it's out :)

[jonleighton]

Regarding that failure, see #228.

[krainboltgreene]

What can be done to help this get fixed? I want Capybara 2 and Poltergeist. I have time and energy.

[aaronchi]

There is a bug in poltergeist cookie handling:

def expires
  Time.parse @attributes['expires']
end

This fails if no expires value is passed in. It should handle a nil/blank value

[kossnocorp]

Can't wait ⏰!

[brutuscat]

Tested with phantomjs 1.8.1 and all specs passing now.

• Add the capybara configuration line at the end of the config block.

Also synced with master (readme was conflicting)

@jonleighton could you confirm that all specs are passing? It seems that Travis doesn't like non fast-forward commits(?)

[brutuscat]

Travis is failing for rubies v1.8 https://travis-ci.org/brutuscat/poltergeist

[mhenrixon]

Awesome work @brutuscat! I do however wonder why the test suite has gotten so slow though. Is it something capybara did or is it new phantomjs version or is there something we can do in the tests?

[allenwyma]

@brutuscat
Uhm, yeah of course it should. 2.0 dropped support for below 1.9 series of Ruby: https://github.com/jnicklas/capybara/blob/master/capybara.gemspec

So take out 1.8 for Travis and you're fine.

[brutuscat]

@mhenrixon I think I read somewhere that Capybara server is not using Thin anymore because of being not thread safe or something like that...

@HangingClowns is good to know that. Thanks! Will see what @jonleighton says about it

[brutuscat]

@mhenrixon You can see it here in the 2.0.1 compare against 2.0.2 teamcapybara/capybara@2.0.1...2.0.2#L3R177

[mhenrixon]

Since we are using unicorn for hosting our app on heroku I thought I should give it a whirl.

Capybara.server do |app, port|
  Unicorn::Configurator::RACKUP[:port] = port
  Unicorn::Configurator::RACKUP[:set_listener] = true

  server = Unicorn::HttpServer.new(app)
  server.start
end

That shaves of between 15-20 seconds every minute on our test suite but it's still much much slower than poltergeist on capybara 1 with thin and when I tried running it against the poltergeist features I had a lot of failures.

[jonleighton]

@brutuscat just ran the test and everything looks good except this performance issue. we really need to resolve the performance problem before we can release this. I'll try to look at it but any help is appreciated.

[jonleighton]

Profiling reveals a lot of time is being spent in Capybara::Node::Base#synchronize. Also, the slowest specs are:

$ be rspec spec/ --profile
Run options: exclude {:requires=>#<Proc:0x000000017ae1e8>}
..............................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................

Top 10 slowest examples (41.01 seconds, 13.0% of total time):
  Capybara::Session Poltergeist #has_select? with selected value should be false if the given field is not on the page
    6.31 seconds 
  Capybara::Session Poltergeist #has_select? with exact options should be false if the given field is not on the page
    5.18 seconds 
  Capybara::Session Poltergeist #has_no_field with value should be false if a field with the given value is on the page
    4.22 seconds 
  Capybara::Session Poltergeist #has_css? with minimum should be false when content occurs fewer times than given
    4.19 seconds 
  Capybara::Session Poltergeist #has_xpath? with count should be false if the content occurs a different number of times than the given
    4.17 seconds 
  Capybara::Session Poltergeist #has_no_xpath? with count should be false if the content occurs the given number of times
    4.16 seconds 
  Capybara::Session Poltergeist #has_no_select? should be false if the field is on the page
    3.2 seconds 
  Capybara::Session Poltergeist #has_no_select? with selected value should be false if a field with the given value is on the page
    3.2 seconds 
  Capybara::Session Poltergeist #has_no_css? with between should be false if the content occurs within the range given
    3.19 seconds 
  Capybara::Session Poltergeist #has_no_selector? with count should be false if the content is on the page the given number of times
    3.19 seconds 

Finished in 5 minutes 16.65 seconds
590 examples, 0 failures

Seems like there are a number of specs that are taking a long time synchronising.

BTW I don't think this is to do with using/not using thing as the web server. Thin is not used for Poltergeist master's tests.

[jonleighton]

It looks like Capybara now sets default_wait_time = 1: https://github.com/jnicklas/capybara/blob/master/lib/capybara/spec/spec_helper.rb#L21

Previously, we were using a 0 wait time everywhere.

Monkey-patching that method to set it to 0 makes the tests run at normal speed again. But it also results in some failures.

I need to investigate why they've made this change but I've run out of time for tonight.

[mhoran]

Looks like the timeout was changed here: teamcapybara/capybara@0b28aa2. The commit message confirms the flickering failures.

[brutuscat]

@jonleighton that´s correct. I've taken a look at my slowest test and I can see that there is one second of delay per assert:

https://github.com/jnicklas/capybara/blob/master/lib/capybara/spec/session/has_select_spec.rb#L24

Capybara::Session Poltergeist #has_select? with selected value should be false if the given field is not on the page
    6.29 seconds

[brutuscat]

Ok, now all is green again

Finished in 1 minute 19.33 seconds
590 examples, 0 failures

What I did was to Capybara.default_wait_time = 1 only for :js tagged tests.

Comments?

[mhenrixon]

I think that's ok then, since poltergeist is faster than selenium by default I don't think that should affect us much. I don't know about you but those minutes really count!

[allenwyma]

@brutuscat I'm currently using your branch and it's not quite working like how it should be. This scenario works fine on Selenium, but seems to error out on yours. I have a complete Backbone app on top of a Rails backend, so it would be nice to switch to poltergeist, but if poltergeist keeps failing, then it's kind of a pain in the butt to have to wait on selenium.

According to Capybara 2, they removed the wait_until, and the waiting is supposed to be better, but poltergeist doesn't seem to be waiting at all. It's the same error I have for Issue #239. As you can see, I've also tried the find_button instead, but still no use. In fact, I'm getting a Capybara::NotSupportedByDriverError (Capybara::NotSupportedByDriverError) when I use that find_button and trigger click instead of the traditional way.

Here is my step:

Given /^I am logged in$/ do
  visit('/login')
  fill_in "user_email", :with => @user.email
  fill_in "user_password", :with => FactoryGirl.build(:user).password
  # find_button("Sign in").trigger('click')
  click_button("Sign in")
  sleep 1
  find_link('Login').should_not be_visible
  find('li.dropdown').should be_visible
end

[brutuscat]

@HangingClowns, guys if you take a look at the code I've "changed", I didn't introduce any behavioural change.

Particularly in your case, and from what I understand, you should not need the sleep at all. Are you sure you are "tagging" the case as "js" so the Poltergeist driver is being picked up?
Did you try finding the elements using #id instead of #value? I read that there were some changes in Capybara, that to simplify the lookup code, it would be better to scope things using within or do the lookup using an id (in case that there are duplicated stuff...)

[allenwyma]

Well, i know it's working this far because it took a screenshot when it fails with capybara-screenshot gem and it's always on that spot. I'm also positive that it's using capybara for more than one reason. Main reason is that the only HTML page I have is a blank template, and the screenshot has content put in from a Backbone view. The other reason I know It's using Poltergeist is because I've changed the default driver to poltergeist and the javascript driver as selenium. When I tag it with javascript, it passes with flying colors, when I untag it, it fails immediately after the button press. It doesn't seem to wait for the menus to switch.

I have 2 menus. Upon successful login, the "guest navbar" gets hidden and the "user navbar" becomes visible. Both items are on the same page already. Does it actually wait for this user navbar to become visible, like selenium or does it just immediately fail after clicking the button?

[jonleighton]

@mhoran thanks for digging out that commit. that's kinda interesting. I previously assumed that visit had to be synchronous since I did get failures like that before. So Poltergeist's visit is designed to be synchronous.

@brutuscat great work on reducing the test time! could you rebase the code please? then I think we're ready to merge!

@HangingClowns I reckon your issue is unrelated to this - best thing to do would be file a new bug report with steps to reproduce.

[allenwyma]

@jonleighton Already filed and linked at #239. The reason I mention it here as it is kind of covered under upgrading because I shouldn't have to sleep to make this pass. It could be dieing for another reason, since I also wasn't able to get it to pass before under 1.1.2.

[brutuscat]

@jonleighton it's already rebased right? Or you want me to merge in your last commit too? I did synced with master your changes because some conflicts in the README (that was 5 days ago)

[jonleighton]

weird, GH it saying " This pull request cannot be automatically merged. "

I'll try to merge it manually.

[jonleighton]

Merged, thank you everyone who helped with this!