From abbd85207bdc692cc5ecd30a68537b0d396a3d06 Mon Sep 17 00:00:00 2001 From: Matthew Pirocchi Date: Thu, 30 May 2019 15:56:41 -0700 Subject: [PATCH] Port 'Code Improvement: Error handling on sam cli calls (#574)' from develop. --- src/shared/loggerUtils.ts | 42 ++++++---- src/shared/sam/cli/samCliBuild.ts | 37 ++++----- src/shared/sam/cli/samCliDeploy.ts | 20 +---- src/shared/sam/cli/samCliInfo.ts | 30 ++----- src/shared/sam/cli/samCliInit.ts | 21 +---- src/shared/sam/cli/samCliInvokerUtils.ts | 28 +++++++ src/shared/sam/cli/samCliPackage.ts | 20 +---- src/test/shared/sam/cli/samCliBuild.test.ts | 82 ++++++++----------- src/test/shared/sam/cli/samCliDeploy.test.ts | 40 +++++++++ src/test/shared/sam/cli/samCliInfo.test.ts | 34 ++++---- src/test/shared/sam/cli/samCliInit.test.ts | 32 +++++--- .../shared/sam/cli/samCliInvokerUtils.test.ts | 59 +++++++++++++ src/test/shared/sam/cli/samCliPackage.test.ts | 39 +++++++++ .../sam/cli/testSamCliProcessInvoker.ts | 76 +++++++++++++++++ 14 files changed, 368 insertions(+), 192 deletions(-) create mode 100644 src/test/shared/sam/cli/samCliInvokerUtils.test.ts diff --git a/src/shared/loggerUtils.ts b/src/shared/loggerUtils.ts index 0d2186d8b41..f40afcf5dc3 100644 --- a/src/shared/loggerUtils.ts +++ b/src/shared/loggerUtils.ts @@ -11,39 +11,45 @@ import * as filesystemUtilities from './filesystemUtilities' import * as l from './logger' export class TestLogger { - private readonly _logger: l.Logger - private readonly _logfile: string - - // initializes a default logger. This persists through all tests. - // initializing a default logger means that any tested files with logger statements will work. - // as a best practice, please initialize a TestLogger before running tests on a file with logger statements. - private constructor(logfile: string, logger: l.Logger) { - this._logger = logger - this._logfile = logfile - } + /** + * initializes a default logger. This persists through all tests. + * initializing a default logger means that any tested files with logger statements will work. + * as a best practice, please initialize a TestLogger before running tests on a file with logger statements. + * + * @param logFolder - Folder to be managed by this object. Will be deleted on cleanup + * @param logger - Logger to work with + */ + private constructor( + private readonly logFolder: string, + private readonly logger: l.Logger + ) { } // cleanupLogger clears out the logger's transports, but the logger will still exist as a default // this means that the default logger will still work for other files but will output an error public async cleanupLogger(): Promise { - this._logger.releaseLogger() - if (await filesystemUtilities.fileExists(this._logfile)) { - await del(this._logfile, { force: true }) + this.logger.releaseLogger() + if (await filesystemUtilities.fileExists(this.logFolder)) { + await del(this.logFolder, { force: true }) } } public async logContainsText(str: string): Promise { - const logText = await filesystemUtilities.readFileAsString(this._logfile as string) + const logText = await filesystemUtilities.readFileAsString(TestLogger.getLogPath(this.logFolder)) - return(logText.includes(str)) + return logText.includes(str) } public static async createTestLogger(): Promise { - const logfile = await filesystemUtilities.makeTemporaryToolkitFolder() + const logFolder = await filesystemUtilities.makeTemporaryToolkitFolder() const logger = await l.initialize({ - logPath: path.join(logfile, 'temp.log'), + logPath: TestLogger.getLogPath(logFolder), logLevel: 'debug' }) - return new TestLogger(logfile, logger) + return new TestLogger(logFolder, logger) + } + + private static getLogPath(logFolder: string): string { + return path.join(logFolder, 'temp.log') } } diff --git a/src/shared/sam/cli/samCliBuild.ts b/src/shared/sam/cli/samCliBuild.ts index c62d0965d74..7a753e92b53 100644 --- a/src/shared/sam/cli/samCliBuild.ts +++ b/src/shared/sam/cli/samCliBuild.ts @@ -7,9 +7,8 @@ import { fileExists } from '../../filesystemUtilities' import { getLogger, Logger } from '../../logger' -import { ChildProcessResult } from '../../utilities/childProcess' import { DefaultSamCliProcessInvoker } from './samCliInvoker' -import { SamCliProcessInvoker } from './samCliInvokerUtils' +import { logAndThrowIfUnexpectedExitCode, SamCliProcessInvoker } from './samCliInvokerUtils' export interface SamCliBuildInvocationArguments { /** @@ -54,6 +53,10 @@ export interface SamCliBuildInvocationArguments { manifestPath?: string } +export interface FileFunctions { + fileExists: typeof fileExists +} + export class SamCliBuildInvocation { private readonly buildDir: string private readonly baseDir?: string @@ -78,6 +81,7 @@ export class SamCliBuildInvocation { skipPullImage = false, ...params }: SamCliBuildInvocationArguments, + private readonly context: { file: FileFunctions } = { file: getDefaultFileFunctions() }, ) { this.buildDir = params.buildDir this.baseDir = params.baseDir @@ -91,7 +95,6 @@ export class SamCliBuildInvocation { } public async execute(): Promise { - const logger: Logger = getLogger() await this.validate() const invokeArgs: string[] = [ @@ -111,25 +114,12 @@ export class SamCliBuildInvocation { ...this.environmentVariables } - const { exitCode, error, stderr, stdout }: ChildProcessResult = await this.invoker.invoke( + const childProcessResult = await this.invoker.invoke( { env }, ...invokeArgs ) - if (exitCode === 0) { - return - } - - console.error('SAM CLI error') - console.error(`Exit code: ${exitCode}`) - console.error(`Error: ${error}`) - console.error(`stderr: ${stderr}`) - console.error(`stdout: ${stdout}`) - - const err = - new Error(`sam build encountered an error: ${error && error.message ? error.message : stderr || stdout}`) - logger.error(err) - throw err + logAndThrowIfUnexpectedExitCode(childProcessResult, 0) } private addArgumentIf(args: string[], addIfConditional: boolean, ...argsToAdd: string[]) { @@ -139,11 +129,18 @@ export class SamCliBuildInvocation { } private async validate(): Promise { - const logger: Logger = getLogger() - if (!await fileExists(this.templatePath)) { + if (!await this.context.file.fileExists(this.templatePath)) { + const logger: Logger = getLogger() + const err = new Error(`template path does not exist: ${this.templatePath}`) logger.error(err) throw err } } } + +function getDefaultFileFunctions(): FileFunctions { + return { + fileExists + } +} diff --git a/src/shared/sam/cli/samCliDeploy.ts b/src/shared/sam/cli/samCliDeploy.ts index b4c32ab1d26..e1678c0e25b 100644 --- a/src/shared/sam/cli/samCliDeploy.ts +++ b/src/shared/sam/cli/samCliDeploy.ts @@ -6,9 +6,8 @@ 'use strict' import { BasicLogger, getLogger } from '../../logger' -import { ChildProcessResult } from '../../utilities/childProcess' import { map } from '../../utilities/collectionUtils' -import { SamCliProcessInvoker } from './samCliInvokerUtils' +import { logAndThrowIfUnexpectedExitCode, SamCliProcessInvoker } from './samCliInvokerUtils' export interface SamCliDeployParameters { templateFile: string @@ -41,20 +40,7 @@ export async function runSamCliDeploy( args.push('--parameter-overrides', ...overrides) } - const { exitCode, error, stderr, stdout }: ChildProcessResult = await invoker.invoke(...args) + const childProcessResult = await invoker.invoke(...args) - if (exitCode === 0) { - return - } - - console.error('SAM deploy error') - console.error(`Exit code: ${exitCode}`) - console.error(`Error: ${error}`) - console.error(`stderr: ${stderr}`) - console.error(`stdout: ${stdout}`) - - const message = error && error.message ? error.message : stderr || stdout - const err = new Error(`sam deploy encountered an error: ${message}`) - logger.error(err) - throw err + logAndThrowIfUnexpectedExitCode(childProcessResult, 0, logger) } diff --git a/src/shared/sam/cli/samCliInfo.ts b/src/shared/sam/cli/samCliInfo.ts index c85821cd3e8..f1c77992cb2 100644 --- a/src/shared/sam/cli/samCliInfo.ts +++ b/src/shared/sam/cli/samCliInfo.ts @@ -6,9 +6,8 @@ 'use strict' import { getLogger, Logger } from '../../logger' -import { ChildProcessResult } from '../../utilities/childProcess' import { DefaultSamCliProcessInvoker } from './samCliInvoker' -import { SamCliProcessInvoker } from './samCliInvokerUtils' +import { logAndThrowIfUnexpectedExitCode, SamCliProcessInvoker } from './samCliInvokerUtils' /** * Maps out the response text from the sam cli command `sam --info` @@ -24,33 +23,16 @@ export class SamCliInfoInvocation { } public async execute(): Promise { - const logger: Logger = getLogger() - const { error, exitCode, stderr, stdout }: ChildProcessResult = await this.invoker.invoke('--info') - - if (exitCode === 0) { - const response = this.convertOutput(stdout) + const childProcessResult = await this.invoker.invoke('--info') - if (!!response) { - return response - } + logAndThrowIfUnexpectedExitCode(childProcessResult, 0) + const response = this.convertOutput(childProcessResult.stdout) + if (!response) { throw new Error('SAM CLI did not return expected data') } - console.error('SAM CLI error') - console.error(`Exit code: ${exitCode}`) - console.error(`Error: ${error}`) - console.error(`stderr: ${stderr}`) - console.error(`stdout: ${stdout}`) - - const err = new Error( - `sam --info encountered an error: ${error} - ${error && error.message ? 'message: ' + error.message : ''} - stderr : ${stderr} - stdout : ${stdout}` - ) - logger.error(err) - throw err + return response } /** diff --git a/src/shared/sam/cli/samCliInit.ts b/src/shared/sam/cli/samCliInit.ts index 85e81bb4810..e1e2e3aca51 100644 --- a/src/shared/sam/cli/samCliInit.ts +++ b/src/shared/sam/cli/samCliInit.ts @@ -6,9 +6,7 @@ 'use strict' import { SamLambdaRuntime } from '../../../lambda/models/samLambdaRuntime' -import { getLogger, Logger } from '../../logger' -import { ChildProcessResult } from '../../utilities/childProcess' -import { SamCliProcessInvoker } from './samCliInvokerUtils' +import { logAndThrowIfUnexpectedExitCode, SamCliProcessInvoker } from './samCliInvokerUtils' export interface SamCliInitArgs { runtime: SamLambdaRuntime @@ -20,25 +18,12 @@ export async function runSamCliInit( initArguments: SamCliInitArgs, invoker: SamCliProcessInvoker ): Promise { - const { exitCode, error, stderr, stdout }: ChildProcessResult = await invoker.invoke( + const childProcessResult = await invoker.invoke( { cwd: initArguments.location }, 'init', '--name', initArguments.name, '--runtime', initArguments.runtime ) - if (exitCode === 0) { - return - } - - console.error('SAM CLI error') - console.error(`Exit code: ${exitCode}`) - console.error(`Error: ${error}`) - console.error(`stderr: ${stderr}`) - console.error(`stdout: ${stdout}`) - - const logger: Logger = getLogger() - const err = new Error(`sam init encountered an error: ${error && error.message || stderr || stdout}`) - logger.error(err) - throw err + logAndThrowIfUnexpectedExitCode(childProcessResult, 0) } diff --git a/src/shared/sam/cli/samCliInvokerUtils.ts b/src/shared/sam/cli/samCliInvokerUtils.ts index 1d534a2245a..a250101a9cb 100644 --- a/src/shared/sam/cli/samCliInvokerUtils.ts +++ b/src/shared/sam/cli/samCliInvokerUtils.ts @@ -6,9 +6,37 @@ 'use strict' import { SpawnOptions } from 'child_process' +import { BasicLogger, getLogger } from '../../logger' import { ChildProcessResult } from '../../utilities/childProcess' export interface SamCliProcessInvoker { invoke(options: SpawnOptions, ...args: string[]): Promise invoke(...args: string[]): Promise } + +export function logAndThrowIfUnexpectedExitCode( + processResult: ChildProcessResult, + expectedExitCode: number, + logger: BasicLogger = getLogger(), +): void { + if (processResult.exitCode === expectedExitCode) { return } + + logger.error(`Unexpected exitcode (${processResult.exitCode}), expecting (${expectedExitCode})`) + logger.error(`Error: ${processResult.error}`) + logger.error(`stderr: ${processResult.stderr}`) + logger.error(`stdout: ${processResult.stdout}`) + + let message: string | undefined + + if (processResult.error instanceof Error) { + if (processResult.error.message) { + message = processResult.error.message + } + } + + if (!message) { + message = processResult.stderr || processResult.stdout || 'No message available' + } + + throw new Error(`Error with child process: ${message}`) +} diff --git a/src/shared/sam/cli/samCliPackage.ts b/src/shared/sam/cli/samCliPackage.ts index 677d28d1c87..29a355f5d5f 100644 --- a/src/shared/sam/cli/samCliPackage.ts +++ b/src/shared/sam/cli/samCliPackage.ts @@ -6,8 +6,7 @@ 'use strict' import { BasicLogger, getLogger } from '../../logger' -import { ChildProcessResult } from '../../utilities/childProcess' -import { SamCliProcessInvoker } from './samCliInvokerUtils' +import { logAndThrowIfUnexpectedExitCode, SamCliProcessInvoker } from './samCliInvokerUtils' export interface SamCliPackageParameters { /** @@ -28,7 +27,7 @@ export async function runSamCliPackage( invoker: SamCliProcessInvoker, logger: BasicLogger = getLogger(), ): Promise { - const { exitCode, error, stderr, stdout }: ChildProcessResult = await invoker.invoke( + const childProcessResult = await invoker.invoke( 'package', '--template-file', packageArguments.sourceTemplateFile, '--s3-bucket', packageArguments.s3Bucket, @@ -37,18 +36,5 @@ export async function runSamCliPackage( '--profile', packageArguments.profile ) - if (exitCode === 0) { - return - } - - console.error('SAM package error') - console.error(`Exit code: ${exitCode}`) - console.error(`Error: ${error}`) - console.error(`stderr: ${stderr}`) - console.error(`stdout: ${stdout}`) - - const message = error && error.message ? error.message : stderr || stdout - const err = new Error(`sam package encountered an error: ${message}`) - logger.error(err) - throw err + logAndThrowIfUnexpectedExitCode(childProcessResult, 0, logger) } diff --git a/src/test/shared/sam/cli/samCliBuild.test.ts b/src/test/shared/sam/cli/samCliBuild.test.ts index a5956dc703a..40955413be1 100644 --- a/src/test/shared/sam/cli/samCliBuild.test.ts +++ b/src/test/shared/sam/cli/samCliBuild.test.ts @@ -12,10 +12,17 @@ import * as path from 'path' import { writeFile } from '../../../../shared/filesystem' import { makeTemporaryToolkitFolder } from '../../../../shared/filesystemUtilities' import { TestLogger } from '../../../../shared/loggerUtils' -import { SamCliBuildInvocation } from '../../../../shared/sam/cli/samCliBuild' +import { FileFunctions, SamCliBuildInvocation } from '../../../../shared/sam/cli/samCliBuild' import { SamCliProcessInvoker } from '../../../../shared/sam/cli/samCliInvokerUtils' import { ChildProcessResult } from '../../../../shared/utilities/childProcess' -import { TestSamCliProcessInvoker } from './testSamCliProcessInvoker' +import { assertThrowsError } from '../../utilities/assertUtils' +import { assertArgNotPresent, assertArgsContainArgument } from './samCliTestUtils' +import { + assertErrorContainsBadExitMessage, + assertLogContainsBadExitInformation, + BadExitCodeSamCliProcessInvoker, + TestSamCliProcessInvoker +} from './testSamCliProcessInvoker' describe('SamCliBuildInvocation', async () => { @@ -43,6 +50,11 @@ describe('SamCliBuildInvocation', async () => { let logger: TestLogger let tempFolder: string let placeholderTemplateFile: string + const badExitCodeProcessInvoker = new BadExitCodeSamCliProcessInvoker({}) + const nonRelevantArg = 'arg is not of interest to this test' + const fakeFileFunctions: FileFunctions = { + fileExists: async (filePath: string): Promise => true + } before(async () => { logger = await TestLogger.createTestLogger() @@ -63,8 +75,6 @@ describe('SamCliBuildInvocation', async () => { }) it('Passes build command to sam cli', async () => { - const nonRelevantArg = 'arg is not of interest to this test' - const processInvoker: SamCliProcessInvoker = new ExtendedTestSamCliProcessInvoker((args: any[]) => { assert.ok(args.length > 0, 'Expected args to be present') assert.strictEqual(args[0], 'build', 'Expected first arg to be the build command') @@ -80,7 +90,6 @@ describe('SamCliBuildInvocation', async () => { it('Passes Build Dir to sam cli', async () => { const expectedBuildDir = '/build' - const nonRelevantArg = 'arg is not of interest to this test' const processInvoker: SamCliProcessInvoker = new ExtendedTestSamCliProcessInvoker((args: any[]) => { assertArgsContainArgument(args, '--build-dir', expectedBuildDir) @@ -96,7 +105,6 @@ describe('SamCliBuildInvocation', async () => { it('Passes Base Dir to sam cli', async () => { const expectedBaseDir = '/src' - const nonRelevantArg = 'arg is not of interest to this test' const processInvoker: SamCliProcessInvoker = new ExtendedTestSamCliProcessInvoker((args: any[]) => { assertArgsContainArgument(args, '--base-dir', expectedBaseDir) @@ -111,8 +119,6 @@ describe('SamCliBuildInvocation', async () => { }) it('Does not pass Base Dir to sam cli', async () => { - const nonRelevantArg = 'arg is not of interest to this test' - const processInvoker: SamCliProcessInvoker = new ExtendedTestSamCliProcessInvoker((args: any[]) => { assertArgNotPresent(args, '--base-dir') }) @@ -125,8 +131,6 @@ describe('SamCliBuildInvocation', async () => { }) it('Passes Template to sam cli', async () => { - const nonRelevantArg = 'arg is not of interest to this test' - const processInvoker: SamCliProcessInvoker = new ExtendedTestSamCliProcessInvoker((args: any[]) => { assertArgsContainArgument(args, '--template', placeholderTemplateFile) }) @@ -140,8 +144,6 @@ describe('SamCliBuildInvocation', async () => { }) it('passes --use-container to sam cli if useContainer is true', async () => { - const nonRelevantArg = 'arg is not of interest to this test' - const processInvoker: SamCliProcessInvoker = new ExtendedTestSamCliProcessInvoker((args: any[]) => { assert.notStrictEqual( args.indexOf('--use-container'), @@ -159,8 +161,6 @@ describe('SamCliBuildInvocation', async () => { }) it('does not pass --use-container to sam cli if useContainer is false', async () => { - const nonRelevantArg = 'arg is not of interest to this test' - const processInvoker: SamCliProcessInvoker = new ExtendedTestSamCliProcessInvoker((args: any[]) => { assertArgNotPresent(args, '--use-container') }) @@ -174,8 +174,6 @@ describe('SamCliBuildInvocation', async () => { }) it('does not pass --use-container to sam cli if useContainer is undefined', async () => { - const nonRelevantArg = 'arg is not of interest to this test' - const processInvoker: SamCliProcessInvoker = new ExtendedTestSamCliProcessInvoker((args: any[]) => { assertArgNotPresent(args, '--use-container') }) @@ -195,7 +193,7 @@ describe('SamCliBuildInvocation', async () => { }) await new SamCliBuildInvocation({ - buildDir: 'arg is not of interest to this test', + buildDir: nonRelevantArg, templatePath: placeholderTemplateFile, invoker: processInvoker, manifestPath: expectedArg, @@ -209,7 +207,7 @@ describe('SamCliBuildInvocation', async () => { }) await new SamCliBuildInvocation({ - buildDir: 'arg is not of interest to this test', + buildDir: nonRelevantArg, templatePath: placeholderTemplateFile, invoker: processInvoker }).execute() @@ -217,7 +215,6 @@ describe('SamCliBuildInvocation', async () => { it('Passes docker network to sam cli', async () => { const expectedDockerNetwork = 'hello-world' - const nonRelevantArg = 'arg is not of interest to this test' const processInvoker: SamCliProcessInvoker = new ExtendedTestSamCliProcessInvoker((args: any[]) => { assertArgsContainArgument(args, '--docker-network', expectedDockerNetwork) @@ -232,8 +229,6 @@ describe('SamCliBuildInvocation', async () => { }) it('Does not pass docker network to sam cli if undefined', async () => { - const nonRelevantArg = 'arg is not of interest to this test' - const processInvoker: SamCliProcessInvoker = new ExtendedTestSamCliProcessInvoker((args: any[]) => { assertArgNotPresent(args, '--docker-network') }) @@ -246,8 +241,6 @@ describe('SamCliBuildInvocation', async () => { }) it('passes --skip-pull-image to sam cli if skipPullImage is true', async () => { - const nonRelevantArg = 'arg is not of interest to this test' - const processInvoker: SamCliProcessInvoker = new ExtendedTestSamCliProcessInvoker((args: any[]) => { assert.notStrictEqual( args.indexOf('--skip-pull-image'), @@ -265,8 +258,6 @@ describe('SamCliBuildInvocation', async () => { }) it('does not pass --skip-pull-image to sam cli if skipPullImageis false', async () => { - const nonRelevantArg = 'arg is not of interest to this test' - const processInvoker: SamCliProcessInvoker = new ExtendedTestSamCliProcessInvoker((args: any[]) => { assertArgNotPresent(args, '--skip-pull-image') }) @@ -280,8 +271,6 @@ describe('SamCliBuildInvocation', async () => { }) it('does not pass --skip-pull-image to sam cli if skipPullImage is undefined', async () => { - const nonRelevantArg = 'arg is not of interest to this test' - const processInvoker: SamCliProcessInvoker = new ExtendedTestSamCliProcessInvoker((args: any[]) => { assertArgNotPresent(args, '--skip-pull-image') }) @@ -293,25 +282,24 @@ describe('SamCliBuildInvocation', async () => { }).execute() }) - function assertArgsContainArgument( - args: any[], - argOfInterest: string, - expectedArgValue: string - ) { - const argPos = args.indexOf(argOfInterest) - assert.notStrictEqual(argPos, -1, `Expected arg ${argOfInterest} was not found`) - assert.ok(args.length >= argPos + 2, `Args does not contain a value for ${argOfInterest}`) - assert.strictEqual(args[argPos + 1], expectedArgValue, `Arg ${argOfInterest} did not have expected value`) - } - - function assertArgNotPresent( - args: any[], - argOfInterest: string, - ) { - assert.strictEqual( - args.indexOf(argOfInterest), - -1, - `Did not expect ${argOfInterest} arg` + it('throws on unexpected exit code', async () => { + const error = await assertThrowsError( + async () => { + await new SamCliBuildInvocation( + { + buildDir: nonRelevantArg, + templatePath: placeholderTemplateFile, + invoker: badExitCodeProcessInvoker, + }, + { + file: fakeFileFunctions + }, + ).execute() + }, + 'Expected an error to be thrown' ) - } + + assertErrorContainsBadExitMessage(error, badExitCodeProcessInvoker.error.message) + await assertLogContainsBadExitInformation(logger, badExitCodeProcessInvoker.makeChildProcessResult(), 0) + }) }) diff --git a/src/test/shared/sam/cli/samCliDeploy.test.ts b/src/test/shared/sam/cli/samCliDeploy.test.ts index 041ae67130c..994b7d150a0 100644 --- a/src/test/shared/sam/cli/samCliDeploy.test.ts +++ b/src/test/shared/sam/cli/samCliDeploy.test.ts @@ -6,25 +6,42 @@ 'use strict' import * as assert from 'assert' +import { TestLogger } from '../../../../shared/loggerUtils' import { runSamCliDeploy } from '../../../../shared/sam/cli/samCliDeploy' +import { assertThrowsError } from '../../utilities/assertUtils' import { assertArgIsPresent, assertArgNotPresent, assertArgsContainArgument, MockSamCliProcessInvoker } from './samCliTestUtils' +import { + assertErrorContainsBadExitMessage, + assertLogContainsBadExitInformation, + BadExitCodeSamCliProcessInvoker +} from './testSamCliProcessInvoker' describe('runSamCliDeploy', async () => { + let logger: TestLogger const fakeProfile = 'profile' const fakeRegion = 'region' const fakeStackName = 'stackName' const fakeTemplateFile = 'template' let invokeCount: number + before(async () => { + logger = await TestLogger.createTestLogger() + }) + beforeEach(() => { invokeCount = 0 }) + after(async () => { + await logger.cleanupLogger() + }) + + it('does not include --parameter-overrides if there are no overrides', async () => { const invoker = new MockSamCliProcessInvoker( args => { @@ -101,4 +118,27 @@ describe('runSamCliDeploy', async () => { assert.strictEqual(invokeCount, 1, 'Unexpected invoke count') }) + + it('throws on unexpected exit code', async () => { + const badExitCodeProcessInvoker = new BadExitCodeSamCliProcessInvoker({}) + + const error = await assertThrowsError( + async () => { + await runSamCliDeploy( + { + profile: fakeProfile, + parameterOverrides: new Map(), + region: fakeRegion, + stackName: fakeStackName, + templateFile: fakeTemplateFile, + }, + badExitCodeProcessInvoker + ) + }, + 'Expected an error to be thrown' + ) + + assertErrorContainsBadExitMessage(error, badExitCodeProcessInvoker.error.message) + await assertLogContainsBadExitInformation(logger, badExitCodeProcessInvoker.makeChildProcessResult(), 0) + }) }) diff --git a/src/test/shared/sam/cli/samCliInfo.test.ts b/src/test/shared/sam/cli/samCliInfo.test.ts index 55b98c9bdde..293e56198b2 100644 --- a/src/test/shared/sam/cli/samCliInfo.test.ts +++ b/src/test/shared/sam/cli/samCliInfo.test.ts @@ -10,7 +10,13 @@ import { TestLogger } from '../../../../shared/loggerUtils' import { SamCliInfoInvocation, SamCliInfoResponse } from '../../../../shared/sam/cli/samCliInfo' import { ChildProcessResult } from '../../../../shared/utilities/childProcess' import { assertThrowsError } from '../../utilities/assertUtils' -import { FakeChildProcessResult, TestSamCliProcessInvoker } from './testSamCliProcessInvoker' +import { + assertErrorContainsBadExitMessage, + assertLogContainsBadExitInformation, + BadExitCodeSamCliProcessInvoker, + FakeChildProcessResult, + TestSamCliProcessInvoker +} from './testSamCliProcessInvoker' describe('SamCliInfoInvocation', async () => { @@ -103,30 +109,18 @@ describe('SamCliInfoInvocation', async () => { ) }) - it('throws when unsuccessful errorcode is encountered', async () => { - const childProcessResult: FakeChildProcessResult = new FakeChildProcessResult( - { - exitCode: 1, - error: new Error('some-error-message'), - stderr: 'somestderr', - stdout: 'somestdout', - } - ) + it('throws on unexpected exit code', async () => { + const badExitCodeProcessInvoker = new BadExitCodeSamCliProcessInvoker({}) - const invoker: TestSamCliProcessInvoker = new TestSamCliProcessInvoker( - (spawnOptions, args: any[]): ChildProcessResult => childProcessResult - ) - const samInfo: SamCliInfoInvocation = new SamCliInfoInvocation(invoker) - - const error: Error = await assertThrowsError( + const error = await assertThrowsError( async () => { + const samInfo: SamCliInfoInvocation = new SamCliInfoInvocation(badExitCodeProcessInvoker) await samInfo.execute() }, - 'Expected a error to be thrown' + 'Expected an error to be thrown' ) - assert.ok(error.message.indexOf(childProcessResult.error!.message) !== -1, 'error message was not in error') - assert.ok(error.message.indexOf(childProcessResult.stderr) !== -1, 'stderr was not in error') - assert.ok(error.message.indexOf(childProcessResult.stdout) !== -1, 'stdout was not in error') + assertErrorContainsBadExitMessage(error, badExitCodeProcessInvoker.error.message) + await assertLogContainsBadExitInformation(logger, badExitCodeProcessInvoker.makeChildProcessResult(), 0) }) }) diff --git a/src/test/shared/sam/cli/samCliInit.test.ts b/src/test/shared/sam/cli/samCliInit.test.ts index b5612d37a5a..3cd3464df59 100644 --- a/src/test/shared/sam/cli/samCliInit.test.ts +++ b/src/test/shared/sam/cli/samCliInit.test.ts @@ -11,7 +11,14 @@ import { TestLogger } from '../../../../shared/loggerUtils' import { runSamCliInit, SamCliInitArgs } from '../../../../shared/sam/cli/samCliInit' import { SamCliProcessInvoker } from '../../../../shared/sam/cli/samCliInvokerUtils' import { ChildProcessResult } from '../../../../shared/utilities/childProcess' -import { TestSamCliProcessInvoker } from './testSamCliProcessInvoker' +import { assertThrowsError } from '../../utilities/assertUtils' +import { assertArgsContainArgument } from './samCliTestUtils' +import { + assertErrorContainsBadExitMessage, + assertLogContainsBadExitInformation, + BadExitCodeSamCliProcessInvoker, + TestSamCliProcessInvoker +} from './testSamCliProcessInvoker' describe('runSamCliInit', async () => { @@ -89,14 +96,17 @@ describe('runSamCliInit', async () => { await runSamCliInit(sampleSamInitArgs, processInvoker) }) - function assertArgsContainArgument( - args: any[], - argOfInterest: string, - expectedArgValue: string - ) { - const argPos = args.indexOf(argOfInterest) - assert.notStrictEqual(argPos, -1, `Expected arg ${argOfInterest} was not found`) - assert.ok(args.length >= argPos + 2, `Args does not contain a value for ${argOfInterest}`) - assert.strictEqual(args[argPos + 1], expectedArgValue, `Arg ${argOfInterest} did not have expected value`) - } + it('throws on unexpected exit code', async () => { + const badExitCodeProcessInvoker = new BadExitCodeSamCliProcessInvoker({}) + + const error = await assertThrowsError( + async () => { + await runSamCliInit(sampleSamInitArgs, badExitCodeProcessInvoker) + }, + 'Expected an error to be thrown' + ) + + assertErrorContainsBadExitMessage(error, badExitCodeProcessInvoker.error.message) + await assertLogContainsBadExitInformation(logger, badExitCodeProcessInvoker.makeChildProcessResult(), 0) + }) }) diff --git a/src/test/shared/sam/cli/samCliInvokerUtils.test.ts b/src/test/shared/sam/cli/samCliInvokerUtils.test.ts new file mode 100644 index 00000000000..a0a6bad3206 --- /dev/null +++ b/src/test/shared/sam/cli/samCliInvokerUtils.test.ts @@ -0,0 +1,59 @@ +/*! + * Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved. + * SPDX-License-Identifier: Apache-2.0 + */ + +'use strict' + +import { TestLogger } from '../../../../shared/loggerUtils' +import { logAndThrowIfUnexpectedExitCode } from '../../../../shared/sam/cli/samCliInvokerUtils' +import { assertThrowsError } from '../../utilities/assertUtils' +import { + assertErrorContainsBadExitMessage, + assertLogContainsBadExitInformation, +} from './testSamCliProcessInvoker' + +describe('logAndThrowIfUnexpectedExitCode', async () => { + + let logger: TestLogger + + before(async () => { + logger = await TestLogger.createTestLogger() + }) + + after(async () => { + await logger.cleanupLogger() + }) + + it('does not throw on expected exit code', async () => { + logAndThrowIfUnexpectedExitCode( + { + exitCode: 123, + error: undefined, + stderr: '', + stdout: '', + }, + 123, + ) + }) + + it('throws on unexpected exit code', async () => { + const exitError = new Error('bad result') + const childProcessResult = { + exitCode: 123, + error: exitError, + stderr: 'stderr text', + stdout: 'stdout text', + } + + const error = await assertThrowsError( + async () => { + logAndThrowIfUnexpectedExitCode(childProcessResult, 456) + }, + 'Expected an error to be thrown' + ) + + assertErrorContainsBadExitMessage(error, exitError.message) + await assertLogContainsBadExitInformation(logger, childProcessResult, 456) + }) +}) diff --git a/src/test/shared/sam/cli/samCliPackage.test.ts b/src/test/shared/sam/cli/samCliPackage.test.ts index 21b41bd0957..acc0ade0190 100644 --- a/src/test/shared/sam/cli/samCliPackage.test.ts +++ b/src/test/shared/sam/cli/samCliPackage.test.ts @@ -6,17 +6,33 @@ 'use strict' import * as assert from 'assert' +import { TestLogger } from '../../../../shared/loggerUtils' import { runSamCliPackage } from '../../../../shared/sam/cli/samCliPackage' +import { assertThrowsError } from '../../utilities/assertUtils' import { assertArgsContainArgument, MockSamCliProcessInvoker } from './samCliTestUtils' +import { + assertErrorContainsBadExitMessage, + assertLogContainsBadExitInformation, + BadExitCodeSamCliProcessInvoker +} from './testSamCliProcessInvoker' describe('SamCliPackageInvocation', async () => { + let logger: TestLogger let invokeCount: number + before(async () => { + logger = await TestLogger.createTestLogger() + }) + beforeEach(() => { invokeCount = 0 }) + after(async () => { + await logger.cleanupLogger() + }) + it('includes a template, s3 bucket, output template file, region, and profile ', async () => { const invoker = new MockSamCliProcessInvoker( args => { @@ -42,4 +58,27 @@ describe('SamCliPackageInvocation', async () => { assert.strictEqual(invokeCount, 1, 'Unexpected invoke count') }) + + it('throws on unexpected exit code', async () => { + const badExitCodeProcessInvoker = new BadExitCodeSamCliProcessInvoker({}) + + const error = await assertThrowsError( + async () => { + await runSamCliPackage( + { + sourceTemplateFile: 'template', + destinationTemplateFile: 'output', + profile: 'profile', + region: 'region', + s3Bucket: 'bucket' + }, + badExitCodeProcessInvoker + ) + }, + 'Expected an error to be thrown' + ) + + assertErrorContainsBadExitMessage(error, badExitCodeProcessInvoker.error.message) + await assertLogContainsBadExitInformation(logger, badExitCodeProcessInvoker.makeChildProcessResult(), 0) + }) }) diff --git a/src/test/shared/sam/cli/testSamCliProcessInvoker.ts b/src/test/shared/sam/cli/testSamCliProcessInvoker.ts index 6aea11d316c..62a13484647 100644 --- a/src/test/shared/sam/cli/testSamCliProcessInvoker.ts +++ b/src/test/shared/sam/cli/testSamCliProcessInvoker.ts @@ -5,8 +5,10 @@ 'use strict' +import * as assert from 'assert' import { SpawnOptions } from 'child_process' +import { TestLogger } from '../../../../shared/loggerUtils' import { SamCliProcessInvoker } from '../../../../shared/sam/cli/samCliInvokerUtils' import { ChildProcessResult } from '../../../../shared/utilities/childProcess' @@ -26,6 +28,46 @@ export class TestSamCliProcessInvoker implements SamCliProcessInvoker { } } +export class BadExitCodeSamCliProcessInvoker extends TestSamCliProcessInvoker { + + public exitCode: number + public error: Error + public stdout: string + public stderr: string + + public constructor({ + exitCode = -1, + error = new Error('Bad Result'), + stdout = 'stdout message', + stderr = 'stderr message', + }: { + exitCode?: number + error?: Error + stdout?: string + stderr?: string + }) { + super((spawnOptions: SpawnOptions, ...args: any[]) => { + return this.makeChildProcessResult() + }) + + this.exitCode = exitCode + this.error = error + this.stdout = stdout + this.stderr = stderr + } + + public makeChildProcessResult(): ChildProcessResult { + const result: ChildProcessResult = { + exitCode: this.exitCode, + error: this.error, + stdout: this.stdout, + stderr: this.stderr, + } + + return result + } +} + export class FakeChildProcessResult implements ChildProcessResult { public exitCode: number public error: Error | undefined @@ -46,3 +88,37 @@ export class FakeChildProcessResult implements ChildProcessResult { this.stderr = stderr } } + +export function assertErrorContainsBadExitMessage( + actualError: Error, + sourceErrorMessage: string +) { + assert.strictEqual( + actualError.message, `Error with child process: ${sourceErrorMessage}`, + 'Unexpected error message' + ) +} + +export async function assertLogContainsBadExitInformation( + logger: TestLogger, + errantChildProcessResult: ChildProcessResult, + expectedExitCode: number, +): Promise { + assert.ok( + // tslint:disable-next-line:max-line-length + await logger.logContainsText(`Unexpected exitcode (${errantChildProcessResult.exitCode}), expecting (${expectedExitCode})`), + 'Log message missing for exit code' + ) + assert.ok( + await logger.logContainsText(`Error: ${errantChildProcessResult.error}`), + 'Log message missing for error' + ) + assert.ok( + await logger.logContainsText(`stderr: ${errantChildProcessResult.stderr}`), + 'Log message missing for stderr' + ) + assert.ok( + await logger.logContainsText(`stdout: ${errantChildProcessResult.stdout}`), + 'Log message missing for stdout' + ) +}