test: fix failure in test-icu-data-dir.js by tniessen · Pull Request #13987 · nodejs/node · GitHub
Skip to content

test: fix failure in test-icu-data-dir.js#13987

Closed
tniessen wants to merge 3 commits into
nodejs:masterfrom
tniessen:revert-13940
Closed

test: fix failure in test-icu-data-dir.js#13987
tniessen wants to merge 3 commits into
nodejs:masterfrom
tniessen:revert-13940

Conversation

@tniessen

@tniessen tniessen commented Jun 29, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

PR #13940 broke tests on Windows.

Ref: #13986

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

src

@tniessen
src: revert 13940
8573ce8 
PR broke tests on Windows.

Ref: nodejs#13940
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jun 29, 2017
@tniessen

tniessen commented Jun 29, 2017

Copy link
Copy Markdown
Member Author

@evanlucas

evanlucas commented Jun 29, 2017

Copy link
Copy Markdown
Contributor

Can you use the default revert commit message? It should look something like Revert "<original commit message>"

@mscdex

mscdex commented Jun 29, 2017

Copy link
Copy Markdown
Contributor

Wouldn't it be better to just fix the tests?

@tniessen

tniessen commented Jun 29, 2017 •
edited
Loading

Copy link
Copy Markdown
Member Author

Wouldn't it be better to just fix the tests?

I will check whether using \r\n in the test file on Windows is enough to solve the problem. A reversion CI was suggested on IRC as a quick solution to get master to pass CI asap.

@tniessen

tniessen commented Jun 29, 2017 •
edited
Loading

Copy link
Copy Markdown
Member Author

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

@tniessen tniessen changed the title src: revert 13940 test: fix failure in test-icu-data-dir.js Jun 29, 2017
@tniessen tniessen added the test Issues and PRs related to the tests. label Jun 29, 2017
@tniessen tniessen self-assigned this Jun 29, 2017
@tniessen

tniessen commented Jun 29, 2017

Copy link
Copy Markdown
Member Author

@mscdex PTAL

@mscdex

mscdex commented Jun 29, 2017

Copy link
Copy Markdown
Contributor

As long as it passes CI it's fine by me.

@mscdex

mscdex commented Jun 29, 2017

Copy link
Copy Markdown
Contributor

It appears this already landed in a1ecdcf.

@mscdex mscdex closed this Jun 29, 2017
@tniessen

tniessen commented Jun 29, 2017 •
edited
Loading

Copy link
Copy Markdown
Member Author

@addaleax addaleax mentioned this pull request Jun 29, 2017
Merged
addaleax pushed a commit that referenced this pull request Jun 29, 2017
@tniessen @addaleax
test: fix failure in test-icu-data-dir.js
e7ff3b9 
This fixes a broken test on Windows caused by EOL conversion.

PR-URL: #13987
Refs: #13940
Refs: #13986
Reviewed-By: Refael Ackermann <refack@gmail.com>
@joaocgreis joaocgreis mentioned this pull request Jun 30, 2017
Closed
addaleax pushed a commit that referenced this pull request Jul 11, 2017
@tniessen @addaleax
test: fix failure in test-icu-data-dir.js
6289b11 
This fixes a broken test on Windows caused by EOL conversion.

PR-URL: #13987
Refs: #13940
Refs: #13986
Reviewed-By: Refael Ackermann <refack@gmail.com>
@tniessen tniessen mentioned this pull request Jul 17, 2017
Closed
4 tasks
addaleax pushed a commit that referenced this pull request Jul 18, 2017
@tniessen @addaleax
test: fix failure in test-icu-data-dir.js
fa9e647 
This fixes a broken test on Windows caused by EOL conversion.

PR-URL: #13987
Refs: #13940
Refs: #13986
Reviewed-By: Refael Ackermann <refack@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers
@refack refack refack approved these changes
@joaocgreis joaocgreis joaocgreis approved these changes
@evanlucas evanlucas evanlucas approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@tniessen tniessen

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@tniessen @evanlucas @mscdex @refack @joaocgreis @MylesBorins @nodejs-github-bot