broken should_receive expectations with do blocks don't cause the test to go red on classes which handle exceptions #203

Closed
froodian opened this Issue Dec 10, 2012 · 40 comments

9 participants

@froodian

If you have some code which rescues from exceptions, and you use the should_receive(:foo) do |arg| syntax to expect that class to make a call to a mock, if the assertions inside the do block fail, the test doesn't go red as expected.

Test-case which demonstrates the bug:

require 'spec_helper'

module BugReport203
  describe "an expectation on a class with a rescue block" do
    class TestClass
      def test_dependency
        @dependency ||= TestDependency.new()
      end

      def test_method
        begin
          self.test_dependency.some_call("I'm calling this method with unexpected arguments - THIS SHOULD CAUSE THE TEST TO GO RED!  But doesn't.")
        rescue
          # Exception-handling logic here
        end
      end
    end

    class TestDependency
      def some_call(arguments)

      end
    end

    # This test should be red!
    it "should know the method was called with wrong arguments" do
      mock_dependency = mock(TestDependency)

      test_object = TestClass.new()
      test_object.stub(:test_dependency).and_return(mock_dependency)

      # Obviously in this trivial case of checking string equality, I could use
      #
      # mock_dependency.should_receive(:some_call).with("expected arguments")
      #
      # which will go red as expected.
      # This bug applies to the more complex case where I actually need the do block to make my assertions
      mock_dependency.should_receive(:some_call) do |arg|
        arg.should == "expected arguments"
      end

      test_object.test_method()
    end
  end
end
@alindeman

I'm not sure this is a problem, nor an issue we should attempt to fix. RSpec::Mocks::MockExpectationError inherits directly from Exception. Any code that rescues Exception is asking for trouble: all sorts of errors you probably should let bubble up are Exceptions, as opposed to more runtime-specific StandardError or RuntimeError.

You might consider changing your code to rescue more specific exceptions, or at the very least least StandardError (default for a bare rescue) or RuntimeError (raised by default when using raise "String").

@froodian

In reality this issue is happening in a rescue_all block on a controller, so a more specific rescue doesn't seem like the right answer for us - our first priority is protecting the user from the experience of having unhandled exceptions thrown in their face.

Also, the inconsistency with the behavior you get when you use .with is somewhat disconcerting - I'd expect those to work the same way, no?

@alindeman

Ah, OK, I misinterpreted the issue. Thanks for the code sample/test.

rspec-expectations raises an ExpectationNotMetError that is a StandardError, so even bare rescue blocks can catch it. Interesting issue. I'll mull it over some more; also interested in what @myronmarston thinks.

@alindeman

Interestingly minitest uses an Exception subclass:

1.9.3-p194 :015 > RSpec::Expectations::ExpectationNotMetError.ancestors
 => [RSpec::Expectations::ExpectationNotMetError, StandardError, Exception, Object, MiniTest::Assertions, PP::ObjectMixin, Kernel, BasicObject] 
1.9.3-p194 :016 > MiniTest::Assertion.ancestors
 => [MiniTest::Assertion, Exception, Object, MiniTest::Assertions, PP::ObjectMixin, Kernel, BasicObject] 

@myronmarston, do you know why our assertion is a StandardError? I can dig into code, but there might be more context/history there.

@myronmarston
RSpec member

I didn't realize RSpec::Mocks::MockExpectationError subclassed Exception. At first glance, that seems wrong, as ruby reserves Exception-subclassed errors for serious things like unhandled signals, out-of-memory, etc. that shouldn't generally be rescued (and thus a bare rescue does not rescue them). On second glance, maybe this is why it subclasses Exception: so that a user-defined bare rescue won't rescue it, but rspec can. @dchelimsky -- do you remember the history here?

Anyhow, as for this specific issue...given that expectation failures are designed to be rescued by rspec, but not by user implementation code, it makes me wonder if ExpectationNotMetError should subclass Exception, too. However, changing it to do so would be a big breaking change, I think....so the soonest we could make that change would be 3.0. And I'm not yet convinced that we necessarily want to make that change.

Note that this isn't just a problem with rspec-mocks; consider this example:

class MyCollection
  def new(*args)
    @array = args
  end

  def each
    @array.each do |item|
      yield item
    end
  rescue
    # exception handling logic here
  end
end

describe MyCollection do
  it 'does not fail even though it should' do
    collection = MyCollection.new(5, -1, 10)
    collection.each do |item|
      item.should be > 0
    end
  end
end

Essentially...anytime an expectation is checked while user code is further up the call stack, there's a possibility of that user code having a rescue that stops the example from properly failing.

For now, you can fix your example like so:

    # This test should be red!
    it "should know the method was called with wrong arguments" do
      mock_dependency = mock(TestDependency)

      test_object = TestClass.new()
      test_object.stub(:test_dependency).and_return(mock_dependency)

      passed_arg = nil
      mock_dependency.should_receive(:some_call) do |arg|
        passed_arg = arg
      end

      test_object.test_method()
      passed_arg.should == "expected arguments"
    end

Essentially, use the should_receive block to capture the arguments, and set an expectation on them later, at a place where you're outside of the rescue in your call stack.

@alindeman

Anyhow, as for this specific issue...given that expectation failures are designed to be rescued by rspec, but not by user implementation code, it makes me wonder if ExpectationNotMetError should subclass Exception, too. However, changing it to do so would be a big breaking change, I think....so the soonest we could make that change would be 3.0. And I'm not yet convinced that we necessarily want to make that change.

That was my thought, especially because this is how test::unit/minitest approaches it.

Under what real-world situations would it be a breaking change? I might agree, but wondering what specific sorts of cases you're thinking of.

@myronmarston
RSpec member

Under what real-world situations would it be a breaking change? I might agree, but wondering what specific sorts of cases you're thinking of.

I'm thinking primarily of gems that hook into RSpec and provide extensions. They can currently rescue expectation failures with a bare rescue but if we changed this, they would break.

@dchelimsky
RSpec member

Both ExpectationNotMetError and MockExpectationError were originally derived from StandardError. MockExpectationError was changed to subclass Exception 4 years ago in dchelimsky/rspec@1b51a79. Unfortunately, issue 830 was before we moved rspec to github and our old tracker on lighthouseapp.com was not kept around for archival purposes, so I can't be sure what the issue was, but my guess is that we changed it to Exception so it would get past bare rescue statements.

We should probably do the same for ExpectationNotMetError, which would solve for the example presented in this issue, but would not solve for rescue Exception.

@dchelimsky
RSpec member

@myronmarston I definitely support your choice to wait for rspec-3 (if you make that choice ;) )

@myronmarston
RSpec member

Thanks for digging in to find that history. I've tagged the issue as "3.0 or later" with the plan to change this in 3.0. @froodian -- does my suggested fix from above work for you for now?

@froodian

Yeah, that workaround should work for now. Thanks for looking into it guys. Is there any vision for how far out 3.0 is at this point?

@myronmarston
RSpec member

Is there any vision for how far out 3.0 is at this point?

We've started having some internal discussions about that, but we're not at a point where we can announce any plan yet.

@samphippen
RSpec member

@myronmarston @dchelimsky what's the status on this one?

@myronmarston
RSpec member

We still need to change the superclass of ExpectationNotMetError in rspec-expectations. Feel free to take a stab at that :).

Although, as we discussed, it'd be great to resolve the outstanding PRs before making too many new ones. What's the status of your build improvement PRs?

@samphippen
RSpec member

@myronmarston I could really do with pairing with @alindeman on that, the failures were all in -rails. I'll talk to him about it and set it up

@myronmarston
RSpec member

@myronmarston I could really do with pairing with @alindeman on that, the failures were all in -rails. I'll talk to him about it and set it up

We can hold off on adding the build improvement for rspec-rails for now then. It's a big improvement to get it for the other libs. We can iterate and add rspec-rails support later.

@samphippen
RSpec member

@myronmarston ok cool, I'll try and get that one and the should warnings closed out soon. There's an open question about what we want to do with the benchmarking tool.

@jarmo

I just stumbled against the same problem today.

Here's a really simple code:

class A
  def b
    c
  rescue Exception
    # make sure that all Exceptions/Errors are ignored
  end

  def c
  end
end


describe A do
  it "should not call c!!!11" do
    a = A.new
    expect(a).not_to receive(:c)

    a.b
  end
end

This spec will pass, but should fail.

Changing the super class won't fix it although i'm not against that change too.

Saying that "you should not rescue from Exception" is not a solution either because rescue-ing from Exception is a really normal behavior of handling all Exceptions in Ruby if you really want to rescue from everything.

And there are valid cases for this too. Even RSpec [1] [2] [3] and Rails [1] use it.

RSpec assertions results should not depend of what is rescued from the code under test.

The correct solution would be to behave as a spy and execute all these assertions at the end of the test so the assertion exceptions could be thrown normally.

In short - this is definitely a BUG and cannot be fixed by just changing the super class of assertion exception.

@myronmarston
RSpec member

I disagree. Every test framework I've ever seen uses exceptions as the way to indicate failure. rescue Exception is generally a bad idea -- but there are exceptions to that rule, such as in a case like RSpec, where an error in an example should not cause the process to exit, and it handles interrupts in a separate way. (I suspect rails has a similar reason).

The correct solution would be to behave as a spy and execute all these assertions at the end of the test so the assertion exceptions could be thrown normally.

This is an interesting idea but would be a HUGE semantic change from how rspec-expectations currently works. Among other things, it means that a failing expectation would not halt the example. I think it's a great idea for an alternate assertion/expectation library, though.

@jarmo

Yeah, i agree that this is generally a bad idea, but there are valid use cases (like you mentioned yourself).

My particular use case is where i have a timer task, which will do some network IO, parse html, so some database queries and so on. In that timer task i would like to survive whatever happens - even apocalypse itself. For that i'm rescuing Exception and sending a failure e-mail to the administrator or doing some other cleanup and then try again after some grace period. I would not like to risk of specifying every possible Exception (or their super classes) and then hope that i did not leave something out. Of course i have to also be prepared that the Exception hierarchy might change in the future versions of Ruby too so i have to keep that list up to date. Do you see any better solutions for this kind of problem?

Just saying that something is a bad idea does not mean that it applies to every case.

This just makes me not feel comfortable using a test runner/assertion library when it is possible to write code which does not cause tests to ever fail...

@myronmarston
RSpec member

This just makes me not feel comfortable using a test runner/assertion library when it is possible to write code which does not cause tests to ever fail...

Do you have an example of a ruby testing framework that doesn't use exceptions? I'm not aware of any.

@myronmarston
RSpec member

Do you see any better solutions for this kind of problem?

Sure:

module AllExceptionsExceptRSpecOnes
  def self.===(other)
    return true unless defined?(::RSpec)

    case other
      when RSpec::Expectations::ExpectationNotMetError, RSpec::Mocks::MockExpectationError
        false
      else
        true
    end
  end
end

class A
  def b
    c
  rescue AllExceptionsExceptRSpecOnes
    # make sure that all Exceptions/Errors are ignored
  end

  def c
  end
end

Ruby uses === to match in rescue (as in case statements), so you can define a custom object that defines === to match everything but the rspec expectations.

@myronmarston
RSpec member

Alternately, you can use rspec-mocks 2.14+ new test spies feature:

describe A do
  it "should not call c!!!11" do
    a = A.new
    a.stub(:c)

    a.b
    expect(a).not_to have_received(:c)
  end
end
@jarmo

AllExceptionsExceptRSpecOnes is not a good solution either because code under test should not know anything about the testing framework (e.g. RSpec).

The spies feature sounds more like it. Are there any other differences between received and have_received in addition that the latter works as expected? Why do i have to stub c when this method exists and is ready to be called?

@myronmarston
RSpec member

The spies feature sounds more like it. Are there any other differences between received and have_received in addition that the latter works as expected? Why do i have to stub c when this method exists and is ready to be called

Because RSpec can't spy on all method calls in a performant way. You stub the method so that RSpec knows to watch that method.

@samphippen
RSpec member

you can also use and_call_original if you need the original implementation.

@moll

There is, however, one alternative way to bubble up in Ruby with its other exception mechanism — throw and catch. That would bypass all greedy rescue statements everywhere.

@myronmarston
RSpec member

There is, however, one alternative way to bubble up in Ruby with its other exception mechanism — throw and catch. That would bypass all greedy rescue statements everywhere.

throw is an interesting idea, but unless I'm mistaken, it doesn't include a backtrace, which is super important. Also, it doesn't truly solve the problem: someone could catch the symbol that we throw, just like someone can rescue the exception we raise.

@moll

You can create an exception object just like with raise and throw that. That'll contain the backtrace.

throw :rspec_assertion_error, ExpectationNotMetError.new("Foo")

Also, it doesn't truly solve the problem.

But fortunately it does solve it practically. Unless someone explicitly wants to catch RSpec's errors. At least it won't be accidental. You can't have a catch-all catch in Ruby AFAIK.

@myronmarston
RSpec member

You can create an exception object just like with raise and throw that. That'll contain the backtrace.

Actually, in your example it won't have a backtrace:

irb(main):001:0> StandardError.new("boom").backtrace
=> nil
@moll

Aah, I misremembered how Exception.exception or Exception.new worked. It was instead Kernel#raise that set the backtrace. Kernel.caller could do the trick then.

@dirkbolte

As Rspec 3.0 entered beta already I wonder whether what the plans are on this issue.

@myronmarston
RSpec member

Still just planning on changing the superclass of ExpectationNotMetError in rspec-expectations. In fact, I'm going to close this and open an issue there since the change is really there -- thanks for the reminder.

Anyhow, if it's an issue for you to have rspec raise exceptions from deep within your code, then you're better of using spies (e.g. expect(obj).to have_received(:msg) after the fact) rather than normal message expectations (e.g. expect(obj).to receive(:msg)), since spies will raise directly in the example, outside of any rescue Exception clauses in your implementation code.

@myronmarston myronmarston referenced this issue in rspec/rspec-expectations Jan 15, 2014
Closed

Make ExpectationNotMetError subclass Exception #425

@jooshbzm

We ran into this problem as well. To work around it, we just handled exception in a method, then monkeypatched that method in spec/support. It's working well for now.

Something like this:

# my_class.rb

class MyClass
  def my_method
    begin
      do_something
    rescue Exception => e
      handle_exception(e)
    end
  end

  def handle_exception(e)
    do_something_else
  end
end
# spec/support/helpers.rb

class MyClass
  original_handle_exception = instance_method(:handle_exception)

  define_method(:handle_exception) do |e|
    raise e if e.is_a?(RSpec::Mocks::MockExpectationError)

    original_handle_exception.bind(self).call(e)
  end
end
@myronmarston
RSpec member

Another solution: rspec 3.3 (to be released ASAP) contains a new aggregate_failures feature that allows multiple failures within a single example and raises a single exception at the end.

@jarmo

Why didn't that throw/catch solution suit that @moll suggested above? Seems to be most natural way of fixing this problem and then you could say something in the lines of all other testing frameworks are fragile :)

@myronmarston
RSpec member

@jarmo -- changing to throw/catch would be a breaking change to the API, and rspec-mocks and rspec-expectations are both usable with other test frameworks (e.g. cucumber and minitest) which expect the current behavior (raising an exception). The best we can do for now is make it configurable.

...which, as of RSpec 3.3 it is :). It's not a documented public API (and therefore may have breaking changes in later 3.x releases) but there's now this:

RSpec::Support.failure_notifier = lambda do |error, options|
  # do something with the error
end

So I believe you could do this:

RSpec::Support.failure_notifier = lambda do |error, options|
  throw :rspec_failure, error
end

RSpec.configure do |c|
  c.around do |ex|
    error = catch(:rspec_failure) do
      ex.run
      nil
    end

    raise error if error
  end
end

RSpec.describe "Rescuing exception failures" do
  it "does not prevent it from failing" do
    begin
      expect(1).to eq(2)
    rescue Exception
    end
  end

  it 'allows passing examples to pass' do
    expect(1).to eq(1)
  end
end
@myronmarston myronmarston added a commit to rspec/rspec-core that referenced this issue Jun 10, 2015
@myronmarston myronmarston Revert "Always verify mocks at the end of each example."
This reverts commit f989168.

This is necessary to avoid the issue discussed in rspec/rspec-mocks#203.
I'm reverting it so we can release 3.3. We may bring this back for
3.4 if we can find a better solution.
11a268f
@jarmo

@myronmarston: instead of "fixing" bad design (tests not failing due to bad rescue Exception clause smells) you see that adding new features/configuration parameters is a better solution?

I hope RSpec is not heading Java's way where backwards compatibility is the most important thing to have so all that crap from it's beginning still exists.

@myronmarston
RSpec member

I hope RSpec is not heading Java's way where backwards compatibility is the most important thing to have so all that crap from it's beginning still exists.

We made lots of backwards incompatible changes when we released 3.0. 4.0 will include more backwards incompatible changes (although much fewer, I believe -- and no major syntax changes).

We do follow SemVer, which means we've promised to our users not to intentionally break backwards compatibility in a minor or patch release, so making that kind of change in a 3.x release simply isn't an option.

All that said: I plan to think more about this to see if there's a backwards-compatible way we can do this in a 3.x release.

The snippet I pasted above was simply meant to show that for people who want this sooner, there is a way to do it.

@jarmo

Actually, I didn't think that this change as a backwards-compatible change. I'd be really fine if that would end up in 4.x series instead of not happening ever and being broken in future major versions as well.

I know what SemVer is and I'm following that with my own OSS libraries as well.

Thank You for all the good work so far!

@myronmarston myronmarston referenced this issue in rspec/rspec-expectations Aug 25, 2015
Closed

Reduce scope of rescue in `safe_sort` #843

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