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

Ensure that supplied request-object is passed through #894

Merged
merged 2 commits into from
Oct 26, 2016

Conversation

ath88
Copy link
Contributor

@ath88 ath88 commented Oct 25, 2016

The request-parameter from createClient is lost for later requests in the WSDL-prototype. This PR fixes that problem.

@coveralls
Copy link

coveralls commented Oct 25, 2016

Coverage Status

Coverage increased (+0.007%) to 93.137% when pulling 1576da8 on cube-io:master into 89c8864 on vpulim:master.

@jsdevel
Copy link
Collaborator

jsdevel commented Oct 25, 2016

@ath88 is there a way this can be tested?

@ath88
Copy link
Contributor Author

ath88 commented Oct 26, 2016

It can be tested, yes.

I did this by supplying a request-object with a custom header added in request.defaults. Using the request-debug-module, I can inspect the requests being made. On the first request to the actualy WSDL-file, the custom header is included, while the second request (to eg. a schemaLocation) lacks the custom header.

I stumbled across this when trying to supply a request-object with a specific certificate and key. The first request would succeed, while the second failed, since the key and certificate was not used in the second request.

With my changes, the request-object (along with new defaults) is retained.

I could probably write a test for this, but it would be difficult to make it fit the current mocha-scheme.

@jsdevel
Copy link
Collaborator

jsdevel commented Oct 26, 2016

can you add a test @ath88 ?

@coveralls
Copy link

coveralls commented Oct 26, 2016

Coverage Status

Coverage increased (+0.007%) to 93.137% when pulling b4b0f68 on cube-io:master into 89c8864 on vpulim:master.

@ath88
Copy link
Contributor Author

ath88 commented Oct 26, 2016

I added an extra assert to an existing test. The new assert would fail without to the first commit I made.

@ath88
Copy link
Contributor Author

ath88 commented Oct 26, 2016

Incidentally, it wasn't as hard as I had thought. :)

@jsdevel jsdevel merged commit 0b1df30 into vpulim:master Oct 26, 2016
@jsdevel
Copy link
Collaborator

jsdevel commented Oct 26, 2016

Thanks!

@ath88
Copy link
Contributor Author

ath88 commented Nov 7, 2016

@jsdevel What is your process regarding publishing new versions? :) I ask this because I'd like to know if we should publish this fix internally, or wait for it to be published.

@jsdevel
Copy link
Collaborator

jsdevel commented Nov 7, 2016

@ath88 normally @herom or myself will publish a new version. Lately I've been swamped so haven't had a chance to publish. I'll try to tonight.

@ath88
Copy link
Contributor Author

ath88 commented Nov 8, 2016

Thanks for responding so quickly! hat tip

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.

3 participants