From 57a1947817baaa51b85cd19341a5390523aa9eac Mon Sep 17 00:00:00 2001 From: MURAKAMI Masahiko Date: Fri, 21 May 2021 23:06:33 +0900 Subject: [PATCH 1/4] fix(cli): make sure to await returned promise from deleteResourceFiles The catch block is not executed when rejected because it was not waiting for the promise to be resolved. --- .../src/extensions/amplify-helpers/remove-resource.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/amplify-cli/src/extensions/amplify-helpers/remove-resource.ts b/packages/amplify-cli/src/extensions/amplify-helpers/remove-resource.ts index ef72e2fd275..a28bb117061 100644 --- a/packages/amplify-cli/src/extensions/amplify-helpers/remove-resource.ts +++ b/packages/amplify-cli/src/extensions/amplify-helpers/remove-resource.ts @@ -108,7 +108,7 @@ export async function removeResource( } try { - return deleteResourceFiles(context, category, resourceName, resourceDir); + return await deleteResourceFiles(context, category, resourceName, resourceDir); } catch (err) { context.print.info(err.stack); context.print.error('An error occurred when removing the resources from the local directory'); From cfbba9a3f35a18eaf634184c253f656d60f391e6 Mon Sep 17 00:00:00 2001 From: MURAKAMI Masahiko Date: Fri, 21 May 2021 23:11:01 +0900 Subject: [PATCH 2/4] fix(cli): fix mistake default value --- .../src/extensions/amplify-helpers/remove-resource.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/amplify-cli/src/extensions/amplify-helpers/remove-resource.ts b/packages/amplify-cli/src/extensions/amplify-helpers/remove-resource.ts index a28bb117061..d4a33756e52 100644 --- a/packages/amplify-cli/src/extensions/amplify-helpers/remove-resource.ts +++ b/packages/amplify-cli/src/extensions/amplify-helpers/remove-resource.ts @@ -36,7 +36,7 @@ export async function removeResource( context: $TSContext, category, resourceName, - questionOptions: { serviceSuffix?; serviceDeletionInfo?: [] } = {}, + questionOptions: { serviceSuffix?; serviceDeletionInfo?: {} } = {}, ) { const amplifyMeta = stateManager.getMeta(); From 31942cb5cb862b3c496bba86ef3cd99b1a3c139a Mon Sep 17 00:00:00 2001 From: MURAKAMI Masahiko Date: Fri, 21 May 2021 23:14:03 +0900 Subject: [PATCH 3/4] test(cli): add test for remove-resource extensions --- .../amplify-helpers/remove-resource.test.ts | 321 ++++++++++++++++++ 1 file changed, 321 insertions(+) create mode 100644 packages/amplify-cli/src/__tests__/extensions/amplify-helpers/remove-resource.test.ts diff --git a/packages/amplify-cli/src/__tests__/extensions/amplify-helpers/remove-resource.test.ts b/packages/amplify-cli/src/__tests__/extensions/amplify-helpers/remove-resource.test.ts new file mode 100644 index 00000000000..6e38a5d82db --- /dev/null +++ b/packages/amplify-cli/src/__tests__/extensions/amplify-helpers/remove-resource.test.ts @@ -0,0 +1,321 @@ +import { removeResource, forceRemoveResource } from '../../../extensions/amplify-helpers/remove-resource'; +import { stateManager, exitOnNextTick, ResourceDoesNotExistError, MissingParametersError } from 'amplify-cli-core'; +import * as inquirer from 'inquirer'; +import { updateBackendConfigAfterResourceRemove } from '../../../extensions/amplify-helpers/update-backend-config'; +import { removeResourceParameters } from '../../../extensions/amplify-helpers/envResourceParams'; + +jest.mock('../../../extensions/amplify-helpers/envResourceParams'); +jest.mock('../../../extensions/amplify-helpers/update-backend-config'); + +jest.mock('inquirer', () => ({ + prompt: jest.fn().mockResolvedValue({ resource: 'lambda1' }), +})); + +const backendDirPathStub = 'backendDirPath'; +jest.mock('amplify-cli-core', () => ({ + ...(jest.requireActual('amplify-cli-core') as {}), + stateManager: { + getCurrentMeta: jest.fn(), + getMeta: jest.fn(), + getTeamProviderInfo: jest.fn(), + setMeta: jest.fn(), + setTeamProviderInfo: jest.fn(), + }, + pathManager: { + getBackendDirPath: jest.fn(() => backendDirPathStub), + }, + exitOnNextTick: jest.fn().mockImplementation(() => { + throw 'process.exit mock'; + }), +})); + +const stateManagerMock = stateManager as jest.Mocked; +const inquirerMock = inquirer as jest.Mocked; + +jest.mock('amplify-cli-core'); + +describe('remove-resource', () => { + let context; + + beforeEach(() => { + context = { + input: { + options: {}, + }, + filesystem: { + remove: jest.fn(), + }, + print: { + info: jest.fn(), + error: jest.fn(), + success: jest.fn(), + }, + usageData: { + emitError: jest.fn(), + }, + amplify: { + confirmPrompt: jest.fn(() => true), + getResourceStatus: jest.fn().mockReturnValue({ + allResources: [ + { + providerPlugin: 'awscloudformation', + service: 'Cognito', + resourceName: 'authResourceName', + }, + { + build: true, + providerPlugin: 'awscloudformation', + service: 'Lambda', + dependsOn: [ + { + category: 'function', + resourceName: 'lambdaLayer1', + }, + ], + resourceName: 'lambda1', + }, + { + build: true, + providerPlugin: 'awscloudformation', + service: 'LambdaLayer', + resourceName: 'lambdaLayer1', + }, + ], + }), + }, + }; + stateManagerMock.getMeta.mockReturnValue({ + auth: { + authResourceName: { + service: 'Cognito', + serviceType: 'imported', + providerPlugin: 'awscloudformation', + }, + }, + function: { + lambda1: { + service: 'Lambda', + dependsOn: [ + { + category: 'function', + resourceName: 'lambdaLayer1', + }, + ], + }, + lambdaLayer1: { + service: 'LambdaLayer', + }, + }, + }); + }); + + afterEach(() => { + jest.clearAllMocks(); + }); + + describe('removeResource', () => { + it('emit an error when the resource of the specified category does not exist', async () => { + await expect(removeResource(context as any, 'api', 'test')).rejects.toBe('process.exit mock'); + + expect(context.print.error).toBeCalledWith('No resources added for this category'); + expect(context.usageData.emitError).toBeCalledWith(new ResourceDoesNotExistError()); + expect(exitOnNextTick).toBeCalledWith(1); + }); + + it('emit an error when the resource of the specified resource name does not exist', async () => { + await expect(removeResource(context as any, 'function', 'lambda2')).rejects.toBe('process.exit mock'); + + const errorMessage = 'Resource lambda2 has not been added to function'; + expect(context.print.error).toBeCalledWith(errorMessage); + expect(context.usageData.emitError).toBeCalledWith(new ResourceDoesNotExistError(errorMessage)); + expect(exitOnNextTick).toBeCalledWith(1); + }); + + it('prompts resource name when not specified resource name', async () => { + await expect( + removeResource(context as any, 'function', undefined, { + serviceDeletionInfo: { + LambdaLayer: 'lambdaLayer deletion info message', + }, + serviceSuffix: { Lambda: '(function)', LambdaLayer: '(layer)' }, + }), + ).resolves.toEqual({ + service: 'Lambda', + resourceName: 'lambda1', + }); + + expect(inquirer.prompt).toBeCalledWith([ + { + name: 'resource', + message: 'Choose the resource you would want to remove', + type: 'list', + choices: [ + { + name: 'lambda1 (function)', + value: 'lambda1', + }, + { + name: 'lambdaLayer1 (layer)', + value: 'lambdaLayer1', + }, + ], + }, + ]); + }); + + it('print the deletion info when choose LambdaLayer', async () => { + inquirerMock.prompt.mockResolvedValue({ resource: 'lambdaLayer1' }), + await expect( + removeResource(context as any, 'function', undefined, { + serviceDeletionInfo: { + LambdaLayer: 'lambdaLayer deletion info message', + }, + serviceSuffix: { Lambda: '(function)', LambdaLayer: '(layer)' }, + }), + ).resolves.toBeUndefined(); + + expect(inquirer.prompt).toBeCalledWith([ + { + name: 'resource', + message: 'Choose the resource you would want to remove', + type: 'list', + choices: [ + { + name: 'lambda1 (function)', + value: 'lambda1', + }, + { + name: 'lambdaLayer1 (layer)', + value: 'lambdaLayer1', + }, + ], + }, + ]); + + expect(context.print.info).toBeCalledWith('lambdaLayer deletion info message'); + }); + + it('remove resource when the resource of the specified resource name does exist', async () => { + await expect(removeResource(context as any, 'function', 'lambda1')).resolves.toEqual({ + service: 'Lambda', + resourceName: 'lambda1', + }); + + expect(stateManagerMock.setMeta).toBeCalledWith(undefined, { + auth: { + authResourceName: { + service: 'Cognito', + serviceType: 'imported', + providerPlugin: 'awscloudformation', + }, + }, + function: { + lambdaLayer1: { + service: 'LambdaLayer', + }, + }, + }); + expect(context.filesystem.remove).toBeCalledWith('backendDirPath/function/lambda1'); + expect(removeResourceParameters).toBeCalledWith(context, 'function', 'lambda1'); + expect(updateBackendConfigAfterResourceRemove).toBeCalledWith('function', 'lambda1'); + expect(context.print.success).toBeCalledWith('Successfully removed resource'); + }); + + it('not remove resource when confirm prompt returns false', async () => { + context.amplify.confirmPrompt.mockReturnValue(false); + + await expect(removeResource(context as any, 'function', 'lambda1')).resolves.toBeUndefined(); + + expect(stateManagerMock.setMeta).toBeCalledTimes(0); + expect(context.filesystem.remove).toBeCalledTimes(0); + expect(removeResourceParameters).toBeCalledTimes(0); + expect(updateBackendConfigAfterResourceRemove).toBeCalledTimes(0); + }); + + it('throw an error when the dependent resources has a specified resource', async () => { + await expect(removeResource(context as any, 'function', 'lambdaLayer1')).resolves.toBeUndefined(); + + expect(context.print.error).toBeCalledWith('Resource cannot be removed because it has a dependency on another resource'); + expect(context.print.error).toBeCalledWith('Dependency: Lambda:lambda1'); + expect(context.print.error).toBeCalledWith('An error occurred when removing the resources from the local directory'); + expect(context.usageData.emitError).toBeCalledWith( + new Error('Resource cannot be removed because it has a dependency on another resource'), + ); + }); + + it('print message to unlink the imported resource on confirm prompt when the specified service is imported resource', async () => { + await expect(removeResource(context as any, 'auth', 'authResourceName')).resolves.toEqual({ + service: 'Cognito', + resourceName: 'authResourceName', + }); + + expect(context.amplify.confirmPrompt).toBeCalledWith( + 'Are you sure you want to unlink this imported resource from this Amplify backend environment? The imported resource itself will not be deleted.', + ); + }); + }); + + describe('forceRemoveResource', () => { + it('force remove the resource even when the dependent resources has a specified resource', async () => { + await expect( + forceRemoveResource(context as any, 'function', 'lambdaLayer1', 'backendDirPath/function/lambdaLayer1'), + ).resolves.toEqual({ + service: 'LambdaLayer', + resourceName: 'lambdaLayer1', + }); + + expect(stateManagerMock.setMeta).toBeCalledWith(undefined, { + auth: { + authResourceName: { + service: 'Cognito', + serviceType: 'imported', + providerPlugin: 'awscloudformation', + }, + }, + function: { + lambda1: { + service: 'Lambda', + dependsOn: [ + { + category: 'function', + resourceName: 'lambdaLayer1', + }, + ], + }, + }, + }); + expect(context.filesystem.remove).toBeCalledWith('backendDirPath/function/lambdaLayer1'); + expect(removeResourceParameters).toBeCalledWith(context, 'function', 'lambdaLayer1'); + expect(updateBackendConfigAfterResourceRemove).toBeCalledWith('function', 'lambdaLayer1'); + expect(context.print.success).toBeCalledWith('Successfully removed resource'); + }); + + it('emit an error when the resource of the specified category does not exist', async () => { + await expect( + forceRemoveResource(context as any, 'hosting', 'S3AndCloudFront', 'backendDirPath/hosting/S3AndCloudFront'), + ).rejects.toBe('process.exit mock'); + + expect(context.print.error).toBeCalledWith('No resources added for this category'); + expect(context.usageData.emitError).toBeCalledWith(new ResourceDoesNotExistError()); + expect(exitOnNextTick).toBeCalledWith(1); + }); + + it('emit an error when parameters missing', async () => { + await expect(forceRemoveResource(context as any, 'function', 'lambdaLayer1', null)).rejects.toBe('process.exit mock'); + + expect(context.print.error).toBeCalledWith('Unable to force removal of resource: missing parameters'); + expect(context.usageData.emitError).toBeCalledWith(new MissingParametersError()); + expect(exitOnNextTick).toBeCalledWith(1); + }); + + it('returns undefined when error deleting files', async () => { + context.filesystem.remove.mockImplementation(() => { + throw new Error('mock remove file error'); + }); + await expect( + forceRemoveResource(context as any, 'function', 'lambdaLayer1', 'backendDirPath/function/lambdaLayer1'), + ).resolves.toBeUndefined(); + expect(context.print.error).toBeCalledWith('Unable to force removal of resource: error deleting files'); + }); + }); +}); From 62481a8dea321e726d75f7de444de0c06998a5dc Mon Sep 17 00:00:00 2001 From: MURAKAMI Masahiko Date: Fri, 21 May 2021 23:45:19 +0900 Subject: [PATCH 4/4] fix(cli): make sure to await returned promise from emitError --- .../src/extensions/amplify-helpers/remove-resource.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/amplify-cli/src/extensions/amplify-helpers/remove-resource.ts b/packages/amplify-cli/src/extensions/amplify-helpers/remove-resource.ts index d4a33756e52..87a4e55be8c 100644 --- a/packages/amplify-cli/src/extensions/amplify-helpers/remove-resource.ts +++ b/packages/amplify-cli/src/extensions/amplify-helpers/remove-resource.ts @@ -112,7 +112,7 @@ export async function removeResource( } catch (err) { context.print.info(err.stack); context.print.error('An error occurred when removing the resources from the local directory'); - context.usageData.emitError(err); + await context.usageData.emitError(err); process.exitCode = 1; } }