Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding compound methods to receive and have_received, fixes #1298. #1299

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

eLod
Copy link

@eLod eLod commented Nov 18, 2019

No description provided.

@pirj
Copy link
Member

pirj commented Nov 25, 2019

I've tried playing with the code, and couldn't get those or examples to work.

@eLod
Copy link
Author

eLod commented Nov 26, 2019

i tried to investigate but it's a bit more perplexing, as the or examples for have_received are working as expected, only the receive matcher behaves differently. i tried to investigate what is going on, but i am not very familiar with rspec's inner workings, what i find odd is how the proxy looks at the end, e.g. the proxy object for the test double has @method_doubles for :foo and :bar, but the expectations look very differently for the two matchers (for have_received the MethodDoubles' expectations are empty and their @order_groups have both expectations, for receive the :foo MethodDouble and its @order_group has a single @expectations for only :foo and the :bar MethodDouble has not expectation, but its @order_group has a single expectation for :foo).

@JonRowe
Copy link
Member

JonRowe commented Nov 27, 2019

Does it help your understanding if I explain that the two matchers check at different points in the lifecycle?

have_received checks when it is called, it should be a bit simpler to understand because of this, that the object under test's stubs / messages received match the expectation.

receive has to setup expectations ahead of time, and is verified after the test has finished in order to see if it was ever called, but is also checked when those mocks are called, its... complex...

@eLod
Copy link
Author

eLod commented Nov 28, 2019

@JonRowe well on the high level i did know that, my concern is rather that though i have been using rspec for a long time i have to admit i know very little about its inner workings, so it's hard for me to investigate/diagnose this (or any other problem) and even harder to articulate questions/thoughts clearly. i can keep investigating (and i plan to), but naturally i will need to get more familiar with rspec first, so it will take some time. meanwhile any pointers would be greatly appreciated.

@pirj
Copy link
Member

pirj commented Jan 10, 2020

@eLod did you reach the enlightenment point? Do you need some guidance here?

@eLod
Copy link
Author

eLod commented Jan 11, 2020

@pirj no sorry i didn't find the time last year, but i haven't given up on it yet, just busy.

@eLod
Copy link
Author

eLod commented Feb 7, 2020

@pirj @JonRowe so finally i have had some time to dig a bit deeper and at least can articulate my thoughts better. so it seems, as @JonRowe wrote, the have_received matcher is simply doing checking after the fact and everything works as expected, on the other hand receive is broken (for the or case) because the way it maps the expectations onto the target.

what's happening is Matchers::BuiltIn::Compound::Or (from rspec-expectations) is simply calling matcher1.matches? || matcher2.matches? which calls Matchers::Receive#setup_expectation (aliased as matches?) on the first, which in turn does the setup for the method (and returns the @describable so the second matcher is not even evaluated), e.g. setting up a simple message expectation of receiving the message. so now the proxy has 2 @method_doubles, the first of them having a single expectation for its own method, the second none. when verify runs the first one simply triggers its own error.

the problem is, as far as i currently understand it, that the compound 'concept' is broken in how the message expectations are handled in the MethodDouble (like putting the rspec-expectations compound expectation into the list, e.g. mixing different types), or specific CompoundMessageExpectation type(s) must be implemented to handle these cases duplicating some of the logic from rspec-expectations. i think it can not be solved with a small change, at least it involves deeper parts that has ramifications for other code paths as well.

i am not sure if i am missing anything, like an easy solution, i am happy to work on this, just don't want to jump to implementation before there is some consensus about the direction.

@JonRowe
Copy link
Member

JonRowe commented Feb 7, 2020

the problem is, as far as i currently understand it, that the compound 'concept' is broken in how the message expectations are handled in the MethodDouble (like putting the rspec-expectations compound expectation into the list, e.g. mixing different types), or specific CompoundMessageExpectation type(s) must be implemented to handle these cases duplicating some of the logic from rspec-expectations. i think it can not be solved with a small change, at least it involves deeper parts that has ramifications for other code paths as well.

Yeah that sounds right, the problem is that rspec-expecations (which owns the compound matcher stuff) has no knowledge of rspec-mocks (in order for it to be isolatable). For or to work we need a mechanism in the matcher protocol that says "this matcher is not immediate, and must be verified later", some kind of deferred_matcher? implementation.

i am not sure if i am missing anything, like an easy solution, i am happy to work on this, just don't want to jump to implementation before there is some consensus about the direction.

I don't think you are missing much 😂

@eLod
Copy link
Author

eLod commented Feb 8, 2020

@JonRowe that's another angle, like how i understand it currently is that receive already is deferred, makes these expectations deferred, e.g. it creates its own expectation on the MethodDouble which are validated later (with verifying). When i wrote 'concept broken' i've meant this, e.g. that when receive is 'translating' (deferring) these expectations (replaying the customisations and etc) that part (eg the MessageExpectations in MethodDouble) specifically has no concept of compound expectations (e.g. the implementation is a simple queue/array). Making a deferred matcher abstraction (in rspec-expectations) would solve this on another level (which could be still a better option, i'm not arguing against that).

Another way out (compromise) i've realised is simply not enabling compound functionality on receive and only on have_received.

@eLod
Copy link
Author

eLod commented Feb 8, 2020

so just to make it explicit, the alternatives are

  • introducing a deferred matcher concept in rspec(-expectations)
  • mixing multiple types of expectations in MethodDouble#expectations e.g. adding the Compound matcher/expectation (from rspec-expectations) here
  • adding new type(s) of MessageExpectation to MethodDouble#expectations e.g. implementing new CompoundMessageExpectation, duplicating compound logic from rspec-expectations
  • only allow compound composition on have_received (and not on receive)

apart from the last one all needs to refactor how the receive matcher creates these deferred expectations on the MethodDouble, the first alternative possibly needing other refactors in other parts as well.

edit: i've also realised alternative 2, is not that a great solution (breaking other concepts to support 1 use-case), while alternative 3, already has easier out (how it already records and replays customization, so we only need to handle and record the compound methods on the receive expectation and replay later) so it feels like more appropriate for the current implementation.

@JonRowe JonRowe changed the base branch from master to main August 2, 2020 02:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants