Skip to content

Make ExpectationError inherit from Exception, rather than StandardError #15

Closed
wants to merge 1 commit into from

3 participants

@jsanders
jsanders commented Nov 9, 2010

Re: http://floehopper.lighthouseapp.com/projects/22289/tickets/1-mochaexpectationerror-exception we would also like to see this behavior incorporated into a release of mocha. It is currently not possible to detect failed expectations that happen within a block that does a bare rescue. For instance:

    describe User
      it "does not call name from display_name" do
        User.any_instance.expects(:name).never
        User.display_name
      end
    end

    class User
      def name
        'bob'
      end

      def display_name
        name
      rescue
        puts "This was caused by Mocha::ExpectationError"
      end
    end

This test would print the text in the rescue, and pass. However, if ExpectationError inherits from Exception, you would have to explicitly rescue Exception for this behavior to happen, which is known to be dangerous.

@lmarlow
lmarlow commented on 22ce933 Nov 9, 2010

Why did you do this? What benefit does it provide?

Sorry for not making this clearer in the commit message.

When a method stubbed by mocha is called more times than its expectation allows, a Mocha::ExpectationError is thrown, which before this commit inherited from StandardError. This means that if that method is called anywhere inside a context surrounded by a naked rescue (begin; do_something; rescue; do_something_in_rescue; end), the Mocha::ExpectationError will be rescued, and if it is not re-raised, the test will pass, despite the method being called more times than expected. By inheriting from Exception, you must explicitly say you are rescuing Exceptions (begin; do_something; rescue Exception; do_something_in_rescue; end), which is done more rarely.

@floehopper
Go Free Range member

Hi James,

Thanks for the pull request. I think the reason I hadn't done it before was that I wanted to think through the potential consequences e.g. whether it needs to be a minor version release or just a patch release. Also I would want to add some tests for the change if possible. Have you tried writing or modifying a test for the change?

Cheers, James.

@floehopper
Go Free Range member

Just to let you know, I'm going to have a go at writing some tests.

@floehopper
Go Free Range member

I've pulled in your change and added tests in e2c5f3e.

@jsanders

Thanks! Appreciate the tests as well, probably should have helped you out by writing them myself. Do you have any idea of when you might release a version that would include this change?

@floehopper
Go Free Range member

I'm just struggling with the tension between this change and a change that might be needed to resolve this other ticket.

@floehopper
Go Free Range member

I've just release a new version of Mocha including this change. I'm marking this issue as closed.

@floehopper floehopper added a commit that referenced this pull request Aug 24, 2012
@floehopper floehopper Use a custom exception handler in the new Test::Unit adapter.
* It turns out that Test::Unit::AssertionFailedError is derived from
  StandardError; whereas MiniTest::Assertion is derived from Exception.
* I had been telling Mocha::ExpectationErrorFactory to use
  Test::Unit::AssertionFailedError in the Test::Unit adapter, but since
  this inherits from StandardError, this would mean a regression on #15.
* So instead I've introduced a custom exception handler in the new
  Test::Unit adapter to rescue Mocha::ExpectationError as if it were a
  Test::Unit::AssertionFailedError, even though the former is derived
  from Exception.
* Unfortunately I have had to call some private methods in Test::Unit
  from within the exception handler method. I'm going to ask the Test::Unit
  developers whether there is an alternative to doing this.
e9d05cc
This issue was closed.
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.