FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: fix test-cli-syntax assertions on windows by not-an-aardvark · Pull Request #12212 · nodejs/node · GitHub

/ node Public

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
ConversationCommits (1)ChecksFiles changed
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 commented Apr 4, 2017

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 added the test Issues and PRs related to the tests. label Apr 4, 2017

not-an-aardvark commented Apr 4, 2017

Copy link
Copy Markdown
Contributor Author

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

not-an-aardvark mentioned this pull request Apr 4, 2017
Closed
3 tasks

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 commented Apr 4, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

I hate the \r\n EOF mess.

bzoz commented Apr 4, 2017

Copy link
Copy Markdown
Contributor

LGTM if the CI is green

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 commented Apr 4, 2017 •
edited
Loading

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.

test: fix test-cli-syntax assertions on windows
9348f31 
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 merged commit 9348f31 into nodejs:master Apr 4, 2017

not-an-aardvark commented Apr 4, 2017

Copy link
Copy Markdown
Contributor Author

Landed in 9348f31

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

italoacasas commented Apr 10, 2017

Copy link
Copy Markdown

cc @not-an-aardvark

not-an-aardvark commented Apr 10, 2017

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.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
Trott Trott approved these changes
+1 more reviewer
refack refack approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL