feat(api) allow overriding what kind of request is constructed #134

mbark · 2017-12-08T15:37:17Z

First of all I just wanted to say that when going through the code it is very legible and well-written!

Background

We use jest to test our angular application and want to introduce pact-testing for the frontend-backend-communication. However, we run into the following problem:

In order to run angular we need to have our environment set to jsdom due to zone.js patching XMLHttpRequest etc;
In order to run pact we need to have our environment set to node (see mockServer does not start() #10 for further discussion and background).

I modified the code so that we always used node-style requests (ignoring environment checks) which got it working for us. With that in mind I thought it might be possible to add an ability to override the default behaviour, allowing us in scenarios such as these to run in the jsdom environment but tell pact to use node-style requests.

System solution

This feature introduces an optional map of values to give when setting up the pact server. For now these options only contain one possible key: environment. The environment option can be used to force pact to use either a node-style request or a web-style request.

As part of this I also added xhr-mock and the ability to mock the window, which allowed running the test for Request preferring the XMLHttpRequest object.

If you think this is a good solution to go ahead with I can improve the tests a bit and document this new functionality in the readme.

coveralls · 2017-12-08T15:42:10Z

Coverage increased (+1.1%) to 92.808% when pulling 3b05da0 on mbark:master into 9ca25de on pact-foundation:master.

coveralls · 2017-12-08T15:42:10Z

Coverage increased (+1.1%) to 92.808% when pulling 3b05da0 on mbark:master into 9ca25de on pact-foundation:master.

mefellows · 2018-01-06T05:11:28Z

Hi @mbark, i'm so sorry for the delay in responding to this. Not sure how it didn't get on my radar (perhaps when I was on parental leave I saw it but didn't respond - crazy time), but in any case let's get into it and thanks for the kind words!

OK so if I understand correctly, the root cause of your problem is this line: if (typeof XMLHttpRequest === "function" || typeof window !== "undefined") {.

Because you are patching with libraries, this statement will never reflect the "true" environment and the pact library may not behave as expected, so we want to maintain complete control if possible, or default to the current behaviour if the user doesn't specify a preference?

In theory, I support this change.

In terms of implementation, here are my additional thoughts:

The assumption is that this functionality is not covered by either the pact (designed for node environments) orpact-web module (designed for non-node/browser environments)
Where possible, maintain backwards compatibility in types - I'd prefer not to bump a major version for this feature, so any new parameters must be optional in any public signatures/constructors, or optional in existing types (e.g. PactOptions)
Rather than have a separate parameter to the Pact constructor, add environment as an optional property in the PactOptionstype (instead of AdditionalOptions).
The use of xhr-mock is 👍

mbark · 2018-01-08T13:04:07Z

I rebased it against master and will go through your comments later today (or at the very latest this week).

coveralls · 2018-01-09T12:01:32Z

Coverage increased (+1.5%) to 93.08% when pulling 1dbcff0 on mbark:master into 0ad8f57 on pact-foundation:master.

mbark · 2018-01-09T12:17:31Z

I added environment to PactOptions and made it optional. I also added it to the table describing the PactOptions parameters. Not entirely sure about the phrasing there to make it most clear how it works though.

Maybe you could take a look and see what you think?

It also seems like travis broke when installing node 8, which is not really related to what I have done at all. The other node versions work and so does 8, that was the version I tested against locally. (I'll try re-triggering the build to hopefully resolve it)

coveralls · 2018-01-09T18:56:54Z

Coverage increased (+1.5%) to 93.08% when pulling 16eeb75 on mbark:master into 0ad8f57 on pact-foundation:master.

This feature introduces an optional map of values to give when setting up the pact server. For now these options only contain one possible key, which is 'environment'. The environment option can be used to force pact to use either a node-style request or a web-style request.

coveralls · 2018-01-10T10:27:47Z

Coverage increased (+1.5%) to 93.08% when pulling 1778a80 on mbark:master into 0ad8f57 on pact-foundation:master.

mefellows · 2018-01-11T21:34:12Z

Thanks @mbark, I'll look into this over the weekend but on first glance it looks promising!

mefellows · 2018-01-13T06:51:57Z

Added @mboudreau for his thoughts on this change also.

mbark · 2018-01-26T10:20:13Z

@mefellows is there any progress on getting this merged? Or anything more I can do to help getting it merged? :)

mboudreau · 2018-01-28T22:30:27Z

Sorry, I needed to look this over. My bad.

…

On Fri., 26 Jan. 2018, 9:20 pm Martin Barksten, ***@***.***> wrote: @mefellows <https://github.com/mefellows> is there any progress on getting this merged? Or anything more I can do to help getting it merged? :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#134 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAjA5FIPNbxFPaxwMJtX6BzGZn-mhmkZks5tOabdgaJpZM4Q7Qj4> .

mboudreau · 2018-01-29T11:06:03Z

@mbark Thanks for your PR. I had a bit of a look and it looks mostly good except for a few opinionated things that I have, but before I want to get into that, I'm trying to understand your scenario better so that we can make the best call possible for the library and our users.

From what I understand (correct me where I'm wrong), but you want to run pact with your jest tests, but because of your environment setup, Request is confusing which actual environment it's in (web vs node) and causing some issues when the requests are actually built. You've found a workaround for now, but it isn't elegant.

I don't know jest that well, so I'm guessing it's a node based testing solution for React, but you want it to run for angular, hence the need to "fake" the environment with jsdom, causing the issue in the first place.

I want to try out a few solutions to see what works best in development. Do you have a very small sample project you can setup using your same scenario, that way I can test out to make sure it works for everyone.

Thanks. I promise that we'll find something that works for everyone :)

mbark · 2018-01-29T11:54:54Z

@mboudreau Regarding opinions about the code structure/names etc I'm all for you just making the changes you want to -- I'm interested in getting the feature merged, not the exact implementation of it. We can take that later though!

When it comes to your understanding of the problem I'd say that it is correct and I'll try to take some time later today to re-create our problem.

mbark · 2018-01-29T14:03:52Z

@mboudreau I have reproduction at: https://github.com/mbark/pact-jest

Run yarn jest --projects jest/test.config.js to run the tests (both pact and the dummy app.spec one). This should give you the error:


    console.error node_modules/jsdom/lib/jsdom/virtual-console.js:29
      Error: Error: connect ECONNREFUSED 127.0.0.1:1234
          at Object.dispatchError (/Users/martin.barksten/repos/pact-jest/node_modules/jsdom/lib/jsdom/living/xhr-utils.js:65:19)
          at EventEmitter.client.on.err (/Users/martin.barksten/repos/pact-jest/node_modules/jsdom/lib/jsdom/living/xmlhttprequest.js:664:20)
          at EventEmitter.emit (events.js:164:20)
          at Request.preflightClient.on.err (/Users/martin.barksten/repos/pact-jest/node_modules/jsdom/lib/jsdom/living/xhr-utils.js:409:47)
          at Request.emit (events.js:159:13)
          at Request.onRequestError (/Users/martin.barksten/repos/pact-jest/node_modules/request/request.js:878:8)
          at ClientRequest.emit (events.js:159:13)
          at Socket.socketErrorListener (_http_client.js:389:9)
          at Socket.emit (events.js:159:13)
          at emitErrorNT (internal/streams/destroy.js:64:8) undefined

If you then edit node_modules/@pact-foundation/pact/common/request.js and change:

typeof XMLHttpRequest === 'function' || typeof window !== 'undefined' to false;
typeof window === 'undefined' to true (both in the constructor and the send method).

Both tests should pass. You can also try setting @jest-environment node in the pact.spec.ts file to see the error when running in node rather than jsdom mode.

Hopefully that works as a simple reproduction of the problem :)

mboudreau · 2018-01-29T22:23:01Z

Amazing work, thank you. This is very helpful. I'll try to have everything sorted out by tomorrow. Cheers.

…

On Tue., 30 Jan. 2018, 1:03 am Martin Barksten, ***@***.***> wrote: @mboudreau <https://github.com/mboudreau> I have reproduction at: https://github.com/mbark/pact-jest Run yarn jest --projects jest/test.config.js to run the tests (both pact and the dummy app.spec one). This should give you the error: console.error node_modules/jsdom/lib/jsdom/virtual-console.js:29 Error: Error: connect ECONNREFUSED 127.0.0.1:1234 at Object.dispatchError (/Users/martin.barksten/repos/pact-jest/node_modules/jsdom/lib/jsdom/living/xhr-utils.js:65:19) at EventEmitter.client.on.err (/Users/martin.barksten/repos/pact-jest/node_modules/jsdom/lib/jsdom/living/xmlhttprequest.js:664:20) at EventEmitter.emit (events.js:164:20) at Request.preflightClient.on.err (/Users/martin.barksten/repos/pact-jest/node_modules/jsdom/lib/jsdom/living/xhr-utils.js:409:47) at Request.emit (events.js:159:13) at Request.onRequestError (/Users/martin.barksten/repos/pact-jest/node_modules/request/request.js:878:8) at ClientRequest.emit (events.js:159:13) at Socket.socketErrorListener (_http_client.js:389:9) at Socket.emit (events.js:159:13) at emitErrorNT (internal/streams/destroy.js:64:8) undefined If you then edit ***@***.***/pact/common/request.js and change: - typeof XMLHttpRequest === 'function' || typeof window !== 'undefined' to false; - typeof window === 'undefined' to true (both in the constructor and the send method). Both tests should pass. You can also try setting @jest-environment node in the pact.spec.ts file to see the error when running in node rather than jsdom mode. Hopefully that works as a simple reproduction of the problem :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#134 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAjA5L-0zuGPKhhmlSM0Sbuam93EHzS9ks5tPc_JgaJpZM4Q7Qj4> .

mboudreau · 2018-01-31T06:10:14Z

@mbark Hey man, I'm looking into using an external library that would handle both node/browser requests called Popsicle. I just tested it using your example project (gj on that btw) which seems to fix the issue. I'm going to add it to the codebase and create a new PR for it and close this one once it's gone through. Cheers and thanks for the help :)

mboudreau · 2018-01-31T06:56:39Z

@mbark I've create a new PR with my proposed changes, let me know what you think #147

mefellows · 2018-02-20T10:25:48Z

Thanks for this @mbark, really appreciate you taking the time with this PR. Will be closing this in favour of #147, however.

chore(request): make Request class work across both Node and Browser using Popsicle Includes a number of changes and tidying ups: * Add 'exit' flag to mocha to exit properly after tests are done * Update superagent dependency * Update dependencies for karma-pact to fix issue with peer dependencies * Update standard version, make sure the ignore glob is correct * Fix Mocha registering ts-node twice * Update nock to 9.1.x and fix tests * Update lock file for e2e tests * Adding union type for http methods to gradually change to the enum See #134 and #10 for background.

…using Popsicle Includes a number of changes and tidying ups: * Add 'exit' flag to mocha to exit properly after tests are done * Update superagent dependency * Update dependencies for karma-pact to fix issue with peer dependencies * Update standard version, make sure the ignore glob is correct * Fix Mocha registering ts-node twice * Update nock to 9.1.x and fix tests * Update lock file for e2e tests * Adding union type for http methods to gradually change to the enum See #134 and #10 for background.

…using Popsicle Includes a number of changes and tidying ups: * Add 'exit' flag to mocha to exit properly after tests are done * Update superagent dependency * Update dependencies for karma-pact to fix issue with peer dependencies * Update standard version, make sure the ignore glob is correct * Fix Mocha registering ts-node twice * Update nock to 9.1.x and fix tests * Update lock file for e2e tests * Adding union type for http methods to gradually change to the enum See pact-foundation#134 and pact-foundation#10 for background.

mefellows added the Triage label Jan 6, 2018

mefellows added the Awaiting feedback label Jan 6, 2018

mefellows requested review from mefellows and mboudreau January 12, 2018 23:28

mefellows closed this Feb 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(api) allow overriding what kind of request is constructed #134

feat(api) allow overriding what kind of request is constructed #134

mbark commented Dec 8, 2017

coveralls commented Dec 8, 2017

coveralls commented Dec 8, 2017 •

edited

Loading

mefellows commented Jan 6, 2018

mbark commented Jan 8, 2018 •

edited

Loading

coveralls commented Jan 9, 2018 •

edited

Loading

mbark commented Jan 9, 2018 •

edited

Loading

coveralls commented Jan 9, 2018 •

edited

Loading

coveralls commented Jan 10, 2018 •

edited

Loading

mefellows commented Jan 11, 2018

mefellows commented Jan 13, 2018

mbark commented Jan 26, 2018

mboudreau commented Jan 28, 2018 via email

mboudreau commented Jan 29, 2018

mbark commented Jan 29, 2018

mbark commented Jan 29, 2018

mboudreau commented Jan 29, 2018 via email

mboudreau commented Jan 31, 2018

mboudreau commented Jan 31, 2018

mefellows commented Feb 20, 2018 •

edited

Loading

feat(api) allow overriding what kind of request is constructed #134

feat(api) allow overriding what kind of request is constructed #134

Conversation

mbark commented Dec 8, 2017

Background

System solution

coveralls commented Dec 8, 2017

coveralls commented Dec 8, 2017 • edited Loading

mefellows commented Jan 6, 2018

mbark commented Jan 8, 2018 • edited Loading

coveralls commented Jan 9, 2018 • edited Loading

mbark commented Jan 9, 2018 • edited Loading

coveralls commented Jan 9, 2018 • edited Loading

coveralls commented Jan 10, 2018 • edited Loading

mefellows commented Jan 11, 2018

mefellows commented Jan 13, 2018

mbark commented Jan 26, 2018

mboudreau commented Jan 28, 2018 via email

mboudreau commented Jan 29, 2018

mbark commented Jan 29, 2018

mbark commented Jan 29, 2018

mboudreau commented Jan 29, 2018 via email

mboudreau commented Jan 31, 2018

mboudreau commented Jan 31, 2018

mefellows commented Feb 20, 2018 • edited Loading

coveralls commented Dec 8, 2017 •

edited

Loading

mbark commented Jan 8, 2018 •

edited

Loading

coveralls commented Jan 9, 2018 •

edited

Loading

mbark commented Jan 9, 2018 •

edited

Loading

coveralls commented Jan 9, 2018 •

edited

Loading

coveralls commented Jan 10, 2018 •

edited

Loading

mefellows commented Feb 20, 2018 •

edited

Loading