test: add hasIntl to failing test by danbev · Pull Request #13699 · nodejs/node

danbev · 2017-06-15T13:02:21Z

Currently when node is configured --without-intl the tests in this
commit fail. This commit adds checks for internationalization for these
tests

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

test

Currently when node is configured --without-intl the tests in this commit fail. This commit adds checks for internationalization for these tests

danbev · 2017-06-15T13:02:56Z

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

refack · 2017-06-15T13:10:22Z

It is documented in anyway that WHATWG URL doesn't work without ICU?

danbev · 2017-06-15T13:18:22Z

It is documented in anyway that WHATWG URL doesn't work without ICU?

Sorry, I don't know if it is documented or not.

refack · 2017-06-15T13:29:21Z

It is documented in anyway that WHATWG URL doesn't work without ICU?
Sorry, I don't know if it is documented or not.

I'll go dig.

TimothyGu · 2017-06-15T13:33:48Z

@refack

It is documented in anyway that WHATWG URL doesn't work without ICU?

Not that I am aware of. However, WHATWG URL does work without ICU, just not the Punycode encoding or decoding part.

cjihrig

LGTM with a question.

cjihrig · 2017-06-15T17:00:43Z

 'use strict';
-require('../common');
+const common = require('../common');
+if (!(common.hasIntl && common.hasSmallICU)) {


Is common.hasSmallICU a thing? I'm only finding process.binding('config').hasSmallICU with a quick search.

Good catch! I was looking at an old commit and did not notice that that function does not exist or has been removed. Going to add it now. Thanks.

Trott

If we're adding a property to common, can you also add documentation for it in test/common/README.md?

danbev · 2017-06-16T03:49:55Z

If we're adding a property to common, can you also add documentation for it in test/common/README.md?

Ah sorry, forgot about that. I'll add that.

Commit d7e4ae1 ("test: add common.hasIntl") added common.hasIntl but I was not able to find it in the README.md so this commit adds it.

docs added, dismissing review

Commit d7e4ae1 ("test: add common.hasIntl") added common.hasIntl but I was not able to find it in the README.md so this commit adds it. PR-URL: nodejs#13699 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Currently when node is configured --without-intl the tests in this commit fail. This commit adds checks for internationalization for these tests PR-URL: nodejs#13699 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

danbev · 2017-06-18T09:23:32Z

Landed in a8979a6 and 282b1ed

Commit d7e4ae1 ("test: add common.hasIntl") added common.hasIntl but I was not able to find it in the README.md so this commit adds it. PR-URL: #13699 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Currently when node is configured --without-intl the tests in this commit fail. This commit adds checks for internationalization for these tests PR-URL: #13699 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Commit d7e4ae1 ("test: add common.hasIntl") added common.hasIntl but I was not able to find it in the README.md so this commit adds it. PR-URL: #13699 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Currently when node is configured --without-intl the tests in this commit fail. This commit adds checks for internationalization for these tests PR-URL: #13699 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Commit d7e4ae1 ("test: add common.hasIntl") added common.hasIntl but I was not able to find it in the README.md so this commit adds it. PR-URL: #13699 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Currently when node is configured --without-intl the tests in this commit fail. This commit adds checks for internationalization for these tests PR-URL: #13699 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins · 2017-07-17T18:38:52Z

should this be backported to v6.x?

test: add hasIntl to failing test
82919d0
Currently when node is configured --without-intl the tests in this commit fail. This commit adds checks for internationalization for these tests

nodejs-github-bot added the test Issues and PRs related to the tests. label Jun 15, 2017

gibfahn approved these changes Jun 15, 2017
View reviewed changes

mscdex added the i18n-api Issues and PRs related to the i18n implementation. label Jun 15, 2017

benjamingr approved these changes Jun 15, 2017
View reviewed changes

cjihrig approved these changes Jun 15, 2017
View reviewed changes

add hasSmallICU to common
6c34c19

Trott previously requested changes Jun 15, 2017
View reviewed changes

danbev added 2 commits June 16, 2017 07:35

doc: add hasIntl to test/common/README.md
379d9cc
Commit d7e4ae1 ("test: add common.hasIntl") added common.hasIntl but I was not able to find it in the README.md so this commit adds it.

doc: add hasSmallICU to common/test/README.md
eb819b7

jasnell approved these changes Jun 16, 2017
View reviewed changes

danbev closed this Jun 18, 2017

addaleax mentioned this pull request Jun 21, 2017
v8.2.0 proposal #13744
Merged

addaleax mentioned this pull request Jun 21, 2017
v8.1.3 proposal #13861
Merged

TimothyGu added url Issues and PRs related to the legacy built-in url module. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Jun 26, 2017

danbev deleted the intl-test-fixes branch June 28, 2017 05:34

MylesBorins added the dont-land-on-v6.x label Jul 17, 2017

Navigation Menu

Uh oh!

Uh oh!

test: add hasIntl to failing test - #13699

test: add hasIntl to failing test#13699
danbev wants to merge 4 commits into
nodejs:masterfrom
danbev:intl-test-fixes

danbev commented Jun 15, 2017

Uh oh!

danbev commented Jun 15, 2017

Uh oh!

refack commented Jun 15, 2017

Uh oh!

danbev commented Jun 15, 2017

Uh oh!

refack commented Jun 15, 2017

Uh oh!

TimothyGu commented Jun 15, 2017

Uh oh!

cjihrig left a comment

Uh oh!

cjihrigJun 15, 2017

Uh oh!

danbevJun 15, 2017

Uh oh!

Trott left a comment

Uh oh!

danbev commented Jun 16, 2017

Uh oh!

danbev commented Jun 18, 2017

Uh oh!

MylesBorins commented Jul 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

Uh oh!

Uh oh!

Conversation

danbev commented Jun 15, 2017

Checklist

Affected core subsystem(s)

Uh oh!

danbev commented Jun 15, 2017

Uh oh!

refack commented Jun 15, 2017

Uh oh!

danbev commented Jun 15, 2017

Uh oh!

refack commented Jun 15, 2017

Uh oh!

TimothyGu commented Jun 15, 2017

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

cjihrigJun 15, 2017

Choose a reason for hiding this comment

Uh oh!

danbevJun 15, 2017

Choose a reason for hiding this comment

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

danbev commented Jun 16, 2017

Uh oh!

danbev commented Jun 18, 2017

Uh oh!

MylesBorins commented Jul 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants