-
Notifications
You must be signed in to change notification settings - Fork 40
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
feat: add 'Optional' to all mocked methods: 0 or more calls is now allowed #95
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @DenKoren ,
Excuse me for the delayed review. I like your idea of being able to control whether the mocked method is required to be called or it's optional. However, it should be done on each individual method level not the whole mocked interface.
Also can you please update documentation? It's a very nice feature that deserves a couple of lines in the readme.
Please let me know if you're willing to make these changes or should I take over.
template.go
Outdated
// restriction in some test scenarios. | ||
// It is NOT RECOMMENDED to use this option by default unless you really need it, as it helps to | ||
// catch the problems when the expected method call is totally skipped during test run. | ||
func (m *{{$mock}}{{(paramsRef)}}) SkipMinimockFinish() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets name this method Optional
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
template.go
Outdated
// catch the problems when the expected method call is totally skipped during test run. | ||
func (m *{{$mock}}{{(paramsRef)}}) SkipMinimockFinish() { | ||
m.skipMinimockFinishCheck = true | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to set this property for each individual method of an interface rather than set this for all methods of the mock to have some flexibility.
I'm thinking about something like:
m := minimock.NewController()
storageMock := NewStorageMock(m).
StoreBoxMock.Optional().Expect(box).Return(nil).
StoreMail().Expect(envelope).Return(nil)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@hexdigest , sorry for the long delay. Found a window in my tight work schedule to fix the PR. Now it works on per-method basis. |
In some (quite rare, but anyway) test cases you cannot predict if the method will be called for particular instance of mocked interface.
Say, you test the concurrency and plan to execute 100 tasks. You expect the test to have 50 of tasks to be done and 50 to be not even started. But you do not know which of tasks the executor will choose, to set that 'the mock here should be called' and 'the mock here should NOT be called'. This is just out of your control, but you still want to check the constraint, that there is 50/50 results division.
For that case, the restriction of '1 or more' on a mocked method makes impossible to implement such test using minimock-generated interface mock implementation.
The default behavior of minimock to require calls for mocked methods is OK, it is just good to have control over it.