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

preserve whitespace for space only text node: <text> </text> #1211

Merged
merged 3 commits into from
Apr 27, 2024

Conversation

yffrankwang
Copy link
Contributor

when preverseWhitespace = true option is specified, the space in the text node should be preversed.
but if the text node's content is all white space, these space is not preserved.
example:
expect: result.text = ' ' (<- one space)
actual: result.text = '' (empty string)

@yffrankwang yffrankwang force-pushed the preserveWhitespace branch 2 times, most recently from bea1569 to 36aaea7 Compare February 3, 2023 02:02
@yffrankwang yffrankwang changed the title fix bug for missing space for <text> </text> (preverseWhitespace = t… preserve white for space only text node: <text> </text> Feb 3, 2023
@yffrankwang yffrankwang changed the title preserve white for space only text node: <text> </text> preserve whitespace for space only text node: <text> </text> Feb 3, 2023
@w666
Copy link
Collaborator

w666 commented Apr 18, 2024

Could you please add tests for this change?

@yffrankwang
Copy link
Contributor Author

test/response-preserve-whitespace-test.js

@w666
Copy link
Collaborator

w666 commented Apr 22, 2024

Looks like new test failed, could you please have a look? Thanks.

@yffrankwang
Copy link
Contributor Author

both test/client-response-options-test.js test/response-preserve-whitespace-test.js use 'test/wsdl/hello.wsdl' to create soap client.
the two soap client instance's schema are different, this cause the secondary test failed.
i don't know why, this may be another bug.

please take a look at the following test.

mocha test/response-preserve-whitespace-test.js

  Preverse whitespace
    ✔ preserves leading and trailing whitespace when preserveWhitespace option is true


  1 passing (39ms)

mocha test/client-response-options-test.js test/response-preserve-whitespace-test.js


  SOAP Client
test server is created
test server is listening in http://127.0.0.1:54454
client created: { Hello_Service: { Hello_Port: { sayHello: [Object] } } }
api finished in ms: 19
    ✔ lastElapsedTime is computed
server is closed

  Preverse whitespace
    1) preserves leading and trailing whitespace when preserveWhitespace option is true


  1 passing (2s)
  1 failing

  1) Preverse whitespace
       preserves leading and trailing whitespace when preserveWhitespace option is true:
     Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (D:\Develop\Projects\frank\node-soap\test\response-preserve-whitespace-test.js)
      at listOnTimeout (internal/timers.js:557:17)
      at processTimers (internal/timers.js:500:7)

@w666
Copy link
Collaborator

w666 commented Apr 24, 2024

I am not sure why you get timeout error, but I cloned your repo and run the test, I got the same probem as in pipeline
image

Seems like it does not work?

@yffrankwang
Copy link
Contributor Author

because the assertion in the test case failed, and the following done() is not called, so the timeout error occurred.

              assert.equal(' ', result.greeting);
              done();

as i commented yesterday, if you run the single test case, it will passed.

mocha test/response-preserve-whitespace-test.js

  Preverse whitespace
    ✔ preserves leading and trailing whitespace when preserveWhitespace option is true
  1 passing (39ms)

but if you run the following 2 test case together, the second test case will failed.

mocha test/client-response-options-test.js test/response-preserve-whitespace-test.js

SOAP Client
test server is created
test server is listening in http://127.0.0.1:54454
client created: { Hello_Service: { Hello_Port: { sayHello: [Object] } } }
api finished in ms: 19
    ✔ lastElapsedTime is computed
server is closed

  Preverse whitespace
    1) preserves leading and trailing whitespace when preserveWhitespace option is true


  1 passing (2s)
  1 failing

  1) Preverse whitespace
       preserves leading and trailing whitespace when preserveWhitespace option is true:
     Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (D:\Develop\Projects\frank\node-soap\test\response-preserve-whitespace-test.js)
      at listOnTimeout (internal/timers.js:557:17)
      at processTimers (internal/timers.js:500:7)

The reason why the second test case failed is that the two test cases use 'test/wsdl/hello.wsdl' to create soap client.
The first soap client is correct, but the second soap client is incorrect.

The cur.schema in the following code is different.
The first soap client cur.schema is string, but the second soap client cur.schema is null.

https://github.com/vpulim/node-soap/pull/1211/files#diff-2c1300f4732a4bdd2d6f0234d611bb7ac264b39f5a173b4f86412a26102b0258R358

image

i don't know why the second soap client is different from the first soap client, this may be another bug.

@w666
Copy link
Collaborator

w666 commented Apr 26, 2024

I think this is useful change, but working tests are required for that. I am happy to accept this when all tests pass. Thanks.

@w666
Copy link
Collaborator

w666 commented Apr 27, 2024

Thanks for fixing the test. I will include this in next release. Thanks.

@w666 w666 merged commit 6f22534 into vpulim:master Apr 27, 2024
1 check passed
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