Negative message expectations interact weirdly with failure aggregation

[myronmarston]

RSpec.describe "Using `aggregate_failures` with a negative message expectation" do
  it "should report only one failure but reports two", :aggregate_failures do
    dbl = double
    expect(dbl).not_to receive(:foo)
    dbl.foo
  end
end

This fails with:

  1) Using `aggregate_failures` with a negative message expectation should report only one failure but reports two
     Got 2 failures:

     1.1) Failure/Error: dbl.foo
            (Double (anonymous)).foo(no args)
                expected: 0 times with any arguments
                received: 1 time
          # ./spec/unit/foo_spec.rb:5:in `block (2 levels) in <top (required)>'

     1.2) Failure/Error: expect(dbl).not_to receive(:foo)
            (Double (anonymous)).foo(*(any args))
                expected: 0 times with any arguments
                received: 2 times with any arguments
          # ./spec/unit/foo_spec.rb:4:in `block (2 levels) in <top (required)>'

Two failures are reported when it's really the same failure. This is really confusing.

I believe the source of the problem is #884 -- with that change we notify the error twice.

I think this should be fixed before 3.3 is released. There's also something weird going on with it claiming to have received foo 2 times...we should look into that as well.

[myronmarston]

So it's pretty non-obvious how to solve this. The problem at its core is that w/o aggregate_failures, you want the eager failures to be re-raised during the verification step to guard against the earlier being rescued and silenced (as could happen if you're running in a context such as sinatra that rescues errors). But w/ aggregate_failures (and potentially other failure notifiers...), you don't want the failure to be re-raised -- otherwise the user is notified twice.

I've thought of a few possible solutions but none of them is obviously the best way:

• We could odd an option to RSpec::Mocks.verify that allows the caller to control if eager failures are re-raised or not. Then rspec-core could pick what value to pass for that option when verifying based on whether or not the example is tagged with :aggregate_failures. Problem is, that option isn't part of rspec-core's mocking adapter interface.
• We could redefine aggregate_failures here in rspec-mocks to (1) toggle some global state (perhaps a thread local); (2) super to the existing definition; (3) toggle the global state back. Then we can use that global state to determine if we should re-raise or not. Redefining an rspec-expectations method feels gross, though.
• When eagerly raising, we could see if RSpec::Support.failure_notifier is the default implementation (e.g. raise) and if so, note on the particular message expectation that it needs to be re-raised during verification. If the failure notifier is something different, we would note on the message expectation that it doesn't need to be re-raised later during verification, since the failure is being handled in some out-of-band way. This too seems gross and brittle.

Another option to consider is reverting #884. It's a nice-to-have for situations like #874 but that's the only time we've ever received a report of that problem from a user, AFAIK. Currently #884 works because rspec-core only verifies mocks if the example does not already have an exception...but I'm mulling over the idea of changing it so that it always verifies if we can find a solution for rspec/rspec-core#1966 -- that way users would always be notified of mock expectation failures. If we made that change, #884 wouldn't be compatible with it, anyway.

Thoughts from @rspec/rspec ? @samphippen, I'm particularly keen to hear your thoughts since you authored #884.

[fables-tales]

Thoughts (in no particular order)

We could odd an option to RSpec::Mocks.verify that allows the caller to control if eager failures are re-raised or not. Then rspec-core could pick what value to pass for that option when verifying based on whether or not the example is tagged with :aggregate_failures. Problem is, that option isn't part of rspec-core's mocking adapter interface.

We could make core check the interface, only passing the option if the arity is present, or we could add a supports_options? method, check it that's present or a billion other little hacks. Although, that is obviously gross

We could redefine aggregate_failures here in rspec-mocks to (1) toggle some global state (perhaps a thread local); (2) super to the existing definition; (3) toggle the global state back. Then we can use that global state to determine if we should re-raise or not. Redefining an rspec-expectations method feels gross, though.

I actually don't think rspec-mocks having it's own aggregate failures implementation is such a problem. Specifically consider the case that we fixed in #884 where multiple mocks are violated and caught during execution. Would it be possible to have the mocks implementation of aggregate_failures show all the failures?

When eagerly raising, we could see if RSpec::Support.failure_notifier is the default implementation (e.g. raise) and if so, note on the particular message expectation that it needs to be re-raised during verification. If the failure notifier is something different, we would note on the message expectation that it doesn't need to be re-raised later during verification, since the failure is being handled in some out-of-band way. This too seems gross and brittle.

Agree this approach feels brittle, probably don't like it.

I have two other thoughts on implementation strategy:

• Have mocks record if they've raised, if the example already failed then we don't raise again, if it didn't then we do
• Add some global state in aggregate_failures itself something like RSpec.aggregating_failures?, then we could modify the behaviour from #884 to switch when raising exceptions on that, only performing the verify-time raise.

Another option to consider is reverting #884. It's a nice-to-have for situations like #874 but that's the only time we've ever received a report of that problem from a user, AFAIK. Currently #884 works because rspec-core only verifies mocks if the example does not already have an exception...but I'm mulling over the idea of changing it so that it always verifies if we can find a solution for rspec/rspec-core#1966 -- that way users would always be notified of mock expectation failures. If we made that change, #884 wouldn't be compatible with it, anyway.

I'm against reverting #884 until another solution exists, I actually encountered this on a client project recently and was like "Oh, I implemented the fix for this exact case recently". I think there's a lot of bug-reporting shyness, especially in weird edge cases like this. I feel like the fact we correctly fail your tests in the case where you catch the generic exception class is a huge win for RSpec. Obviously, if we always run verify and only raise exceptions there, then everything will be fine.

Thoughts?

[myronmarston]

We could make core check the interface, only passing the option if the arity is present, or we could add a supports_options? method, check it that's present or a billion other little hacks. Although, that is obviously gross

Yep, gross :cry:.

I actually don't think rspec-mocks having it's own aggregate failures implementation is such a problem. Specifically consider the case that we fixed in #884 where multiple mocks are violated and caught during execution. Would it be possible to have the mocks implementation of aggregate_failures show all the failures?

There's no need for rspec-mocks to have an implementation of aggregate_failures to achieve that. We already have that functionality. All that's necessary is for rspec-mocks to notify failures via RSpec::Support.notify_failure, which it already does.

Agree this approach feels brittle, probably don't like it.

I don't like the brittleness, either, but the more I think about it, there's an aspect of this solution that seems really worthwhile: it keeps track on the message expectation itself whether or not it needs to re-raise. Really, I think that's what we'd like: during verification, each failing message expectation would notify unless user_already_notified?. The tricky part is being able to track user_already_notified?. I'm thinking that we might be able to use the aggregate_failures override solution to facilitate that.

[fables-tales]

@myronmarston did you think either of my alternate implementations were useful? It seems like you're leaning towards checking RSpec::Support.failure_notifier and switching based on that?

[myronmarston]

No, the solution I'm leaning towards is a hybrid one:

• Override aggregate_failures in rspec-mocks.
• Have it toggle some global state (essentially the RSpec.aggregating_failures? flag you suggested).
• Use that state to determine the value of user_already_notified? on the specific message expectations.
• Check user_already_notified? when verifying.

[fables-tales]

Right, got it :) Seems good.

[JonRowe]

Why not have an option on RSpec::Support.failure_notifier for 'eager' failures, which then aggregate_failures could ignore?

[myronmarston]

Why not have an option on RSpec::Support.failure_notifier for 'eager' failures, which then aggregate_failures could ignore?

Can you expand on that? How do you imagine it working? It's unclear to me how that would solve the problem here...

[JonRowe]

Add the capacity for options to the method and pass to the notifier, in the default case we'd ignore the option and just raise, in the aggregate failures case we'd use the option to not store the exception?

[myronmarston]

Add the capacity for options to the method and pass to the notifier, in the default case we'd ignore the option and just raise, in the aggregate failures case we'd use the option to not store the exception?

I think I like this idea! One concern, though: the eagerly notified exception is potentially more useful than the end-of-example verify one. The eagerly notified one contains the stack trace pointing at the call site where the method was called, which is pretty useful. The end-of-example verify exception simply states that it was called, but provides no clues as to where.

[JonRowe]

The failure notifier generates a backtrace though would that not be sufficient?

[JonRowe]

Hmm probably not, however we could flip this idea and consider the inverse, 'fallback' errors, again same principle normally raise but aggregate failures would ignore them (as it should always hit both)

[myronmarston]

The failure notifier generates a backtrace though would that not be sufficient?

It's not a backtrace from the site where the method was called, though. It's a backtrace from the point where the error was generated, which happens later during verify.

Hmm probably not, however we could flip this idea and consider the inverse, 'fallback' errors, again same principle normally raise but aggregate failures would ignore them (as it should always hit both)

That would work. I'm thinking that instead of :eager vs :fallback we may want to pass a :source_id -- the idea being that the failure aggregator could use that to de-dup. Since the eager and fallback errors both originate from the same message expectation instance it would generate the same source id both times (using something like "#{self.class.name} - #{__id__} as the source id).

[JonRowe]

I like it!

[myronmarston]

So this issue as resurfaced. Specifically, rspec/rspec-core@f989168c9dbbcd930af6e7cc61e08057194929a8 (part of rspec/rspec-core#1985) has caused the same issue for a simple example when you are not using aggregate_failures:

RSpec.describe "A negative message expectation" do
  it "fails when the message is received" do
    dbl = double("Some Collaborator").as_null_object
    expect(dbl).not_to receive(:foo)
    dbl.foo
  end
end

The rspec-mocks build is currently failing due to this. It's only causing a couple cukes to fail (no specs) which is why rspec/rspec-core#1985 was green and we didn't realize this. I realized it because #969 is failing due to this issue.

The underlying problem is that while we now notify the errors with a :source_id, that id is only used by aggregate_failures, and when aggregate_failures is not used, it's ignored. With the rspec-core change to always verify mocks, we get the duplicate error again. So...we need to figure out what to do again. Ideas:

• Revert the rspec-core change to always verify mocks (with the plan to revert the revert after 3.3 ships). This would allow us to release 3.3 right away.
• Find a way to make rspec-core use the :source_id for this case. Currently it doesn't have access to it because the default failure notifier raises the exception and ignores the options hash.
• Switch to one of the other solutions discussed in this ticket. The problem with the :source_id solution is that it only worked for aggregate_failures. The other solutions are more general and should cover this case as well.

Any thoughts on what route to take, @rspec/rspec? I'm of a split mind here; on the one hand, the rspec-core change that triggered this is a nice-to-have and fits with the general aggregate_failures feature, and it'd be nice to keep it in 3.3. OTOH, I'm kinda tired of pushing out the 3.3 release and reverting so we can release ASAP sounds appealing. Or maybe the fix to make this work properly is fast and simple and we can fix it ASAP. I'm not sure yet.

[myronmarston]

I've thought of an alternate solution that should work. It's inspired by the discussion in #203. Using raise to notify a failure means someone can rescue it and silence it...which in turn necessitates the need to re-raise at the end of the example, which in turn requires these work arounds. What if we could notify the failure w/o using raise? Specifically, we can use throw. This works for example:

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

The rescue Exception in the first example does not cause the expectation failure to be silenced. If we used this technique, we wouldn't have to re-raise eager failures at the end of the example since the only way to silence it would be for a user to put a catch(:rspec_failure) in their example...which would mean they were specifically trying to silence it.

All that said...switching to throw is potentially risky and there may be complications I'm not thinking of. So I'm currently leaning towards reverting rspec/rspec-core@f989168 so we can release 3.3 and then un-reverting it after 3.3 goes out and trying the throw solution.

[JonRowe]

We could try it, we'd probably want to bench mark it