Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from Aug 30, 2011

Conversation

Projects
None yet
5 participants
Contributor

ebeigarts commented Aug 22, 2011

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

Collaborator

jnicklas commented Aug 22, 2011

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.

Contributor

ebeigarts commented Aug 22, 2011

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

Contributor

ebeigarts commented Aug 22, 2011

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

@ebeigarts ebeigarts closed this Aug 22, 2011

dblock commented Aug 23, 2011

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!

@ebeigarts ebeigarts reopened this Aug 23, 2011

Collaborator

jnicklas commented Aug 23, 2011

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.

Contributor

dchelimsky commented Aug 23, 2011

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 :)

dblock commented Aug 23, 2011

@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.

dblock commented Aug 23, 2011

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

Contributor

ebeigarts commented Aug 23, 2011

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

Contributor

dchelimsky commented Aug 23, 2011

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?

Contributor

dchelimsky commented Aug 23, 2011

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

Contributor

ebeigarts commented Aug 23, 2011

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.

Contributor

ebeigarts commented Aug 24, 2011

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
Contributor

dchelimsky commented Aug 24, 2011

@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?

Contributor

ebeigarts commented Aug 25, 2011

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

dblock commented Aug 30, 2011

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.

Collaborator

jnicklas commented Aug 30, 2011

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

@jnicklas jnicklas added a commit that referenced this pull request Aug 30, 2011

@jnicklas jnicklas Merge pull request #463 from ebeigarts/master
Make sure that exit codes of tests are propagated properly when using selenium webdriver.
52dbbf6

@jnicklas jnicklas merged commit 52dbbf6 into teamcapybara:master Aug 30, 2011

@radar radar added a commit to reinteractive/capybara that referenced this pull request Nov 1, 2011

@ebeigarts @radar ebeigarts + radar Tests for selenium driver exit codes. #463 cf79901

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?

Contributor

ebeigarts commented Jul 28, 2012

Have you tried searching for "at_exit" in poltergeist and also in the dependent gems or maybe its not poltergeist?

I am also using poltergeist, but the exit codes are correct.

On 28 Jul 2012, at 17:03, Mikael Henrikssonreply@reply.github.com wrote:

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?


Reply to this email directly or view it on GitHub:
jnicklas#463 (comment)

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

Contributor

ebeigarts commented Jul 28, 2012

In the sorce code, this is the actual bug - http://redmine.ruby-lang.org/issues/5218

On 28 Jul 2012, at 17:12, Mikael Henrikssonreply@reply.github.com wrote:

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


Reply to this email directly or view it on GitHub:
jnicklas#463 (comment)

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

@robertodecurnex robertodecurnex referenced this pull request in cucumber/cucumber-ruby Apr 25, 2013

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