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

Fixed checking for empty obj.Body before further actions #986

Merged
merged 1 commit into from
Feb 8, 2018

Conversation

raviolin
Copy link
Contributor

@raviolin raviolin commented Nov 9, 2017

An error is thrown even if response was succesful.

This is the same as #981 only this doesn't break tests. I decided to make a new pull request because it have been almost a month since #981 was created, and it's author doesn't seem willing to fix tests.

@coveralls
Copy link

coveralls commented Nov 9, 2017

Coverage Status

Coverage remained the same at 93.648% when pulling ccac52d on raviolin:fix-checking-response-body into a142aee on vpulim:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 93.648% when pulling ccac52d on raviolin:fix-checking-response-body into a142aee on vpulim:master.

@raviolin
Copy link
Contributor Author

raviolin commented Nov 9, 2017

I swear it passed all tests on my local machine, but one Travis job failed to npm install for some reason...
https://travis-ci.org/vpulim/node-soap/jobs/299410819

@herom
Copy link
Contributor

herom commented Dec 5, 2017

@raviolin sorry for letting you down for this long... I had some time off and lost sight of this issue. Could you please try and rebase against the latest master?

I updated our .travis.yaml file in order to test against the latest LTS instead of the latest available version (which seems to fail the tests) - now it should run as expected.

@coveralls
Copy link

coveralls commented Dec 6, 2017

Coverage Status

Coverage remained the same at 93.648% when pulling 21e4d0b on raviolin:fix-checking-response-body into 3db3355 on vpulim:master.

@herom
Copy link
Contributor

herom commented Dec 6, 2017

Thanks a lot @raviolin 👍

In order to get your PR pushed I'll have to ask for at least 1 test for your contribution as requested per our Guidelines on Submitting a Pull Request

@jsdevel
Copy link
Collaborator

jsdevel commented Dec 6, 2017

why check for obj.html anyway? Does this lib ever use obj.html? Seems we could just move the if statement up and call this good.

@raviolin
Copy link
Contributor Author

raviolin commented Dec 7, 2017

@jsdevel apparently it's necessary in case the client gets an HTML response. There are tests for that.

@herom I'll add a test with a response sample, please don't close this PR yet.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 93.648% when pulling c7cca26 on raviolin:fix-checking-response-body into 3db3355 on vpulim:master.

@coveralls
Copy link

coveralls commented Dec 7, 2017

Coverage Status

Coverage remained the same at 93.587% when pulling 24736cd on raviolin:fix-checking-response-body into 905eadb on vpulim:master.

@jsdevel
Copy link
Collaborator

jsdevel commented Dec 7, 2017

LGTM

@raviolin
Copy link
Contributor Author

raviolin commented Feb 8, 2018

Can you remove 'needs tests' label? Or should I add more tests?

@herom
Copy link
Contributor

herom commented Feb 8, 2018

@raviolin I don't know why the label is still there and we haven't merged your PR yet.

Sorry for that... again 🐑 🐑 🐑 😞

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.

4 participants