Skip to content

test: fix test-cli-syntax assertions on windows#12212

Merged
not-an-aardvark merged 1 commit into
nodejs:masterfrom
not-an-aardvark:fix-windows-cli-test
Apr 4, 2017
Merged

test: fix test-cli-syntax assertions on windows#12212
not-an-aardvark merged 1 commit into
nodejs:masterfrom
not-an-aardvark:fix-windows-cli-test

Conversation

@not-an-aardvark

Copy link
Copy Markdown
Contributor

The test introduced in a5f91ab accidentally introduced failures on some windows builds. Update the assertion that was causing the failures.

Ref: #11689

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

test

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Apr 4, 2017
@not-an-aardvark

Copy link
Copy Markdown
Contributor Author

@gibfahn

gibfahn commented Apr 4, 2017

Copy link
Copy Markdown
Member

We should land this ASAP (once CI passes) to fix CI on windows master.

cc/ @nodejs/platform-windows

@refack

refack commented Apr 4, 2017

Copy link
Copy Markdown
Contributor

I hate the \r\n EOF mess.

@bzoz

bzoz commented Apr 4, 2017

Copy link
Copy Markdown
Contributor

LGTM if the CI is green

@Trott

Trott commented Apr 4, 2017

Copy link
Copy Markdown
Member

This LGTM as-is, although I think I'd prefer assert.strictEqual(c.stderr.trim(), ...);. That would provide a more helpful message in the event that the assertion fires.

@not-an-aardvark

not-an-aardvark commented Apr 4, 2017

Copy link
Copy Markdown
Contributor Author

@Trott I agree that would have been better, but for now I think I'm going to land this as-is so that the build on master can be fixed ASAP. I can follow up with another PR to improve the error message.

The test introduced in a5f91ab
accidentally introduced failures on some windows builds. Update the
assertion that was causing the failures.

PR-URL: nodejs#12212
Ref: nodejs#11689
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@not-an-aardvark not-an-aardvark merged commit 9348f31 into nodejs:master Apr 4, 2017
@not-an-aardvark

Copy link
Copy Markdown
Contributor Author

Landed in 9348f31

@not-an-aardvark not-an-aardvark deleted the fix-windows-cli-test branch April 4, 2017 16:16
@gibfahn gibfahn mentioned this pull request Apr 4, 2017
4 tasks
@jasnell jasnell mentioned this pull request Apr 4, 2017
@italoacasas

Copy link
Copy Markdown

cc @not-an-aardvark

@not-an-aardvark

Copy link
Copy Markdown
Contributor Author

This fixes a bug that was introduced in a semver-major commit -- it doesn't seem like it makes sense to backport.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test Issues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants