Tests from iconv-lite are not passing #7

bpasero · 2020-07-08T06:46:46Z

I did an experiment where I let all tests from iconv-lite run against this webpacked version. Here are the steps:

copy the test folder of iconv-lite into the root of this project
copy all devDependencies of iconv-lite into the package.json of this project
copy the "test": "mocha --reporter spec --grep ." over into the scripts section of the package.json of this project
remove the UTF-32/UTF-7 exclusions from webpack.config.js
ensure you are not webpacking with target: "node" but the default (web)
run yarn test

The result is a couple of test failures:

1) Full DBCS encoding tests Encode DBCS encoding 'shiftjis':
     Error: Bad argument.
      at convert (node_modules/iconv/lib/iconv.js:103:11)
      at Iconv.convert (node_modules/iconv/lib/iconv.js:64:12)
      at convertWithDefault (test/dbcs-test.js:44:26)
      at Context.<anonymous> (test/dbcs-test.js:230:32)
      at processImmediate (internal/timers.js:456:21)

  2) Full DBCS encoding tests Encode DBCS encoding 'eucjp':
     Error: Bad argument.
      at convert (node_modules/iconv/lib/iconv.js:103:11)
      at Iconv.convert (node_modules/iconv/lib/iconv.js:64:12)
      at convertWithDefault (test/dbcs-test.js:44:26)
      at Context.<anonymous> (test/dbcs-test.js:230:32)
      at processImmediate (internal/timers.js:456:21)

  3) Full DBCS encoding tests Encode DBCS encoding 'gb18030':
     Error: Bad argument.
      at convert (node_modules/iconv/lib/iconv.js:103:11)
      at Iconv.convert (node_modules/iconv/lib/iconv.js:64:12)
      at convertWithDefault (test/dbcs-test.js:44:26)
      at Context.<anonymous> (test/dbcs-test.js:230:32)
      at processImmediate (internal/timers.js:456:21)

  4) Full DBCS encoding tests Encode DBCS encoding 'cp950':
     Error: Bad argument.
      at convert (node_modules/iconv/lib/iconv.js:103:11)
      at Iconv.convert (node_modules/iconv/lib/iconv.js:64:12)
      at convertWithDefault (test/dbcs-test.js:44:26)
      at Context.<anonymous> (test/dbcs-test.js:230:32)
      at processImmediate (internal/timers.js:456:21)

  5) Full DBCS encoding tests Encode DBCS encoding 'big5hkscs':
     Error: Bad argument.
      at convert (node_modules/iconv/lib/iconv.js:103:11)
      at Iconv.convert (node_modules/iconv/lib/iconv.js:64:12)
      at convertWithDefault (test/dbcs-test.js:44:26)
      at Context.<anonymous> (test/dbcs-test.js:230:32)
      at processImmediate (internal/timers.js:456:21)

  6) Generic UTF8-UCS2 tests Return values are of correct types:

      AssertionError [ERR_ASSERTION]: The expression evaluated to a falsy value:

  assert.ok(Buffer.isBuffer(iconv.encode(testString, "utf8")))

      + expected - actual

      -false
      +true
      
      at Context.<anonymous> (test/main-test.js:13:16)
      at processImmediate (internal/timers.js:456:21)

  7) Generic UTF8-UCS2 tests Convert to string, not buffer (utf8 used):

      AssertionError [ERR_ASSERTION]: The expression evaluated to a falsy value:

  assert.ok(Buffer.isBuffer(res))

      + expected - actual

      -false
      +true
      
      at Context.<anonymous> (test/main-test.js:43:16)
      at processImmediate (internal/timers.js:456:21)

  8) UTF-16 decoder handles very short buffers nice:

      AssertionError [ERR_ASSERTION]: '\u0000' == ''
      + expected - actual

      -�
      
      at Context.<anonymous> (test/utf16-test.js:45:16)
      at processImmediate (internal/timers.js:456:21)

@gyzerok @ashtuchkin this worries me a bit. Some notes:

all tests run green when using target: "node" (not to any surprise)
I am seeing the same test errors when I try [email protected]

I would appreciate some help in fixing these issues as they would probably block the July release where I would like to go back to the webpacked version that does not target node. Thanks.

The text was updated successfully, but these errors were encountered:

gyzerok · 2020-07-08T06:49:16Z

@bpasero have you also tried [email protected]?

bpasero · 2020-07-08T06:53:00Z

@gyzerok yes I am seeing the same result with either 0.6.1 or 0.6.2. Especially Error: Bad argument worries me because I thought any Buffer usage is removed in 0.6.2, the line that throws this is:

function convert(conv, input, context) {
  if (!Buffer.isBuffer(input) && input !== FLUSH) {
    throw new Error('Bad argument.');  // Not a buffer or a string.
  }

bpasero · 2020-07-08T06:56:51Z

The object that is checked, when using JSON.stringify seems to be: {"type":"Buffer","data":[63]}

gyzerok · 2020-07-08T06:57:36Z

@bpasero I see.

No worries. I've decided to take a small break. Was using most of my free time before to make everything work 😄 But I am confident we can fix everything for July release. I will get back to it this weekend then.

bpasero · 2020-07-08T07:01:57Z

Sounds good, thanks.

bpasero · 2020-07-08T15:24:50Z

@ashtuchkin if there are no objections, I wonder if we should simply copy the tests folder from iconv-lite over into this repo to be able to run them? Let me know if you have a better idea...

ashtuchkin · 2020-07-08T19:15:31Z

No objections from me, I think that might be the best we can do until I support UMD natively. -- Alexander Shtuchkin

…

On Wed, Jul 8, 2020 at 11:25 AM Benjamin Pasero ***@***.***> wrote: @ashtuchkin <https://github.com/ashtuchkin> if there are no objections, I wonder if we should simply copy the tests folder from iconv-lite over into this repo to be able to run them? Let me know if you have a better idea... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEZKHPDLWD6DGA6UI2PMOTR2SFVDANCNFSM4OUHVAQA> .

bpasero · 2020-07-09T06:28:12Z

I pushed the tests to this repo so that they can be run via yarn test. Here is the result:

  244 passing (19s)
  33 pending
  8 failing

  1) Full DBCS encoding tests Encode DBCS encoding 'shiftjis':
     Error: Bad argument.
      at convert (node_modules/iconv/lib/iconv.js:103:11)
      at Iconv.convert (node_modules/iconv/lib/iconv.js:64:12)
      at convertWithDefault (test/dbcs-test.js:44:26)
      at Context.<anonymous> (test/dbcs-test.js:230:32)
      at processImmediate (internal/timers.js:456:21)

  2) Full DBCS encoding tests Encode DBCS encoding 'eucjp':
     Error: Bad argument.
      at convert (node_modules/iconv/lib/iconv.js:103:11)
      at Iconv.convert (node_modules/iconv/lib/iconv.js:64:12)
      at convertWithDefault (test/dbcs-test.js:44:26)
      at Context.<anonymous> (test/dbcs-test.js:230:32)
      at processImmediate (internal/timers.js:456:21)

  3) Full DBCS encoding tests Encode DBCS encoding 'gb18030':
     Error: Bad argument.
      at convert (node_modules/iconv/lib/iconv.js:103:11)
      at Iconv.convert (node_modules/iconv/lib/iconv.js:64:12)
      at convertWithDefault (test/dbcs-test.js:44:26)
      at Context.<anonymous> (test/dbcs-test.js:230:32)
      at processImmediate (internal/timers.js:456:21)

  4) Full DBCS encoding tests Encode DBCS encoding 'cp950':
     Error: Bad argument.
      at convert (node_modules/iconv/lib/iconv.js:103:11)
      at Iconv.convert (node_modules/iconv/lib/iconv.js:64:12)
      at convertWithDefault (test/dbcs-test.js:44:26)
      at Context.<anonymous> (test/dbcs-test.js:230:32)
      at processImmediate (internal/timers.js:456:21)

  5) Full DBCS encoding tests Encode DBCS encoding 'big5hkscs':
     Error: Bad argument.
      at convert (node_modules/iconv/lib/iconv.js:103:11)
      at Iconv.convert (node_modules/iconv/lib/iconv.js:64:12)
      at convertWithDefault (test/dbcs-test.js:44:26)
      at Context.<anonymous> (test/dbcs-test.js:230:32)
      at processImmediate (internal/timers.js:456:21)

  6) Generic UTF8-UCS2 tests Return values are of correct types:

      AssertionError [ERR_ASSERTION]: The expression evaluated to a falsy value:

  assert.ok(Buffer.isBuffer(iconv.encode(testString, "utf8")))

      + expected - actual

      -false
      +true
      
      at Context.<anonymous> (test/main-test.js:13:16)
      at processImmediate (internal/timers.js:456:21)

  7) Generic UTF8-UCS2 tests Convert to string, not buffer (utf8 used):

      AssertionError [ERR_ASSERTION]: The expression evaluated to a falsy value:

  assert.ok(Buffer.isBuffer(res))

      + expected - actual

      -false
      +true
      
      at Context.<anonymous> (test/main-test.js:43:16)
      at processImmediate (internal/timers.js:456:21)

  8) UTF-16 decoder handles very short buffers nice:

      AssertionError [ERR_ASSERTION]: '\u0000' == ''
      + expected - actual

      -�
      
      at Context.<anonymous> (test/utf16-test.js:45:16)
      at processImmediate (internal/timers.js:456:21)

gyzerok · 2020-07-11T02:02:46Z

Hello @bpasero! I've looked into this as promised.

Especially Error: Bad argument worries me because I thought any Buffer usage is removed in 0.6.2

It's not exactly like that. In 0.6.2 what changed is that iconv-lite won't expect input to be a Buffer. So it won't use any Buffer specific methods on the input (like Buffer.concat) for example. This makes it safe for us to pass in Uint8Array. However internally Buffer is still used.

With most errors you are seeing the problem is not with the library itself or with our webpack build, but with the fact that tests are written without any expectation that they could be run against umd build where Buffer is shimmed.

So the library creates and returns buffers using it's shimmed Buffer, but tests are expecting it to be Node Buffer. These are not compatible and that's why everywhere Buffer.isBuffer is used the problem arises.

After reading this you might start wondering if it means same problem for VSCode when running in Electron. And according to my understanding it is not the case, because we don't expect Buffer as an output, but simply Uint8Array, which means that we won't be using any Buffer specific methods on the output and thus won't have these errors.

Now the only error here which concerns me personally is

  8) UTF-16 decoder handles very short buffers nice:

      AssertionError [ERR_ASSERTION]: '\u0000' == ''
      + expected - actual

      -�
      
      at Context.<anonymous> (test/utf16-test.js:45:16)
      at processImmediate (internal/timers.js:456:21)

I will look into it. And here is my suggestion: I don't think VSCode nor iconv-lite benefit from tests extracted and modified here. For VSCode team it means additional maintenance burden and for iconv-lite it means that nothing gets improved. To avoid this I will try to contribute to the iconv-lite tests directly and hopefully help to move more of them in the browser. Does this make sense?

gyzerok · 2020-07-11T04:29:18Z

I have identified the source of the last error.

// Node
Buffer.from([0x61]).toString('utf16le') == ''
// feross/buffer
Buffer.from([0x61]).toString('utf16le') == '\u0000'

Will see what I can do about it.

bpasero · 2020-07-11T05:37:47Z

Sounds good, thanks. And agree, ideally tests would remain in iconv-lite and run in a mode that tests the web scenario 👍

bpasero self-assigned this Jul 8, 2020

bpasero mentioned this issue Jul 8, 2020

How to ensure that install is called before new version is published #6

Open

bpasero added a commit that referenced this issue Jul 9, 2020

add iconv-lite tests (#7)

02d179a

bpasero removed their assignment Jan 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tests from iconv-lite are not passing #7

Tests from iconv-lite are not passing #7

bpasero commented Jul 8, 2020

gyzerok commented Jul 8, 2020 •

edited

Loading

bpasero commented Jul 8, 2020

bpasero commented Jul 8, 2020

gyzerok commented Jul 8, 2020

bpasero commented Jul 8, 2020

bpasero commented Jul 8, 2020

ashtuchkin commented Jul 8, 2020 via email

bpasero commented Jul 9, 2020

gyzerok commented Jul 11, 2020

gyzerok commented Jul 11, 2020

bpasero commented Jul 11, 2020

Tests from iconv-lite are not passing #7

Tests from iconv-lite are not passing #7

Comments

bpasero commented Jul 8, 2020

gyzerok commented Jul 8, 2020 • edited Loading

bpasero commented Jul 8, 2020

bpasero commented Jul 8, 2020

gyzerok commented Jul 8, 2020

bpasero commented Jul 8, 2020

bpasero commented Jul 8, 2020

ashtuchkin commented Jul 8, 2020 via email

bpasero commented Jul 9, 2020

gyzerok commented Jul 11, 2020

gyzerok commented Jul 11, 2020

bpasero commented Jul 11, 2020

gyzerok commented Jul 8, 2020 •

edited

Loading