-
-
Notifications
You must be signed in to change notification settings - Fork 231
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
Introduce support for matchers: #89
Conversation
@eproxus What do you think about that? |
Oops, I made a faulty force push. So please hold on with accepting this pull request. Tomorrow I will fix that. Fortunately I have another clone of this repo at work. |
Ok, I am done. Now this pull request brings fully fledged (To reflect that I renamed the pull request) support for matchers. The syntax is uniform on both expectation and verification side. |
Look at the size of that pull request... Will review as soon as I have time. 😃 |
It brings quite a feature. Besides it is not as big as the previous one :-) |
@eproxus If you could comment on the suggested API now that would be helpful. |
@eproxus off topic: for erlang projects you might wanna give a try to IntelliJ IDEA + Erlang plugin. It appeared just at the end of this August but the author is of the plugin is very active. |
|
|
@eproxus by the way recently some changes to |
And let's not split this pull request in pieces please for the latter changes depend on |
Why do the later changes depend on |
Ok I will move them to another pull request. But thispull request puts ret_specs to a dedicated module so they would need to be accepted in order. First changes with passthrough a func and then this one |
Dependencies between pull requests are fine. I just don't want to pull in a lot of changes in one huge pull request. It's easier to review and discuss them if they are separated. |
@eproxus I created another pull request for |
@horkhe Sure, sound good! |
Yes and no. Meck introduces its own abstraction for matchers that is
Well as I already explained in one of my comments I create them not because I like a lot of modules :) but rather because I like to keep different abstractions separately. For example So this is it. Please let me know what do you think. And by the way, happy new year!!! :-) |
@eproxus Hi Adam, I left a reply to your comment, please review when you have time. |
@@ -1112,7 +1133,8 @@ remote_setup() -> | |||
{Node, meck_test_module}. | |||
|
|||
remote_teardown({Node, _Mod}) -> | |||
ok = slave:stop(Node). | |||
ok = slave:stop(Node), | |||
ok = net_kernel:stop(). |
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.
Just out of curiosity, why is this needed?
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.
I noticed that running rebar eunit
results in the following output:
All 143 tests passed.
Cover analysis: /home/travis/builds/eproxus/meck/.eunit/index.html
Code Coverage:
meck : 96%
...
Total : 88%
ERROR: eunit failed while processing /home/travis/builds/eproxus/meck: {'EXIT',net_kernel_stop_failed}
This explicit net_kernel:stop()
fixed that.
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.
Might it be this issue fixed in rebar already? https://github.com/basho/rebar/issues/286
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.
I tried the latest Rebar with this fix and saw that error anyway.
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.
Then please report that as a bug to the rebar project instead.
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.
@eproxus Do you want me to rollback this change?
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.
... besides, it does seem like a problem on our side, for we do start net_kernel in this test but had not stopped it before I made this change.
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.
Ah, then it should definately be there. 😄
I also noticed that we should probably do {ok, _} = net_kernel:start([Myself, shortnames]),
instead of just running it...
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.
Let's do that later or we will never get this pull request merged ;-). Are there any blocking issues left?
Just a note, I'd prefer more, smaller commits with isolated, atomic changes. As it is now, it's a bit hard to follow the development and get an overview of how interdependent changes are. It's also okay with smaller pull request containing clean ups and other minor changes separately. They'll more likely be accepted faster in that case. |
Yep. I did such a humangous pull request in an effort to push more changes to the upstream faster. It is obvious now that that was a biiiiiig mistake, for the effect was reverse. In the future I will do smaller commits/pull requests as you suggested. |
The code looks good. I also want to try out the API a bit manually before the final merge. Will let you know when it's done. |
Glad to hear that :-) |
Seems that the dialyzer plt creation somehow sneaked into the build scripts, making them take longer than 30 secs on Travis CI. Can't see any dependencies on the dialyzer targets in the makefile though, any ideas? |
Well, I made that on purpose to make sure that the code is checked by static analyzer on pull requests. But it seems that on R14 it takes too long to build PLT. As you can see R15 is much faster with this respect. So I should probably remove the dialyzer check from the build. |
@eproxus Have you had a chance to give the API a try? |
Let's remove it for now then. Yes, I'll merge as soon as the compilation problems are fixed and all travis builds passes. 👍 |
@eproxus Hey Adam, it seems I am finally done with that :) |
@eproxus could you please merge this. I have other changes coming. |
Introduce support for matchers:
Thanks a lot! |
Np. Forgot to check the build on Travis, but seems the GitHub hooks works now as well. |
This pull introduces support for matchers in both expectation creation and history digging functions.
Here is an example how an expectation can be created using matchers:
Here is an example how can be checked if a function has been called during test using matchers:
As you can see
meck:is/1
can create a matcher from any single argument function or a hamcrest matcher. Matchers can be used instead of any expected argument.