Skip to content
This repository has been archived by the owner on Jun 27, 2023. It is now read-only.

Add loose mocks #188

Closed
wants to merge 12 commits into from
Closed

Add loose mocks #188

wants to merge 12 commits into from

Conversation

jjm3x3
Copy link

@jjm3x3 jjm3x3 commented May 12, 2018

The Gist

This pull request adds the ability to create a controller which will allow for Loose mocks. This means that they will not error when unexpected calls are made to them. This should close #7. I welcome any and all feedback and would love to see this get merged in to make this library even more useful.

Nuts and bolts:

  • I have decided to go with the keyword Loose as opposed to Dynamic since Dynamic can mean so many things and is used quite often in software. I am certainly open to a discussion on this.
  • I opted to add LooseMode as a property on the controller. I did not add it as a parameter to the "constructor" since I wasn't sure if that was the appropriate way to do this. As such I opted for the simpler solution by exposing it and allowing it to be changed after the fact. (changing it after the fact is not tested. Since this is used in the Call method during test execution I would imagine it can only really be in one of the two states, implying that you can't create strict mocks and loose mocks which are backed by the same controller)

Testing

What I did do for testing is multi fasted. I added a few simple tests to make sure the feature works as expected. I changed the createFixtures method to make the controller be in LooseMode for all the tests, and I watched several break. In order to fix this I went test by test assessing weather it should break (i.e. the test was assuming strict mode (or LooseMode = false)), in which case I created a comparable test which would expect the right behavior if it was in loose mode. If it should succeed in LooseMode I fixed the code so that it doesn't fail. Then, as a final step I added a new method createLooseFixtures which creates the fixtures such that the controller is setup in LooseMode. For tests which are specific to loose mode createLooseFixtures is called. For all the other tests which are either specific to strict mode (!LooseMode) or are strict/loose mode agnostic the existing createFixures function is called.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

1 similar comment
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

I thought that VS Code did this for me but for some reason it seemed to
miss this one thing
@googlebot
Copy link

CLAs look good, thanks!

1 similar comment
@googlebot
Copy link

CLAs look good, thanks!

@jjm3x3
Copy link
Author

jjm3x3 commented May 13, 2018

I signed it!

@jjm3x3 jjm3x3 closed this May 13, 2018
@jjm3x3 jjm3x3 reopened this May 13, 2018
@balshetzer
Copy link
Collaborator

It seems like the mock is only loose when there are no expected calls to a function at all. If there are any expected calls to the function and an unexpected call is made then the mock will call Fatalf. Is this intentional? In my experience, a loose mock would treat all unexpected calls the same: print something and just return default values.

One will explicitly assert what I meant for it too and there is a new
one to catch the fact that there are more possible matching errors that
exist which must be handled accordingly
@jjm3x3
Copy link
Author

jjm3x3 commented Jun 2, 2018

@balshetzer If I understand correctly, I think this is actually the case. It is true that it is loose when there are no expected calls. However I went back to check the case where there are expected calls. It seems like this is something that I covered however I did so poorly. I don't think I added a test for my expectation. So I have added a couple tests called TestMakingAnUnexpectedCallWhereCallsAreExpectedLooseMode and TestMakingAnUnexpectedCallWhereCallsAreExpectedStrictMode, which both pass just fine. After some careful consideration it looks like is much more going on in a Call's matches method which I am assuming that is what you are referring to. I was able to write a failing test called TestMakingAnUnMatchingCallWhereSpecificCallsAreExpectedLooseMode. I am currently looking at how one might solve this. One thought is to add another FindLooseMatch method on the callSet. Then pass a bool to the matches method since loose mode would mean we never really care if it is an exact match, unless it is one (we can ignore matching errors until the end). This is clearly a WIP however I am open to suggestions seeing as you where able to catch my gap in understanding of how this library works. Thanks!

In the previous commit I have a test which is broken partially because
it is and partially because it is the wrong test. This is fixed in this
commit. As well as another test which discovered during development.
After adding some failing test cases I made the necessary changes to get
it working now some refactor
I have always undetstood passing booleans into functions to be a
codesmell so instead of calling `FindMatch` with the isLoose flag I have
opted to call two different functions bassed on weather the isLoose flag
is set. This chould be taken one step further and be made part of the
`CallSet` itself but was not sure if that abstraction made sense
After some further dive down I have realized that After does not work in
LooseMode assuming we want to maintain a close to exact feature set I
have added a test to demonstrate this failing behavior. Fix to come.
weather a calls prereqs are met doesn't seem semntically signifigant to
weather it is a match so I removed that as a dependancy and now call it
in the `callSet`s `FindMatch` methods which allows each version to
decide if that is necessary. This change enables us to fix the fact
that `After` does not work with LooseMode
Copy link

@byxor byxor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a feature I'd really like to see in gomock.

@amghost
Copy link

amghost commented Aug 8, 2019

Do we have a plan of merging this to the master?

@poy
Copy link
Collaborator

poy commented Sep 27, 2019

Sorry for the slowness on my end here, however this is a feature I would love to see make it in so I'm going to join the conversation.

So I believe if we look around at how other mocking frameworks behave, we can gain some insight on how to solve this. I made a proposal in #246, I'm wondering if that would help with some of the complexities and edge cases we're bumping into there.

Thoughts?

@poy poy self-requested a review September 27, 2019 06:13
@poy poy self-assigned this Sep 27, 2019
@codyoss
Copy link
Member

codyoss commented Nov 20, 2019

@jjm3x3 Still interested on iterating on this?

@codyoss
Copy link
Member

codyoss commented Dec 27, 2019

I am going to close this for now due to lack of inactivity. Please reopen if you are still interested in working on this. Thanks.

@codyoss codyoss closed this Dec 27, 2019
@hardikmodha
Copy link

hardikmodha commented Aug 22, 2020

I would also like to see this feature. Right now, a simple test looks too complex because multiple methods need to be mocked which is kind of unnecessary. It also makes tests very brittle and implementation-dependent. If something changes, tests start breaking even though the behavior is not changed.

@codyoss I can continue on this if no one else is working on this.

@codyoss
Copy link
Member

codyoss commented Aug 22, 2020

Lets move conversation over to #7.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow generating loose/dynamic mocks
8 participants