Fix issue #127 - calling pipe of resolved ProcessPromise should throw Error instead of producing empty output
#129
Conversation
…ted `ProcessPromise`
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
|
@googlebot I fixed it. |
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
test.mjs
Outdated
| console.log("This was expected", e) | ||
| } | ||
| assert(processOutput.exitCode === 0) | ||
| assert(processOutput.stdout === '') // this is unexpected - intuitively would expect "Hello" |
There was a problem hiding this comment.
This wasn't intuitive for me (maybe only because i'm not very familiar with NodeJS and how pipes work in there)
| assert(processOutput.stdout === '') // this is unexpected - intuitively would expect "Hello" | ||
| assert(processOutput.stderr === '') | ||
| if (processOutput) { | ||
| assert.fail('Expected failure, but got ' + processOutput[inspect.custom]()) |
There was a problem hiding this comment.
this new (changed behaviour) fail-fast approach makes more sense to me - would have saved me from a bit of debugging
atsu85
force-pushed
the
issue-127-pipe-with-resolved-promise-should-throw
branch
from
67f542f to
07bbcd9
Compare
|
@googlebot I fixed it. |
|
Something failing. |
|
Fixed in 423d77d |
|
thanks, @antonmedv! I missed these lines from my original implementation that caused test suit to remain running (because of child process) even after the test completed successfully |
Fixes #127 using fail-fast approach - if
pipeis called on resolved/awaitedProcessPromise, then throwErrorinstead of producing empty output