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

test: add toASCII and toUnicode punycode tests #9741

Conversation

claudiorodriguez
Copy link
Contributor

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test

Description of change
  • Add toUnicode and toASCII tests to test-punycode
  • Refactor test-punycode.js to better organize test cases
  • Change assert.equal to assert.strictEqual in test-punycode.js

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Nov 22, 2016
@claudiorodriguez
Copy link
Contributor Author

{
string: 'ليهمابتكلموشعربي؟',
encoded: 'egbpdaj6bu4bxfgehfvwxn',
decoded: '\u0644\u064A\u0647\u0645\u0627\u0628\u062A\u0643\u0644\u0645' +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't decoded just a copy of string? If so can't we just use string and encoded?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we could. Why did the test use the \u codes?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, maybe because that's how the sample strings are listed in the RFC?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that we can probably just remove them if they server no purpose.

Copy link
Contributor Author

@claudiorodriguez claudiorodriguez Nov 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that's how they appear in the RFC, I'd say maybe leave them just in case? It might also help in debugging, if the test ever starts failing, to have the bytecodes handy. We could just stick with decoded though

'\uC5B4\uB97C\uC774\uD574\uD55C\uB2E4\uBA74\uC5BC\uB9C8\uB098\uC88B' +
'\uC744\uAE4C',
{
string: '세계의모든사람들이한국어를이해한다면얼마나좋을까',

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I Guide this sentence like "세계의 모든 사람들이 한국어를 이해한다면 얼마나 좋을까"

@claudiorodriguez
Copy link
Contributor Author

claudiorodriguez commented Nov 23, 2016

I've removed the duplicitous string and left the \u encoded ones since they appear like that in the RFC, and also I've rewritten the toUnicode test to reflect its actual logic.

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

toASCII: (test) => assert.strictEqual(
punycode.toASCII(test.decoded),
regexNonASCII.test(test.decoded)
? 'xn--' + test.encoded
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: use template literal?

- Add toUnicode and toASCII tests to test-punycode
- Refactor test-punycode.js to better organize test cases
- Change assert.equal to assert.strictEqual in test-punycode.js
@claudiorodriguez
Copy link
Contributor Author

CI Failure seems unrelated

@claudiorodriguez
Copy link
Contributor Author

I'll merge this tomorrow if there's no objections

claudiorodriguez added a commit that referenced this pull request Nov 30, 2016
- Add toUnicode and toASCII tests to test-punycode
- Refactor test-punycode.js to better organize test cases
- Change assert.equal to assert.strictEqual in test-punycode.js

PR-URL: #9741
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@claudiorodriguez
Copy link
Contributor Author

Landed in c2f8487

addaleax pushed a commit that referenced this pull request Dec 5, 2016
- Add toUnicode and toASCII tests to test-punycode
- Refactor test-punycode.js to better organize test cases
- Change assert.equal to assert.strictEqual in test-punycode.js

PR-URL: #9741
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@Fishrock123 Fishrock123 mentioned this pull request Dec 5, 2016
2 tasks
addaleax pushed a commit to addaleax/node that referenced this pull request Dec 8, 2016
- Add toUnicode and toASCII tests to test-punycode
- Refactor test-punycode.js to better organize test cases
- Change assert.equal to assert.strictEqual in test-punycode.js

PR-URL: nodejs#9741
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@addaleax addaleax added land-on-v6.x punycode Issues and PRs related to the punycode module bundled in Node.js. labels Dec 8, 2016
MylesBorins pushed a commit that referenced this pull request Dec 20, 2016
- Add toUnicode and toASCII tests to test-punycode
- Refactor test-punycode.js to better organize test cases
- Change assert.equal to assert.strictEqual in test-punycode.js

PR-URL: #9741
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
- Add toUnicode and toASCII tests to test-punycode
- Refactor test-punycode.js to better organize test cases
- Change assert.equal to assert.strictEqual in test-punycode.js

PR-URL: #9741
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
- Add toUnicode and toASCII tests to test-punycode
- Refactor test-punycode.js to better organize test cases
- Change assert.equal to assert.strictEqual in test-punycode.js

PR-URL: #9741
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
This was referenced Dec 21, 2016
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. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants