test: improve punycode test coverage #11144

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
7 participants
@seppevs
Contributor

seppevs commented Feb 3, 2017

Adds two additional tests for mapDomain function in punycode.js.

When an email address is given to the toASCII() and toUnicode() functions, only the parts before the '@' character should be encoded/decoded.

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

N/A

test: improve punycode test coverage
Adds two additional tests for mapDomain function in punycode.js.
When an email address is given to the toASCII() and toUnicode() functions, only the parts before the '@' character should be encoded/decoded.

@hiroppy hiroppy added the punycode label Feb 3, 2017

@jasnell

jasnell approved these changes Feb 3, 2017

@hiroppy

This comment has been minimized.

Show comment
Hide comment
@thefourtheye

LGTM if the CI is happy.

@seppevs

This comment has been minimized.

Show comment
Hide comment
@seppevs

seppevs Feb 6, 2017

Contributor

The CI fails, but I don't think it's caused by my changes?

Contributor

seppevs commented Feb 6, 2017

The CI fails, but I don't think it's caused by my changes?

@joyeecheung

This comment has been minimized.

Show comment
Hide comment
@joyeecheung

joyeecheung Feb 6, 2017

Member

Refs: #10990 (comment)

@addaleax @mathiasbynens If we move punycode.js to deps, should we upstream the tests?

Member

joyeecheung commented Feb 6, 2017

Refs: #10990 (comment)

@addaleax @mathiasbynens If we move punycode.js to deps, should we upstream the tests?

@joyeecheung

This comment has been minimized.

Show comment
Hide comment
@joyeecheung

joyeecheung Feb 6, 2017

Member

BTW: CI failure looks unrelated.

Member

joyeecheung commented Feb 6, 2017

BTW: CI failure looks unrelated.

@mathiasbynens

This comment has been minimized.

Show comment
Hide comment
@mathiasbynens

mathiasbynens Feb 6, 2017

Contributor

@joyeecheung There’s an upstream test for this already: https://github.com/bestiejs/punycode.js/blob/9aeca525bba478206c6e1b5501e063f3db7bda7f/tests/tests.js#L211-L215 If I’m missing something and this patch somehow tests a different code path, please submit a PR to https://github.com/bestiejs/punycode.js as well.

Contributor

mathiasbynens commented Feb 6, 2017

@joyeecheung There’s an upstream test for this already: https://github.com/bestiejs/punycode.js/blob/9aeca525bba478206c6e1b5501e063f3db7bda7f/tests/tests.js#L211-L215 If I’m missing something and this patch somehow tests a different code path, please submit a PR to https://github.com/bestiejs/punycode.js as well.

@seppevs

This comment has been minimized.

Show comment
Hide comment
@seppevs

seppevs Feb 7, 2017

Contributor

@mathiasbynens : the upstream test seems to cover the same code.

Contributor

seppevs commented Feb 7, 2017

@mathiasbynens : the upstream test seems to cover the same code.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

This comment has been minimized.

Show comment
Hide comment

jasnell added a commit that referenced this pull request Feb 13, 2017

test: improve punycode test coverage
Adds two additional tests for mapDomain function in punycode.js.
When an email address is given to the toASCII() and toUnicode()
functions, only the parts before the '@' character should be
encoded/decoded.

PR-URL: #11144
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Feb 13, 2017

Member

Landed in c6238e2

Member

jasnell commented Feb 13, 2017

Landed in c6238e2

@jasnell jasnell closed this Feb 13, 2017

italoacasas added a commit to italoacasas/node that referenced this pull request Feb 14, 2017

test: improve punycode test coverage
Adds two additional tests for mapDomain function in punycode.js.
When an email address is given to the toASCII() and toUnicode()
functions, only the parts before the '@' character should be
encoded/decoded.

PR-URL: nodejs#11144
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

jasnell added a commit that referenced this pull request Mar 7, 2017

test: improve punycode test coverage
Adds two additional tests for mapDomain function in punycode.js.
When an email address is given to the toASCII() and toUnicode()
functions, only the parts before the '@' character should be
encoded/decoded.

PR-URL: #11144
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

jasnell added a commit that referenced this pull request Mar 7, 2017

test: improve punycode test coverage
Adds two additional tests for mapDomain function in punycode.js.
When an email address is given to the toASCII() and toUnicode()
functions, only the parts before the '@' character should be
encoded/decoded.

PR-URL: #11144
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

MylesBorins added a commit that referenced this pull request Mar 9, 2017

test: improve punycode test coverage
Adds two additional tests for mapDomain function in punycode.js.
When an email address is given to the toASCII() and toUnicode()
functions, only the parts before the '@' character should be
encoded/decoded.

PR-URL: #11144
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

@MylesBorins MylesBorins referenced this pull request Mar 9, 2017

Merged

v6.10.1 proposal #11759

MylesBorins added a commit that referenced this pull request Mar 9, 2017

test: improve punycode test coverage
Adds two additional tests for mapDomain function in punycode.js.
When an email address is given to the toASCII() and toUnicode()
functions, only the parts before the '@' character should be
encoded/decoded.

PR-URL: #11144
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

@MylesBorins MylesBorins referenced this pull request Mar 9, 2017

Merged

v4.8.1 proposal #11760

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment