-
Notifications
You must be signed in to change notification settings - Fork 3
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
The "exit" event will be emitted before written to stdout/stderr. #25
Comments
This is actually very much intended, for this specific reason. It's meant to help folks weed out bugs in their code from assuming that stdio streams are completely done when the exit event fires. Yes, in your tests it happens to fire after stdout is written to, but that's not guaranteed. The docs for node's child_process explain further
So, the 'close' event is the one your code needs to listen to from a child process if it wants to guarantee that the stdio streams are closed and ready to evaluate. The github offices are closed next week, but when we're all back I'll dig into the |
I took a peek real quick at the actions code and it does look like it makes a distinction between https://github.com/actions/toolkit/blob/main/packages/exec/src/toolrunner.ts#L517-L523 cp.on('close', (code: number) => {
state.processExitCode = code
state.processExited = true
state.processClosed = true
this._debug(`STDIO streams have closed for tool '${this.toolPath}'`)
state.CheckComplete()
}) I'm going to close this since it doesn't look like there's anything that needs to happen either in spawk or the github action code. |
After investigating, I found out the cause of my mistake. I was using Jest to run the tests with the However, the In addition, it turned out that lines with a newline code at the end were displayed in the Jest results. spawk.spawn('/path/to/git', ['diff', '--name-only'])
.stdout(
'file1\n'
+ 'file2'
)
So, I tested with Node only, without Jest, and all lines were displayed correctly. $ node
Welcome to Node.js v12.19.0.
Type ".help" for more information.
> const spawk = require('spawk')
undefined
> const { exec } = require('@actions/exec')
undefined
> spawk.preventUnmatched()
undefined
> void spawk.spawn('/path/to/git', ['diff', '--name-only']).stdout('file1\nfile2')
undefined
> void exec('git diff --name-only').then(v => console.log({v})).catch(e => console.error({e}))
undefined
> [command]/path/to/git diff --name-only
file1
file2{ v: 0 }
> spawk.clean()
undefined
> void spawk.spawn('/path/to/git', ['diff', '--name-only']).stdout('file1\nfile2\n')
undefined
> void exec('git diff --name-only').then(v => console.log({v})).catch(e => console.error({e}))
undefined
> [command]/path/to/git diff --name-only
file1
file2
{ v: 0 } From this, it seems that the cause is probably in the display process of Jest. To summarize, there was no bug in |
Thank you for that thorough follow up. It does look like jest only ever listens for either a child.on('message', this._onMessage.bind(this));
child.on('exit', this._onExit.bind(this)); Because the stderr and stdout are things that ultimately can be overridden via forkOptions this does feel like a bug. I am not familiar enough with the source code of jest, or what level of support they intend to make for overriding stdio like that, so I can't say for sure. |
Currently,
[email protected]
emits anexit
event and then writes to stdout and stderr.spawk/lib/interceptor.js
Lines 287 to 299 in 22a4d5b
However, when I tried it with Node.js 12.19.0 and Node.js 16.0.0, the
exit
event was emitted after it was written to stdout.Due to this difference in behavior,
@actions/[email protected]
is unable to get the contents of stdout.I believe this is a bug in spawk, not in @actions/exec.
The text was updated successfully, but these errors were encountered: