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: fix failure in test-icu-data-dir.js #13987

Closed
wants to merge 3 commits into from

Conversation

tniessen
Copy link
Member

@tniessen tniessen commented Jun 29, 2017

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

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

@evanlucas
Copy link
Contributor

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

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

@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

@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

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
addaleax pushed a commit that referenced this pull request Jun 29, 2017
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>
addaleax pushed a commit that referenced this pull request Jul 11, 2017
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>
addaleax pushed a commit that referenced this pull request Jul 18, 2017
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
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
Development

Successfully merging this pull request may close these issues.

7 participants