Instance variables get reset when an exception is raised within before(:all) #558

Closed
pfleidi opened this Issue Jan 20, 2012 · 9 comments

Comments

Projects
None yet
5 participants
@pfleidi

pfleidi commented Jan 20, 2012

Hello everyone,

today, I was debugging some selenium webdriver test code and ran into the following problem: When an instance variable is set within the before(:all) hook and an exception is raised within the same hook, the instance variable is nil when acessed inside the after(:all) hook that gets called after the exception is raised.

Here is a simplified example:

require "rubygems"
require "bundler/setup"
require "rspec"

describe "Test" do

  before(:all) do
    @var = "I have some kind of state"
    raise "trolololo"
  end

  before(:each) do
    puts "Would run each ..."
  end

  after(:all) do
    puts @var # nil
    puts "Arghhhhhh" if @var.nil? # true
  end

  it 'would run a test' do
    puts "Would run test ..."
  end
end

We use the before(:all) hook to set up a selenium browser instance and kill it within after(:all), so we don't need to launch a new instance for every test. This works fine, as long as no exception is thrown within before(:all). Is this intended behavior or a bug? If it is intended: Is there a way to prevent a reset of the mentioned instance variables?

@justinko

This comment has been minimized.

Show comment Hide comment
@justinko

justinko Jan 22, 2012

Contributor

The ivars are not getting "reset", they're never getting "stored" because of the exception. Here are the steps that RSpec takes:

1.) Eval the before(:all) block.
2.) "store" the ivars produced from the block.

Step 2 is never called because of the exception in step 1. I started working on a fix for this, but then I started thinking "what benefit would this provide?". We shouldn't need to keep access to ivars if an exception happens in a before hook -- it is against the normal flow of how RSpec works. The ivars are available in the after hooks if the "flow" happens correctly (no exceptions were raised).

I understand the pain in your situation; you probably have to manually kill the selenium instance. Whatever you're holding in that ivar in the before(:all), you could probably hold it in a global variable and still have access to it in the after(:all) (even with an exception being raised).

@dchelimsky @myronmarston - do you guys think we should retain access to ivars in after(:all)'s even if an exception is raised in the before(:all)?

Contributor

justinko commented Jan 22, 2012

The ivars are not getting "reset", they're never getting "stored" because of the exception. Here are the steps that RSpec takes:

1.) Eval the before(:all) block.
2.) "store" the ivars produced from the block.

Step 2 is never called because of the exception in step 1. I started working on a fix for this, but then I started thinking "what benefit would this provide?". We shouldn't need to keep access to ivars if an exception happens in a before hook -- it is against the normal flow of how RSpec works. The ivars are available in the after hooks if the "flow" happens correctly (no exceptions were raised).

I understand the pain in your situation; you probably have to manually kill the selenium instance. Whatever you're holding in that ivar in the before(:all), you could probably hold it in a global variable and still have access to it in the after(:all) (even with an exception being raised).

@dchelimsky @myronmarston - do you guys think we should retain access to ivars in after(:all)'s even if an exception is raised in the before(:all)?

@pfleidi

This comment has been minimized.

Show comment Hide comment
@pfleidi

pfleidi Jan 27, 2012

Okay. That is nice to know. We've fixed the problem by manually catching the exceptions and dealing with it. Retaining access to the ivars in case of an exception would be great, nonetheless.

pfleidi commented Jan 27, 2012

Okay. That is nice to know. We've fixed the problem by manually catching the exceptions and dealing with it. Retaining access to the ivars in case of an exception would be great, nonetheless.

@justinko

This comment has been minimized.

Show comment Hide comment
@justinko

justinko Jan 27, 2012

Contributor

After thinking about it some more, it does make sense to make the ivars available, regardless if an exception was raised. We always "ensure" after hooks are run, so we should also ensure ivars are always available.

Contributor

justinko commented Jan 27, 2012

After thinking about it some more, it does make sense to make the ivars available, regardless if an exception was raised. We always "ensure" after hooks are run, so we should also ensure ivars are always available.

@ghost ghost assigned justinko Feb 1, 2012

@samphippen

This comment has been minimized.

Show comment Hide comment
@samphippen

samphippen Sep 23, 2012

Owner

yo, it seems to print the correct stuff out when the hook is wrapped in a simple begin rescue like this:
https://gist.github.com/3770411

Should I manufacture similar blocks for each of the other hooks and make a pull request?

Owner

samphippen commented Sep 23, 2012

yo, it seems to print the correct stuff out when the hook is wrapped in a simple begin rescue like this:
https://gist.github.com/3770411

Should I manufacture similar blocks for each of the other hooks and make a pull request?

@samphippen

This comment has been minimized.

Show comment Hide comment
@samphippen

samphippen Sep 23, 2012

Owner

This is the output from running with that patch applied https://gist.github.com/3770433

Owner

samphippen commented Sep 23, 2012

This is the output from running with that patch applied https://gist.github.com/3770433

@myronmarston

This comment has been minimized.

Show comment Hide comment
@myronmarston

myronmarston Sep 24, 2012

Owner

@samphippen -- Thanks for looking into this! I think rescuing errors like that will have the affect of silencing errors that happen in before(:all). When an error happens in a before(:all) hook, all examples in the example group (and any subgroups) should fail, as per the documented behavior, and I think your suggested change would change that.

Instead, I think we want to move the storing of the instance variables into an ensure clause so that it happens even if an error was raised. The change would be in run_before_all_hooks in example_group.rb.

Want to take a stab at that?

Owner

myronmarston commented Sep 24, 2012

@samphippen -- Thanks for looking into this! I think rescuing errors like that will have the affect of silencing errors that happen in before(:all). When an error happens in a before(:all) hook, all examples in the example group (and any subgroups) should fail, as per the documented behavior, and I think your suggested change would change that.

Instead, I think we want to move the storing of the instance variables into an ensure clause so that it happens even if an error was raised. The change would be in run_before_all_hooks in example_group.rb.

Want to take a stab at that?

@samphippen

This comment has been minimized.

Show comment Hide comment
@samphippen

samphippen Sep 24, 2012

Owner

On 24 Sep 2012, at 04:36, Myron Marston wrote:

@samphippen -- Thanks for looking into this! I think rescuing errors like that will have the affect of silencing errors that happen in before(:all). When an error happens in a before(:all) hook, all examples in the example group (and any subgroups) should fail, as per the documented behavior-block), and I think your suggested change would change that.

Instead, I think we want to move the storing of the instance variables into an ensure clause so that it happens even if an error was raised. The change would be in run_before_all_hooks in example_group.rb.

Want to take a stab at that?

I'll take a look and see if I can make it work.


Reply to this email directly or view it on GitHub.

Owner

samphippen commented Sep 24, 2012

On 24 Sep 2012, at 04:36, Myron Marston wrote:

@samphippen -- Thanks for looking into this! I think rescuing errors like that will have the affect of silencing errors that happen in before(:all). When an error happens in a before(:all) hook, all examples in the example group (and any subgroups) should fail, as per the documented behavior-block), and I think your suggested change would change that.

Instead, I think we want to move the storing of the instance variables into an ensure clause so that it happens even if an error was raised. The change would be in run_before_all_hooks in example_group.rb.

Want to take a stab at that?

I'll take a look and see if I can make it work.


Reply to this email directly or view it on GitHub.

@samphippen

This comment has been minimized.

Show comment Hide comment
@samphippen

samphippen Sep 24, 2012

Owner

I've got a patch that does this: https://gist.github.com/3775318

and output: https://gist.github.com/3775317

Should I manufacture a pull request around this?

Owner

samphippen commented Sep 24, 2012

I've got a patch that does this: https://gist.github.com/3775318

and output: https://gist.github.com/3775317

Should I manufacture a pull request around this?

@alindeman

This comment has been minimized.

Show comment Hide comment
@alindeman

alindeman Sep 25, 2012

Contributor

@samphippen, it seems reasonable to me, especially if you can get some specs around it. Thanks!

Contributor

alindeman commented Sep 25, 2012

@samphippen, it seems reasonable to me, especially if you can get some specs around it. Thanks!

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