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

test: add known_issues test for GH-2148 by Trott · Pull Request #5920 · nodejs/node · GitHub

/ node Public

test: add known_issues test for GH-2148 - #5920

Closed
Trott wants to merge 3 commits into
nodejs:masterfrom
Trott:known-2148
ConversationCommits (3)ChecksFiles changed
Closed

test: add known_issues test for GH-2148#5920
Trott wants to merge 3 commits into
nodejs:masterfrom
Trott:known-2148

Conversation

Trott commented Mar 27, 2016

Copy link
Copy Markdown
Member

Pull Request check-list

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to [CONTRIBUTING.md][0]?
  • If this change fixes a bug (or a performance problem), is a regression
    test (or a benchmark) included?
  • Is a documentation update included (if this change modifies
    existing APIs, or introduces new ones)?

Affected core subsystem(s)

test, process

Description of change

test: add known_issues test for GH-2148

Refs: #2148

Trott added process Issues and PRs related to the process subsystem. known issue test labels Mar 27, 2016
Trott mentioned this pull request Mar 27, 2016
Closed
mscdex reviewed Mar 28, 2016
View reviewed changes
Comment thread test/known_issues/test-stdout-buffer-flush-on-exit.js Outdated

mscdex Mar 28, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Won't this technically be + 2 on Windows?

Trott Mar 28, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Probably. Good point. Maybe the thing to do is compare stdout.trim().length in the assertion so as to avoid any assumptions whatsoever about the line separator?

mscdex Mar 28, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yeah, and we may want to compare the (trimmed) output instead of comparing the lengths just to be extra sure we get what we expect.

Trott Mar 28, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

OK, done. PTAL

mscdex reviewed Mar 28, 2016
View reviewed changes
Comment thread test/known_issues/test-stdout-buffer-flush-on-exit.js Outdated
}

const stdout = execSync(`${process.execPath} ${__filename} child`)
.toString()

mscdex Mar 28, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

nit: maybe these would look a little less oddly placed if they were lined up with the first e in execSync()?

Fishrock123 Mar 28, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Maybe just do this?

const argv = `${process.execPath} ${__filename} child`

Trott Mar 28, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks! Nit addressed.

mscdex commented Mar 28, 2016

Copy link
Copy Markdown
Contributor

One nit but otherwise LGTM.

Trott commented Mar 30, 2016

Copy link
Copy Markdown
Member Author

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

Trott added a commit to Trott/io.js that referenced this pull request Mar 31, 2016
test: add known_issues test for nodejsGH-2148
33c27f8 
PR-URL: nodejs#5920
Refs: nodejs#2148
Reviewed-By: Brian White <mscdex@mscdex.net>

Trott commented Mar 31, 2016

Copy link
Copy Markdown
Member Author

Landed in 33c27f8

Trott closed this Mar 31, 2016
evanlucas pushed a commit that referenced this pull request Mar 31, 2016
test: add known_issues test for GH-2148
8e8768e 
PR-URL: #5920
Refs: #2148
Reviewed-By: Brian White <mscdex@mscdex.net>
evanlucas mentioned this pull request Mar 31, 2016
Closed
evanlucas pushed a commit that referenced this pull request Mar 31, 2016
test: add known_issues test for GH-2148
c5c7ae8 
PR-URL: #5920
Refs: #2148
Reviewed-By: Brian White <mscdex@mscdex.net>
MylesBorins pushed a commit that referenced this pull request Apr 11, 2016
test: add known_issues test for GH-2148
fe0233b 
PR-URL: #5920
Refs: #2148
Reviewed-By: Brian White <mscdex@mscdex.net>
MylesBorins mentioned this pull request Apr 11, 2016
Merged
Trott deleted the known-2148 branch January 13, 2022 22:42
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

No reviews

Assignees

No one assigned

Labels

process Issues and PRs related to the process subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL