util: fix Latin1 decoding to return string output

[mertcanaltin]

returned a string instead of a buffer

Fixes: #56219

[aduh95]

Can you add a test?

[mertcanaltin]

Can you add a test?

added to cctest

[aduh95]

Could we have a test with TextDecoder to validate the reported issue is fixed and won't come back?

[mertcanaltin]

Could we have a test with TextDecoder to validate the reported issue is fixed and won't come back?

Yes, that's right. I'm sorry, I was focusing on the cctest.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/63997/

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.54%. Comparing base (2063245) to head (e8cf59a).
Report is 35 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #56222      +/-   ##
==========================================
+ Coverage   88.52%   88.54%   +0.01%     
==========================================
  Files         657      657              
  Lines      189860   189972     +112     
  Branches    36448    36483      +35     
==========================================
+ Hits       168080   168206     +126     
+ Misses      14978    14952      -26     
- Partials     6802     6814      +12     

Files with missing lines	Coverage Δ
src/encoding_binding.cc	79.09% <100.00%> (+7.66%)	⬆️

... and 34 files with indirect coverage changes

[lemire]

To my knowledge, it is the first TextDecoder test for Latin1 that we have! So that's progress. :-)

[mertcanaltin]

To my knowledge, it is the first TextDecoder test for Latin1 that we have! So that's progress. :-)

I'm so happy to hear that! 🚀

[anonrig]

Good work!

[aduh95]

Related test failure on build compiled --without-intl:

not ok 3162 parallel/test-util-text-decoder
  ---
  duration_ms: 125.69700
  severity: fail
  exitcode: 1
  stack: |-
    ✖ TextDecoder correctly decodes windows-1252 encoded data (1.319017ms)
      RangeError [ERR_ENCODING_NOT_SUPPORTED]: The "windows-1252" encoding is not supported
          at new TextDecoder (node:internal/encoding:490:15)
          at TestContext.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux-containered/test/parallel/test-util-text-decoder.js:13:19)
          at Test.runInAsyncScope (node:async_hooks:211:14)
          at Test.run (node:internal/test_runner/test:931:25)
          at Test.start (node:internal/test_runner/test:829:17)
          at startSubtestAfterBootstrap (node:internal/test_runner/harness:297:17) {
        code: 'ERR_ENCODING_NOT_SUPPORTED'
      }
    
    ℹ tests 1
    ℹ suites 0
    ℹ pass 0
    ℹ fail 1
    ℹ cancelled 0
    ℹ skipped 0
    ℹ todo 0
    ℹ duration_ms 12.534744
    
    ✖ failing tests:
    
    test at test/parallel/test-util-text-decoder.js:8:1
    ✖ TextDecoder correctly decodes windows-1252 encoded data (1.319017ms)
      RangeError [ERR_ENCODING_NOT_SUPPORTED]: The "windows-1252" encoding is not supported
          at new TextDecoder (node:internal/encoding:490:15)
          at TestContext.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux-containered/test/parallel/test-util-text-decoder.js:13:19)
          at Test.runInAsyncScope (node:async_hooks:211:14)
          at Test.run (node:internal/test_runner/test:931:25)
          at Test.start (node:internal/test_runner/test:829:17)
          at startSubtestAfterBootstrap (node:internal/test_runner/harness:297:17) {
        code: 'ERR_ENCODING_NOT_SUPPORTED'
      }
  ...

[mertcanaltin]

I'm considering whether skipping the test in such scenarios would be the correct approach.

Alternatively, I managed to implement a custom solution to decode the sequence myself, but I’m unsure if this is a reliable or sustainable approach. Do you have any suggestions or best practices for handling this situation?

Thank you in advance for your guidance!

[BridgeAR]

For this specific case, it's probably fine to skip the test for builds without intl. We could theoretically implement further support, but I believe that is not currently necessary, as it has not been requested.

[mertcanaltin]

For this specific case, it's probably fine to skip the test for builds without intl. We could theoretically implement further support, but I believe that is not currently necessary, as it has not been requested.

I passed the test with the condition !common.hasIntl

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/64013/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/64024/

[nodejs-github-bot]

Landed in e7a397a