From 1a0f0d46296c6abdd9e98689f6963900a853cb43 Mon Sep 17 00:00:00 2001
From: Emilien Escalle <neilime@users.noreply.github.com>
Date: Sun, 11 Nov 2018 16:29:48 +0100
Subject: [PATCH] Improve cleanup process

---
 lib/cleanup.js        | 14 ++++++++---
 tests/cleanup.test.js | 57 ++++++++++++++++++++++++++++++++++++-------
 2 files changed, 58 insertions(+), 13 deletions(-)

diff --git a/lib/cleanup.js b/lib/cleanup.js
index 9173529ff..9664c9bb5 100644
--- a/lib/cleanup.js
+++ b/lib/cleanup.js
@@ -1,20 +1,26 @@
 'use strict';
 
+const _ = require('lodash');
 const BbPromise = require('bluebird');
 const fse = require('fs-extra');
 
 module.exports = {
   cleanup() {
     const webpackOutputPath = this.webpackOutputPath;
-
     const keepOutputDirectory = this.keepOutputDirectory;
+    const cli = this.options.verbose ? this.serverless.cli : { log: _.noop };
+
     if (!keepOutputDirectory) {
-      this.options.verbose && this.serverless.cli.log(`Remove ${webpackOutputPath}`);
+      cli.log(`Remove ${webpackOutputPath}`);
       if (this.serverless.utils.dirExistsSync(webpackOutputPath)) {
-        fse.removeSync(webpackOutputPath);
+        // Remove async to speed up process
+        fse
+          .remove(webpackOutputPath)
+          .then(() => cli.log(`Removing ${webpackOutputPath} done`))
+          .catch(error => cli.log(`Error occurred while removing ${webpackOutputPath}: ${error}`));
       }
     } else {
-      this.options.verbose && this.serverless.cli.log(`Keeping ${webpackOutputPath}`);
+      cli.log(`Keeping ${webpackOutputPath}`);
     }
 
     return BbPromise.resolve();
diff --git a/tests/cleanup.test.js b/tests/cleanup.test.js
index 2be4cd3a2..dec18893a 100644
--- a/tests/cleanup.test.js
+++ b/tests/cleanup.test.js
@@ -14,7 +14,7 @@ const expect = chai.expect;
 
 const FseMock = sandbox => ({
   copy: sandbox.stub(),
-  removeSync: sandbox.stub()
+  remove: sandbox.stub()
 });
 
 describe('cleanup', () => {
@@ -47,6 +47,7 @@ describe('cleanup', () => {
     serverless = new Serverless();
     serverless.cli = {
       log: sandbox.stub(),
+      error: sandbox.stub(),
       consoleLog: sandbox.stub()
     };
     dirExistsSyncStub = sandbox.stub(serverless.utils, 'dirExistsSync');
@@ -54,7 +55,9 @@ describe('cleanup', () => {
     module = _.assign(
       {
         serverless,
-        options: {},
+        options: {
+          verbose: true
+        },
         webpackOutputPath: 'my/Output/Path'
       },
       baseModule
@@ -64,42 +67,78 @@ describe('cleanup', () => {
   afterEach(() => {
     // This will reset the webpackMock too
     sandbox.restore();
+    fseMock.remove.reset();
+    serverless.cli.log.reset();
   });
 
   it('should remove output dir if it exists', () => {
     dirExistsSyncStub.returns(true);
-    fseMock.removeSync.reset();
+    fseMock.remove.resolves(true);
 
     return expect(module.cleanup()).to.be.fulfilled.then(() => {
       expect(dirExistsSyncStub).to.have.been.calledOnce;
       expect(dirExistsSyncStub).to.have.been.calledWith('my/Output/Path');
-      expect(fseMock.removeSync).to.have.been.calledOnce;
+      expect(fseMock.remove).to.have.been.calledOnce;
+      expect(serverless.cli.log).to.have.been.calledWith('Removing my/Output/Path done');
       return null;
     });
   });
 
-  it('should not call removeSync if output dir does not exists', () => {
+  it('should log nothing is verbose is false', () => {
+    dirExistsSyncStub.returns(true);
+    fseMock.remove.resolves(true);
+
+    module = _.assign(
+      {
+        serverless,
+        options: {
+          verbose: false
+        },
+        webpackOutputPath: 'my/Output/Path'
+      },
+      baseModule
+    );
+
+    return expect(module.cleanup()).to.be.fulfilled.then(() => {
+      expect(dirExistsSyncStub).to.have.been.calledOnce;
+      expect(dirExistsSyncStub).to.have.been.calledWith('my/Output/Path');
+      expect(fseMock.remove).to.have.been.calledOnce;
+      expect(serverless.cli.log).not.to.have.been.called;
+      return null;
+    });
+  });
+
+  it('should log an error if it occurs', () => {
+    dirExistsSyncStub.returns(true);
+    fseMock.remove.rejects('remove error');
+
+    return expect(module.cleanup()).to.be.fulfilled.then(() => {
+      expect(serverless.cli.log).to.have.been.calledWith('Error occurred while removing my/Output/Path: remove error');
+
+      return null;
+    });
+  });
+
+  it('should not call remove if output dir does not exists', () => {
     dirExistsSyncStub.returns(false);
-    fseMock.removeSync.reset();
 
     return expect(module.cleanup()).to.be.fulfilled.then(() => {
       expect(dirExistsSyncStub).to.have.been.calledOnce;
       expect(dirExistsSyncStub).to.have.been.calledWith('my/Output/Path');
-      expect(fseMock.removeSync).to.not.have.been.called;
+      expect(fseMock.remove).to.not.have.been.called;
       return null;
     });
   });
 
   it('should keep output dir if keepOutputDir = true', () => {
     dirExistsSyncStub.returns(true);
-    fseMock.removeSync.reset();
 
     const configuredModule = _.assign({}, module, {
       keepOutputDirectory: true
     });
     return expect(configuredModule.cleanup()).to.be.fulfilled.then(() => {
       expect(dirExistsSyncStub).to.not.have.been.calledOnce;
-      expect(fseMock.removeSync).to.not.have.been.called;
+      expect(fseMock.remove).to.not.have.been.called;
       return null;
     });
   });