Ensure negative expectations raise during RSpec verification time. #884

fables-tales · 2015-02-01T13:21:25Z

During the execution of an example (as noted in #874) it is possible
for a framework that wraps a mocked object to catch the
RSpec::Mocks::MockExpectationError raised by a negative expectation. This
exception is thrown immediately and so if it is caught the running
example will pass.

This patch causes message expectations to also raise the same error at
verification time.

This commit does not have a passing spec suite, that is intentional. I
want to get feedback on the implementation before I go through all the
broken tests and fix them. The relevant new test can be found on lines
228 to 239 of receive_spec.rb

myronmarston · 2015-02-01T21:10:25Z

Thanks for tackling this, @samphippen. Thinking about this some more, I realized that a negative message expectation is just one case where we eagerly fail rather than failing on verify. There are other cases, such as when the message is received with the wrong args, or received too many times, or received out of order. It seems to me that all of these cases are prone to the same problem: if the eagerly raised error is rescued by something like Sinatra, then the example won't fail as it should. It seems to me that we'd be better off fixing this issue for all the eager cases, and not just for the negative expectation case. For example, for the verify_messages_received case, I think it can be simplified too:

        def verify_messages_received
          return if expected_messages_received?
          InsertOntoBacktrace.line(@expected_from) { generate_error }
        end

Much simpler, eh? I tried out that change and noticed that it caused about 30 failures but the handful I looked at looked correct since they were cases that failed eagerly that the spec itself caught, so a simple reset object will do the trick. Before making such a change we should also catalog all the situations where eager exceptions are raised, and be sure to add specs for each for this new "should also raise on verification" behavior.

fables-tales · 2015-02-01T21:14:31Z

@myronmarston 👍 I'll continue to work on this over the coming week.

fables-tales · 2015-02-02T17:59:51Z

@myronmarston I used your implementation and started fixing the specs up.
My understanding of the other cases required are:

argument mismatched
Invocation count mismatches

I think that's it?

myronmarston · 2015-02-03T08:43:39Z

I'll try to look at this tomorrow...ran out of time today.

myronmarston · 2015-02-09T07:38:26Z

Sorry it took me so long to get back to you; I had to do an audit to answer your question!

Here are the cases I've found that raise errors eagerly (e.g. when the message is received):

When a non-as_null_object test-double receives a message that hasn't been allowed or expected,
When a stub is made using with and different args are received.
When a verified double receives a message with arguments that the method signature cannot support.
When a message expectation configured using with receives unexpected args.
When a message expectation is configured with an expected count and that count is exceeded.
When an ordered message expectation is received out of order.
When a stub or message expectation is configured to and_yield and no block has been passed.
When a stub or message expectation is configured to and_yield and a block is passed with a different signature (e.g. number of args).

Initially, I was thinking we would want verify to re-raise errors for all of these cases but now I'm realizing that's a really bad idea. I think we only want to re-raise on verify for the expectations -- if it's just that wrong args were passed or some other thing raising again later is a terrible idea. RSpec should not allow an example to pass if an expectation is violated (and thus, raising during verifying is a good idea, IMO for expectation cases). But for cases where no expectation was violated raising would obviously be wrong because the user would have no way to make the example pass even though it's not an expectation failure.

So I think the cases to re-raise for are:

When a message expectation configured using with receives unexpected args.
When a message expectation is configured with an expected count and that count is exceeded.
When an ordered message expectation is received out of order.

...which I guess is the same list you had plus the ordering one.

This is necessary because in rspec/rspec-mocks#884 we've added behaviour that raises even if an eager raise occured. The call to generated description happens outside of the test, but before the mock is reset, so this resets it.

fables-tales · 2015-02-09T19:35:48Z

@myronmarston I started writing specs for all the cases. I think having them in their own file makes sense, do you agree? I'm aware the new spec file is a bit duplicated right now, so don't feel the need to leave a review note to that effect (I'll fix it up). I'm thinking I'm going to put the cases that don't reraise into their own context, and the ones that do also into their own context. Do you see the current implementation causing any of the cases that shouldn't raise (that you've outlined above) causing any issues?

fables-tales · 2015-02-14T14:32:41Z

@myronmarston can you take a look at this when you get a chance. Thanks in advance :)

myronmarston · 2015-02-14T17:20:36Z

spec/rspec/mocks/at_most_spec.rb

@@ -79,6 +79,7 @@ module Mocks
        expect {
          @double.do_something
        }.to raise_error(/expected: at most 2 times.*received: 3 times/m)
+        reset @double


So this expect { }.to raise_error; reset double pattern is showing up over, and over and over again through the spec suite. I'm thinking we should come up with a common abstraction to handle that.

WDYT of this?

expect_fast_failure_from(double, /expected: at most 2 times.*received: 3 times/m) do double.do_something end

expect_fast_failure_from would take of expect { }.to raise_error and the reset.

@samphippen

Specifically, in #1826, we changed the conditional for whether or not to assign a generated description from RSpec::Matchers for examples with no doc string. Before #1826, the conditional was: assign_generated_description if RSpec.configuration.expecting_with_rspec? In #1826 it changed to: assign_generated_description if defined?(::RSpec::Matchers) We didn’t update the spec meant for that case to match, but it continued to pass (as a false positive) due to rspec/rspec-mocks#874. @samphippen’s fix in rspec/rspec-mocks#884 surfaced the issue (as the spec now failed) so I decided to improve the tests. - The spec now simulates the `RSpec::Matchers` constant being undefined to simulate the correct condition. We also have to prevent it from being autoloaded. - The cukes for minitest/test-unit did not sufficiently cover this case, because the aforementioned autoload would autoload RSpec::Matchers, so we have to simulate rspec-expectations being completely uninstalled. Then the cukes properly fail if we break the `if defined?(::RSpec::Matchers)` conditional.

myronmarston · 2015-02-15T20:34:39Z

I just kicked the build now that rspec/rspec-core#1879 has been merged...hopefully this will go green now.

JonRowe · 2015-02-16T02:54:31Z

Seems not, :/

myronmarston · 2015-02-16T06:12:39Z

Seems not, :/

It's much better, though -- in the last build (https://travis-ci.org/rspec/rspec-mocks/builds/49933519) it was failing when running rspec-core's specs. Now it's not. It's just 1.8.7 that's failing on some rspec-mocks specs now.

myronmarston · 2015-03-11T04:33:56Z

@samphippen, are you planning to get back to this?

fables-tales · 2015-03-11T08:53:47Z

yup. I'm gonna spend some time this weekend on this.

fables-tales · 2015-03-14T12:31:17Z

https://travis-ci.org/rspec/rspec-mocks/jobs/54355732#L2334 💀 I will see if I can debug this locally.

fables-tales · 2015-03-14T13:20:16Z

I'm having trouble getting REE set up on my mac. @myronmarston can you look at the failure and reckon if it's legit. I'm going to see if I can get a linux box set up with REE inside it.

myronmarston · 2015-03-15T02:28:56Z

spec/spec_helper.rb

    reset d
  end
+
+  def expect_fast_failure_from(double, class_or_message=Exception, message=/.*/, &blk)


Exception seems like a poor choice for the default here, given that at virtually all the call sites above you pass RSpec::Mocks::MockExpectationError (and in the places you don't, I'm wondering if you should!). It seems like the norm is RSpec::Mocks::MockExpectationError -- after all, in rspec-mocks, that's who we signal failures -- so we should make that the default and only make the caller pass a different class if it really needs something different.

Actually, I suspect that there is no need to specify a class other than RSpec::Mocks::MockExpectationError. If that's the case, this can be simplified to:

def expect_fast_failure_from(double, *fail_with_args) expect { yield double }.to fail_with(*fail_with_args) reset double end

myronmarston · 2015-03-15T03:04:36Z

@samphippen -- The problem isn't ree-specific. It's failing on MRI 1.8.7 for me. I'm realizing that the failure has to do with hash ordering -- on 1.9+ it's deterministic and this spec consistently passes, but on 1.8.7 the hash ordering is non-deterministic, and as a result, in RSpec::Mocks::Proxy#verify, when it does @method_doubles.each_value, it's not deterministic whether the method double for :foo or the method double for :bar is verified first. Both method doubles trigger failures:

foo was never called but was expected, so it has a verify error.
bar was called out of order (e.g. before foo was called) so it has a verify error.

Note, however, that the message you are expecting (/\.foo/) has nothing to do with the ordering; it's simply that the message was never received, which isn't an eager raise situation anyway. It's the bar one that raised eagerly, and your expectation doesn't touch on that at all!

I think you want something like this instead:

  it "reraises when an expectation is called out of order" do
    with_unfulfilled_double do |dbl|
      expect(dbl).to receive(:foo).ordered
      expect(dbl).to receive(:bar).ordered
      expect { dbl.bar }.to fail
      dbl.foo # satisfy the `foo` expectation so that only the bar one fails below
      expect { RSpec::Mocks.verify }.to fail_with(/received :bar out of order/)
    end
  end

However, that doesn't pass on any ruby; instead we get:

#<RSpec::Mocks::MockExpectationError: (Double "double").bar(*(any args))
           expected: 1 time with any arguments
           received: 0 times with any arguments>

...which is simply wrong. bar was received 1 time, so the error message is false. Given that this is a bug that's existing and not really part of the change you are working, it's probably OK to make the spec pending for now, although it would be great if you would come back to it once this is merged (if you have the time, of course).

fables-tales · 2015-03-18T08:26:56Z

@myronmarston how's this looking? I went through the feedback and made edits.

myronmarston · 2015-03-18T14:09:24Z

LGTM. Can you squash it into a smaller number of commits before merging?

… eagerly raised.

Ensure negative expectations raise during RSpec verification time.

@samphippen

Specifically, in rspec#1826, we changed the conditional for whether or not to assign a generated description from RSpec::Matchers for examples with no doc string. Before rspec#1826, the conditional was: assign_generated_description if RSpec.configuration.expecting_with_rspec? In rspec#1826 it changed to: assign_generated_description if defined?(::RSpec::Matchers) We didn’t update the spec meant for that case to match, but it continued to pass (as a false positive) due to rspec/rspec-mocks#874. @samphippen’s fix in rspec/rspec-mocks#884 surfaced the issue (as the spec now failed) so I decided to improve the tests. - The spec now simulates the `RSpec::Matchers` constant being undefined to simulate the correct condition. We also have to prevent it from being autoloaded. - The cukes for minitest/test-unit did not sufficiently cover this case, because the aforementioned autoload would autoload RSpec::Matchers, so we have to simulate rspec-expectations being completely uninstalled. Then the cukes properly fail if we break the `if defined?(::RSpec::Matchers)` conditional.

fables-tales force-pushed the samphippen/fix-874 branch from 8b57533 to a60bbf5 Compare February 2, 2015 17:58

fables-tales force-pushed the samphippen/fix-874 branch 2 times, most recently from ec8b088 to accd614 Compare February 8, 2015 10:41

fables-tales mentioned this pull request Feb 9, 2015

Add a RSpec::Mocks reset call to example_spec rspec/rspec-core#1873

Closed

myronmarston reviewed Feb 14, 2015
View reviewed changes

myronmarston mentioned this pull request Feb 14, 2015

Beef up tests for when rspec-expectations is not available. rspec/rspec-core#1879

Merged

fables-tales force-pushed the samphippen/fix-874 branch from d574d30 to 9fde951 Compare March 14, 2015 11:51

myronmarston reviewed Mar 15, 2015
View reviewed changes

fables-tales force-pushed the samphippen/fix-874 branch from 4266b76 to f3f3d3f Compare March 20, 2015 13:45

fables-tales pushed a commit that referenced this pull request Mar 20, 2015

Changelog entry for #884

f3f3d3f

Sam Phippen added 2 commits March 20, 2015 13:46

Make message expectations raise at the end of tests even if they have…

e147665

… eagerly raised.

Changelog entry for #884

fb2e0f1

fables-tales force-pushed the samphippen/fix-874 branch from f3f3d3f to fb2e0f1 Compare March 20, 2015 13:47

fables-tales pushed a commit that referenced this pull request Mar 20, 2015

Merge pull request #884 from rspec/samphippen/fix-874

84e7da2

Ensure negative expectations raise during RSpec verification time.

fables-tales merged commit 84e7da2 into master Mar 20, 2015

fables-tales deleted the samphippen/fix-874 branch March 20, 2015 15:04

fables-tales pushed a commit that referenced this pull request Mar 20, 2015

Fix changelog entry for #884

b5e514d

fables-tales mentioned this pull request Mar 25, 2015

Negative expectation failures should be surfaced during verification #874

Closed

myronmarston mentioned this pull request May 27, 2015

Negative message expectations interact weirdly with failure aggregation #956

Open

myronmarston mentioned this pull request Jun 3, 2015

Pass source_id when notifying failures #962

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure negative expectations raise during RSpec verification time. #884

Ensure negative expectations raise during RSpec verification time. #884

fables-tales commented Feb 1, 2015

myronmarston commented Feb 1, 2015

fables-tales commented Feb 1, 2015

fables-tales commented Feb 2, 2015

myronmarston commented Feb 3, 2015

myronmarston commented Feb 9, 2015

fables-tales commented Feb 9, 2015

fables-tales commented Feb 14, 2015

myronmarston Feb 14, 2015

myronmarston commented Feb 15, 2015

JonRowe commented Feb 16, 2015

myronmarston commented Feb 16, 2015

myronmarston commented Mar 11, 2015

fables-tales commented Mar 11, 2015

fables-tales commented Mar 14, 2015

fables-tales commented Mar 14, 2015

myronmarston Mar 15, 2015

myronmarston Mar 15, 2015

myronmarston commented Mar 15, 2015

fables-tales commented Mar 18, 2015

myronmarston commented Mar 18, 2015

Ensure negative expectations raise during RSpec verification time. #884

Ensure negative expectations raise during RSpec verification time. #884

Conversation

fables-tales commented Feb 1, 2015

myronmarston commented Feb 1, 2015

fables-tales commented Feb 1, 2015

fables-tales commented Feb 2, 2015

myronmarston commented Feb 3, 2015

myronmarston commented Feb 9, 2015

fables-tales commented Feb 9, 2015

fables-tales commented Feb 14, 2015

myronmarston Feb 14, 2015

Choose a reason for hiding this comment

myronmarston commented Feb 15, 2015

JonRowe commented Feb 16, 2015

myronmarston commented Feb 16, 2015

myronmarston commented Mar 11, 2015

fables-tales commented Mar 11, 2015

fables-tales commented Mar 14, 2015

fables-tales commented Mar 14, 2015

myronmarston Mar 15, 2015

Choose a reason for hiding this comment

myronmarston Mar 15, 2015

Choose a reason for hiding this comment

myronmarston commented Mar 15, 2015

fables-tales commented Mar 18, 2015

myronmarston commented Mar 18, 2015