Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Make sure that exit codes of tests are propagated properly when using selenium webdriver. #463

Merged
merged 3 commits into from

5 participants

Edgars Beigarts Jonas Nicklas Daniel Doubrovkine (dB.) @dblockdotorg David Chelimsky Mikael Henriksson
Edgars Beigarts

If you run the rspec test suite with capyabara with selenium driver it will always return exit status code 0.
I have fixed the at_exit part in selenium driver the same way that it is fixed in colszowka/simplecov#5

Jonas Nicklas
Owner

Do you have something which can verify that this was broken and is now fixed? Doesn't have to be a unit test, a shell script or soemthing would be fine.

Edgars Beigarts

At least for me these test cases fail without my patch on Ruby 1.9.2-p290 under OS X.

Edgars Beigarts

I just found out that it has already been reported - #178.
And it is fixed in rspec side rspec/rspec-core#410

Edgars Beigarts ebeigarts closed this
Daniel Doubrovkine (dB.) @dblockdotorg

It's not fixed on the RSpec side. There's a giant thread of how RSpec is failing to fix it (the nested at_exit blocks are causing all kinds of pain with rspec autorun). +10 to merge this!

Edgars Beigarts ebeigarts reopened this
Jonas Nicklas
Owner

Ehh, guy, I'm confused. I have no idea what's going on. Do I merge this? Feels like a hack upon a hack, and I don't like that. I don't like how this whole situation is risking getting even more convoluted.

David Chelimsky

As I understand it, quit calls @browser.quit, which stops the child process if it's still running. This code bypasses that call, which means child processes would be left running. I may be wrong, but that would clearly be an unacceptable outcome.

That said, we've been unable to find an acceptable solution within rspec for this, so it would be great to see it addressed in Capybara (or in Childprocess). Of course, I have no ideas on how :)

Daniel Doubrovkine (dB.) @dblockdotorg

@jnicklas: I'll try to help.

The root failure is rspec + capybara + selenium driver eats the process exit code. So when a spec fails rspec says exit 1, then on cleanup capybara says no, no, exit 0. This is happening in @browser.kill, although I am not sure how. IMO, it's a bug.

In RSpec we tried a workaround of saying capybara sets exit code last, nomatter what. It worked around this particular problem, but broke existing functionality. We can't figure out a fix that works for all scenarios. Saying my exit code is last is tough because we end up in a nested at_exit block that execute in some weird order in Ruby.

You're the boss to Capybara to figure out what the best way to proceed is. I think this or a variant of this fix should be committed, Capybara shouldn't be setting the process exit code ever, it's not in the business of doing that.

Daniel Doubrovkine (dB.) @dblockdotorg

@dchelimsky: the code in this patch saves the old exit code and restores it afterwards, it does quit the browser

Edgars Beigarts

I just tested this and the browser is closing on exit.

David Chelimsky

To be clear, it is Childprocess that is actually setting the process exit code, not Capybara. That said, it seems reasonable to me that Childprocess should be in the business of setting process exit codes, and that Capy has a responsibility to prevent that. Maybe Childprocess is missing an API for this?

David Chelimsky

@dblock - ah, yes - I misread the patch.

Edgars Beigarts

I did some digging around and it seems that ruby doesn't like that childprocess uses raise and rescue within at_exit so I opened a new issue at http://redmine.ruby-lang.org/issues/5218

I don't know if this is intended, but it looks like a ruby bug to me.

Edgars Beigarts

To fix ruby 1.9 I came up with this workaround and added this to my spec_helper.rb and it works

if defined?(RUBY_ENGINE) && RUBY_ENGINE == "ruby" && RUBY_VERSION >= "1.9"
  module Kernel
    alias :__at_exit :at_exit
    def at_exit(&block)
      __at_exit do
        exit_status = $!.status if $!.is_a?(SystemExit)
        block.call
        exit exit_status if exit_status
      end
    end
  end
end
David Chelimsky

@ebeigarts - I don't think the redef on Kernel belongs in Capybara as it has wide-reaching impact on the runtime and Capybara users would be unable to control it. It might, however, make sense as an at-exit-fix gem (especially in light of the fact that it's being fixed in future versions of Ruby). WDYT?

@jnicklas - does @dblock's explanation help you to understand this patch any better?

Edgars Beigarts

I think there is no easy/clean solution for this, it will be a hack anyway.

  1. Create an at-exit-fix gem and add this to capybara dependancies (this will also have wide-reaching impact on the runtime)
  2. Create an at-exit-fix gem and leave this up to users to include this additional gem if they are using MRI ruby 1.9 and add some note about this in the README
  3. Just merge this pull request
Daniel Doubrovkine (dB.) @dblockdotorg

I am a huge fan of monkey patching. I say hack it (3). I think @jnicklas should spend a few minutes understanding what's at stake here (poor users can't get CI to work properly) and decide what to do. In the meantime @ebeigarts could make a gem, but I would call it at-exit-code-warrior - the real functionality of this code is not to overwrite the previous exit code if set to non-zero.

Jonas Nicklas
Owner

Okay, I will merge this. If the shit hits the fan, we can always pull this later on.

Jonas Nicklas jnicklas merged commit 52dbbf6 into from
Ryan Bigg radar referenced this pull request from a commit in reInteractive/capybara
Edgars Beigarts ebeigarts Tests for selenium driver exit codes. #463 cf79901
Mikael Henriksson

I am having this problem using Capybara 1.1, RSpec 2.11.1 and poltergeist 0.7.0. Problem is I have no idea where to start looking I just know that the exit code is zero and I can't figure out why. I collected a few links where of the top one is for an issue with RSpec that was closed.

Could someone push me in the right direction of finding out what is causing the exit code to always be zero?

Edgars Beigarts
Mikael Henriksson

@ebeigarts sorry about being daft but I'm not following. Are you referring to log output or source code?

Edgars Beigarts
Mikael Henriksson

I get it, just add the monkey patch to the spec_helper.
Thanks for that! I truly love working in dynamic and open platforms! :)

Roberto Decurnex robertodecurnex referenced this pull request in cucumber/cucumber
Closed

at_exit is not an AfterAll alternative #441

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
3  lib/capybara/selenium/driver.rb
View
@@ -16,7 +16,10 @@ def browser
main = Process.pid
at_exit do
+ # Store the exit status of the test run since it goes away after calling the at_exit proc...
+ @exit_status = $!.status if $!.is_a?(SystemExit)
quit if Process.pid == main
+ exit @exit_status if @exit_status # Force exit with stored status
end
end
@browser
21 spec/driver/selenium_driver_spec.rb
View
@@ -26,4 +26,25 @@
browser.quit
end
end
+
+ describe "exit codes" do
+ before do
+ @current_dir = Dir.getwd
+ Dir.chdir(File.join(File.dirname(__FILE__), '..', '..'))
+ end
+
+ after do
+ Dir.chdir(@current_dir)
+ end
+
+ it "should have return code 1 when running selenium_driver_rspec_failure.rb" do
+ `rspec spec/fixtures/selenium_driver_rspec_failure.rb`
+ $?.exitstatus.should be 1
+ end
+
+ it "should have return code 0 when running selenium_driver_rspec_success.rb" do
+ `rspec spec/fixtures/selenium_driver_rspec_success.rb`
+ $?.exitstatus.should be 0
+ end
+ end
end
8 spec/fixtures/selenium_driver_rspec_failure.rb
View
@@ -0,0 +1,8 @@
+require 'spec_helper'
+
+describe Capybara::Selenium::Driver do
+ it "should exit with a non-zero exit status" do
+ browser = Capybara::Selenium::Driver.new(TestApp).browser
+ true.should == false
+ end
+end
8 spec/fixtures/selenium_driver_rspec_success.rb
View
@@ -0,0 +1,8 @@
+require 'spec_helper'
+
+describe Capybara::Selenium::Driver do
+ it "should exit with a zero exit status" do
+ browser = Capybara::Selenium::Driver.new(TestApp).browser
+ true.should == true
+ end
+end
Something went wrong with that request. Please try again.