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

Allowing empty response body #672

Merged
merged 1 commit into from
Jan 16, 2024
Merged

Allowing empty response body #672

merged 1 commit into from
Jan 16, 2024

Conversation

braj1999
Copy link
Contributor

@braj1999 braj1999 commented Dec 14, 2023

Description

Allowing empty response body case

Related issues

For #671

  • connect to <link_to_referenced_issue>

Checklist

  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style
    guide

@braj1999
Copy link
Contributor Author

@raymondfeng Could you please confirm if the proposed fix looks okay?

@dhmlau
Copy link
Member

dhmlau commented Jan 7, 2024

@braj1999, could you please add test(s) for your changes?

@braj1999
Copy link
Contributor Author

braj1999 commented Jan 9, 2024

@dhmlau Added unit test to the PR, please have a look.

@dhmlau
Copy link
Member

dhmlau commented Jan 12, 2024

@braj1999, there's a test failure which is likely caused by your change. Could you please take a look?

https://github.com/loopbackio/strong-soap/actions/runs/7456242838/job/20427083943?pr=672

 1) SOAP Client
       Handle HTML answer from non-SOAP server
         should return an error:
     Uncaught AssertionError [ERR_ASSERTION]: The expression evaluated to a falsy value:

  assert.ok(err)

      at /home/runner/work/strong-soap/strong-soap/test/client-test.js:795:18
      at /home/runner/work/strong-soap/strong-soap/src/client.js:24:8
      at Request._callback (src/http.js:34:560)
      at Request.self.callback (node_modules/request/request.js:185:22)
      at Request.emit (node:events:513:28)
      at Request.<anonymous> (node_modules/request/request.js:1154:10)
      at Request.emit (node:events:513:28)
      at IncomingMessage.<anonymous> (node_modules/request/request.js:1076:12)
      at Object.onceWrapper (node:events:627:28)
      at IncomingMessage.emit (node:events:525:35)
      at endReadableNT (node:internal/streams/readable:1358:12)
      at processTicksAndRejections (node:internal/process/task_queues:83:21)

@braj1999 braj1999 marked this pull request as draft January 15, 2024 12:57
@dhmlau
Copy link
Member

dhmlau commented Jan 15, 2024

@braj1999, with the latest code in your branch, the tests failed for Node.js 20 and higher:

 1) wsdl-tests
       wsdl parsing test cases
         should not parse connection error:
     Uncaught 
  AggregateError
      at internalConnectMultiple (node:net:1114:18)
      at afterConnectMultiple (node:net:1667:5)

This corresponds to this test case: https://github.com/loopbackio/strong-soap/blob/master/test/wsdl-test.js#L81-L86.

Since the error changes to an AggregateError after Node.js 18, we could possible have the assert statement from:

assert.ok(/EADDRNOTAVAIL|ECONNREFUSED/.test(err), err);

to:

if (err.message != '') {
          err.message.should.containEql('ECONNREFUSED');
        } else {
          assert.ok(/ECONNREFUSED/.test(err.errors[0]), err);
}

I've tested the above snippet. It works for both Node.js 18 and 20.

@dhmlau
Copy link
Member

dhmlau commented Jan 15, 2024

About the commit linter error, if you could squad your commits and have a commit message something along the line as below, that should get rid of the error.

fix: allow empty response body

@braj1999 braj1999 marked this pull request as ready for review January 16, 2024 06:17
@braj1999
Copy link
Contributor Author

@dhmlau Fixed the UT

@dhmlau dhmlau merged commit 18b358f into loopbackio:master Jan 16, 2024
10 of 11 checks passed
@dhmlau
Copy link
Member

dhmlau commented Jan 16, 2024

@braj1999, thank you for fixing the test cases as well. I've also resolved the last failing test in your PR.
Your PR has been merged! 🎉

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.

2 participants