Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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>
# for free to join this conversation on GitHub. Already have an account? # 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