loopbackio / strong-soap

SOAP driver for Node.js (A complete rewrite of node-soap)
Other
391 stars 163 forks source link

Allowing empty response body #672

Closed braj1999 closed 9 months ago

braj1999 commented 10 months ago

Description

Allowing empty response body case

Related issues

For #671

Checklist

braj1999 commented 10 months ago

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

dhmlau commented 10 months ago

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

braj1999 commented 10 months ago

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

dhmlau commented 10 months ago

@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)
dhmlau commented 9 months ago

@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 commented 9 months ago

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 commented 9 months ago

@dhmlau Fixed the UT

dhmlau commented 9 months ago

@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! 🎉