Prevent error formatting from causing recursive errors #1275

jamesdabbs-procore · 2019-05-03T02:14:03Z

This comes from tracking down rspec/rspec-rails#2049 wherein

it 'should show only 2 times' do
  expect(@student).to     receive(:email).once.and_call_original
  @student.email
  @student.email
end

overflows. There Devise has defined an inspect method which calls email, so when the second call to email errors, the error generator's inspect call goes into a loop.

I've added a spec that I think describes the desired behavior, but which is currently failing against master.

The included lib change fixes the spec, but this is my first time digging into the RSpec codebase, and it's entirely possible there's a better place to make that change.

Please let me know if I've mis-understood the desired behavior, or if there's anything else you'd like to see changed. Thanks!

as initially reported in rspec/rspec-rails#2049 The issue occurs when a class defines an `inspect` method which depends on methods that have been stubbed to e.g. `receive(...).at_most(n).times`. Failures try to call `inspect` which fails which calls `inspect` which ...

The error generator may make subsequent calls back in to the erroring method. This prevents those calls from cascading errors or incrementing the call count.

JonRowe

Nice catch, a few tweaks :)

spec/rspec/mocks/partial_double_spec.rb

lib/rspec/mocks/message_expectation.rb

JonRowe · 2019-05-03T07:29:02Z

lib/rspec/mocks/message_expectation.rb

@@ -424,7 +425,11 @@ def safe_invoke(parent_stub, *args, &block)
        end

        def invoke(parent_stub, *args, &block)
-          invoke_incrementing_actual_calls_by(1, true, parent_stub, *args, &block)
+          if @raised
+            invoke_incrementing_actual_calls_by(0, false, parent_stub, *args, &block)


You don't need to do this at all.

I'm not entirely sure what you're saying here. Are you saying this could just be?:

def invoke(...) return if @failed invoke_incrementing_actual_calls_by(1, true, ...) end

If so, that breaks the new spec as written b/c inspect is expecting to receive the correct name post-failure. I also considered extracting an "invoke directly" from around here for clarity, or unsetting the method proxy once it failed, but those seemed like larger changes. If any of those approaches sound better, or if I'm misunderstanding the comment, please let me know.

Well wouldn't invoke_without_incrementing_received_count be a better choice?

So after digging a bit, I'm not entirely sure what's best here.

invoke_without_incrementing_received_count is still allowed_to_fail here, so this hits the same stack overflow

safe_invoke, on the other hand, stops the error, but increments the count so this error message ends up being expected: 1 time with any arguments\n received: 3 times with any arguments instead, which is a little counter-intuitive for end users.

It seems like invoke_incrementing_actual_calls_by(0, false, ...) is what needs to happen here, unless you're envisioning larger changes elsewhere.

Thanks for your patience here, I'd rather the code expressed what it does, if we have to change invoke_without_incrementing_received_count so we can send the false thats ok, we might just need two methods, personally I think the whole matcher implementation needs refactoring a bit breaking apart the methods so we don't have to pass these counts and a boolean flag...

Totally! Thanks for the feedback. I should have a little bit of downtime over the Thanksgiving weekend, so I'll take a look and aim to have something up next week.

following comments from CR

fables-tales · 2019-06-12T13:42:34Z

@jamesdabbs-procore would you be able to finish this one off and close it out?

jamesdabbs-procore · 2019-06-12T15:51:21Z

Ah, sorry. Lost this thread. Definitely, although it might be sometime this weekend.

pirj · 2019-11-15T20:04:41Z

@jamesdabbs-procore ping!

jamesdabbs-procore · 2019-11-18T15:53:23Z

Oh, sorry. I posted a threaded response earlier - #1275 (comment) - and could use some feedback there. (I'm not entirely sure what change is being requested.)

lib/rspec/mocks/message_expectation.rb

JonRowe · 2019-11-27T15:19:19Z

lib/rspec/mocks/message_expectation.rb

@@ -556,6 +561,7 @@ def invoke_incrementing_actual_calls_by(increment, allowed_to_fail, parent_stub,
          args.unshift(orig_object) if yield_receiver_to_implementation_block?

          if negative? || (allowed_to_fail && (@exactly || @at_most) && (@actual_received_count == @expected_received_count))
+            @failed = true


I want to remove this, and replace it with an @invoking check further down.

e.g

@invoking = true if implementation.present? implementation.call(*args, &block) elsif parent_stub parent_stub.invoke(nil, *args, &block) end @invoking = false

I think thats safer and I think theres a risk of false positives as is, as it won't record further invocations?

If the error generator sends a message that we've expected `exactly` or `at_most` some number of times, we get a recursive error and stack overflow. This ensures that if we re-`invoke` while already `invoke`ing, we don't increment the count or allow errors to bubble up.

jamesdabbs-procore · 2019-12-11T00:56:12Z

@JonRowe - I think that last implements the changes you were requesting. I also don't love the proliferation of flags / method-name combinations. Are the existing safe_invoke and invoke_without_incrementing_received_count intended to be part of the publicly supported API here? (Is invoke itself?) If not, we could standardize on something like

def invoke(parent_stub, *args, increment: 1, allow_failure: !@invoking, &block)

and update the existing callers.

What were you thinking in terms of refactoring the existing invoke?

benoittgt · 2019-12-13T09:26:02Z

I find the current implementation pretty clean and readable.

I don't think we want this to be accessible to the public API. Do you see a reason to have it public? I am not against your proposal but it is little bit more complicated. :)

Could you rebase master for the CI?

JonRowe · 2019-12-14T12:33:01Z

Ok I'm happy to merge this once CI is green, if I feel that strongly about the method names I'll refactor it myself later 😂, can you give it a rebase?

jamesdabbs-procore added 2 commits May 2, 2019 20:59

Prevent stack overflow when presenting error messages

7b230f2

The error generator may make subsequent calls back in to the erroring method. This prevents those calls from cascading errors or incrementing the call count.

jamesdabbs-procore mentioned this pull request May 3, 2019

"stack level too deep" when "expect().to receive().once" fails rspec/rspec-rails#2049

Open

JonRowe requested changes May 3, 2019

View reviewed changes

Rename variable; tweak style

bccc110

following comments from CR

JonRowe reviewed Nov 27, 2019

View reviewed changes

lib/rspec/mocks/message_expectation.rb Outdated Show resolved Hide resolved

JonRowe reviewed Nov 27, 2019

View reviewed changes

lib/rspec/mocks/message_expectation.rb Outdated Show resolved Hide resolved

JonRowe reviewed Nov 27, 2019

View reviewed changes

JonRowe changed the base branch from master to main August 2, 2020 02:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent error formatting from causing recursive errors #1275

Prevent error formatting from causing recursive errors #1275

jamesdabbs-procore commented May 3, 2019

JonRowe left a comment

JonRowe May 3, 2019

jamesdabbs-procore May 3, 2019

JonRowe May 14, 2019

jamesdabbs-procore Jun 19, 2019

JonRowe Nov 27, 2019 •

edited

Loading

jamesdabbs-procore Nov 27, 2019

fables-tales commented Jun 12, 2019

jamesdabbs-procore commented Jun 12, 2019

pirj commented Nov 15, 2019

jamesdabbs-procore commented Nov 18, 2019 •

edited

Loading

JonRowe Nov 27, 2019

JonRowe Nov 27, 2019

jamesdabbs-procore commented Dec 11, 2019

benoittgt commented Dec 13, 2019 •

edited

Loading

JonRowe commented Dec 14, 2019

Prevent error formatting from causing recursive errors #1275

Are you sure you want to change the base?

Prevent error formatting from causing recursive errors #1275

Conversation

jamesdabbs-procore commented May 3, 2019

JonRowe left a comment

Choose a reason for hiding this comment

JonRowe May 3, 2019

Choose a reason for hiding this comment

jamesdabbs-procore May 3, 2019

Choose a reason for hiding this comment

JonRowe May 14, 2019

Choose a reason for hiding this comment

jamesdabbs-procore Jun 19, 2019

Choose a reason for hiding this comment

JonRowe Nov 27, 2019 • edited Loading

Choose a reason for hiding this comment

jamesdabbs-procore Nov 27, 2019

Choose a reason for hiding this comment

fables-tales commented Jun 12, 2019

jamesdabbs-procore commented Jun 12, 2019

pirj commented Nov 15, 2019

jamesdabbs-procore commented Nov 18, 2019 • edited Loading

JonRowe Nov 27, 2019

Choose a reason for hiding this comment

JonRowe Nov 27, 2019

Choose a reason for hiding this comment

jamesdabbs-procore commented Dec 11, 2019

benoittgt commented Dec 13, 2019 • edited Loading

JonRowe commented Dec 14, 2019

JonRowe Nov 27, 2019 •

edited

Loading

jamesdabbs-procore commented Nov 18, 2019 •

edited

Loading

benoittgt commented Dec 13, 2019 •

edited

Loading