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

renamed 'Verifier' -> 'Mock' and updated docs #1

Merged
merged 1 commit into from
Jun 10, 2016
Merged

Conversation

mefellows
Copy link
Member

This is just an FYI for discussion.

@coveralls
Copy link

coveralls commented Jun 10, 2016

Coverage Status

Coverage remained the same at 75.862% when pulling 6168625b7ef185e4c90a1206aec4dd55e7b2afc1 on feature/docs into 9752eb9 on master.


Check out [Pact JS Mocha](https://github.com/pact-foundation/pact-js-mocha).

#### Using Jasmine?
Copy link
Contributor

Choose a reason for hiding this comment

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

Jasmine is WIP and far more complicated to setup than Mocha. I would remove this line.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair enough, I noticed it had no docs so makes sense. When in doubt, leave it out!

@tarciosaraiva
Copy link
Contributor

@mefellows not sure if Mock is the correct name here either... The Verifier is actually doing too much by exposing a method to add interactions and then verifying them.

Perhaps we should reduce the scope of this file?

@mefellows
Copy link
Member Author

@tarciosaraiva I think so. It probably just confuses things more.

Looking at it holistically, I actually think it's preferable to bring that functionality into the pact.js (Pact) module itself. It just reads better. In fact, in the readme, you call the Verifier you instantiate pact anyway!

e.g. this sort of thing:

pact = Pact({ consumer: 'My Consumer', provider: 'My Provider' })
...
      pact.interaction()
        .given('i have a list of projects')
        .uponReceiving('a request for projects')
        .withRequest('get', '/projects', null, { 'Accept': 'application/json' })
        .willRespondWith(200, { 'Content-Type': 'application/json' }, EXPECTED_BODY)

      pact.verify(requestProjects)
      ...

What do you think? The code can still be kept modularised if that helps organise things.

@tarciosaraiva
Copy link
Contributor

Yup, I do like that, better experience. Keep in mind that the Verifier keeps an array of interactions.

Was thinking of passing the control of this store back to the developer and he just provides it to the Verifier function but in two minds with it.

Thoughts?

@mefellows
Copy link
Member Author

Nice. Can you provide an example of what you mean? My brain is at that point of the week where making that leap is tough ;)

@tarciosaraiva
Copy link
Contributor

tarciosaraiva commented Jun 10, 2016

What I mean is that instead of the Verifier keeping an array of interactions, all we do is expose a new Interaction object to the user and he is responsible for keeping a list of Interactions. When he's ready, simply pass them along with the function to verify. The signature of the Verifier#verify will look like this

verify: (integrationFn, interactions)

Also, we need an index file and pact.js works like that right now. Make sure that we keep the other stuff exposed - Matchers and Interceptor.

@mefellows
Copy link
Member Author

Ahhh. Gotcha.

I think I prefer the function interface verify: (integrationFn), but perhaps there is an advantage of the latter (maybe other UIs can wrap it better?).

@tarciosaraiva
Copy link
Contributor

Ya, it's really a design decision, don't think there's an advantage over
one or another. Personally, the less I have to deal with as a user, the
better thus the original signature makes more sense.

On Sat, 11 Jun 2016, 07:17 Matt Fellows notifications@github.com wrote:

Ahhh. Gotcha.

I think I prefer the function interface verify: (integrationFn), but
perhaps there is an advantage of the latter (maybe other UIs can wrap it
better?).


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AA2h9GKi371qKAYfgD8Zp4pTfE99AtW5ks5qKdRagaJpZM4IynDB
.

@mefellows
Copy link
Member Author

👍 How about I re-submit the PR (i.e. just docs) without the code changes and we separate them out?

@tarciosaraiva
Copy link
Contributor

Yup, let's do that.

@mefellows mefellows force-pushed the feature/docs branch 2 times, most recently from b3cfc91 to 21a8af3 Compare June 10, 2016 21:41
@coveralls
Copy link

coveralls commented Jun 10, 2016

Coverage Status

Coverage remained the same at 83.333% when pulling e3c8647 on feature/docs into 4cdd806 on master.

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

3 participants