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: add known_issues test for GH-2148 #5920

Closed
wants to merge 3 commits into from
Closed

Conversation

Trott
Copy link
Member

@Trott Trott commented Mar 27, 2016

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 Trott added process Issues and PRs related to the process subsystem. known issue test labels Mar 27, 2016
const execSync = require('child_process').execSync;

const longLine = 'foo bar baz quux quuz aaa bbb ccc'.repeat(80);
const expectedLength = (longLine.length * 999) + 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't this technically be + 2 on Windows?

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, done. PTAL

}

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just do this?

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! Nit addressed.

@mscdex
Copy link
Contributor

mscdex commented Mar 28, 2016

One nit but otherwise LGTM.

@Trott
Copy link
Member Author

Trott commented Mar 30, 2016

Trott added a commit to Trott/io.js that referenced this pull request Mar 31, 2016
PR-URL: nodejs#5920
Refs: nodejs#2148
Reviewed-By: Brian White <mscdex@mscdex.net>
@Trott
Copy link
Member Author

Trott commented Mar 31, 2016

Landed in 33c27f8

@Trott Trott closed this Mar 31, 2016
evanlucas pushed a commit that referenced this pull request Mar 31, 2016
PR-URL: #5920
Refs: #2148
Reviewed-By: Brian White <mscdex@mscdex.net>
evanlucas pushed a commit that referenced this pull request Mar 31, 2016
PR-URL: #5920
Refs: #2148
Reviewed-By: Brian White <mscdex@mscdex.net>
MylesBorins pushed a commit that referenced this pull request Apr 11, 2016
PR-URL: #5920
Refs: #2148
Reviewed-By: Brian White <mscdex@mscdex.net>
@MylesBorins MylesBorins mentioned this pull request Apr 11, 2016
@Trott Trott deleted the known-2148 branch January 13, 2022 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
process Issues and PRs related to the process subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants