Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

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

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.

Sign up for GitHub

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

Jump to bottom
Closed
tniessen wants to merge 3 commits into from
Closed

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

tniessen wants to merge 3 commits into from

Conversation

@tniessen
Copy link
Member

@tniessen tniessen commented Jun 29, 2017 •
edited
Loading

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
Copy link
Member Author

tniessen commented Jun 29, 2017

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

@evanlucas
Copy link
Contributor

evanlucas commented Jun 29, 2017

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

@mscdex
Copy link
Contributor

mscdex commented Jun 29, 2017

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

@tniessen
Copy link
Member Author

tniessen commented Jun 29, 2017 •
edited
Loading

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
Copy link
Member Author

tniessen commented Jun 29, 2017 •
edited
Loading

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
Copy link
Member Author

tniessen commented Jun 29, 2017

@mscdex PTAL

@mscdex
Copy link
Contributor

mscdex commented Jun 29, 2017

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

@mscdex
Copy link
Contributor

mscdex commented Jun 29, 2017

It appears this already landed in a1ecdcf.

@mscdex mscdex closed this Jun 29, 2017
@tniessen
Copy link
Member Author

tniessen commented Jun 29, 2017 •
edited
Loading

Landed in a1ecdcf.

Windows CI: https://ci.nodejs.org/job/node-test-commit-windows-fanned/10072/
Linux One CI: https://ci.nodejs.org/job/node-test-commit-linuxone/6941/

@mscdex Yes, I pushed it seconds ago, after going through all the CI failures and making sure they were unrelated. (I just rebased it an hour ago, I didn't land it back then.)

@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
Morty Proxy This is a proxified and sanitized view of the page, visit original site.