diff --git a/src/model/system/system.integration.test.ts b/src/model/system/system.integration.test.ts index da623a64..894f02d4 100644 --- a/src/model/system/system.integration.test.ts +++ b/src/model/system/system.integration.test.ts @@ -18,8 +18,16 @@ describe('System', () => { expect(result.output.trim()).toBe('test'); }); - test('throws when command writes to stderr', async () => { - await expect(System.run('echo fail >&2')).rejects.toThrow(); + test('succeeds when a command writes to stderr but exits 0', async () => { + // Was 'throws when command writes to stderr' - codified the bug + // this fixes (game-ci/cli#84): docker writes informational + // messages to stderr on otherwise-successful runs, so stderr + // content alone can't mean failure. Exit code does. + await expect(System.run('echo fail >&2')).resolves.not.toBeNull(); + }); + + test('throws when a command exits non-zero', async () => { + await expect(System.run('exit 1')).rejects.toThrow(); }); } }); diff --git a/src/model/system/system.test.ts b/src/model/system/system.test.ts index 18af0e0d..eb03d4ee 100644 --- a/src/model/system/system.test.ts +++ b/src/model/system/system.test.ts @@ -36,3 +36,34 @@ describe('System.run env option', () => { } }); }); + +describe('System.run exit-code-based failure', () => { + // Real bug (game-ci/unity-activate#111): this used to throw on any + // stderr output regardless of exit code. `docker run` writes "Unable to + // find image '...' locally" to stderr when auto-pulling, then succeeds + // with exit code 0 - which was being thrown as a fatal error anyway. + test('succeeds when stderr has output but the command exits 0', async () => { + const command = 'node -e "process.stderr.write(\'just a warning\\n\')"'; + + const result = await System.run(command, undefined, { silent: true }); + + expect(result.status?.success).toBe(true); + expect(result.error).toContain('just a warning'); + }); + + test('throws when the command exits non-zero, even with empty stderr', async () => { + // Exit code propagation through the shell wrapper (sh -c vs + // powershell -Command) isn't consistent enough across platforms to + // assert a specific code here - what matters is that a non-zero exit + // with no stderr still throws, instead of silently resolving. + const command = 'node -e "process.exit(3)"'; + + await expect(System.run(command, undefined, { silent: true })).rejects.toThrow(/Command exited with code \d+/); + }); + + test('throws with the stderr content when the command exits non-zero', async () => { + const command = 'node -e "process.stderr.write(\'boom\\n\'); process.exit(1)"'; + + await expect(System.run(command, undefined, { silent: true })).rejects.toThrow('boom'); + }); +}); diff --git a/src/model/system/system.ts b/src/model/system/system.ts index 91f7f4d8..700b563a 100644 --- a/src/model/system/system.ts +++ b/src/model/system/system.ts @@ -19,15 +19,17 @@ class System { * Run any command as if you're typing in shell. * Make sure it's Windows/MacOS/Ubuntu compatible or has alternative commands. * - * If any error is written to stderr, this method will throw them. - * new Error(stdoutErrors) + * If the command exits with a non-zero code, this method throws (message + * built from stderr, falling back to a generic "exited with code N"). + * A non-empty stderr alone is not treated as failure - many commands + * (e.g. `docker run` auto-pulling an uncached image) write informational + * output there on a genuinely successful run. * - * In case of no errors, this will return an object similar to these examples + * In case of success, this will return an object similar to these examples * { status: { success: true, code: 0 }, output: 'output from the command' } - * { status: { success: false, code: 1~255 }, output: 'output from the command' } * - * @returns {string} output of the command on success or failure - * @throws {Error} if anything was output to stderr or return code wasn't 0 + * @returns {string} output of the command on success + * @throws {Error} if the command's exit code wasn't 0 */ static async run(command: string, windowsSpecificCommand?: string, options: RunOptions = { silent: false }): Promise { let shell: string; @@ -81,13 +83,19 @@ class System { const exitCode = code ?? 1; runResult.status = { success: exitCode === 0, code: exitCode }; - if (runResult.error !== '') { - // Make sure we don't swallow any output if silent and there is an error + if (exitCode !== 0) { + // Real bug (game-ci/unity-activate#111): this used to throw on + // *any* stderr output, regardless of exit code. `docker run` on + // an image not yet cached locally writes "Unable to find image + // '...' locally" to stderr as pure status output, then pulls it + // and succeeds with exit code 0 - which this treated as a fatal + // error anyway, discarding the successful run. Exit code is the + // actual signal; stderr content is still included below for + // debugging when the command genuinely failed. const errorMessage = runResult.output && options.silent ? `${runResult.error}\n\n---\n\nOutput before the error:\n${runResult.output}` - : runResult.error; + : runResult.error || `Command exited with code ${exitCode}`; - // Throw instead of returning when any output was written to stderr reject(new Error(errorMessage)); return; }