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

Feature/verify #68

Closed
wants to merge 1 commit into from
Closed

Feature/verify #68

wants to merge 1 commit into from

Conversation

horkhe
Copy link
Contributor

@horkhe horkhe commented Jun 21, 2012

I would like to suggest extending the meck API with a family of verify functions. They are inspired by Mockito Java mocking framework and its verify capabilities in particular.

The idea behind verify that it checks a condition and either succeeds of raises a runtime error. It does not need to be wrapped in any ?assert(...) function, it is assertion itself. Therefore it makes tests a little bit shorter.

If the specified condition is not meant then a nice readable error message is displayed. Consider:

meck:verify({at_least, 3}, mymod, test, ['', 2, '']).

{"Unexpected number of calls",
{pattern,{mymod,test,['',2,'']}},
{expected,{at_least,3}},
{actual,2}}

As you probably noticed verify effectively covers functionality provided by called and num_calls and adds more that is not yet present (like at_least and at_most predicates)

If you approve such API extension then I will add another function to this family that will receive a list of calls specs along with times to verify that a particular call order has been adhered.

@eproxus
Copy link
Owner

eproxus commented Jun 25, 2012

I've been thinking about such an API for some time as well. I had as idea to integrate Meck with Hamcrest: https://github.com/hyperthunk/hamcrest-erlang

The API you've provided so far is interesting, but it's very oriented around the number of calls only (correct me if I'm wrong). I'd like a more general approach. I've put up a wiki page with some ideas.

@horkhe
Copy link
Contributor Author

horkhe commented Jun 25, 2012

My primary goal was to have a unified verify method to substitute for called and num_calls. That acts similar to verify in Mockito. This commit is just the first iteration of this interface. If you accepted it I would continued to improve it to allow checking that calls were made in a particular order. consider:

verify([{{at_least, 3}, mod1, test, ['', 2, '']},
{at_least_once, mod1, test2, ['', '']},
{never, mod2, test3, []},
{{at_most, 2}, mod1, test, ['_', 3, 4]}]).

Latter we could allow using hamcrest expressions inside of the parameter list, just as you said.

Anyway, we are using your library in our company, and I would like to thank you for the great work you have done.

@eproxus
Copy link
Owner

eproxus commented Jun 26, 2012

Yeah, a unified verify function is the most optimal I think.

Makes sense to allow for Hamcrest-like expressions (maybe we don't have to depend on Hamcrest at all).

I'd like for us to have in the end:

  • A meck.hrl file with includes for all Hamcrest-like functions
  • A way of writing every assertion you have so far in the Hamcrest-like syntax

Keeping the verify function is fine. Personally I think the different arguments can be a bit limiting and would rather see a more dynamic approach. Any chance we can refine the API to be more general?

@horkhe
Copy link
Contributor Author

horkhe commented Jun 27, 2012

Could you please clarify what particular changes to verify you suggest by an example? E.g. how the following calls should look:

verify(never, mod, test1, [1, '_', 3]).

verify({at_least, 3}, mod, test2, [1, '', '']).

@eproxus
Copy link
Owner

eproxus commented Jul 7, 2012

How about this?

verify(Mod, has(no(calls_to(test1, [1, '_', 3]))))
verify(Mod, has(at_least(3, calls_to(test1, [1, '', '']))))

@horkhe
Copy link
Contributor Author

horkhe commented Jul 9, 2012

Hi Adam, the interface you suggest is definitely more readable but the at the same time it is more verbose. With this interface users would need to write has and calls_to in every verify. That might be fine, it all depends on what you think is more important in the interface of your library: readability or terseness.

My suggestion of the verify method was backed by a desire to make the Meck interface:

  • more consistent, one method to check everything instead: calls_num and called;
  • more concise, that is why I made verify method through exception instead of returning a value, for in that case it is not necessary to use ?assert() with this method.

So your suggestion is not what I was looking for. As you can see I was trying to push (in a good sense :)) the interface towards Mockito. Which is the most popular mocking framework in the Java world nowadays. Since you are the farther of this wonderful library it is up to you to decide in what direction the API of the library should evolve. But I am not passionate enough to implement something that I won't be using. Though in the original pull request I would put MFA arguments of the verify function in a tuple to emphasise logical grouping of these arguments.

@horkhe
Copy link
Contributor Author

horkhe commented Aug 2, 2012

I have been think about that for some time. And now it seems to me that a little bit more verbosity will pay off on account of better clarity. I will try to implement that more functional verify syntax.

@eproxus
Copy link
Owner

eproxus commented Aug 3, 2012

We could also design a more dynamic "nested tuple"-API which could be turned into the function call-interface like the one I proposed above. This is probably how it would work internally anyway, the functions just being shortcuts to the actual format.

@horkhe
Copy link
Contributor Author

horkhe commented Sep 13, 2012

@eproxus So let's move definitions of all matching functions at_least, never, times, etc. along with syntactic sugar functions from feature/args-filter: val, seq, etc. to another module. That later module we will recommend to import then both expectation and verification definitions will be shorter in the user's unit tests.

If you support this motion then how do we name that module? meck_ss that stands for Meck Syntactic Sugar?

@eproxus
Copy link
Owner

eproxus commented Sep 15, 2012

Something which would make more sense, like meck_assert:at_least or meck_assert:never? Thevalandseqcould be in another module called for examplemeck_gen. That would give usmeck_gen:valandmeck_gen:seq` etc.

@horkhe
Copy link
Contributor Author

horkhe commented Sep 15, 2012

@eproxus Consider this, if we have them in two different modules then users would need to have two -import directives instead of just one. Yes it is just one extra line to write but I would prefer to limit this number as possible. Do you think I am aggravating?

@eproxus
Copy link
Owner

eproxus commented Sep 16, 2012

My idea was to use a similar method to EUnit where you just include "meck.hrl" which will import any needed functions, maybe even selected functions from the meck itself.

*.sublime-workspace
*.sublime-project
=======
*~
>>>>>>> e5d147d... Introduce family of meck:verify functions
Copy link
Owner

Choose a reason for hiding this comment

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

This is a diff-leftover, please remove before final commit. :-)

@horkhe
Copy link
Contributor Author

horkhe commented Sep 16, 2012

That is even better. So let's keep everything in one place (meck.erl) just to make it simple for users. The fewer module names they need to remember the better.

@eproxus
Copy link
Owner

eproxus commented Sep 16, 2012

Well, I'd also like to split Meck into several smaller modules since the main module is becoming a bit too long. I liked the idea of separating the verification from the actual mocking into a separate module for this reason. Possibly all functions could exist as thin wrappers in the main module still.

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

Successfully merging this pull request may close these issues.

None yet

2 participants