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

chore: bump CI to run against node v10 #856

Closed
wants to merge 1 commit into from
Closed

Conversation

nfischer
Copy link
Member

@nfischer nfischer commented Jun 8, 2018

This adds node v10 to both Travis and Appveyor.

This adds node v10 to both Travis and Appveyor.
@nfischer nfischer added the chore label Jun 8, 2018
@nfischer nfischer requested a review from freitagbr June 8, 2018 06:55
@nfischer
Copy link
Member Author

nfischer commented Jun 8, 2018

Well, I'm confused about maxBuffer now. Even when it triggers shell.error() on pre-v10, exec() still seems to work despite exceeding the buffer...

> s.exec('echo 0123456789', { maxBuffer: 6 }) // How can this have correct output??
0123456789
{ [String: '0123456789\n']
  stdout: '0123456789\n',
  stderr: '',
  code: 0,
  cat: [Function: bound ],
  exec: [Function: bound ],
  grep: [Function: bound ],
  head: [Function: bound ],
  sed: [Function: bound ],
  sort: [Function: bound ],
  tail: [Function: bound ],
  to: [Function: bound ],
  toEnd: [Function: bound ],
  uniq: [Function: bound ] }

@nfischer nfischer self-assigned this Jun 8, 2018
@nfischer
Copy link
Member Author

nfischer commented Jun 8, 2018

Maybe maxBuffer behavior changed during the exec() refactors around v0.8, and we got lucky by not breaking the test? I'll take a look.

freitagbr
freitagbr previously approved these changes Jun 21, 2018
@freitagbr
Copy link
Contributor

Woops, got a bit eager with the approve button.

I did some investigating, and it turns out that in v10, using child_process.exec and piping the child process' stdout to the parent process' stdout will always work, regardless of the value of maxBuffer. maxBuffer only applies to the data saved in the buffer.

If maxBuffer is exceeded, the callback to child_process.exec will be called with this error: RangeError [ERR_CHILD_PROCESS_STDIO_MAXBUFFER]: stdout maxBuffer length exceeded. But the data will still go to stdout.

@freitagbr freitagbr dismissed their stale review June 21, 2018 03:20

Accidental approval

@nfischer
Copy link
Member Author

If maxBuffer is exceeded, the callback to child_process.exec will be called with this error: RangeError [ERR_CHILD_PROCESS_STDIO_MAXBUFFER]: stdout maxBuffer length exceeded. But the data will still go to stdout.

Ack. It sounds like we need to rewrite the test, and to also modify exec() to expect that error and surface that information to the user.

@nfischer nfischer added medium priority fix Bug/defect, or a fix for such a problem labels Nov 21, 2018
@nfischer
Copy link
Member Author

nfischer commented Dec 2, 2018

Blocked by #915

@nfischer
Copy link
Member Author

nfischer commented Dec 3, 2018

Close in favor of #921

@nfischer nfischer closed this Dec 3, 2018
@nfischer nfischer deleted the chore-bump-ci branch December 3, 2018 07:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked chore fix Bug/defect, or a fix for such a problem medium priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants