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 test-cli-syntax assertions on windows #12212

Merged

Conversation

@not-an-aardvark
Copy link
Member

commented Apr 4, 2017

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

@not-an-aardvark

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2017

@not-an-aardvark not-an-aardvark referenced this pull request Apr 4, 2017
3 of 3 tasks complete
@gibfahn

This comment has been minimized.

Copy link
Member

commented Apr 4, 2017

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

cc/ @nodejs/platform-windows

@refack

This comment has been minimized.

Copy link
Member

commented Apr 4, 2017

I hate the \r\n EOF mess.

@refack
refack approved these changes Apr 4, 2017
@bzoz

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2017

LGTM if the CI is green

@jasnell
jasnell approved these changes Apr 4, 2017
@Trott

This comment has been minimized.

Copy link
Member

commented Apr 4, 2017

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.

@Trott
Trott approved these changes Apr 4, 2017
@not-an-aardvark

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2017

@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.

test: fix test-cli-syntax assertions on windows
The test introduced in a5f91ab
accidentally introduced failures on some windows builds. Update the
assertion that was causing the failures.

PR-URL: #12212
Ref: #11689
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>

@not-an-aardvark not-an-aardvark force-pushed the not-an-aardvark:fix-windows-cli-test branch to 9348f31 Apr 4, 2017

@not-an-aardvark not-an-aardvark merged commit 9348f31 into nodejs:master Apr 4, 2017

@not-an-aardvark

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2017

Landed in 9348f31

@not-an-aardvark not-an-aardvark deleted the not-an-aardvark:fix-windows-cli-test branch Apr 4, 2017

@gibfahn gibfahn referenced this pull request Apr 4, 2017
4 of 4 tasks complete
@jasnell jasnell referenced this pull request Apr 4, 2017
@italoacasas

This comment has been minimized.

Copy link
Member

commented Apr 10, 2017

@not-an-aardvark

This comment has been minimized.

Copy link
Member Author

commented Apr 10, 2017

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
Projects
None yet
8 participants
You can’t perform that action at this time.