From 083c671d988e2c05e4eb618aa5f580194f1fc216 Mon Sep 17 00:00:00 2001 From: Lucas Vieira Date: Thu, 17 Oct 2024 11:38:10 -0300 Subject: [PATCH] fix: capture stdout on the publish executor to check the error message (#252) --- .../src/executors/publish/executor.spec.ts | 172 +++++++++++++++--- .../src/executors/publish/executor.ts | 56 ++++-- packages/nx-python/src/executors/utils/cmd.ts | 56 ++++++ 3 files changed, 242 insertions(+), 42 deletions(-) create mode 100644 packages/nx-python/src/executors/utils/cmd.ts diff --git a/packages/nx-python/src/executors/publish/executor.spec.ts b/packages/nx-python/src/executors/publish/executor.spec.ts index 65c3df2..5a8063d 100644 --- a/packages/nx-python/src/executors/publish/executor.spec.ts +++ b/packages/nx-python/src/executors/publish/executor.spec.ts @@ -12,6 +12,12 @@ const nxDevkitMocks = vi.hoisted(() => { }; }); +const childProcessMocks = vi.hoisted(() => { + return { + spawn: vi.fn(), + }; +}); + vi.mock('@nx/devkit', async (importOriginal) => { const actual = await importOriginal(); return { @@ -28,11 +34,18 @@ vi.mock('fs-extra', async (importOriginal) => { }; }); +vi.mock('child_process', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + ...childProcessMocks, + }; +}); + import chalk from 'chalk'; -import '../../utils/mocks/cross-spawn.mock'; import * as poetryUtils from '../utils/poetry'; import executor from './executor'; -import spawn from 'cross-spawn'; +import { EventEmitter } from 'events'; describe('Publish Executor', () => { let checkPoetryExecutableMock: MockInstance; @@ -64,15 +77,6 @@ describe('Publish Executor', () => { .spyOn(poetryUtils, 'activateVenv') .mockReturnValue(undefined); - vi.mocked(spawn.sync).mockReturnValue({ - status: 0, - output: [''], - pid: 0, - signal: null, - stderr: null, - stdout: null, - }); - vi.spyOn(process, 'chdir').mockReturnValue(undefined); }); @@ -96,7 +100,7 @@ describe('Publish Executor', () => { const output = await executor(options, context); expect(checkPoetryExecutableMock).toHaveBeenCalled(); expect(activateVenvMock).toHaveBeenCalledWith('.'); - expect(spawn.sync).not.toHaveBeenCalled(); + expect(childProcessMocks.spawn).not.toHaveBeenCalled(); expect(output.success).toBe(false); }); @@ -112,7 +116,7 @@ describe('Publish Executor', () => { const output = await executor(options, context); expect(checkPoetryExecutableMock).toHaveBeenCalled(); expect(activateVenvMock).toHaveBeenCalledWith('.'); - expect(spawn.sync).not.toHaveBeenCalled(); + expect(childProcessMocks.spawn).not.toHaveBeenCalled(); expect(output.success).toBe(false); }); @@ -129,7 +133,7 @@ describe('Publish Executor', () => { const output = await executor(options, context); expect(checkPoetryExecutableMock).toHaveBeenCalled(); expect(activateVenvMock).toHaveBeenCalledWith('.'); - expect(spawn.sync).not.toHaveBeenCalled(); + expect(childProcessMocks.spawn).not.toHaveBeenCalled(); expect(output.success).toBe(false); }); @@ -145,13 +149,24 @@ describe('Publish Executor', () => { dryRun: false, }; + const spawnEvent = new EventEmitter(); + childProcessMocks.spawn.mockReturnValue({ + stdout: new EventEmitter(), + stderr: new EventEmitter(), + on: vi.fn().mockImplementation((event, callback) => { + spawnEvent.on(event, callback); + spawnEvent.emit('close', 0); + }), + }); + const output = await executor(options, context); expect(checkPoetryExecutableMock).toHaveBeenCalled(); expect(activateVenvMock).toHaveBeenCalledWith('.'); - expect(spawn.sync).toHaveBeenCalledWith('poetry', ['publish'], { + expect(childProcessMocks.spawn).toHaveBeenCalledWith('poetry publish', { cwd: 'tmp', - shell: false, - stdio: 'inherit', + env: { ...process.env, FORCE_COLOR: 'true' }, + shell: true, + stdio: ['inherit', 'pipe', 'pipe'], }); expect(output.success).toBe(true); expect(nxDevkitMocks.runExecutor).toHaveBeenCalledWith( @@ -181,16 +196,26 @@ describe('Publish Executor', () => { __unparsed__: ['-vvv', '--dry-run'], }; + const spawnEvent = new EventEmitter(); + childProcessMocks.spawn.mockReturnValue({ + stdout: new EventEmitter(), + stderr: new EventEmitter(), + on: vi.fn().mockImplementation((event, callback) => { + spawnEvent.on(event, callback); + spawnEvent.emit('close', 0); + }), + }); + const output = await executor(options, context); expect(checkPoetryExecutableMock).toHaveBeenCalled(); expect(activateVenvMock).toHaveBeenCalledWith('.'); - expect(spawn.sync).toHaveBeenCalledWith( - 'poetry', - ['publish', '-vvv', '--dry-run'], + expect(childProcessMocks.spawn).toHaveBeenCalledWith( + 'poetry publish -vvv --dry-run', { cwd: 'tmp', - shell: false, - stdio: 'inherit', + env: { ...process.env, FORCE_COLOR: 'true' }, + shell: true, + stdio: ['inherit', 'pipe', 'pipe'], }, ); expect(output.success).toBe(true); @@ -207,4 +232,107 @@ describe('Publish Executor', () => { ); expect(fsExtraMocks.removeSync).toHaveBeenCalledWith('tmp'); }); + + it('should run poetry publish and not throw an exception when the message contains "File already exists"', async () => { + nxDevkitMocks.runExecutor.mockResolvedValueOnce([ + { success: true, buildFolderPath: 'tmp' }, + ]); + fsExtraMocks.removeSync.mockReturnValue(undefined); + + const options = { + buildTarget: 'build', + dryRun: false, + silent: false, + }; + + const spawnEvent = new EventEmitter(); + const stdoutEvent = new EventEmitter(); + childProcessMocks.spawn.mockReturnValue({ + stdout: { + on: vi.fn().mockImplementation((event, callback) => { + stdoutEvent.on(event, callback); + stdoutEvent.emit(event, 'HTTP Error 400: File already exists'); + }), + }, + stderr: new EventEmitter(), + on: vi.fn().mockImplementation((event, callback) => { + spawnEvent.on(event, callback); + spawnEvent.emit('close', 1); + }), + }); + + const output = await executor(options, context); + expect(checkPoetryExecutableMock).toHaveBeenCalled(); + expect(activateVenvMock).toHaveBeenCalledWith('.'); + expect(childProcessMocks.spawn).toHaveBeenCalledWith('poetry publish', { + cwd: 'tmp', + env: { ...process.env, FORCE_COLOR: 'true' }, + shell: true, + stdio: ['inherit', 'pipe', 'pipe'], + }); + expect(output.success).toBe(true); + expect(nxDevkitMocks.runExecutor).toHaveBeenCalledWith( + { + configuration: undefined, + project: 'app', + target: 'build', + }, + { + keepBuildFolder: true, + }, + context, + ); + expect(fsExtraMocks.removeSync).toHaveBeenCalledWith('tmp'); + }); + + it('should throw an exception when status code is not 0 and the message does not contains "File already exists"', async () => { + nxDevkitMocks.runExecutor.mockResolvedValueOnce([ + { success: true, buildFolderPath: 'tmp' }, + ]); + fsExtraMocks.removeSync.mockReturnValue(undefined); + + const options = { + buildTarget: 'build', + dryRun: false, + silent: false, + }; + + const spawnEvent = new EventEmitter(); + const stdoutEvent = new EventEmitter(); + childProcessMocks.spawn.mockReturnValue({ + stdout: { + on: vi.fn().mockImplementation((event, callback) => { + stdoutEvent.on(event, callback); + stdoutEvent.emit('data', 'Some other error message'); + }), + }, + stderr: new EventEmitter(), + on: vi.fn().mockImplementation((event, callback) => { + spawnEvent.on(event, callback); + spawnEvent.emit('close', 1); + }), + }); + + const output = await executor(options, context); + expect(checkPoetryExecutableMock).toHaveBeenCalled(); + expect(activateVenvMock).toHaveBeenCalledWith('.'); + expect(childProcessMocks.spawn).toHaveBeenCalledWith('poetry publish', { + cwd: 'tmp', + env: { ...process.env, FORCE_COLOR: 'true' }, + shell: true, + stdio: ['inherit', 'pipe', 'pipe'], + }); + expect(output.success).toBe(false); + expect(nxDevkitMocks.runExecutor).toHaveBeenCalledWith( + { + configuration: undefined, + project: 'app', + target: 'build', + }, + { + keepBuildFolder: true, + }, + context, + ); + }); }); diff --git a/packages/nx-python/src/executors/publish/executor.ts b/packages/nx-python/src/executors/publish/executor.ts index 816b97d..539e4e0 100644 --- a/packages/nx-python/src/executors/publish/executor.ts +++ b/packages/nx-python/src/executors/publish/executor.ts @@ -5,10 +5,11 @@ import { Logger } from '../utils/logger'; import { activateVenv, checkPoetryExecutable, - runPoetry, + POETRY_EXECUTABLE, } from '../utils/poetry'; import { BuildExecutorOutput } from '../build/schema'; import { removeSync } from 'fs-extra'; +import { spawnPromise } from '../utils/cmd'; const logger = new Logger(); @@ -19,12 +20,12 @@ export default async function executor( logger.setOptions(options); const workspaceRoot = context.root; process.chdir(workspaceRoot); + let buildFolderPath = ''; + try { activateVenv(workspaceRoot); await checkPoetryExecutable(); - let buildFolderPath = ''; - for await (const output of await runExecutor( { project: context.projectName, @@ -51,33 +52,48 @@ export default async function executor( chalk`\n {bold Publishing project {bgBlue ${context.projectName} }...}\n`, ); - await runPoetry( - [ - 'publish', - ...(options.dryRun ? ['--dry-run'] : []), - ...(options.__unparsed__ ?? []), - ], - { - cwd: buildFolderPath, - }, + const commandArgs = [ + 'publish', + ...(options.dryRun ? ['--dry-run'] : []), + ...(options.__unparsed__ ?? []), + ]; + const commandStr = `${POETRY_EXECUTABLE} ${commandArgs.join(' ')}`; + + console.log( + chalk`{bold Running command}: ${commandStr} ${ + buildFolderPath && buildFolderPath !== '.' + ? chalk`at {bold ${buildFolderPath}} folder` + : '' + }\n`, ); + await spawnPromise(commandStr, buildFolderPath); removeSync(buildFolderPath); return { success: true, }; } catch (error) { - if (error.message && error.message.includes('File already exists')) { - logger.info( - chalk`\n {bgYellow.bold WARNING } {bold The package is already published}\n`, - ); - return { - success: true, - }; + if (buildFolderPath) { + removeSync(buildFolderPath); + } + + if (typeof error === 'object' && 'code' in error && 'output' in error) { + if (error.code !== 0 && error.output.includes('File already exists')) { + logger.info( + chalk`\n {bgYellow.bold WARNING } {bold The package is already published}\n`, + ); + + return { + success: true, + }; + } else if (error.code !== 0) { + logger.info( + chalk`\n {bgRed.bold ERROR } {bold The publish command failed}\n`, + ); + } } - logger.info(chalk`\n {bgRed.bold ERROR } ${error.message}\n`); return { success: false, }; diff --git a/packages/nx-python/src/executors/utils/cmd.ts b/packages/nx-python/src/executors/utils/cmd.ts new file mode 100644 index 0000000..2f96874 --- /dev/null +++ b/packages/nx-python/src/executors/utils/cmd.ts @@ -0,0 +1,56 @@ +import type { SpawnSyncOptions } from 'child_process'; +import { spawn } from 'child_process'; + +export type SpawnPromiseResult = { + success: boolean; + code: number | null; + output: string; +}; + +export const spawnPromise = function ( + command: string, + cwd: string, + envVars?: Record, + output = true, +): Promise { + return new Promise((resolve, reject) => { + console.log(`Running command: ${command}`); + const env: Record = { + ...process.env, + ...(envVars ?? {}), + ...(output ? { FORCE_COLOR: 'true' } : {}), + }; + + const args: SpawnSyncOptions = { + cwd, + shell: true, + stdio: output ? ['inherit', 'pipe', 'pipe'] : 'inherit', + env, + }; + + const child = spawn(command, args); + let outputStr = ''; + + if (output) { + child.stdout?.on('data', (data) => { + process.stdout.write(data); + outputStr += data; + }); + + child.stderr?.on('data', (data) => { + process.stderr.write(data); + outputStr += data; + }); + } + + child.on('close', (code) => { + const result = { + success: code === 0, + code, + output: outputStr, + }; + + code === 0 ? resolve(result) : reject(result); + }); + }); +};