From e85bac0d6b7fd1c8100d14c0210e16a4324fba76 Mon Sep 17 00:00:00 2001 From: Maksim Gruzdev Date: Mon, 5 Jan 2026 17:13:05 +0300 Subject: [PATCH] #123 extract ShellCommandLogged, use on 'close' event to prevent situation where stderr not flushed --- lib/GitBranch.js | 5 +-- lib/GitOld.js | 2 +- lib/GitRepo.js | 2 +- lib/RunCommand.js | 9 +++-- lib/ShellCommand.js | 41 ++++------------------ lib/ShellCommandLogged.js | 68 +++++++++++++++++++++++++++++++++++++ mr.js | 4 +++ tests/GitBranch.js | 2 +- tests/GitRepo.js | 2 +- tests/MergeCommand.js | 3 +- tests/MrCommand.js | 3 +- tests/ShellCommand.js | 23 +++++-------- tests/ShellCommandLogged.js | 50 +++++++++++++++++++++++++++ 13 files changed, 152 insertions(+), 62 deletions(-) create mode 100644 lib/ShellCommandLogged.js create mode 100644 tests/ShellCommandLogged.js diff --git a/lib/GitBranch.js b/lib/GitBranch.js index 21430ac..042ea72 100644 --- a/lib/GitBranch.js +++ b/lib/GitBranch.js @@ -1,4 +1,5 @@ const ShellCommand = require ('./ShellCommand') +const ShellCommandLogged = require ('./ShellCommandLogged') module.exports = class GitBranch { @@ -23,7 +24,7 @@ module.exports = class GitBranch { async push () { for (let i of await this.pushTodo ()) { - await ShellCommand.withText(i).run () + await ShellCommandLogged.withText(i).run () } } @@ -109,6 +110,6 @@ module.exports = class GitBranch { } async run (cmd) { - return ShellCommand.withText(cmd).runSilent () + return ShellCommand.withText(cmd).run () } } diff --git a/lib/GitOld.js b/lib/GitOld.js index 81793a9..19071a3 100644 --- a/lib/GitOld.js +++ b/lib/GitOld.js @@ -19,7 +19,7 @@ module.exports = class OldGit { async translate (o) { if (!o.version) { - let v = await ShellCommand.withText (`git --version`).runSilent () + let v = await ShellCommand.withText (`git --version`).run () o.version = v.split ('git version')[1] } diff --git a/lib/GitRepo.js b/lib/GitRepo.js index 1df69b0..270b5b7 100644 --- a/lib/GitRepo.js +++ b/lib/GitRepo.js @@ -79,6 +79,6 @@ module.exports = class { } async run (cmd) { - return ShellCommand.withText (cmd).runSilent () + return ShellCommand.withText (cmd).run () } } diff --git a/lib/RunCommand.js b/lib/RunCommand.js index e9ebd06..487d517 100644 --- a/lib/RunCommand.js +++ b/lib/RunCommand.js @@ -1,5 +1,5 @@ -const ShellCommand = require ('./ShellCommand') +const ShellCommandLogged = require ('./ShellCommandLogged') const UserInput = require ('./UserInput') module.exports = class { @@ -32,7 +32,7 @@ module.exports = class { for (let idx = 0; idx < this.todo.length; idx++) { let i = this.todo [idx] - let cmd = ShellCommand.withText (i) + let cmd = ShellCommandLogged.withText (i) try { await cmd.run () @@ -44,11 +44,10 @@ module.exports = class { async fallback (idx, x) { let tail = await this.print (idx) - let xx = x.message if (tail) { - xx = xx + `\nFix above and then run rest of the plan:\n${tail}\n` + x.message = x.message + `\nFix above and then run rest of the plan:\n${tail}\n` } - throw new Error (xx) + throw x } async print (idx) { diff --git a/lib/ShellCommand.js b/lib/ShellCommand.js index 6a0ef7c..a4d4263 100644 --- a/lib/ShellCommand.js +++ b/lib/ShellCommand.js @@ -1,54 +1,39 @@ const { spawn } = require('child_process') -// @todo #0:30m decompose -// runSilent go to ShellCommand -// run go to ShellCommandLogged module.exports = class ShellCommand { static withText (cmd) { return new ShellCommand ({ cmd, spawn }) } - constructor(o) { + constructor (o) { this.cwd = o.cwd this.cmd = o.cmd this.spawn = o.spawn } async run () { - let cwdLabel = this.cwd ? ` (${this.cwd})` : '' - this.log(`${cwdLabel} > ${await this.print ()}`) - return this.runSilent({log_flow: true}) - } - - async runSilent (options = {}) { - const {log_flow} = options if (global.FUZZ) { - // @todo #120:1h move to injected ShellCommandFuzz return global.FUZZ_SHELL_REPLY } + return new Promise((ok, fail) => { - const cmd = 'sh' + const cmd = 'sh' const args = ['-c', this.cmd] - - const env = { ...process.env, - TERM: 'xterm-256color' - } - const o = {env} - const subprocess = this.spawn(cmd, args, o) + const env = { ...process.env, TERM: 'xterm-256color' } + const subprocess = this.spawn(cmd, args, { env }) let stdout = '', stderr = '' + subprocess.stdout.on('data', (data) => { - if (log_flow) process.stdout.write(data) stdout = stdout + data }) subprocess.stderr.on('data', (data) => { - if (log_flow) process.stdout.write(data) stderr = stderr + data }) - subprocess.on('exit', (code) => { + subprocess.on('close', (code) => { if (code === 0) { ok(stdout.toString().trim()) } else { @@ -62,16 +47,4 @@ module.exports = class ShellCommand { }) } - - async print () { - return this.cmd - } - - log (label) { - if (global.FUZZ) { - return '' - } - console.log (label) - } - } diff --git a/lib/ShellCommandLogged.js b/lib/ShellCommandLogged.js new file mode 100644 index 0000000..2003cb7 --- /dev/null +++ b/lib/ShellCommandLogged.js @@ -0,0 +1,68 @@ +const { spawn } = require('child_process') + +module.exports = class ShellCommandLogged { + + static withText (cmd) { + return new ShellCommandLogged ({ cmd, spawn }) + } + + constructor (o) { + this.cwd = o.cwd + this.cmd = o.cmd + this.spawn = o.spawn + } + + async run () { + let cwdLabel = this.cwd ? ` (${this.cwd})` : '' + this.log(`${cwdLabel} > ${await this.print()}`) + + if (global.FUZZ) { + return global.FUZZ_SHELL_REPLY + } + + return new Promise((ok, fail) => { + const cmd = 'sh' + const args = ['-c', this.cmd] + const env = { ...process.env, TERM: 'xterm-256color' } + const subprocess = this.spawn(cmd, args, { env }) + + let stdout = '', stderr = '' + + subprocess.stdout.on('data', (data) => { + process.stdout.write(data) + stdout = stdout + data + }) + + subprocess.stderr.on('data', (data) => { + process.stdout.write(data) + stderr = stderr + data + }) + + subprocess.on('close', (code) => { + if (code === 0) { + ok(stdout.toString().trim()) + } else { + const err = new Error(stderr.toString().trim()) + err.toString = function () { return '' } + fail(err) + } + }) + + subprocess.on('error', (error) => { + fail(new Error(error.toString().trim())) + }) + }) + } + + async print () { + return this.cmd + } + + log (label) { + if (global.FUZZ) { + return '' + } + console.log(label) + } + +} diff --git a/mr.js b/mr.js index d2442d3..8258af1 100644 --- a/mr.js +++ b/mr.js @@ -35,6 +35,10 @@ if (!nodeVersion.isValid()) { try { await main (process.argv.slice (2)) } catch (x) { + const msg = x.toString().trim() + if (msg) { + console.error(msg) + } process.exit(1) } diff --git a/tests/GitBranch.js b/tests/GitBranch.js index 2259922..4c567e8 100644 --- a/tests/GitBranch.js +++ b/tests/GitBranch.js @@ -30,7 +30,7 @@ describe('git branch equals', () => { }) -mock.method(ShellCommand.prototype, 'runSilent', function () { +mock.method(ShellCommand.prototype, 'run', function () { switch (this.cmd) { case 'git log --reverse --pretty=format:%s origin/master..TASK-42': return 'TASK-42 commit msg' diff --git a/tests/GitRepo.js b/tests/GitRepo.js index aba5e2c..6292e71 100644 --- a/tests/GitRepo.js +++ b/tests/GitRepo.js @@ -6,7 +6,7 @@ const GitRepo = require ('../lib/GitRepo') describe('random input', () => { - mock.method(ShellCommand.prototype, 'runSilent', function () { + mock.method(ShellCommand.prototype, 'run', function () { switch (this.cmd) { case 'git log --oneline origin/main..origin/main': return '' diff --git a/tests/MergeCommand.js b/tests/MergeCommand.js index 79a5af7..f9ea6ae 100644 --- a/tests/MergeCommand.js +++ b/tests/MergeCommand.js @@ -1,6 +1,7 @@ const {describe, it, mock} = require ('node:test') const assert = require ('assert') const ShellCommand = require ('../lib/ShellCommand') +const ShellCommandLogged = require ('../lib/ShellCommandLogged') const GitRepo = require ('../lib/GitRepo') const GitBranch = require ('../lib/GitBranch') const MergeCommand = require ('../lib/MergeCommand') @@ -26,7 +27,7 @@ describe('MergeCommand', async () => { } mock.method(ShellCommand.prototype, 'run', f) - mock.method(ShellCommand.prototype, 'runSilent', f) + mock.method(ShellCommandLogged.prototype, 'run', f) mock.method(GitBranch.prototype, 'isOriginGitlab', function () { return false diff --git a/tests/MrCommand.js b/tests/MrCommand.js index 304c042..2d85764 100644 --- a/tests/MrCommand.js +++ b/tests/MrCommand.js @@ -1,6 +1,7 @@ const {describe, it, mock} = require ('node:test') const assert = require ('assert') const ShellCommand = require ('../lib/ShellCommand') +const ShellCommandLogged = require ('../lib/ShellCommandLogged') const GitRepo = require ('../lib/GitRepo') const GitBranch = require ('../lib/GitBranch') const GitOld = require ('../lib/GitOld') @@ -28,7 +29,7 @@ describe('random input', () => { } mock.method(ShellCommand.prototype, 'run', f) - mock.method(ShellCommand.prototype, 'runSilent', f) + mock.method(ShellCommandLogged.prototype, 'run', f) mock.method(GitBranch.prototype, 'isOriginGitlab', function () { return false diff --git a/tests/ShellCommand.js b/tests/ShellCommand.js index bc7d9bd..d366028 100644 --- a/tests/ShellCommand.js +++ b/tests/ShellCommand.js @@ -3,7 +3,7 @@ const assert = require ('assert') const ShellCommand = require ('../lib/ShellCommand') -describe('random input', () => { +describe('ShellCommand', () => { const makeSpawnMock = (o = {}) => { @@ -25,8 +25,8 @@ describe('random input', () => { }, }, on: (event, callback) => { - if (event === 'exit') { - return callback(o.exit) + if (event === 'close') { + return callback(o.close) } }, }) @@ -43,26 +43,19 @@ describe('random input', () => { }) it ('exec ok', async (t) => { - const spawn = makeSpawnMock ({stdout: '', stderr: '', exit: 0}) - assert.strictEqual(await (new ShellCommand ({cmd: 'git fetch', spawn}).runSilent ()), '') + const spawn = makeSpawnMock ({stdout: '', stderr: '', close: 0}) + assert.strictEqual(await (new ShellCommand ({cmd: 'git fetch', spawn}).run ()), '') }) it ('exec fail', async (t) => { - const spawn = makeSpawnMock ({stdout: '', stderr: 'not a repo', exit: -127}) - assert.rejects(new ShellCommand ({cmd: 'git fetch', spawn}).runSilent (), {message: 'not a repo'}) + const spawn = makeSpawnMock ({stdout: '', stderr: 'not a repo', close: -127}) + assert.rejects(new ShellCommand ({cmd: 'git fetch', spawn}).run (), {message: 'not a repo'}) }) - it ('git status fuzz', async (t) => { + it ('fuzz', async (t) => { global.FUZZ = 1 global.FUZZ_SHELL_REPLY = 'fake ok' assert.strictEqual(await (new ShellCommand ({cmd: 'git status'}).run ()), 'fake ok') global.FUZZ = 0 }) - - it ('git status silent fuzz', async (t) => { - global.FUZZ = 1 - global.FUZZ_SHELL_REPLY = 'fake ok' - assert.strictEqual(await (new ShellCommand ({cmd: 'git status'}).runSilent ()), 'fake ok') - global.FUZZ = 0 - }) }) diff --git a/tests/ShellCommandLogged.js b/tests/ShellCommandLogged.js new file mode 100644 index 0000000..2fb6d16 --- /dev/null +++ b/tests/ShellCommandLogged.js @@ -0,0 +1,50 @@ +const {describe, it} = require ('node:test') +const assert = require ('assert') +const ShellCommandLogged = require ('../lib/ShellCommandLogged') + +describe('ShellCommandLogged', () => { + + const makeSpawnMock = (o = {}) => { + const {stdout, stderr, close} = o + return (command, args, options) => ({ + stdout: { + on: (event, callback) => { + if (event === 'data') { + return callback(`${stdout}\n`) + } + }, + }, + stderr: { + on: (event, callback) => { + if (event === 'data') { + return callback(`${stderr}\n`) + } + }, + }, + on: (event, callback) => { + if (event === 'close') { + return callback(o.close) + } + }, + }) + } + + it ('fuzz', async (t) => { + global.FUZZ = 1 + global.FUZZ_SHELL_REPLY = 'fake ok' + assert.strictEqual(await (new ShellCommandLogged ({cmd: 'git status'}).run ()), 'fake ok') + global.FUZZ = 0 + }) + + it ('#113 streamed jest output not duplicated on failure', async (t) => { + const longOutput = Array(10).fill('PASS tests/GitBranch.js').join('\n') + '\nTests: 10 passed, 10 total' + const spawn = makeSpawnMock ({stdout: '', stderr: longOutput, close: 1}) + try { + await new ShellCommandLogged ({cmd: 'npm test', spawn}).run () + assert.fail('should throw') + } catch (err) { + assert.ok(err.message.includes('PASS')) + assert.strictEqual(err.toString(), '') + } + }) +})