Skip to content

test & util: Add a missing test and a conditonal teller - #21455

Closed
ghost wants to merge 1 commit into
masterfrom
unknown repository
Closed

test & util: Add a missing test and a conditonal teller#21455
ghost wants to merge 1 commit into
masterfrom
unknown repository

Conversation

@ghost

@ghostghost commented Jun 22, 2018•
edited by ghost
Loading

Copy link
Copy Markdown
  1. Add missing unit tests by ucs-2 in different kinds of cases.
  2. Add missing unit tests by usc-2 in different kinds of cases.
  3. Fix a bug：We cannot find ucs-2 in case 5's if condition after toLowerCase().
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-botnodejs-github-bot added the util Issues and PRs related to the built-in util module. label Jun 22, 2018
@ghostghost changed the title util: Remove useless formation indicator and add a missing unit testutil: Add a missing unit test case with a type checkJun 22, 2018
@ghostghost changed the title util: Add a missing unit test case with a type checkutil: Add a missing test with a template indicator removedJun 22, 2018
@ghostghost changed the title util: Add a missing test with a template indicator removedtest: Add a missing testJun 22, 2018
@ghost

ghost commented Jun 22, 2018

Copy link
Copy Markdown
Author

@mscdex：At last I abandoned re-writing after many tests about the efficiency but I changed the title and submit to add a missing test for utils. Plz have a review. Thanks anyway!

@apapirovski

apapirovski commented Jun 25, 2018

Copy link
Copy Markdown
Contributor

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite/893/

@Maledong you might want to associate your commit email with your account as otherwise you won't get proper credit for contributing to the project from Github.

@ghostghost changed the title test: Add a missing testtest & util: Add a missing test and a conditonal tellerJun 25, 2018
@ghost

ghost commented Jun 25, 2018•
edited by ghost
Loading

Copy link
Copy Markdown
Author

OK, I've reset my accout with a new one verified by GitHub. And everything goes well with me.
@apapirovski ：I see this test failed in CI. Is there anything to be fixed with? Can I restart or make a new CI test? If OK, how?
Thks anyway

@apapirovski

apapirovski commented Jun 25, 2018

Copy link
Copy Markdown
Contributor

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

@ghost

ghost commented Jun 25, 2018•
edited by ghost
Loading

Copy link
Copy Markdown
Author

Thanks anyway!
Is there anything to change? I see that it can pass all the tests except for on mac……

MaleDongMaleDong
test & util: Add missing tests and conditions
b287ae2
1) Add missing unit tests by `ucs-2` in different kinds of cases.
2) Add missing unit tests by `usc-2` in different kinds of cases.
3) Fix a bug：We cannot find `ucs-2` in `case 5`'s `if` condition after
`toLowerCase()`
@joyeecheung

joyeecheung commented Jun 26, 2018

Copy link
Copy Markdown
Member

Jenkins failed to fetch the source code on macOS. Just to be safe, a new CI: https://ci.nodejs.org/job/node-test-pull-request/15632/

@ghost

ghost commented Jul 6, 2018

Copy link
Copy Markdown
Author

@joyeecheung & @apapirovski ：
Sorry to interrupt you both but I wanna say, since this has fixed a missing code conversion for ucs-2 and others, and this has passed all the tests, so just a reminder to merge them into your Node master if you can.
Thanks anyway!

@Trott

Trott commented Jul 8, 2018

Copy link
Copy Markdown
Member

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

@TrottTrott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 8, 2018
@Trott

Trott commented Jul 8, 2018

Copy link
Copy Markdown
Member

Adding author ready so this doesn't get missed again if it's ready to land. Whoever lands it will need to edit the commit message. (test & util -> test, util:).

@Trott

Trott commented Jul 8, 2018

Copy link
Copy Markdown
Member

Re-running Windows only: https://ci.nodejs.org/job/node-test-commit-windows-fanned/19156/

Trott pushed a commit to Trott/io.js that referenced this pull request Jul 8, 2018
MaleDong@Trott
test,util: add missing tests and conditions
a478259
1) Add missing unit tests by `ucs-2` in different kinds of cases.
2) Add missing unit tests by `usc-2` in different kinds of cases.
3) Fix a bug：We cannot find `ucs-2` in `case 5`'s `if` condition after
`toLowerCase()`
PR-URL: nodejs#21455
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@Trott

Trott commented Jul 8, 2018

Copy link
Copy Markdown
Member

Landed in a478259.

Thanks for the contribution! 🎉

@TrottTrott closed this Jul 8, 2018
@ghost
ghost deleted the SimplifiedFornormalizeEncoding branch July 9, 2018 03:28
@ghost

ghost commented Jul 9, 2018

Copy link
Copy Markdown
Author

Thanks anyway.

targos pushed a commit that referenced this pull request Jul 10, 2018
MaleDong@targos
test,util: add missing tests and conditions
dc84858
1) Add missing unit tests by `ucs-2` in different kinds of cases.
2) Add missing unit tests by `usc-2` in different kinds of cases.
3) Fix a bug：We cannot find `ucs-2` in `case 5`'s `if` condition after
`toLowerCase()`
PR-URL: #21455
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@targostargos mentioned this pull request Jul 17, 2018
Merged
This was referenced Jul 18, 2018
Closed
Closed
Closed
Closed
@beevelopbeevelop mentioned this pull request Jul 19, 2018
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.utilIssues and PRs related to the built-in util module.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@apapirovski@joyeecheung@Trott@jasnell@trivikr@nodejs-github-bot