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

https support added #7

Merged
merged 1 commit into from
Jul 27, 2016
Merged

https support added #7

merged 1 commit into from
Jul 27, 2016

Conversation

sk1talets
Copy link
Contributor

No description provided.

@sk1talets sk1talets closed this Jul 27, 2016
@coveralls
Copy link

coveralls commented Jul 27, 2016

Coverage Status

Coverage increased (+0.09%) to 95.111% when pulling 2026943 on sk1talets:https into b2020c3 on pact-foundation:master.

@sk1talets sk1talets reopened this Jul 27, 2016
@coveralls
Copy link

coveralls commented Jul 27, 2016

Coverage Status

Coverage increased (+0.09%) to 95.111% when pulling 2026943 on sk1talets:https into b2020c3 on pact-foundation:master.

@@ -54,6 +55,7 @@ export default class Request {

request.end()
} else {
const req = this._request
Copy link
Member

Choose a reason for hiding this comment

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

I'm not super familiar with the browser request component of this, does it not also support https?

Copy link
Contributor Author

@sk1talets sk1talets Jul 27, 2016

Choose a reason for hiding this comment

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

@mefellows, It does support https as far as I know. https://www.w3.org/TR/XMLHttpRequest/#introduction

@mefellows
Copy link
Member

Thanks @sk1talets, it's looking good!

I'd suggest before merging that we also add an integration-style test that spins up a Mock Server with SSL enabled and smoke test a basic scenario.

This will also likely require another configuration item for the Pact server to allow self-signed certificates (this would be the vast majority of cases i'd expect).

@sk1talets
Copy link
Contributor Author

sk1talets commented Jul 27, 2016

@mefellows, I did this already, if I got you right.. Take a look at test/dsl/integration.spec.js, I just run the whole suite for https:

-describe('Pact random mock port', () => {
+['http', 'https'].forEach((PROTOCOL) => {
+
+describe(`Pact random mock port, protocol: ${PROTOCOL}`, (protocol) => {

   const MOCK_PORT = Math.floor(Math.random() * 999) + 9000
-  const PROVIDER_URL = `http://localhost:${MOCK_PORT}`
+  const PROVIDER_URL = `${PROTOCOL}://localhost:${MOCK_PORT}`
   const mockServer = wrapper.createServer({
     port: MOCK_PORT,
     log: path.resolve(process.cwd(), 'logs', 'mockserver-integration.log'),
     dir: path.resolve(process.cwd(), 'pacts'),
+    ssl: PROTOCOL === 'https' ? true : false,
     spec: 2
   })

@@ -37,7 +40,11 @@ describe('Pact random mock port', () => {

   beforeEach((done) => {
     mockServer.start().then(() => {
-      provider = Pact({ consumer: `Consumer ${counter}`, provider: `Provider ${counter}`, port: MOCK_PORT })
+      provider = Pact({ consumer: `Consumer ${counter}`,
+                        provider: `Provider ${counter}`,
+                        port: MOCK_PORT,
+                        ssl: PROTOCOL === 'https' ? true : false
+                      })

I hope it's fine.

@mefellows mefellows merged commit d4c28ef into pact-foundation:master Jul 27, 2016
@mefellows
Copy link
Member

@sk1talets Quite right LGTM - merging! We can look at adding https support for browsers in another PR. Much appreciated!

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