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

Add .Unset method to mock #982

Merged
merged 11 commits into from Jun 22, 2022
Merged

Conversation

pdufour
Copy link
Contributor

@pdufour pdufour commented Jul 22, 2020

Summary

This adds the ability to remove handlers that were added with .On

Changes

  • Add .Unset Method
  • Add Unset method docs
  • Add Off method tests

Motivation

Nice to have this in tests where test will share majority of same mock handlers expect you might want to remove just 1 handler in one test.

Related issues

#558

@boyan-soubachov
Copy link
Collaborator

boyan-soubachov commented Jul 27, 2020

I'm not sure about the pattern used here. It seems less clear & implicit. Would it not be clearer if you attached the On() call to each of your test-cases?

Seems like the kind of situation where a bit of repetition is better than abstraction. :)

If the community feels strongly about this, I'd be happy to have another look.

@pdufour
Copy link
Contributor Author

pdufour commented Jul 27, 2020

@boyan-soubachov it forces all the logic to be consolidated in one Setup function if you can't override / remove individual mock handlers OR you can duplicate logic across tests. Use case:

SetupJobTest (run before each test)

  • Here I create a bunch of stubs using .On
  • Say getLocation, getEmployee, getTransfer, getWorkHistory

TestInvalidJobTransfer

  • Here I want to have getTransfer throw an error so I can make sure that that is handled correctly
    • If I have an .Off method, I can call .Off and then have the original handler removed so I can add a new one
    • Right now my options are: to add some conditionals in SetupJobTest that checks if test == TestInvalidJob and if so, add a different stub. This causes setup code related to TestInvalidJobTransfer to leak into the global setup code though and is messy to maintain.
    • Create the .On job transfer stub in each test that needs it. In this case I'm writing twenty-thirty tests and this is very repetitive and difficult to maintain over time.
    • Another option I'm not thinking off?

I think providing different options allows more flexibility for different types of use cases.

LMK if what I said makes sense.

@pdufour
Copy link
Contributor Author

pdufour commented Jul 27, 2020

Another option would be if you could prepend handlers to the list that would allow you to override handlers created before. That's probably the crux of the issue and having .Off is 1 way to solve it. Right now as soon as you create a handler using .On there is no way to override it which IMO makes managing large test suites very difficult. I think there has demonstrated community support for a feature like that #558.

@pdufour
Copy link
Contributor Author

pdufour commented Jul 27, 2020

Just had some more thoughts, I don't really have to call .Off at any time in test suites I've set up using Sinon / Mocha.js in JS. And I think that is because if you define a stub, then if you try to define another stub for the same method, that one overrides the previous one. This feels like the expected behavior, LIFO, but maybe Go paradigms are different, I'm still learning the language.

That would be a quick and easy change though, just changing this to do a prepend https://github.com/stretchr/testify/blob/master/mock/mock.go#L274.

@nbaztec
Copy link

nbaztec commented Aug 18, 2020

@pdufour for your particular use case wouldn't it be easier to create a func that returns a lowest common denominator stub and attach individual assertions per test? Or even a few create funcs to ease the code duplication?

func createStub() mock.Mock {
     m := MyMock{}
     m.On("CommonMethod1").Returns(...)
     m.On("CommonMethod2").Returns(...)
}

func createRelaxedStub() mock.Mock {
     m := MyMock{}
     m.On("CommonMethod1").Returns(...)
     m.On("CommonMethod2").Returns(...)
     m.On("CommonMethod3").Returns(...)
}


func TestFoo(t testing.T) {
   m := createStub()
   m.On("SpecificMethod").Returns(...)
}

In Go it seems most projects either wrap complex Mocks within nice interfaces or create the objects per test (which would also be my preference in majority of the cases)

Copy link

@HangjianQian HangjianQian left a comment

LGTM

@CyborgMaster
Copy link

CyborgMaster commented Sep 17, 2021

On of the issues that this PR would solve is if your unit under test has a few dependencies and you want to set them all up at the beginning in setup.

func (s *ATestSuite) SetupTest() {
	s.dep1 = &mocks.Dep1{}
	s.dep2 = &mocks.Dep2{}
	s.dep3 = &mocks.Dep3{}
	s.uut = NewUnitUnderTest(s.Dep1, s.Dep2, s.Dep3)

	s.dep1.On("CommonCall1", mock.Anything, mock.Anything).Return(false, nil).Maybe()
	s.dep2.On("CommonCall2", mock.Anything, mock.Anything).Return(false, nil).Maybe()
	s.dep3.On("CommonCall3", mock.Anything, mock.Anything).Return(false, nil).Maybe()
}

Then in each test I want to focus on the interactions with only one dependency. I don't want to have to setup the UUT at the beginning of each test; I want it int the setup function. If I can just override one of these default stubs, then I can focus on what is unique to this one test. If I cannot, then I'm forced to repeat the setup logic in each of my tests.

@pbmlunar
Copy link

pbmlunar commented Jan 26, 2022

I just ran into a scenario where I wanted this feature. Sad to see that it has not been merged into the project yet.
I was in the middle of writing my own similar logic, but ended up changing my tests around quite a bit just to handle this "missing" feature.

Please implemented it soon as it would be a great help

@timothy-baker
Copy link

timothy-baker commented Feb 1, 2022

+1, I recently wrote a very large test suite where this would've been very useful.

@Gilwe
Copy link

Gilwe commented Feb 3, 2022

Same here, it would extremely helpful if you were to merge this feature (or a newer version of it, if needs)

@boyan-soubachov
Copy link
Collaborator

boyan-soubachov commented Feb 10, 2022

Excuse me, I've been quite busy.

Given that the .On() receiver function's resultant behaviour is actually specified with the function that follows it (e.g. .Return()), would it not make more sense if we added a function like .Unset() instead of Off()?

e.g. test.On("func", mock.Anything).Unset() ... or maybe test.On("func", mock.Anything).RemoveMock()?

IMO that would be more consistent and intuitive when factoring in the use case context.

Whilst "Off" makes sense as being the opposite of "On", it doesn't seem to make sense in this context since the word "On" being used isn't the verb but rather the adverb.

@Gilwe
Copy link

Gilwe commented Feb 12, 2022

@boyan-soubachov I agree with you that the method name suggested isn't consistent with the other method names of the mocks but the main issue of removing or clearing ExpectedCalls with a defined interface is still needed.

Unset sounds great IMO.
Just to make sure, the requested feature should support those two actions:

  1. Clearing ExpectedCalls by method name
  2. Clearing all ExpectedCalls

@boyan-soubachov
Copy link
Collaborator

boyan-soubachov commented Feb 15, 2022

@boyan-soubachov I agree with you that the method name suggested isn't consistent with the other method names of the mocks but the main issue of removing or clearing ExpectedCalls with a defined interface is still needed.

Agreed, the feedback seems to suggest it would be a useful feature for the community; contrary to my earlier comment :)

Unset sounds great IMO. Just to make sure, the requested feature should support those two actions:

1. Clearing ExpectedCalls by method name

2. Clearing all ExpectedCalls

Fair enough, we could have something like mock.On(..., ...).Unset() for the first case and mock.UnsetAll() for the 2nd?

Maybe even a Remove()/RemoveAll() combination would be more intuitive and better named?

@huan-Mongo
Copy link

huan-Mongo commented Mar 7, 2022

That sounds great to me! Wondering when we can get this feature merged? Thanks!

@mssalnikov
Copy link

mssalnikov commented Mar 17, 2022

Is there anything needed yet that's blocking this? We have to use mock.ExpectedCalls = nil as a workaround for now, and it's a really bad practice. Is there any help needed or something? I'd really like to see this in the master

@jordaniversen
Copy link

jordaniversen commented Apr 3, 2022

Any update on this? Looking for such a feature.

Thanks

@stephenjayakar
Copy link

stephenjayakar commented May 11, 2022

I would also appreciate this feature

@pdufour
Copy link
Contributor Author

pdufour commented May 20, 2022

@boyan-soubachov I agree with you that the method name suggested isn't consistent with the other method names of the mocks but the main issue of removing or clearing ExpectedCalls with a defined interface is still needed.

Agreed, the feedback seems to suggest it would be a useful feature for the community; contrary to my earlier comment :)

Unset sounds great IMO. Just to make sure, the requested feature should support those two actions:

1. Clearing ExpectedCalls by method name

2. Clearing all ExpectedCalls

Fair enough, we could have something like mock.On(..., ...).Unset() for the first case and mock.UnsetAll() for the 2nd?

Maybe even a Remove()/RemoveAll() combination would be more intuitive and better named?

I'm just going to close this since it's just a bunch of +1,s the author of the repo provided an API he'd be happy with, so if you'd like the feature, go ahead and develop it and make a PR.

@pdufour pdufour closed this May 20, 2022
@boyan-soubachov
Copy link
Collaborator

boyan-soubachov commented Jun 20, 2022

@pdufour , no need to close it, IMO. We're just debating the naming :)

Would you be happy with renaming the Off(...) function to Unset(...) instead?

@pdufour
Copy link
Contributor Author

pdufour commented Jun 22, 2022

@boyan-soubachov I no longer use mockery but that seems reasonable to me.

@pdufour pdufour reopened this Jun 22, 2022
@pdufour
Copy link
Contributor Author

pdufour commented Jun 22, 2022

@boyan-soubachov I renamed it as Unset but I'm not sure if you were looking for the different API still or just the rename?

@pdufour pdufour changed the title Add .Off method to mock Add .Unset method to mock Jun 22, 2022
@pdufour
Copy link
Contributor Author

pdufour commented Jun 22, 2022

@boyan-soubachov Went ahead and updated the API, PTAL

Copy link
Collaborator

@boyan-soubachov boyan-soubachov left a comment

Looks great! Thank you for your contribution :)

@boyan-soubachov boyan-soubachov merged commit ba1076d into stretchr:master Jun 22, 2022
3 checks passed
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