Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

An error occurred in an after(:each) hook #651

Closed
gavinlaking opened this Issue · 8 comments

3 participants

@gavinlaking

Myron (@myronmarston) recommended I open an issue relating to both (27059bf) and (d0cb324).

An error occurred in an after(:each) hook
  RSpec::Mocks::MockExpectationError: (Mock "RecruitmentSystem::Category_1001").jobs(any args)
    expected: 1 time
    received: 0 times
  occurred at /Users/gavinlaking/Sites/recruitment_system_gem/spec/controllers/recruitment_system/admins/categories_controller_spec.rb:17:in `block (3 levels) in <top (required)>'

My spec doesn't contain anything which could be deemed too sexy: e.g.

require "spec_helper"

describe RecruitmentSystem::Admins::CategoriesController do
  let(:category) { mock_model(RecruitmentSystem::Category) }
  let(:params) { { :id => "61", :job_id => "18",  } }

  before do
    RecruitmentSystem::Category.stub(:find_using_slug) { category }
  end

  describe "GET index" do
    let(:jobs) { mock_model(RecruitmentSystem::Job) }

    before do
      category.stub(:jobs) { jobs }
    end

    subject { get :index, params }

    it "should get the jobs for the category" do
      category.should_receive(:jobs) { jobs }
      subject
    end
  end
end

If there is any more information you need, please let me know :-)

@myronmarston

Thanks for reporting this. Did the test have another error (which would have been reported as the test failure)?

I'm not sure of the right fix for this, but here are a couple ideas:

  • We can keep the printing of the error but change the wording so that it is clear that it wasn't in an after(:each) hook but rather from a mock expectation failure.
  • We can change the verifying of mocks here so that additional errors raised by mock expectations do not get printed. Mock expectation errors that are the first/only error in an example will still get raised and fail the example, of course.
  • We can change it so that the mocks are verified only if there hasn't already been an exception.

In general, I think silencing errors is evil--that's why I added this code to print out the error when an additional error occurs in after hook or around hook. However, mock expectation errors feel like a bit of a special case to me.

Thoughts?

@dchelimsky
Owner

What we want, conceptually, is the first error that is part of the example plus all errors that happen in after hooks (which are, semantically, for cleanup - not verification). I'll have to follow up w/ more later, but that's the guiding principle IMO.

@gavinlaking

The 'real' error was:

RecruitmentSystem::Admins::CategoriesController GET index should get the jobs for the category
     Failure/Error: subject { get :index, params }
     ActionController::RoutingError:
       No route matches {:id=>"61", :job_id=>"18", :controller=>"recruitment_system/admins/categories"}
     # ./spec/controllers/recruitment_system/admins/categories_controller_spec.rb:15:in `block (3 levels) in <top (required)>'
     # ./spec/controllers/recruitment_system/admins/categories_controller_spec.rb:18:in `block (3 levels) in <top (required)>'

I'm sure my routes are fine (but that's another issue grin)

If I may add my 2 pence, I think that your first option is correct Myron; changing the wording to reflect that the problem is due to a mock expectation error. If I understand both you and David correctly, I think RSpec may hint towards other problems as part of the error message, but the primary focus must be that a test failed (with reason) irrespective of how I set it up. (I think that's right?!).

Hope that helps, I'm nowhere near your level, so I don't even know if I'm being helpful! :-)

@dchelimsky
Owner

In terms of printing errors for more than one message expectation failure: generally rspec's philosophy has been print one failure per example. Errors in after hooks are different because they are reserved for cleanup, and shouldn't contain logical failures. It's important to know that we left connections open or left files on the system.

Now jasmine, for example, allows you to set multiple expectations per example and reports on all of them. There are some tradeoffs to doing this that relate to the motivations behind one-expectation-per-example. If we were to print 3 failed expectations in one example, the 2nd and 3rd are unreliable failures because the might be triggered by the first failure. In that sense they are really just noise. Imagine seeing 3 failures, getting the last one to go away, but when you get the 2nd one to go away the last one comes back. That would be frustrating.

That said, if we decide to show more than one failure, we'll need to support that in both rspec-mocks and rspec-expectations (I sometimes think these should be merged).

@myronmarston that all make sense to you?

@myronmarston

Having multiple failures for example is an interesting feature of Jasmine, but I'm not super interested in it at this point (among other things, it would take a fairly massive refactoring to properly support it) and the issue that motivated this change was far different.

VCR hooks into RSpec using before/after hooks. The hooks work great to allow VCR to do its (largely orthogonal) thing, while allowing the end user of RSpec/VCR to put their test code in the example block. The problem was that I had accidentally misconfigured VCR so that what it did in the after hook raised an error. The conditions that triggered the error in the after hook occurred with an example that had a test failure of its own. In RSpec 2.10, the error in the after hook was handled and it called set_exception, but due to its use of ||=, the exception was silently ignored.

While I'm not ready to do a large refactoring to fully support multiple failures per example, I think it's important that RSpec does not silently swallow errors from after/around hooks when the example has already had a failure. That's what motivated my original change, and I still want to keep it (or something like it) in some form.

@myronmarston

I'm planning to get a 2.11.1 release of rspec-core out in the next day or two, and I want to include some kind of fix for this, even if it's not the final solution. We can always implement a better solution for 2.12.

For the 2.11.1 fix I'm currently leaning towards not printing anything for mock expectation errors that happen in examples that have already failed. The only reason there is something printed now is because the mock expectations are verified in the same method that runs after hooks--it was never intended, and RSpec has never printed errors for mock expectations in this kind of circumstance before. The point in the example where mock expectations are verified is an implementation detail we don't need to expose to end users.

I'll try to have this fix ready shortly.

@myronmarston myronmarston closed this issue from a commit
@myronmarston myronmarston Ignore mock expectation failures when the example has already failed.
Mock expectation failures have always been ignored in this situation,
but due to my changes in 27059bf it was printing a confusing message.

Closes #651.
e22ce0b
@myronmarston

@gavinlaking -- want to give master a shot and let me know if it fixes your problem?

@gavinlaking

@myronmarston Nice one mate, it's working fine now; the real error is being reported.

As for my specs, they're still failing. Don't suppose you want to fix those too, eh? ;-)

@myronmarston myronmarston referenced this issue from a commit
@myronmarston myronmarston Ignore mock expectation failures when the example has already failed.
Mock expectation failures have always been ignored in this situation,
but due to my changes in 27059bf it was printing a confusing message.

Closes #651.
30e81b5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.