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 Unicode characters regression test #11423

Closed
wants to merge 1 commit into from

Conversation

aqrln
Copy link
Contributor

@aqrln aqrln commented Feb 16, 2017

This test ensures that UTF-8 characters can be used in core JavaScript modules built into Node's binary.

Refs: #11129

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)

test

This test ensures that UTF-8 characters can be used in core JavaScript
modules built into Node's binary.

Refs: nodejs#11129
@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Feb 16, 2017
@jasnell
Copy link
Member

jasnell commented Feb 16, 2017

hmm... I can't say that I'm too fond of bundling in an internal module whose sole purpose is to support a single test. Adding a simple property to the existing internal/util would work just as well.

@jasnell
Copy link
Member

jasnell commented Feb 16, 2017

Ugh, nevermind... that would cause the entire file to be stored larger than it needs to be.

'use strict';

// This module exists entirely for regression testing purposes.
// See `test/parallel/test-internal-unicode.js`.
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe worth putting unicode in the comment, it looks like some tools care about this.

Copy link
Contributor Author

@aqrln aqrln Feb 17, 2017

Choose a reason for hiding this comment

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

That doesn't actually matter much if Unicode characters in comments are encoded properly in Node's binary, if that's what you are talking about, IMO, though they may be useful for debugging, as @Fishrock123 noted. It is quite easy to test Unicode in comments automatically, either exporting a function with a comment inside in this module and checking fn.toString() in the test, or checking the whole value of process.binding('natives')['internal/test/unicode'], but I'd like the status of #11371 and #11417 to become clear before doing this so that this part of the test won't become obsolete as soon as either of these lands (if any). Not to mention that I don't think it is necessary to test Unicode in comments specially at all: how are they different from any other parts of the source code?

@jasnell
Copy link
Member

jasnell commented Mar 22, 2017

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM if CI is green

jasnell pushed a commit that referenced this pull request Apr 4, 2017
This test ensures that UTF-8 characters can be used in core JavaScript
modules built into Node's binary.

PR-URL: #11423
Ref: #11129
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Apr 4, 2017

Landed in f4e8a6f

@jasnell jasnell closed this Apr 4, 2017
@aqrln aqrln deleted the test-unicode branch April 4, 2017 18:54
@jasnell jasnell mentioned this pull request Apr 4, 2017
italoacasas pushed a commit to italoacasas/node that referenced this pull request Apr 10, 2017
This test ensures that UTF-8 characters can be used in core JavaScript
modules built into Node's binary.

PR-URL: nodejs#11423
Ref: nodejs#11129
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@italoacasas italoacasas mentioned this pull request Apr 10, 2017
2 tasks
@MylesBorins
Copy link
Contributor

assuming this is related to new URL implementation. added don't land

@aqrln
Copy link
Contributor Author

aqrln commented Apr 18, 2017

@MylesBorins no, it's a regression test for issues like this one: #10673.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants