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

Punycode fails for Russian(Cyrillic) #8691

Closed
ALJCepeda opened this issue Sep 21, 2016 · 4 comments
Closed

Punycode fails for Russian(Cyrillic) #8691

ALJCepeda opened this issue Sep 21, 2016 · 4 comments
Labels
punycode Issues and PRs related to the punycode module bundled in Node.js.

Comments

@ALJCepeda
Copy link
Contributor

ALJCepeda commented Sep 21, 2016

REF #4640

There's a portion of the test that's commented out because it fails, it was done in 0e19476 10d4bd8. The comment suggests there's a bug in the RFC.

Is this something that can be fixed? Or should that portion be deleted?

@gibfahn
Copy link
Member

gibfahn commented Sep 21, 2016

Just to add the info from #4640:

Test:
parallel/test-punycode.js:

Comment:

// (I) Russian (Cyrillic)
/* XXX disabled, fails - possibly a bug in the RFC
'b1abfaaepdrnnbgefbaDotcwatmq2g4l':
'\u043F\u043E\u0447\u0435\u043C\u0443\u0436\u0435\u043E\u043D\u0438' +

The comment was actually added in 10d4bd8 (but the file was moved in 0e19476).

I think the original issue for this was: nodejs/node-v0.x-archive#2072

cc/ @bnoordhuis as the original comment author.

@gibfahn
Copy link
Member

gibfahn commented Sep 21, 2016

This comment suggests the error was a typo: nodejs/node-v0.x-archive#2072 (comment)

@jasnell
Copy link
Member

jasnell commented Sep 21, 2016

We have recently switched the punycode implementation to use the one provided by the ICU module by default. It is possible that whatever the original issue was here, it may be resolved. I'll see about testing it later on today.

@jasnell jasnell added the punycode Issues and PRs related to the punycode module bundled in Node.js. label Sep 21, 2016
@bnoordhuis
Copy link
Member

It was a bug in the spec's test suite. The fix is trivial, the capital D in the input should be lower-cased.

diff --git a/test/parallel/test-punycode.js b/test/parallel/test-punycode.js
index 4292754..abd495e 100644
--- a/test/parallel/test-punycode.js
+++ b/test/parallel/test-punycode.js
@@ -62,12 +62,10 @@ var tests = {
       '\uC744\uAE4C',

   // (I) Russian (Cyrillic)
-  /* XXX disabled, fails - possibly a bug in the RFC
-  'b1abfaaepdrnnbgefbaDotcwatmq2g4l':
+  'b1abfaaepdrnnbgefbadotcwatmq2g4l':
       '\u043F\u043E\u0447\u0435\u043C\u0443\u0436\u0435\u043E\u043D\u0438' +
       '\u043D\u0435\u0433\u043E\u0432\u043E\u0440\u044F\u0442\u043F\u043E' +
       '\u0440\u0443\u0441\u0441\u043A\u0438',
-  */

   // (J) Spanish: Porqu<eacute>nopuedensimplementehablarenEspa<ntilde>ol
   'PorqunopuedensimplementehablarenEspaol-fmd56a':

bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue Sep 23, 2016
The test from RFC 3492 contains a bug: the uppercase D in the input
should be lowercased.  Fix that and enable the test.

Fixes: nodejs#8691
PR-URL: nodejs#8695
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Ilkka Myller <[email protected]>
Reviewed-By: James M Snell <[email protected]>
jasnell pushed a commit that referenced this issue Sep 29, 2016
The test from RFC 3492 contains a bug: the uppercase D in the input
should be lowercased.  Fix that and enable the test.

Fixes: #8691
PR-URL: #8695
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Ilkka Myller <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Fishrock123 pushed a commit that referenced this issue Oct 11, 2016
The test from RFC 3492 contains a bug: the uppercase D in the input
should be lowercased.  Fix that and enable the test.

Fixes: #8691
PR-URL: #8695
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Ilkka Myller <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this issue Nov 18, 2016
The test from RFC 3492 contains a bug: the uppercase D in the input
should be lowercased.  Fix that and enable the test.

Fixes: #8691
PR-URL: #8695
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Ilkka Myller <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
punycode Issues and PRs related to the punycode module bundled in Node.js.
Projects
None yet
Development

No branches or pull requests

4 participants