Modifying pact-consumer-js-dsl to work with both Nodejs and Web projects

[mboudreau]

Changed a few things around the build process which I think is needed to maintain the codebase, adding nodejs example, updating readme to make it easier for people to start using pact-mock-service. Updated bower file to ignore certain files that aren't needed. Registered pact-consumer-js-dsl with Bower, can view information with bower info pact-consumer-js-dsl.

[mboudreau]

Forgot to mention, version number has been incremented to 0.0.5 and would be nice to create a new release after the pull request is done.

[bethesque]

If you have multiple versions of the gem installed, and you omit the Gemfile and bundle exec, then you may not get the gem version you expect. It is not good Ruby practise to use a gem without a Gemfile. Can you put that back in please?

[mboudreau]

@bethesque I added the Gemfile back in, but I'm leaving the README how it is. It might be good Ruby practice, but this isn't for people that write Ruby. As of yet, I haven't had the need of more than one version of pact-mock-service.

[bethesque]

That's like me claiming that I don't need to follow good node practice because I'm not a node developer :P
The problems caused by not using Gemfiles and bundle exec happen to people who write Ruby, and people who don't write Ruby. When they happen to people who don't write Ruby, then it's even harder for them to diagnose the cause. If there is a backwards incompatible change to the mock server, and you upgrade one project but not another, then one of your projects is going to break.

[mboudreau]

@bethesque @BenSayers @markdalgleish

I've updated the build to include a UMD wrapper, effectively fixing all our issues with the js consumer, for all our use cases :)

[bethesque]

Thanks Michael. I'm going to merge #21 first when I have a moment, because it's a bigger change, and I don't want a potential conflict to block it.

[mboudreau]

Sure, but be aware that there are some conflicts between our two pulls. However, async http requests are good :)

[bethesque]

Sorry, I know I've made your life harder now by merging in the node stuff, but it's for the best! Can you have a go at merging that into your branch, and then ping me when it's good to go? Thank you kindly!

Btw, I'm a bit confused about the browserify stuff. What's the difference between this approach and using browserify (apart from not having the extra dependency)?

[mboudreau]

@bethesque Browserify is essentially trying to do the same thing I'm doing with UMD. The end goal is about having one source that works for all use cases (web, node, etc). I like UMD in this case since this library is fairly small and doesn't have any extra libraries except for xmlhttprequest in nodejs, which I've fixed in this fork.

It's a matter of preference and in my opinion, Browserify is a tad overkill for what we're doing here since we don't really have any other requirements needed. Unless we have a lot of them in a future, I can't recommend this at this very moment.

I'm merging in your changes right now and try to sort it all out. Shouldn't be too hard.

[mboudreau]

@bethesque Well, I'm not entirely sure if it went missing, but it's not part of the release on the github page. It used to be there if I recall, but somebody seems to have removed it?

EDIT - nvm, the issue was with out firewall.

[mboudreau]

@bethesque I've merged master into my PR. There's still some conflicts of course, but you should be able to simply merge my changes and hard reset (removing all merge conflicts) to my latest commit. I've tested everything and it works fine.

However, I've noticed that the 'clean' function is now missing from the mockService, and that when creating it, I must give a done function in the options. I'm not too sure if this is your doing, but I can't say that I'm a fan of it. Having the extra functions there for workflow is fine, but changing my control over how I want to use pact for my use case is not.

If this is not one of your changes (something added from the PR), then I can modify them.

[bethesque]

The clean function is going to become irrelevant soon, because there is a new PUT /interactions endpoint that will take care of DELETE /interactions and POST /interactions. There's no point to setting up interactions sequentially in the Javascript use case - it was just a legacy of the way the Ruby code uses the service, and it makes the code HORRIBLE! Callback hell. I will modify the code to use the new endpoint as soon as I have a moment.

The "done" function is required now that we have gone fully async. There needs to be a way for the mock service verify function to fail a test after the pact run has completed, if the expected interactions do not match the actual interactions. I need to document this properly in the README.

[bethesque]

As you mentioned, there are still conflicts. This means that master has not been correctly merged into your branch. Can you resolve the conflicts, and then ping me when you've finished? Hard resetting is not a PR merging option!

[bethesque]

Thanks for the browserify explanation. I also prefer a smaller dependency.

[mboudreau]

Alright, fixing merge conflicts right now. I already did it earlier, but I think it didn't "register" that the merges were fixed for some reason.

[mboudreau]

@bethesque Fixed :)

[mboudreau]

@bethesque I'm not sure I agree completely with the direction you're going. So, you're saying that there can only be one 'setup' of the interactions. Fine for us since we're not using it like that, but I'm not positive that it would be the use case for everyone. It could be that some interactions needs to be added along the way in the tests (you know, nested tests potentially).

I'd also like to have the option to write out the schema without verification. This of course shouldn't be used for production code, but while we're setting up the project I've created a lot of the interactions that we will be using on the front-end, but aren't using right away (still have to create the tests). However, I still want the backend to test on something as a first version until we lock everything down. This is a bit of an edge case, but I can see where it can get useful for many people starting.

I've also noticed that with version 0.2.3 of pact-mock-service, when I call write, it doesn't write all interactions, only the ones that's been hit, making my use case to give the backend guys something to start on completely useless. ie. The backend can't be even built before the front-end interactions are complete, which is a big problem since we expect that the backend will in fact be ahead of the front-end since we can't deploy the front-end before the api is out.

This is not a showstopper exactly, but it is a bit annoying and would like for the pact service to consider our use case.

[mboudreau]

@bethesque Don't take my current example as word for word. The synchronous part is only there because that's how the past pact-consumer-js-dsl was working. It definitely can be asynchronous and probably should. Frankly, this is not my issue with it, it's the use case where we have to use the library exactly the same way; have to be unit tests, have to setup and teardown within the same javascript file.

The way I envision pact working for us, is to be both mock server while developing, and the same interactions can be used as the contract. This makes a lot of sense for us since we wouldn't have to duplicate our efforts for both pact and mocking. We're also using pact with protractor tests (interaction tests) which are very thorough on how we want to use our application in the real world.

In our case, some of the design concepts for the consumer dsl simply won't work.

Maybe we should get together and nail this out more. I feel that we're close to getting something that'll work for everyone and I will admit that I'm not a Ruby expert (or even a user), so I'm not sure what is the standard in that world, but I'm sure we can create something that can work for everyone.

[BenSayers]

@mboudreau what you are asking for sounds very similar to something we are pondering on as well.

In essence Pact right now is very oriented toward unit testing consumer abstractions over provider apis. And these tests are geared toward single interactions. However obviously that on its own is not enough to give you confidence. So in addition functional/integration tests are needed, which can test the consumer application as a whole with all the provider api's stubbed out. I say stubbed instead of mocked intentionally here as in these tests it is important that the stub behaves the same way as the provider api when it is called, but we do not care how many times it was called or if it was even called at all.

So far it seems the recommendation is to not use Pact to write api/integration tests, but instead create a separate stub that shares data used in the Pact tests. The concern with this is it relies on developers not making a mistake when setting up the stub. The stub may have endpoints added to it that do not have any corresponding Pact tests for example. These sort of mistakes will go unnoticed as the stub is not validated against the real provider like the Pact tests are.

I've been thinking that Pact is in a good position to solve this problem and have wondered what a solution that generated the stub server off the Pact file would look like. This seems to me to be half of what you are asking for @mboudreau. I think the other half of what you are asking for is in a world where Pact is solving the stub server problem for you, how do you write the integration/api tests before you write the Pact unit tests (if you are following a BDD style of development for example).

@bethesque what are your thoughts on this subject?
This whole conversation is a bit off topic from the PR - does it belong somewhere else?

[mboudreau]

@BenSayers Thanks for that, that clears up a lot of things. That is essentially what I'm trying to accomplish with our project. I agree that the overall discussion should be taken elsewhere, but it is relevant to this PR somewhat since I'm trying to change the consumer dsl to work for both our use cases.

This is where @bethesque comes in to say if she likes my idea so I can go ahead and change my master on my fork. I wouldn't want to spend time fixing the consumer dsl if it's not going to get merged into the main project.

I would definitely love to be continually part of the direction of pact. I think it shows great potential for solving a lot of issues in the software lifecycle.

[bethesque]

I don't see a problem with making the methods to set up and tear down interactions public so that they can be used independently of run method. That should allow @mboudreau to continue his integration testing approach.

The idea of setting up pact to work as a stub server from an existing pact file could work, but we'd need to work out an approach to a certain issue. One of pact's strengths is that you can test the same request, with different responses (eg. 200, 404, when x has a y, when x has no y etc...). This works because you set up the interactions on the mock server on a test-by-test basis - you only have one response mocked for a given request at a time, so it always knows which response to give.

If we used the generated pact file to set up a stub server, it is inevitable that there would be more than one response found for a given request - we'd need to work out how to handle this situation. We can't just take the first response found, because the order of the unit tests determines the order in which the interactions are written to the file, so it would not be predictable.

The other subtlety is that in a unit test, we're at a fine grained level, and we generally want to check exact values of an outgoing request - that's what tells us that "orderId: 345" on our model is being converted to "{order_id: 345}" in our request body. If we use pact for stubbing for integration tests, we want to be much less strict about that - we don't care what the value of the order id is, we just want the mock server to respond to any PUT order request that has the right document structure. We already have this type of matching implemented, we'd just need a way to turn it on when in "stub" mode.

Sorry this is a bit long winded. What we need is:

1. A way to choose which of the interactions that we used in a unit test end up being used in a stub test.
2. A way to relax the matching so the integration tests don't become brittle.

I will give these some thought.

[bethesque]

Ok, I've started a new issue on the mock server repo, let's take this discussion there, because it's more about the mock server than it is about the DSL. The DSL is just javascript syntax sugar over the mock server behaviour.

[mboudreau]

@bethesque Any way of getting my PR in?

Also, should we put the 'term' matching in the dsl?

[bethesque]

I would think that installing gulp, bower and karma as global or local is the user's choice, is it not? There's nothing in the pact dsl that requires them to be global is there?

[mboudreau]

It is, just makes it easier to run things. I guess I can change that.

[bethesque]

Hi Michael, I am not happy to merge these changes as it now pulls too far towards the integration (stub) use case, and makes it too easy for errors to be lost. I would like verification to be on by default (done must be a mandatory option) and if you want to turn the error checking off, you can pass in an empty function. I am going to have a try to do this myself, but if it gets too complex, I will pass it back to you.

[bethesque]

Also, there are WAY too many changes in this PR! I'll add some instructions to the CONTRIBUTING.md file, but there are some basic PR guidelines for open source contributions which are:

1. Keep the change set focussed and as small as possible to achieve one goal.
2. Put each change set on a branch, and create a pull request from that branch (otherwise, every commit you make to your own master gets added to the PR)

[bethesque]

Also, rule number 3: Always always submit tests for the code that you have changed or added.

[mboudreau]

Other than wrapping the library in a UMD (which is tested by actually running gulp), merging the changes from master (from your request as per Ben's PR) and making the write/verify public (which is already tested). I guess I can add some tests around write and verify to be public, but their functionality should already be tested by the tests that were already there.

I tried to achieve one goal through this PR, but you asked me to wrap several things, so I did.

[bethesque]

I had a go, but ran out of time today, can you pick this up? This is what I'd like:

1. Revert all the changes that made the done function optional - an empty function can be provided if someone does not want to check for errors, but I don't want to make it easy for errors to be swallowed.
2. Revert the changes to run() and let's make cleanAndSetup() public and call it instead, because that is all the run function is doing if you call it with no args. The name is a bit messy, how about just setup?

[bethesque]

I appreciate all the work on the README and the examples, that looks good.

[bethesque]

I don't get a notification when there are new commits to the PR, so I haven't been ignoring this, just didn't know the previous changes were ready for review!

[mboudreau]

@bethesque I've changed what you asked for.

[bethesque]

Many thanks! I will be looking at this as soon as I have a spare moment.

[mboudreau]

@bethesque What's the status on this? I saw that you did some merging from my branch yesterday.

[bethesque]

Hi Michael,

I've got it on a branch while I add some unit tests, but I'm having some very strange problems. I've added unit tests, and now have a task called run-unit-tests. When I run run-unit-tests and run-browser-tests separately, they pass, but when I run them together as part of run-tests, then I get:

1) MockService run when there are no errors invokes the done callback with null
1.1) ReferenceError: Pact is not defined
1.2) TypeError: Cannot call method 'run' of undefined

You're more experienced with gulp than I am, if you have any idea what might be causing that, I'd appreciate a pointer! It's on this branch: https://github.com/DiUS/pact-consumer-js-dsl/tree/mboudreau-master

[mboudreau]

@bethesque Created a new pull request with what you're asking for. It should fix every problem. The major one being that the tests used to run in parallel and was competing for the same pact mock service. There was also a few issues with the integration code that was doing web or node specific stuff (which it shouldn't). Should just be a simple pull, npm install/update and running gulp :)

#29

[BenSayers]

It's not clear to me what this pull request is trying to achieve. I am under the impression the project already does support nodejs and web projects.

I'm also concerned about the way in which node-xhr2 has been introduced by this PR

 global.XMLHttpRequest = require('xhr2');

I understand it is desirable to stop the code from caring about if it is running inside a nodejs context or a browser based one and agree it is something we should refactor in the code. However this particular attempt at doing so creates a global that will be leaked into every javascript file in the node executable and is bad practice. I think using browserify we can solve this problem more cleanly.

[mboudreau]

@BenSayers This is a matter of preference in the end, and Beth and I agree that having one codebase and not 2 (one for node, one for web) is a lot easier to maintain.

Frankly, I like Browserify, but I feel like you're trying to use a sledgehammer for a job needed for a regular hammer. The Pact consumer DSL is really not that complex. Browserify's main point of use is to reduce complexity with require.js and AMD modules. We're not doing any of that. Seems to me that it's just another added layer of abstraction/complexity for a project of this size.

If Pact gets bigger (and the consumer dsl), then we should maybe look into Browserify to reduce complexity, but I don't think that time is now.

Also, as per your suggestion, I actually did remove the XMLHttpRequest from the global and added it to the Pact.Http section within scope so it doesn't pollute other closures.

[bethesque]

Closing, this is being merged manually.