From 03bddad91cf7bd85e9579c229c98ab87ed46b58d Mon Sep 17 00:00:00 2001
From: Gojko Adzic <gojko@gojko.com>
Date: Fri, 30 Nov 2018 13:14:31 +0100
Subject: [PATCH 01/34] simple flow - trivial copying and executing npm

---
 aws_lambda_builders/workflows/__init__.py     |   1 +
 .../workflows/nodejs_npm/__init__.py          |   5 +
 .../workflows/nodejs_npm/actions.py           |  30 +++++
 .../workflows/nodejs_npm/packager.py          | 117 ++++++++++++++++++
 .../workflows/nodejs_npm/workflow.py          |  42 +++++++
 .../workflows/python_pip/utils.py             |   8 +-
 6 files changed, 201 insertions(+), 2 deletions(-)
 create mode 100644 aws_lambda_builders/workflows/nodejs_npm/__init__.py
 create mode 100644 aws_lambda_builders/workflows/nodejs_npm/actions.py
 create mode 100644 aws_lambda_builders/workflows/nodejs_npm/packager.py
 create mode 100644 aws_lambda_builders/workflows/nodejs_npm/workflow.py

diff --git a/aws_lambda_builders/workflows/__init__.py b/aws_lambda_builders/workflows/__init__.py
index 25258ca3e..5df1b9f15 100644
--- a/aws_lambda_builders/workflows/__init__.py
+++ b/aws_lambda_builders/workflows/__init__.py
@@ -3,3 +3,4 @@
 """
 
 import aws_lambda_builders.workflows.python_pip
+import aws_lambda_builders.workflows.nodejs_npm
diff --git a/aws_lambda_builders/workflows/nodejs_npm/__init__.py b/aws_lambda_builders/workflows/nodejs_npm/__init__.py
new file mode 100644
index 000000000..e62562dc9
--- /dev/null
+++ b/aws_lambda_builders/workflows/nodejs_npm/__init__.py
@@ -0,0 +1,5 @@
+"""
+Builds NodeJS Lambda functions using NPM dependency manager
+"""
+
+from .workflow import NodejsNpmWorkflow
diff --git a/aws_lambda_builders/workflows/nodejs_npm/actions.py b/aws_lambda_builders/workflows/nodejs_npm/actions.py
new file mode 100644
index 000000000..94db0e85e
--- /dev/null
+++ b/aws_lambda_builders/workflows/nodejs_npm/actions.py
@@ -0,0 +1,30 @@
+"""
+Action to resolve NodeJS dependencies using NPM
+"""
+
+from aws_lambda_builders.actions import BaseAction, Purpose, ActionFailedError
+from .packager import NodejsNpmDependencyBuilder, PackagerError
+
+
+class NodejsNpmBuildAction(BaseAction):
+
+    NAME = 'ResolveDependencies'
+    DESCRIPTION = "Installing dependencies from NPM"
+    PURPOSE = Purpose.RESOLVE_DEPENDENCIES
+
+    def __init__(self, artifacts_dir, scratch_dir, manifest_path, runtime):
+        self.artifacts_dir = artifacts_dir
+        self.manifest_path = manifest_path
+        self.scratch_dir = scratch_dir
+        self.runtime = runtime
+        self.package_builder = NodejsNpmDependencyBuilder(runtime=runtime)
+
+    def execute(self):
+        try:
+            self.package_builder.build_dependencies(
+                self.artifacts_dir,
+                self.scratch_dir,
+                self.manifest_path
+            )
+        except PackagerError as ex:
+            raise ActionFailedError(str(ex))
diff --git a/aws_lambda_builders/workflows/nodejs_npm/packager.py b/aws_lambda_builders/workflows/nodejs_npm/packager.py
new file mode 100644
index 000000000..8dd262a55
--- /dev/null
+++ b/aws_lambda_builders/workflows/nodejs_npm/packager.py
@@ -0,0 +1,117 @@
+"""
+Installs packages using NPM
+"""
+
+import logging
+
+from aws_lambda_builders.workflows.python_pip.utils import OSUtils
+
+LOG = logging.getLogger(__name__)
+
+
+class PackagerError(Exception):
+    pass
+
+
+class RequirementsFileNotFoundError(PackagerError):
+    def __init__(self, requirements_path):
+        super(RequirementsFileNotFoundError, self).__init__(
+            'Requirements file not found: %s' % requirements_path)
+
+
+class NpmNotFoundError(PackagerError):
+    def __init__(self, npm_path):
+        super(NpmNotFoundError, self).__init__(
+            'NPM executable not found: %s' % npm_path)
+
+
+class SubprocessNpm(object):
+    """Wrapper around calling npm through a subprocess."""
+    def __init__(self, osutils=None, npm_exe=None):
+        if osutils is None:
+            osutils = OSUtils()
+        self._osutils = osutils
+
+        if npm_exe is None:
+            npm_exe = osutils.find_executable('npm')
+
+        if not osutils.file_exists(npm_exe):
+            raise NpmNotFoundError(npm_exe)
+
+        self.npm_exe = npm_exe
+
+    def main(self, args, cwd=None, env_vars=None):
+        if env_vars is None:
+            env_vars = self._osutils.environ()
+
+        invoke_npm = [self.npm_exe] + args
+
+        LOG.debug("executing NPM: %s", invoke_npm)
+
+        p = self._osutils.popen(invoke_npm,
+                                stdout=self._osutils.pipe,
+                                stderr=self._osutils.pipe,
+                                env=env_vars,
+                                cwd=cwd)
+        out, err = p.communicate()
+        rc = p.returncode
+        return rc, out, err
+
+
+class NodejsNpmDependencyBuilder(object):
+    def __init__(self, runtime, osutils=None, dependency_builder=None):
+        """Initialize a NodejsNpmDependencyBuilder.
+
+        :type runtime: str
+        :param runtime: Python version to build dependencies for. This can
+            either be python2.7, python3.6 or python3.7. These are currently the
+            only supported values.
+
+        :type osutils: :class:`lambda_builders.utils.OSUtils`
+        :param osutils: A class used for all interactions with the
+            outside OS.
+
+        :type dependency_builder: :class:`DependencyBuilder`
+        :param dependency_builder: This class will be used to build the
+            dependencies of the project.
+        """
+        self.osutils = osutils
+        if osutils is None:
+            self.osutils = OSUtils()
+
+        # if dependency_builder is None:
+        #    dependency_builder = DependencyBuilder(self.osutils, runtime)
+        # self._dependency_builder = dependency_builder
+
+    def build_dependencies(self, project_dir, scratch_dir, manifest_path, ui=None, config=None):
+        """Builds a NodeJS project's dependencies into an artifact directory.
+
+        :type project_dir: str
+        :param project_dir: Directory where to store artefacts
+
+        :type scratch_dir_path: str
+        :param scratch_dir_path: Directory to write temp files into.
+
+        :type manifest_path: str
+        :param manifest_path: Location of the project package.json
+
+        :type ui: :class:`lambda_builders.utils.UI` or None
+        :param ui: A class that traps all progress information such as status
+            and errors. If injected by the caller, it can be used to monitor
+            the status of the build process or forward this information
+            elsewhere.
+
+        :type config: :class:`lambda_builders.utils.Config` or None
+        :param config: To be determined. This is an optional config object
+            we can extend at a later date to add more options to how pip is
+            called.
+        """
+
+        LOG.debug("NODEJS building in: %s from: %s", project_dir, manifest_path)
+
+        if not self.osutils.file_exists(manifest_path):
+            raise RequirementsFileNotFoundError(manifest_path)
+
+        subprocess = SubprocessNpm(self.osutils)
+
+        subprocess.main(['install', '--production'], cwd=project_dir)
diff --git a/aws_lambda_builders/workflows/nodejs_npm/workflow.py b/aws_lambda_builders/workflows/nodejs_npm/workflow.py
new file mode 100644
index 000000000..e02345ae0
--- /dev/null
+++ b/aws_lambda_builders/workflows/nodejs_npm/workflow.py
@@ -0,0 +1,42 @@
+"""
+NodeJS NPM Workflow
+"""
+
+from aws_lambda_builders.workflow import BaseWorkflow, Capability
+from aws_lambda_builders.actions import CopySourceAction
+
+from .actions import NodejsNpmBuildAction
+
+
+class NodejsNpmWorkflow(BaseWorkflow):
+
+    NAME = "NodejsNpmBuilder"
+
+    CAPABILITY = Capability(language="nodejs",
+                            dependency_manager="npm",
+                            application_framework=None)
+
+    # Common source files to exclude from build artifacts output
+    # note that NPM will ignore most of the garbage anyway
+    EXCLUDED_FILES = (
+                      ".aws-sam", ".chalice"
+                     )
+
+    def __init__(self,
+                 source_dir,
+                 artifacts_dir,
+                 scratch_dir,
+                 manifest_path,
+                 runtime=None, **kwargs):
+
+        super(NodejsNpmWorkflow, self).__init__(source_dir,
+                                                artifacts_dir,
+                                                scratch_dir,
+                                                manifest_path,
+                                                runtime=runtime,
+                                                **kwargs)
+
+        self.actions = [
+            CopySourceAction(source_dir, artifacts_dir, excludes=self.EXCLUDED_FILES),
+            NodejsNpmBuildAction(artifacts_dir, scratch_dir, manifest_path, runtime)
+        ]
diff --git a/aws_lambda_builders/workflows/python_pip/utils.py b/aws_lambda_builders/workflows/python_pip/utils.py
index 2cdd1941c..a136adec3 100644
--- a/aws_lambda_builders/workflows/python_pip/utils.py
+++ b/aws_lambda_builders/workflows/python_pip/utils.py
@@ -10,6 +10,7 @@
 import shutil
 import tarfile
 import subprocess
+import distutils
 
 
 class OSUtils(object):
@@ -76,8 +77,8 @@ def tempdir(self):
         finally:
             shutil.rmtree(tempdir)
 
-    def popen(self, command, stdout=None, stderr=None, env=None):
-        p = subprocess.Popen(command, stdout=stdout, stderr=stderr, env=env)
+    def popen(self, command, stdout=None, stderr=None, env=None, cwd=None):
+        p = subprocess.Popen(command, stdout=stdout, stderr=stderr, env=env, cwd=cwd)
         return p
 
     def mtime(self, path):
@@ -86,3 +87,6 @@ def mtime(self, path):
     @property
     def pipe(self):
         return subprocess.PIPE
+
+    def find_executable(self, execname):
+        return distutils.spawn.find_executable(execname)

From 50b55cf6f64373cba725a4dd91cccb58dc1d232d Mon Sep 17 00:00:00 2001
From: Gojko Adzic <gojko@gojko.com>
Date: Fri, 30 Nov 2018 13:48:48 +0100
Subject: [PATCH 02/34] clean up

---
 .../workflows/nodejs_npm/actions.py           |  35 ++++--
 .../workflows/nodejs_npm/npm.py               |  52 ++++++++
 .../workflows/nodejs_npm/packager.py          | 117 ------------------
 .../workflows/nodejs_npm/workflow.py          |   4 +-
 4 files changed, 79 insertions(+), 129 deletions(-)
 create mode 100644 aws_lambda_builders/workflows/nodejs_npm/npm.py
 delete mode 100644 aws_lambda_builders/workflows/nodejs_npm/packager.py

diff --git a/aws_lambda_builders/workflows/nodejs_npm/actions.py b/aws_lambda_builders/workflows/nodejs_npm/actions.py
index 94db0e85e..2e93984c4 100644
--- a/aws_lambda_builders/workflows/nodejs_npm/actions.py
+++ b/aws_lambda_builders/workflows/nodejs_npm/actions.py
@@ -3,28 +3,43 @@
 """
 
 from aws_lambda_builders.actions import BaseAction, Purpose, ActionFailedError
-from .packager import NodejsNpmDependencyBuilder, PackagerError
+from aws_lambda_builders.workflows.python_pip.utils import OSUtils
+from .npm import SubprocessNpm, NpmError
 
+import logging
 
-class NodejsNpmBuildAction(BaseAction):
+LOG = logging.getLogger(__name__)
+
+
+class NodejsNpmInstallAction(BaseAction):
 
     NAME = 'ResolveDependencies'
     DESCRIPTION = "Installing dependencies from NPM"
     PURPOSE = Purpose.RESOLVE_DEPENDENCIES
 
-    def __init__(self, artifacts_dir, scratch_dir, manifest_path, runtime):
+    def __init__(self, artifacts_dir, scratch_dir, manifest_path, runtime, osutils=None, subprocess_npm=None):
         self.artifacts_dir = artifacts_dir
         self.manifest_path = manifest_path
         self.scratch_dir = scratch_dir
         self.runtime = runtime
-        self.package_builder = NodejsNpmDependencyBuilder(runtime=runtime)
+
+        self.osutils = osutils
+        if osutils is None:
+            self.osutils = OSUtils()
+
+        self.subprocess_npm = subprocess_npm
+
+        if self.subprocess_npm is None:
+            self.subprocess_npm = SubprocessNpm(self.osutils)
 
     def execute(self):
         try:
-            self.package_builder.build_dependencies(
-                self.artifacts_dir,
-                self.scratch_dir,
-                self.manifest_path
-            )
-        except PackagerError as ex:
+            LOG.debug("NODEJS building in: %s from: %s", self.artifacts_dir, self.manifest_path)
+
+            if not self.osutils.file_exists(self.osutils.joinpath(self.artifacts_dir, 'package.json')):
+                raise ActionFailedError('package.json not found in: %s' % self.artifacts_dir)
+
+            self.subprocess_npm.main(['install', '--production'], cwd=self.artifacts_dir)
+
+        except NpmError as ex:
             raise ActionFailedError(str(ex))
diff --git a/aws_lambda_builders/workflows/nodejs_npm/npm.py b/aws_lambda_builders/workflows/nodejs_npm/npm.py
new file mode 100644
index 000000000..907c39d94
--- /dev/null
+++ b/aws_lambda_builders/workflows/nodejs_npm/npm.py
@@ -0,0 +1,52 @@
+"""
+Wrapper around calling npm through a subprocess.
+"""
+
+import logging
+
+from aws_lambda_builders.workflows.python_pip.utils import OSUtils
+
+LOG = logging.getLogger(__name__)
+
+
+class NpmError(Exception):
+    pass
+
+
+class NpmNotFoundError(NpmError):
+    def __init__(self, npm_path):
+        super(NpmNotFoundError, self).__init__(
+            'NPM executable not found: %s' % npm_path)
+
+
+class SubprocessNpm(object):
+
+    def __init__(self, osutils=None, npm_exe=None):
+        if osutils is None:
+            osutils = OSUtils()
+        self._osutils = osutils
+
+        if npm_exe is None:
+            npm_exe = osutils.find_executable('npm')
+
+        if not osutils.file_exists(npm_exe):
+            raise NpmNotFoundError(npm_exe)
+
+        self.npm_exe = npm_exe
+
+    def main(self, args, cwd=None, env_vars=None):
+        if env_vars is None:
+            env_vars = self._osutils.environ()
+
+        invoke_npm = [self.npm_exe] + args
+
+        LOG.debug("executing NPM: %s", invoke_npm)
+
+        p = self._osutils.popen(invoke_npm,
+                                stdout=self._osutils.pipe,
+                                stderr=self._osutils.pipe,
+                                env=env_vars,
+                                cwd=cwd)
+        out, err = p.communicate()
+        rc = p.returncode
+        return rc, out, err
diff --git a/aws_lambda_builders/workflows/nodejs_npm/packager.py b/aws_lambda_builders/workflows/nodejs_npm/packager.py
deleted file mode 100644
index 8dd262a55..000000000
--- a/aws_lambda_builders/workflows/nodejs_npm/packager.py
+++ /dev/null
@@ -1,117 +0,0 @@
-"""
-Installs packages using NPM
-"""
-
-import logging
-
-from aws_lambda_builders.workflows.python_pip.utils import OSUtils
-
-LOG = logging.getLogger(__name__)
-
-
-class PackagerError(Exception):
-    pass
-
-
-class RequirementsFileNotFoundError(PackagerError):
-    def __init__(self, requirements_path):
-        super(RequirementsFileNotFoundError, self).__init__(
-            'Requirements file not found: %s' % requirements_path)
-
-
-class NpmNotFoundError(PackagerError):
-    def __init__(self, npm_path):
-        super(NpmNotFoundError, self).__init__(
-            'NPM executable not found: %s' % npm_path)
-
-
-class SubprocessNpm(object):
-    """Wrapper around calling npm through a subprocess."""
-    def __init__(self, osutils=None, npm_exe=None):
-        if osutils is None:
-            osutils = OSUtils()
-        self._osutils = osutils
-
-        if npm_exe is None:
-            npm_exe = osutils.find_executable('npm')
-
-        if not osutils.file_exists(npm_exe):
-            raise NpmNotFoundError(npm_exe)
-
-        self.npm_exe = npm_exe
-
-    def main(self, args, cwd=None, env_vars=None):
-        if env_vars is None:
-            env_vars = self._osutils.environ()
-
-        invoke_npm = [self.npm_exe] + args
-
-        LOG.debug("executing NPM: %s", invoke_npm)
-
-        p = self._osutils.popen(invoke_npm,
-                                stdout=self._osutils.pipe,
-                                stderr=self._osutils.pipe,
-                                env=env_vars,
-                                cwd=cwd)
-        out, err = p.communicate()
-        rc = p.returncode
-        return rc, out, err
-
-
-class NodejsNpmDependencyBuilder(object):
-    def __init__(self, runtime, osutils=None, dependency_builder=None):
-        """Initialize a NodejsNpmDependencyBuilder.
-
-        :type runtime: str
-        :param runtime: Python version to build dependencies for. This can
-            either be python2.7, python3.6 or python3.7. These are currently the
-            only supported values.
-
-        :type osutils: :class:`lambda_builders.utils.OSUtils`
-        :param osutils: A class used for all interactions with the
-            outside OS.
-
-        :type dependency_builder: :class:`DependencyBuilder`
-        :param dependency_builder: This class will be used to build the
-            dependencies of the project.
-        """
-        self.osutils = osutils
-        if osutils is None:
-            self.osutils = OSUtils()
-
-        # if dependency_builder is None:
-        #    dependency_builder = DependencyBuilder(self.osutils, runtime)
-        # self._dependency_builder = dependency_builder
-
-    def build_dependencies(self, project_dir, scratch_dir, manifest_path, ui=None, config=None):
-        """Builds a NodeJS project's dependencies into an artifact directory.
-
-        :type project_dir: str
-        :param project_dir: Directory where to store artefacts
-
-        :type scratch_dir_path: str
-        :param scratch_dir_path: Directory to write temp files into.
-
-        :type manifest_path: str
-        :param manifest_path: Location of the project package.json
-
-        :type ui: :class:`lambda_builders.utils.UI` or None
-        :param ui: A class that traps all progress information such as status
-            and errors. If injected by the caller, it can be used to monitor
-            the status of the build process or forward this information
-            elsewhere.
-
-        :type config: :class:`lambda_builders.utils.Config` or None
-        :param config: To be determined. This is an optional config object
-            we can extend at a later date to add more options to how pip is
-            called.
-        """
-
-        LOG.debug("NODEJS building in: %s from: %s", project_dir, manifest_path)
-
-        if not self.osutils.file_exists(manifest_path):
-            raise RequirementsFileNotFoundError(manifest_path)
-
-        subprocess = SubprocessNpm(self.osutils)
-
-        subprocess.main(['install', '--production'], cwd=project_dir)
diff --git a/aws_lambda_builders/workflows/nodejs_npm/workflow.py b/aws_lambda_builders/workflows/nodejs_npm/workflow.py
index e02345ae0..492030f78 100644
--- a/aws_lambda_builders/workflows/nodejs_npm/workflow.py
+++ b/aws_lambda_builders/workflows/nodejs_npm/workflow.py
@@ -5,7 +5,7 @@
 from aws_lambda_builders.workflow import BaseWorkflow, Capability
 from aws_lambda_builders.actions import CopySourceAction
 
-from .actions import NodejsNpmBuildAction
+from .actions import NodejsNpmInstallAction
 
 
 class NodejsNpmWorkflow(BaseWorkflow):
@@ -38,5 +38,5 @@ def __init__(self,
 
         self.actions = [
             CopySourceAction(source_dir, artifacts_dir, excludes=self.EXCLUDED_FILES),
-            NodejsNpmBuildAction(artifacts_dir, scratch_dir, manifest_path, runtime)
+            NodejsNpmInstallAction(artifacts_dir, scratch_dir, manifest_path, runtime)
         ]

From bab2c3d948ff55833af86d19507e6c53f108e423 Mon Sep 17 00:00:00 2001
From: Gojko Adzic <gojko@gojko.com>
Date: Fri, 30 Nov 2018 20:21:51 +0100
Subject: [PATCH 03/34] use npm pack to build the source

---
 .../workflows/nodejs_npm/actions.py           | 51 ++++++++++++++++++-
 .../workflows/nodejs_npm/npm.py               | 14 ++++-
 .../workflows/nodejs_npm/workflow.py          | 11 +---
 .../workflows/python_pip/utils.py             |  6 +++
 4 files changed, 69 insertions(+), 13 deletions(-)

diff --git a/aws_lambda_builders/workflows/nodejs_npm/actions.py b/aws_lambda_builders/workflows/nodejs_npm/actions.py
index 2e93984c4..71844700d 100644
--- a/aws_lambda_builders/workflows/nodejs_npm/actions.py
+++ b/aws_lambda_builders/workflows/nodejs_npm/actions.py
@@ -11,6 +11,50 @@
 LOG = logging.getLogger(__name__)
 
 
+class NodejsNpmPackAction(BaseAction):
+
+    NAME = 'CopySource'
+    DESCRIPTION = "Packaging source using NPM"
+    PURPOSE = Purpose.COPY_SOURCE
+
+    def __init__(self, artifacts_dir, scratch_dir, manifest_path, runtime, osutils=None, subprocess_npm=None):
+        self.artifacts_dir = artifacts_dir
+        self.manifest_path = manifest_path
+        self.scratch_dir = scratch_dir
+        self.runtime = runtime
+
+        self.osutils = osutils
+        if osutils is None:
+            self.osutils = OSUtils()
+
+        self.subprocess_npm = subprocess_npm
+
+        if self.subprocess_npm is None:
+            self.subprocess_npm = SubprocessNpm(self.osutils)
+
+    def execute(self):
+        try:
+            if not self.osutils.file_exists(self.manifest_path):
+                raise ActionFailedError('package.json not found in: %s' % self.manifest_path)
+
+            package_path = "file:%s" % self.osutils.abspath(self.osutils.dirname(self.manifest_path))
+
+            LOG.debug("NODEJS packaging %s to %s", package_path, self.scratch_dir)
+
+            tarfile_name = self.subprocess_npm.main(['pack', '-q', package_path], cwd=self.scratch_dir)
+
+            LOG.debug("NODEJS packed to %s", tarfile_name)
+
+            tarfile_path = self.osutils.joinpath(self.scratch_dir, tarfile_name)
+
+            self.osutils.extract_tarfile(tarfile_path, tarfile_path + '-unpacked')
+
+            self.osutils.copytree(self.osutils.joinpath(tarfile_path + '-unpacked', 'package'), self.artifacts_dir)
+
+        except NpmError as ex:
+            raise ActionFailedError(str(ex))
+
+
 class NodejsNpmInstallAction(BaseAction):
 
     NAME = 'ResolveDependencies'
@@ -34,12 +78,15 @@ def __init__(self, artifacts_dir, scratch_dir, manifest_path, runtime, osutils=N
 
     def execute(self):
         try:
-            LOG.debug("NODEJS building in: %s from: %s", self.artifacts_dir, self.manifest_path)
+            LOG.debug("NODEJS installing in: %s from: %s", self.artifacts_dir, self.manifest_path)
 
             if not self.osutils.file_exists(self.osutils.joinpath(self.artifacts_dir, 'package.json')):
                 raise ActionFailedError('package.json not found in: %s' % self.artifacts_dir)
 
-            self.subprocess_npm.main(['install', '--production'], cwd=self.artifacts_dir)
+            self.subprocess_npm.main(
+                    ['install', '-q', '--no-audit', '--no-save', '--production'],
+                    cwd=self.artifacts_dir
+            )
 
         except NpmError as ex:
             raise ActionFailedError(str(ex))
diff --git a/aws_lambda_builders/workflows/nodejs_npm/npm.py b/aws_lambda_builders/workflows/nodejs_npm/npm.py
index 907c39d94..c0053f11f 100644
--- a/aws_lambda_builders/workflows/nodejs_npm/npm.py
+++ b/aws_lambda_builders/workflows/nodejs_npm/npm.py
@@ -19,6 +19,12 @@ def __init__(self, npm_path):
             'NPM executable not found: %s' % npm_path)
 
 
+class NpmExecutionError(NpmError):
+    def __init__(self, err):
+        super(NpmExecutionError, self).__init__(
+            'NPM failed: %s' % err)
+
+
 class SubprocessNpm(object):
 
     def __init__(self, osutils=None, npm_exe=None):
@@ -47,6 +53,10 @@ def main(self, args, cwd=None, env_vars=None):
                                 stderr=self._osutils.pipe,
                                 env=env_vars,
                                 cwd=cwd)
+
         out, err = p.communicate()
-        rc = p.returncode
-        return rc, out, err
+
+        if (p.returncode != 0):
+            raise NpmExecutionError(err)
+
+        return out.decode('utf8').strip()
diff --git a/aws_lambda_builders/workflows/nodejs_npm/workflow.py b/aws_lambda_builders/workflows/nodejs_npm/workflow.py
index 492030f78..475822bad 100644
--- a/aws_lambda_builders/workflows/nodejs_npm/workflow.py
+++ b/aws_lambda_builders/workflows/nodejs_npm/workflow.py
@@ -3,9 +3,8 @@
 """
 
 from aws_lambda_builders.workflow import BaseWorkflow, Capability
-from aws_lambda_builders.actions import CopySourceAction
 
-from .actions import NodejsNpmInstallAction
+from .actions import NodejsNpmPackAction, NodejsNpmInstallAction
 
 
 class NodejsNpmWorkflow(BaseWorkflow):
@@ -16,12 +15,6 @@ class NodejsNpmWorkflow(BaseWorkflow):
                             dependency_manager="npm",
                             application_framework=None)
 
-    # Common source files to exclude from build artifacts output
-    # note that NPM will ignore most of the garbage anyway
-    EXCLUDED_FILES = (
-                      ".aws-sam", ".chalice"
-                     )
-
     def __init__(self,
                  source_dir,
                  artifacts_dir,
@@ -37,6 +30,6 @@ def __init__(self,
                                                 **kwargs)
 
         self.actions = [
-            CopySourceAction(source_dir, artifacts_dir, excludes=self.EXCLUDED_FILES),
+            NodejsNpmPackAction(artifacts_dir, scratch_dir, manifest_path, runtime),
             NodejsNpmInstallAction(artifacts_dir, scratch_dir, manifest_path, runtime)
         ]
diff --git a/aws_lambda_builders/workflows/python_pip/utils.py b/aws_lambda_builders/workflows/python_pip/utils.py
index a136adec3..75006dadf 100644
--- a/aws_lambda_builders/workflows/python_pip/utils.py
+++ b/aws_lambda_builders/workflows/python_pip/utils.py
@@ -90,3 +90,9 @@ def pipe(self):
 
     def find_executable(self, execname):
         return distutils.spawn.find_executable(execname)
+
+    def dirname(self, path):
+        return os.path.dirname(path)
+
+    def abspath(self, path):
+        return os.path.abspath(path)

From de3221211069653995c88126ff92fb2c343c9edc Mon Sep 17 00:00:00 2001
From: Gojko Adzic <gojko@gojko.com>
Date: Fri, 30 Nov 2018 22:22:53 +0100
Subject: [PATCH 04/34] tests for changes in osutils

---
 tests/functional/testdata/cwd.py              |  3 ++
 .../workflows/python_pip/test_utils.py        | 42 +++++++++++++++++++
 2 files changed, 45 insertions(+)
 create mode 100644 tests/functional/testdata/cwd.py

diff --git a/tests/functional/testdata/cwd.py b/tests/functional/testdata/cwd.py
new file mode 100644
index 000000000..ea065561f
--- /dev/null
+++ b/tests/functional/testdata/cwd.py
@@ -0,0 +1,3 @@
+import os
+
+print(os.getcwd())
diff --git a/tests/functional/workflows/python_pip/test_utils.py b/tests/functional/workflows/python_pip/test_utils.py
index d89a46a1d..d103db1b2 100644
--- a/tests/functional/workflows/python_pip/test_utils.py
+++ b/tests/functional/workflows/python_pip/test_utils.py
@@ -2,6 +2,10 @@
 
 import pytest
 
+import sys
+
+import os
+
 from aws_lambda_builders.workflows.python_pip import utils
 
 
@@ -20,3 +24,41 @@ def test_can_read_unicode(self, tmpdir, osutils):
         content = osutils.get_file_contents(filename, binary=False,
                                             encoding='utf-16')
         assert content == checkmark
+
+    def test_find_executable_returns_path_for_known_exe(self, tmpdir, osutils):
+        found = osutils.find_executable('python')
+
+        assert os.path.realpath(found) == os.path.realpath(sys.executable)
+
+    def test_find_executable_returns_none_for_non_existing_exe(self, tmpdir, osutils):
+        found = osutils.find_executable('not-python')
+
+        assert found is None
+
+    def test_dirname_returns_directory_for_path(self, tmpdir, osutils):
+        dirname = osutils.dirname(sys.executable)
+
+        assert dirname == os.path.dirname(sys.executable)
+
+    def test_abspath_returns_absolute_path(self, tmpdir, osutils):
+
+        result = osutils.abspath('.')
+
+        assert os.path.isabs(result)
+
+        assert result == os.path.abspath('.')
+
+    def test_popen_can_accept_cwd(self, tmpdir, osutils):
+
+        testdata_dir = os.path.join(os.path.dirname(__file__), '..', '..', 'testdata')
+
+        p = osutils.popen([sys.executable, 'cwd.py'],
+                          stdout=osutils.pipe,
+                          stderr=osutils.pipe,
+                          cwd=testdata_dir)
+
+        out, err = p.communicate()
+
+        assert p.returncode == 0
+
+        assert out.decode('utf8').strip() == os.path.abspath(testdata_dir)

From ccf9e6deacb2996ced123a4534d240195da3e09d Mon Sep 17 00:00:00 2001
From: Gojko Adzic <gojko@gojko.com>
Date: Sat, 1 Dec 2018 13:05:02 +0100
Subject: [PATCH 05/34] integration tests for nodejs_npm

---
 .../workflows/nodejs_npm/npm.py               |  2 +-
 .../workflows/nodejs_npm/test_nodejs_npm.py   | 79 +++++++++++++++++++
 .../testdata/broken-deps/excluded.js          |  2 +
 .../testdata/broken-deps/included.js          |  2 +
 .../testdata/broken-deps/package.json         | 12 +++
 .../testdata/broken-package/excluded.js       |  2 +
 .../testdata/broken-package/included.js       |  2 +
 .../testdata/broken-package/package.json      |  9 +++
 .../nodejs_npm/testdata/no-deps/excluded.js   |  2 +
 .../nodejs_npm/testdata/no-deps/included.js   |  2 +
 .../nodejs_npm/testdata/no-deps/package.json  |  9 +++
 .../nodejs_npm/testdata/npm-deps/excluded.js  |  2 +
 .../nodejs_npm/testdata/npm-deps/included.js  |  2 +
 .../nodejs_npm/testdata/npm-deps/package.json | 12 +++
 14 files changed, 138 insertions(+), 1 deletion(-)
 create mode 100644 tests/integration/workflows/nodejs_npm/test_nodejs_npm.py
 create mode 100644 tests/integration/workflows/nodejs_npm/testdata/broken-deps/excluded.js
 create mode 100644 tests/integration/workflows/nodejs_npm/testdata/broken-deps/included.js
 create mode 100644 tests/integration/workflows/nodejs_npm/testdata/broken-deps/package.json
 create mode 100644 tests/integration/workflows/nodejs_npm/testdata/broken-package/excluded.js
 create mode 100644 tests/integration/workflows/nodejs_npm/testdata/broken-package/included.js
 create mode 100644 tests/integration/workflows/nodejs_npm/testdata/broken-package/package.json
 create mode 100644 tests/integration/workflows/nodejs_npm/testdata/no-deps/excluded.js
 create mode 100644 tests/integration/workflows/nodejs_npm/testdata/no-deps/included.js
 create mode 100644 tests/integration/workflows/nodejs_npm/testdata/no-deps/package.json
 create mode 100644 tests/integration/workflows/nodejs_npm/testdata/npm-deps/excluded.js
 create mode 100644 tests/integration/workflows/nodejs_npm/testdata/npm-deps/included.js
 create mode 100644 tests/integration/workflows/nodejs_npm/testdata/npm-deps/package.json

diff --git a/aws_lambda_builders/workflows/nodejs_npm/npm.py b/aws_lambda_builders/workflows/nodejs_npm/npm.py
index c0053f11f..484fa7d1f 100644
--- a/aws_lambda_builders/workflows/nodejs_npm/npm.py
+++ b/aws_lambda_builders/workflows/nodejs_npm/npm.py
@@ -57,6 +57,6 @@ def main(self, args, cwd=None, env_vars=None):
         out, err = p.communicate()
 
         if (p.returncode != 0):
-            raise NpmExecutionError(err)
+            raise NpmExecutionError(err.decode('utf8').strip())
 
         return out.decode('utf8').strip()
diff --git a/tests/integration/workflows/nodejs_npm/test_nodejs_npm.py b/tests/integration/workflows/nodejs_npm/test_nodejs_npm.py
new file mode 100644
index 000000000..7588a5ab2
--- /dev/null
+++ b/tests/integration/workflows/nodejs_npm/test_nodejs_npm.py
@@ -0,0 +1,79 @@
+
+import os
+import shutil
+import tempfile
+from unittest import TestCase
+
+from aws_lambda_builders.builder import LambdaBuilder
+from aws_lambda_builders.exceptions import WorkflowFailedError
+
+
+class TestNodejsNpmWorkflow(TestCase):
+    """
+    Verifies that `nodejs_npm` workflow works by building a Lambda that requires Numpy
+    """
+
+    TEST_DATA_FOLDER = os.path.join(os.path.dirname(__file__), "testdata")
+
+    def setUp(self):
+        self.artifacts_dir = tempfile.mkdtemp()
+        self.scratch_dir = tempfile.mkdtemp()
+
+        self.no_deps = os.path.join(self.TEST_DATA_FOLDER, "no-deps")
+
+        self.builder = LambdaBuilder(language="nodejs",
+                                     dependency_manager="npm",
+                                     application_framework=None)
+        self.runtime = "nodejs8.10"
+
+    def tearDown(self):
+        shutil.rmtree(self.artifacts_dir)
+        shutil.rmtree(self.scratch_dir)
+
+    def test_builds_project_without_dependencies(self):
+        source_dir = os.path.join(self.TEST_DATA_FOLDER, "no-deps")
+
+        self.builder.build(source_dir, self.artifacts_dir, self.scratch_dir,
+                           os.path.join(source_dir, "package.json"),
+                           runtime=self.runtime)
+
+        expected_files = {"package.json", "included.js"}
+        output_files = set(os.listdir(self.artifacts_dir))
+        self.assertEquals(expected_files, output_files)
+
+    def test_builds_project_with_remote_dependencies(self):
+        source_dir = os.path.join(self.TEST_DATA_FOLDER, "npm-deps")
+
+        self.builder.build(source_dir, self.artifacts_dir, self.scratch_dir,
+                           os.path.join(source_dir, "package.json"),
+                           runtime=self.runtime)
+
+        expected_files = {"package.json", "included.js", "node_modules"}
+        output_files = set(os.listdir(self.artifacts_dir))
+        self.assertEquals(expected_files, output_files)
+
+        expected_modules = {"minimal-request-promise"}
+        output_modules = set(os.listdir(os.path.join(self.artifacts_dir, "node_modules")))
+        self.assertEquals(expected_modules, output_modules)
+
+    def test_fails_if_npm_cannot_resolve_dependencies(self):
+
+        source_dir = os.path.join(self.TEST_DATA_FOLDER, "broken-deps")
+
+        with self.assertRaises(WorkflowFailedError) as ctx:
+            self.builder.build(source_dir, self.artifacts_dir, self.scratch_dir,
+                               os.path.join(source_dir, "package.json"),
+                               runtime=self.runtime)
+
+        self.assertIn("No matching version found for minimal-request-promise@0.0.0-NON_EXISTENT", str(ctx.exception))
+
+    def test_fails_if_package_json_is_broken(self):
+
+        source_dir = os.path.join(self.TEST_DATA_FOLDER, "broken-package")
+
+        with self.assertRaises(WorkflowFailedError) as ctx:
+            self.builder.build(source_dir, self.artifacts_dir, self.scratch_dir,
+                               os.path.join(source_dir, "package.json"),
+                               runtime=self.runtime)
+
+        self.assertIn("Unexpected end of JSON input", str(ctx.exception))
diff --git a/tests/integration/workflows/nodejs_npm/testdata/broken-deps/excluded.js b/tests/integration/workflows/nodejs_npm/testdata/broken-deps/excluded.js
new file mode 100644
index 000000000..8bf8be437
--- /dev/null
+++ b/tests/integration/workflows/nodejs_npm/testdata/broken-deps/excluded.js
@@ -0,0 +1,2 @@
+//excluded
+const x = 1;
diff --git a/tests/integration/workflows/nodejs_npm/testdata/broken-deps/included.js b/tests/integration/workflows/nodejs_npm/testdata/broken-deps/included.js
new file mode 100644
index 000000000..e8f963aee
--- /dev/null
+++ b/tests/integration/workflows/nodejs_npm/testdata/broken-deps/included.js
@@ -0,0 +1,2 @@
+//included
+const x = 1;
diff --git a/tests/integration/workflows/nodejs_npm/testdata/broken-deps/package.json b/tests/integration/workflows/nodejs_npm/testdata/broken-deps/package.json
new file mode 100644
index 000000000..d7c526360
--- /dev/null
+++ b/tests/integration/workflows/nodejs_npm/testdata/broken-deps/package.json
@@ -0,0 +1,12 @@
+{
+  "name": "brokendeps",
+  "version": "1.0.0",
+  "description": "",
+  "files": ["included.js"],
+  "keywords": [],
+  "author": "",
+  "license": "APACHE2.0",
+  "dependencies": {
+    "minimal-request-promise": "0.0.0-NON_EXISTENT" 
+  }
+}
diff --git a/tests/integration/workflows/nodejs_npm/testdata/broken-package/excluded.js b/tests/integration/workflows/nodejs_npm/testdata/broken-package/excluded.js
new file mode 100644
index 000000000..8bf8be437
--- /dev/null
+++ b/tests/integration/workflows/nodejs_npm/testdata/broken-package/excluded.js
@@ -0,0 +1,2 @@
+//excluded
+const x = 1;
diff --git a/tests/integration/workflows/nodejs_npm/testdata/broken-package/included.js b/tests/integration/workflows/nodejs_npm/testdata/broken-package/included.js
new file mode 100644
index 000000000..e8f963aee
--- /dev/null
+++ b/tests/integration/workflows/nodejs_npm/testdata/broken-package/included.js
@@ -0,0 +1,2 @@
+//included
+const x = 1;
diff --git a/tests/integration/workflows/nodejs_npm/testdata/broken-package/package.json b/tests/integration/workflows/nodejs_npm/testdata/broken-package/package.json
new file mode 100644
index 000000000..14157d2dd
--- /dev/null
+++ b/tests/integration/workflows/nodejs_npm/testdata/broken-package/package.json
@@ -0,0 +1,9 @@
+{
+  "name": "broken-package-json",
+  "version": "1.0.0",
+  "description": "",
+  "author": "",
+  "license": "APACHE2.0",
+  "dependencies": {
+  
+}
diff --git a/tests/integration/workflows/nodejs_npm/testdata/no-deps/excluded.js b/tests/integration/workflows/nodejs_npm/testdata/no-deps/excluded.js
new file mode 100644
index 000000000..8bf8be437
--- /dev/null
+++ b/tests/integration/workflows/nodejs_npm/testdata/no-deps/excluded.js
@@ -0,0 +1,2 @@
+//excluded
+const x = 1;
diff --git a/tests/integration/workflows/nodejs_npm/testdata/no-deps/included.js b/tests/integration/workflows/nodejs_npm/testdata/no-deps/included.js
new file mode 100644
index 000000000..e8f963aee
--- /dev/null
+++ b/tests/integration/workflows/nodejs_npm/testdata/no-deps/included.js
@@ -0,0 +1,2 @@
+//included
+const x = 1;
diff --git a/tests/integration/workflows/nodejs_npm/testdata/no-deps/package.json b/tests/integration/workflows/nodejs_npm/testdata/no-deps/package.json
new file mode 100644
index 000000000..b7874b36e
--- /dev/null
+++ b/tests/integration/workflows/nodejs_npm/testdata/no-deps/package.json
@@ -0,0 +1,9 @@
+{
+  "name": "nodeps",
+  "version": "1.0.0",
+  "description": "",
+  "files": ["included.js"],
+  "keywords": [],
+  "author": "",
+  "license": "APACHE2.0"
+}
diff --git a/tests/integration/workflows/nodejs_npm/testdata/npm-deps/excluded.js b/tests/integration/workflows/nodejs_npm/testdata/npm-deps/excluded.js
new file mode 100644
index 000000000..8bf8be437
--- /dev/null
+++ b/tests/integration/workflows/nodejs_npm/testdata/npm-deps/excluded.js
@@ -0,0 +1,2 @@
+//excluded
+const x = 1;
diff --git a/tests/integration/workflows/nodejs_npm/testdata/npm-deps/included.js b/tests/integration/workflows/nodejs_npm/testdata/npm-deps/included.js
new file mode 100644
index 000000000..e8f963aee
--- /dev/null
+++ b/tests/integration/workflows/nodejs_npm/testdata/npm-deps/included.js
@@ -0,0 +1,2 @@
+//included
+const x = 1;
diff --git a/tests/integration/workflows/nodejs_npm/testdata/npm-deps/package.json b/tests/integration/workflows/nodejs_npm/testdata/npm-deps/package.json
new file mode 100644
index 000000000..05e779165
--- /dev/null
+++ b/tests/integration/workflows/nodejs_npm/testdata/npm-deps/package.json
@@ -0,0 +1,12 @@
+{
+  "name": "npmdeps",
+  "version": "1.0.0",
+  "description": "",
+  "files": ["included.js"],
+  "keywords": [],
+  "author": "",
+  "license": "APACHE2.0",
+  "dependencies": {
+    "minimal-request-promise": "*" 
+  }
+}

From 3ac1a380bef9a7237a8c7bb99ab94d06ef86c1b6 Mon Sep 17 00:00:00 2001
From: Gojko Adzic <gojko@gojko.com>
Date: Mon, 3 Dec 2018 09:35:34 +0100
Subject: [PATCH 06/34] styling fixes

---
 .../workflows/nodejs_npm/actions.py            |  5 ++---
 .../workflows/nodejs_npm/npm.py                | 18 +++++++-----------
 2 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/aws_lambda_builders/workflows/nodejs_npm/actions.py b/aws_lambda_builders/workflows/nodejs_npm/actions.py
index 71844700d..cacd05fee 100644
--- a/aws_lambda_builders/workflows/nodejs_npm/actions.py
+++ b/aws_lambda_builders/workflows/nodejs_npm/actions.py
@@ -2,12 +2,11 @@
 Action to resolve NodeJS dependencies using NPM
 """
 
+import logging
 from aws_lambda_builders.actions import BaseAction, Purpose, ActionFailedError
 from aws_lambda_builders.workflows.python_pip.utils import OSUtils
 from .npm import SubprocessNpm, NpmError
 
-import logging
-
 LOG = logging.getLogger(__name__)
 
 
@@ -37,7 +36,7 @@ def execute(self):
             if not self.osutils.file_exists(self.manifest_path):
                 raise ActionFailedError('package.json not found in: %s' % self.manifest_path)
 
-            package_path = "file:%s" % self.osutils.abspath(self.osutils.dirname(self.manifest_path))
+            package_path = "file:{}".format(self.osutils.abspath(self.osutils.dirname(self.manifest_path)))
 
             LOG.debug("NODEJS packaging %s to %s", package_path, self.scratch_dir)
 
diff --git a/aws_lambda_builders/workflows/nodejs_npm/npm.py b/aws_lambda_builders/workflows/nodejs_npm/npm.py
index 484fa7d1f..439523a2e 100644
--- a/aws_lambda_builders/workflows/nodejs_npm/npm.py
+++ b/aws_lambda_builders/workflows/nodejs_npm/npm.py
@@ -30,7 +30,7 @@ class SubprocessNpm(object):
     def __init__(self, osutils=None, npm_exe=None):
         if osutils is None:
             osutils = OSUtils()
-        self._osutils = osutils
+        self.osutils = osutils
 
         if npm_exe is None:
             npm_exe = osutils.find_executable('npm')
@@ -40,23 +40,19 @@ def __init__(self, osutils=None, npm_exe=None):
 
         self.npm_exe = npm_exe
 
-    def main(self, args, cwd=None, env_vars=None):
-        if env_vars is None:
-            env_vars = self._osutils.environ()
-
+    def main(self, args, cwd=None):
         invoke_npm = [self.npm_exe] + args
 
         LOG.debug("executing NPM: %s", invoke_npm)
 
-        p = self._osutils.popen(invoke_npm,
-                                stdout=self._osutils.pipe,
-                                stderr=self._osutils.pipe,
-                                env=env_vars,
-                                cwd=cwd)
+        p = self.osutils.popen(invoke_npm,
+                               stdout=self.osutils.pipe,
+                               stderr=self.osutils.pipe,
+                               cwd=cwd)
 
         out, err = p.communicate()
 
-        if (p.returncode != 0):
+        if p.returncode != 0:
             raise NpmExecutionError(err.decode('utf8').strip())
 
         return out.decode('utf8').strip()

From 3810cd815a707d96b9e2d131f648adbcd251a30e Mon Sep 17 00:00:00 2001
From: Gojko Adzic <gojko@gojko.com>
Date: Mon, 3 Dec 2018 09:51:42 +0100
Subject: [PATCH 07/34] remove executable autodetection, as per
 https://github.com/awslabs/aws-lambda-builders/pull/44#discussion_r238133147

---
 aws_lambda_builders/workflows/nodejs_npm/npm.py     |  9 +++++----
 aws_lambda_builders/workflows/python_pip/utils.py   |  4 ----
 tests/functional/workflows/python_pip/test_utils.py | 10 ----------
 3 files changed, 5 insertions(+), 18 deletions(-)

diff --git a/aws_lambda_builders/workflows/nodejs_npm/npm.py b/aws_lambda_builders/workflows/nodejs_npm/npm.py
index 439523a2e..72d7ceebc 100644
--- a/aws_lambda_builders/workflows/nodejs_npm/npm.py
+++ b/aws_lambda_builders/workflows/nodejs_npm/npm.py
@@ -33,14 +33,15 @@ def __init__(self, osutils=None, npm_exe=None):
         self.osutils = osutils
 
         if npm_exe is None:
-            npm_exe = osutils.find_executable('npm')
-
-        if not osutils.file_exists(npm_exe):
-            raise NpmNotFoundError(npm_exe)
+            npm_exe = 'npm'
 
         self.npm_exe = npm_exe
 
     def main(self, args, cwd=None):
+
+        if not isinstance(args, list):
+            raise NpmExecutionError('args must be a list')
+
         invoke_npm = [self.npm_exe] + args
 
         LOG.debug("executing NPM: %s", invoke_npm)
diff --git a/aws_lambda_builders/workflows/python_pip/utils.py b/aws_lambda_builders/workflows/python_pip/utils.py
index 75006dadf..4871299e7 100644
--- a/aws_lambda_builders/workflows/python_pip/utils.py
+++ b/aws_lambda_builders/workflows/python_pip/utils.py
@@ -10,7 +10,6 @@
 import shutil
 import tarfile
 import subprocess
-import distutils
 
 
 class OSUtils(object):
@@ -88,9 +87,6 @@ def mtime(self, path):
     def pipe(self):
         return subprocess.PIPE
 
-    def find_executable(self, execname):
-        return distutils.spawn.find_executable(execname)
-
     def dirname(self, path):
         return os.path.dirname(path)
 
diff --git a/tests/functional/workflows/python_pip/test_utils.py b/tests/functional/workflows/python_pip/test_utils.py
index d103db1b2..e7c33982c 100644
--- a/tests/functional/workflows/python_pip/test_utils.py
+++ b/tests/functional/workflows/python_pip/test_utils.py
@@ -25,16 +25,6 @@ def test_can_read_unicode(self, tmpdir, osutils):
                                             encoding='utf-16')
         assert content == checkmark
 
-    def test_find_executable_returns_path_for_known_exe(self, tmpdir, osutils):
-        found = osutils.find_executable('python')
-
-        assert os.path.realpath(found) == os.path.realpath(sys.executable)
-
-    def test_find_executable_returns_none_for_non_existing_exe(self, tmpdir, osutils):
-        found = osutils.find_executable('not-python')
-
-        assert found is None
-
     def test_dirname_returns_directory_for_path(self, tmpdir, osutils):
         dirname = osutils.dirname(sys.executable)
 

From a08723ab4f7e7de0ba41d9b2ed77555a80a84095 Mon Sep 17 00:00:00 2001
From: Gojko Adzic <gojko@gojko.com>
Date: Mon, 3 Dec 2018 09:54:33 +0100
Subject: [PATCH 08/34] remove executable autodetection, as per
 https://github.com/awslabs/aws-lambda-builders/pull/44#discussion_r238133147

---
 aws_lambda_builders/workflows/nodejs_npm/npm.py | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/aws_lambda_builders/workflows/nodejs_npm/npm.py b/aws_lambda_builders/workflows/nodejs_npm/npm.py
index 72d7ceebc..8a6b4ceca 100644
--- a/aws_lambda_builders/workflows/nodejs_npm/npm.py
+++ b/aws_lambda_builders/workflows/nodejs_npm/npm.py
@@ -13,12 +13,6 @@ class NpmError(Exception):
     pass
 
 
-class NpmNotFoundError(NpmError):
-    def __init__(self, npm_path):
-        super(NpmNotFoundError, self).__init__(
-            'NPM executable not found: %s' % npm_path)
-
-
 class NpmExecutionError(NpmError):
     def __init__(self, err):
         super(NpmExecutionError, self).__init__(

From 9ac2e18df8bd48a06983fcb01e3258c8ad132f80 Mon Sep 17 00:00:00 2001
From: Gojko Adzic <gojko@gojko.com>
Date: Mon, 3 Dec 2018 09:59:08 +0100
Subject: [PATCH 09/34] fix copy/paste error

---
 tests/integration/workflows/nodejs_npm/test_nodejs_npm.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/integration/workflows/nodejs_npm/test_nodejs_npm.py b/tests/integration/workflows/nodejs_npm/test_nodejs_npm.py
index 7588a5ab2..fabf86c99 100644
--- a/tests/integration/workflows/nodejs_npm/test_nodejs_npm.py
+++ b/tests/integration/workflows/nodejs_npm/test_nodejs_npm.py
@@ -10,7 +10,7 @@
 
 class TestNodejsNpmWorkflow(TestCase):
     """
-    Verifies that `nodejs_npm` workflow works by building a Lambda that requires Numpy
+    Verifies that `nodejs_npm` workflow works by building a Lambda using NPM
     """
 
     TEST_DATA_FOLDER = os.path.join(os.path.dirname(__file__), "testdata")

From cec44aea77e1b8d557ac501927c8d1506a4b8647 Mon Sep 17 00:00:00 2001
From: Gojko Adzic <gojko@gojko.com>
Date: Mon, 3 Dec 2018 19:30:51 +0100
Subject: [PATCH 10/34] - moved osutils into nodejs_npm (as per
 https://github.com/awslabs/aws-lambda-builders/pull/44#discussion_r238380009)
 - restored old osutils into python_pip to avoid unnecessary changes - removed
 the guard for manifest not existing (as per
 https://github.com/awslabs/aws-lambda-builders/pull/44#discussion_r238380262)

---
 .../workflows/nodejs_npm/actions.py           |  5 +-
 .../workflows/nodejs_npm/npm.py               |  2 +-
 .../workflows/nodejs_npm/utils.py             | 57 +++++++++++++++++++
 .../workflows/python_pip/utils.py             | 10 +---
 .../workflows/nodejs_npm/test_utils.py        | 44 ++++++++++++++
 .../workflows/python_pip/test_utils.py        | 32 -----------
 6 files changed, 105 insertions(+), 45 deletions(-)
 create mode 100644 aws_lambda_builders/workflows/nodejs_npm/utils.py
 create mode 100644 tests/functional/workflows/nodejs_npm/test_utils.py

diff --git a/aws_lambda_builders/workflows/nodejs_npm/actions.py b/aws_lambda_builders/workflows/nodejs_npm/actions.py
index cacd05fee..05705c3bd 100644
--- a/aws_lambda_builders/workflows/nodejs_npm/actions.py
+++ b/aws_lambda_builders/workflows/nodejs_npm/actions.py
@@ -4,7 +4,7 @@
 
 import logging
 from aws_lambda_builders.actions import BaseAction, Purpose, ActionFailedError
-from aws_lambda_builders.workflows.python_pip.utils import OSUtils
+from .utils import OSUtils
 from .npm import SubprocessNpm, NpmError
 
 LOG = logging.getLogger(__name__)
@@ -33,9 +33,6 @@ def __init__(self, artifacts_dir, scratch_dir, manifest_path, runtime, osutils=N
 
     def execute(self):
         try:
-            if not self.osutils.file_exists(self.manifest_path):
-                raise ActionFailedError('package.json not found in: %s' % self.manifest_path)
-
             package_path = "file:{}".format(self.osutils.abspath(self.osutils.dirname(self.manifest_path)))
 
             LOG.debug("NODEJS packaging %s to %s", package_path, self.scratch_dir)
diff --git a/aws_lambda_builders/workflows/nodejs_npm/npm.py b/aws_lambda_builders/workflows/nodejs_npm/npm.py
index 8a6b4ceca..496f5e937 100644
--- a/aws_lambda_builders/workflows/nodejs_npm/npm.py
+++ b/aws_lambda_builders/workflows/nodejs_npm/npm.py
@@ -4,7 +4,7 @@
 
 import logging
 
-from aws_lambda_builders.workflows.python_pip.utils import OSUtils
+from .utils import OSUtils
 
 LOG = logging.getLogger(__name__)
 
diff --git a/aws_lambda_builders/workflows/nodejs_npm/utils.py b/aws_lambda_builders/workflows/nodejs_npm/utils.py
new file mode 100644
index 000000000..4183cd635
--- /dev/null
+++ b/aws_lambda_builders/workflows/nodejs_npm/utils.py
@@ -0,0 +1,57 @@
+"""
+Commonly used utilities
+"""
+
+import io
+import os
+import zipfile
+import contextlib
+import tempfile
+import shutil
+import tarfile
+import subprocess
+
+
+class OSUtils(object):
+
+    def environ(self):
+        return os.environ
+
+    def file_exists(self, filename):
+        return os.path.isfile(filename)
+
+    def extract_tarfile(self, tarfile_path, unpack_dir):
+        with tarfile.open(tarfile_path, 'r:*') as tar:
+            tar.extractall(unpack_dir)
+
+    def get_directory_contents(self, path):
+        return os.listdir(path)
+
+    def joinpath(self, *args):
+        return os.path.join(*args)
+
+    def copytree(self, source, destination):
+        if not os.path.exists(destination):
+            self.makedirs(destination)
+        names = self.get_directory_contents(source)
+        for name in names:
+            new_source = os.path.join(source, name)
+            new_destination = os.path.join(destination, name)
+            if os.path.isdir(new_source):
+                self.copytree(new_source, new_destination)
+            else:
+                shutil.copy2(new_source, new_destination)
+
+    def popen(self, command, stdout=None, stderr=None, env=None, cwd=None):
+        p = subprocess.Popen(command, stdout=stdout, stderr=stderr, env=env, cwd=cwd)
+        return p
+
+    @property
+    def pipe(self):
+        return subprocess.PIPE
+
+    def dirname(self, path):
+        return os.path.dirname(path)
+
+    def abspath(self, path):
+        return os.path.abspath(path)
diff --git a/aws_lambda_builders/workflows/python_pip/utils.py b/aws_lambda_builders/workflows/python_pip/utils.py
index 4871299e7..2cdd1941c 100644
--- a/aws_lambda_builders/workflows/python_pip/utils.py
+++ b/aws_lambda_builders/workflows/python_pip/utils.py
@@ -76,8 +76,8 @@ def tempdir(self):
         finally:
             shutil.rmtree(tempdir)
 
-    def popen(self, command, stdout=None, stderr=None, env=None, cwd=None):
-        p = subprocess.Popen(command, stdout=stdout, stderr=stderr, env=env, cwd=cwd)
+    def popen(self, command, stdout=None, stderr=None, env=None):
+        p = subprocess.Popen(command, stdout=stdout, stderr=stderr, env=env)
         return p
 
     def mtime(self, path):
@@ -86,9 +86,3 @@ def mtime(self, path):
     @property
     def pipe(self):
         return subprocess.PIPE
-
-    def dirname(self, path):
-        return os.path.dirname(path)
-
-    def abspath(self, path):
-        return os.path.abspath(path)
diff --git a/tests/functional/workflows/nodejs_npm/test_utils.py b/tests/functional/workflows/nodejs_npm/test_utils.py
new file mode 100644
index 000000000..56c1e7f41
--- /dev/null
+++ b/tests/functional/workflows/nodejs_npm/test_utils.py
@@ -0,0 +1,44 @@
+import io
+
+import pytest
+
+import sys
+
+import os
+
+from aws_lambda_builders.workflows.nodejs_npm import utils
+
+
+@pytest.fixture
+def osutils():
+    return utils.OSUtils()
+
+
+class TestOSUtils(object):
+    def test_dirname_returns_directory_for_path(self, tmpdir, osutils):
+        dirname = osutils.dirname(sys.executable)
+
+        assert dirname == os.path.dirname(sys.executable)
+
+    def test_abspath_returns_absolute_path(self, tmpdir, osutils):
+
+        result = osutils.abspath('.')
+
+        assert os.path.isabs(result)
+
+        assert result == os.path.abspath('.')
+
+    def test_popen_can_accept_cwd(self, tmpdir, osutils):
+
+        testdata_dir = os.path.join(os.path.dirname(__file__), '..', '..', 'testdata')
+
+        p = osutils.popen([sys.executable, 'cwd.py'],
+                          stdout=osutils.pipe,
+                          stderr=osutils.pipe,
+                          cwd=testdata_dir)
+
+        out, err = p.communicate()
+
+        assert p.returncode == 0
+
+        assert out.decode('utf8').strip() == os.path.abspath(testdata_dir)
diff --git a/tests/functional/workflows/python_pip/test_utils.py b/tests/functional/workflows/python_pip/test_utils.py
index e7c33982c..d89a46a1d 100644
--- a/tests/functional/workflows/python_pip/test_utils.py
+++ b/tests/functional/workflows/python_pip/test_utils.py
@@ -2,10 +2,6 @@
 
 import pytest
 
-import sys
-
-import os
-
 from aws_lambda_builders.workflows.python_pip import utils
 
 
@@ -24,31 +20,3 @@ def test_can_read_unicode(self, tmpdir, osutils):
         content = osutils.get_file_contents(filename, binary=False,
                                             encoding='utf-16')
         assert content == checkmark
-
-    def test_dirname_returns_directory_for_path(self, tmpdir, osutils):
-        dirname = osutils.dirname(sys.executable)
-
-        assert dirname == os.path.dirname(sys.executable)
-
-    def test_abspath_returns_absolute_path(self, tmpdir, osutils):
-
-        result = osutils.abspath('.')
-
-        assert os.path.isabs(result)
-
-        assert result == os.path.abspath('.')
-
-    def test_popen_can_accept_cwd(self, tmpdir, osutils):
-
-        testdata_dir = os.path.join(os.path.dirname(__file__), '..', '..', 'testdata')
-
-        p = osutils.popen([sys.executable, 'cwd.py'],
-                          stdout=osutils.pipe,
-                          stderr=osutils.pipe,
-                          cwd=testdata_dir)
-
-        out, err = p.communicate()
-
-        assert p.returncode == 0
-
-        assert out.decode('utf8').strip() == os.path.abspath(testdata_dir)

From 4a00c1e05cc58b57ffdbf1de2d1250ed9a534881 Mon Sep 17 00:00:00 2001
From: Gojko Adzic <gojko@gojko.com>
Date: Mon, 3 Dec 2018 19:37:09 +0100
Subject: [PATCH 11/34] - removing guard against manifest missing, as per
 https://github.com/awslabs/aws-lambda-builders/pull/44#discussion_r238382435

---
 aws_lambda_builders/workflows/nodejs_npm/actions.py | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/aws_lambda_builders/workflows/nodejs_npm/actions.py b/aws_lambda_builders/workflows/nodejs_npm/actions.py
index 05705c3bd..aad54c2fb 100644
--- a/aws_lambda_builders/workflows/nodejs_npm/actions.py
+++ b/aws_lambda_builders/workflows/nodejs_npm/actions.py
@@ -76,9 +76,6 @@ def execute(self):
         try:
             LOG.debug("NODEJS installing in: %s from: %s", self.artifacts_dir, self.manifest_path)
 
-            if not self.osutils.file_exists(self.osutils.joinpath(self.artifacts_dir, 'package.json')):
-                raise ActionFailedError('package.json not found in: %s' % self.artifacts_dir)
-
             self.subprocess_npm.main(
                     ['install', '-q', '--no-audit', '--no-save', '--production'],
                     cwd=self.artifacts_dir

From 3e945b281e63e82d83e0fe3fe8f71b9c50f978aa Mon Sep 17 00:00:00 2001
From: Gojko Adzic <gojko@gojko.com>
Date: Mon, 3 Dec 2018 19:57:28 +0100
Subject: [PATCH 12/34] - using "run" as the npm method entry point (as per
 https://github.com/awslabs/aws-lambda-builders/pull/44#discussion_r238385498)
 - using ValueError for argument type error (as per
 https://github.com/awslabs/aws-lambda-builders/pull/44#discussion_r238386060)

---
 .../workflows/nodejs_npm/actions.py            | 10 +++++-----
 .../workflows/nodejs_npm/npm.py                | 18 +++++++-----------
 2 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/aws_lambda_builders/workflows/nodejs_npm/actions.py b/aws_lambda_builders/workflows/nodejs_npm/actions.py
index aad54c2fb..86621b9ab 100644
--- a/aws_lambda_builders/workflows/nodejs_npm/actions.py
+++ b/aws_lambda_builders/workflows/nodejs_npm/actions.py
@@ -5,7 +5,7 @@
 import logging
 from aws_lambda_builders.actions import BaseAction, Purpose, ActionFailedError
 from .utils import OSUtils
-from .npm import SubprocessNpm, NpmError
+from .npm import SubprocessNpm, NpmExecutionError
 
 LOG = logging.getLogger(__name__)
 
@@ -37,7 +37,7 @@ def execute(self):
 
             LOG.debug("NODEJS packaging %s to %s", package_path, self.scratch_dir)
 
-            tarfile_name = self.subprocess_npm.main(['pack', '-q', package_path], cwd=self.scratch_dir)
+            tarfile_name = self.subprocess_npm.run(['pack', '-q', package_path], cwd=self.scratch_dir)
 
             LOG.debug("NODEJS packed to %s", tarfile_name)
 
@@ -47,7 +47,7 @@ def execute(self):
 
             self.osutils.copytree(self.osutils.joinpath(tarfile_path + '-unpacked', 'package'), self.artifacts_dir)
 
-        except NpmError as ex:
+        except NpmExecutionError as ex:
             raise ActionFailedError(str(ex))
 
 
@@ -76,10 +76,10 @@ def execute(self):
         try:
             LOG.debug("NODEJS installing in: %s from: %s", self.artifacts_dir, self.manifest_path)
 
-            self.subprocess_npm.main(
+            self.subprocess_npm.run(
                     ['install', '-q', '--no-audit', '--no-save', '--production'],
                     cwd=self.artifacts_dir
             )
 
-        except NpmError as ex:
+        except NpmExecutionError as ex:
             raise ActionFailedError(str(ex))
diff --git a/aws_lambda_builders/workflows/nodejs_npm/npm.py b/aws_lambda_builders/workflows/nodejs_npm/npm.py
index 496f5e937..3b0a589aa 100644
--- a/aws_lambda_builders/workflows/nodejs_npm/npm.py
+++ b/aws_lambda_builders/workflows/nodejs_npm/npm.py
@@ -4,19 +4,15 @@
 
 import logging
 
+from aws_lambda_builders.exceptions import LambdaBuilderError
+
 from .utils import OSUtils
 
 LOG = logging.getLogger(__name__)
 
 
-class NpmError(Exception):
-    pass
-
-
-class NpmExecutionError(NpmError):
-    def __init__(self, err):
-        super(NpmExecutionError, self).__init__(
-            'NPM failed: %s' % err)
+class NpmExecutionError(LambdaBuilderError):
+    MESSAGE = "NPM Failed: {message}"
 
 
 class SubprocessNpm(object):
@@ -31,10 +27,10 @@ def __init__(self, osutils=None, npm_exe=None):
 
         self.npm_exe = npm_exe
 
-    def main(self, args, cwd=None):
+    def run(self, args, cwd=None):
 
         if not isinstance(args, list):
-            raise NpmExecutionError('args must be a list')
+            raise ValueError('args must be a list')
 
         invoke_npm = [self.npm_exe] + args
 
@@ -48,6 +44,6 @@ def main(self, args, cwd=None):
         out, err = p.communicate()
 
         if p.returncode != 0:
-            raise NpmExecutionError(err.decode('utf8').strip())
+            raise NpmExecutionError(message=err.decode('utf8').strip())
 
         return out.decode('utf8').strip()

From d91c537ce91a60745f99f4259b5ca2f490ae9dad Mon Sep 17 00:00:00 2001
From: Gojko Adzic <gojko@gojko.com>
Date: Mon, 3 Dec 2018 20:26:32 +0100
Subject: [PATCH 13/34] using standard copysource action (as per
 https://github.com/awslabs/aws-lambda-builders/pull/44#discussion_r238381760)

---
 .../workflows/nodejs_npm/actions.py           | 10 ++++----
 .../workflows/nodejs_npm/utils.py             |  4 ---
 .../workflows/nodejs_npm/workflow.py          | 25 +++++++++++++++++--
 3 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/aws_lambda_builders/workflows/nodejs_npm/actions.py b/aws_lambda_builders/workflows/nodejs_npm/actions.py
index 86621b9ab..5aba58f39 100644
--- a/aws_lambda_builders/workflows/nodejs_npm/actions.py
+++ b/aws_lambda_builders/workflows/nodejs_npm/actions.py
@@ -12,9 +12,9 @@
 
 class NodejsNpmPackAction(BaseAction):
 
-    NAME = 'CopySource'
+    NAME = 'NpmPack'
     DESCRIPTION = "Packaging source using NPM"
-    PURPOSE = Purpose.COPY_SOURCE
+    PURPOSE = Purpose.COMPILE_SOURCE
 
     def __init__(self, artifacts_dir, scratch_dir, manifest_path, runtime, osutils=None, subprocess_npm=None):
         self.artifacts_dir = artifacts_dir
@@ -43,9 +43,9 @@ def execute(self):
 
             tarfile_path = self.osutils.joinpath(self.scratch_dir, tarfile_name)
 
-            self.osutils.extract_tarfile(tarfile_path, tarfile_path + '-unpacked')
+            LOG.debug("NODEJS extracting to %s", self.artifacts_dir)
 
-            self.osutils.copytree(self.osutils.joinpath(tarfile_path + '-unpacked', 'package'), self.artifacts_dir)
+            self.osutils.extract_tarfile(tarfile_path, self.artifacts_dir)
 
         except NpmExecutionError as ex:
             raise ActionFailedError(str(ex))
@@ -53,7 +53,7 @@ def execute(self):
 
 class NodejsNpmInstallAction(BaseAction):
 
-    NAME = 'ResolveDependencies'
+    NAME = 'NpmInstall'
     DESCRIPTION = "Installing dependencies from NPM"
     PURPOSE = Purpose.RESOLVE_DEPENDENCIES
 
diff --git a/aws_lambda_builders/workflows/nodejs_npm/utils.py b/aws_lambda_builders/workflows/nodejs_npm/utils.py
index 4183cd635..edf38b5c7 100644
--- a/aws_lambda_builders/workflows/nodejs_npm/utils.py
+++ b/aws_lambda_builders/workflows/nodejs_npm/utils.py
@@ -2,11 +2,7 @@
 Commonly used utilities
 """
 
-import io
 import os
-import zipfile
-import contextlib
-import tempfile
 import shutil
 import tarfile
 import subprocess
diff --git a/aws_lambda_builders/workflows/nodejs_npm/workflow.py b/aws_lambda_builders/workflows/nodejs_npm/workflow.py
index 475822bad..a1e64fe92 100644
--- a/aws_lambda_builders/workflows/nodejs_npm/workflow.py
+++ b/aws_lambda_builders/workflows/nodejs_npm/workflow.py
@@ -2,9 +2,19 @@
 NodeJS NPM Workflow
 """
 
+import hashlib
+
 from aws_lambda_builders.workflow import BaseWorkflow, Capability
+from aws_lambda_builders.actions import CopySourceAction
 
 from .actions import NodejsNpmPackAction, NodejsNpmInstallAction
+from .utils import OSUtils
+
+
+def unique_dir(manifest_path):
+    md5 = hashlib.md5()
+    md5.update(manifest_path.encode('utf8'))
+    return md5.hexdigest()
 
 
 class NodejsNpmWorkflow(BaseWorkflow):
@@ -15,12 +25,16 @@ class NodejsNpmWorkflow(BaseWorkflow):
                             dependency_manager="npm",
                             application_framework=None)
 
+    EXCLUDED_FILES = (".aws-sam")
+
     def __init__(self,
                  source_dir,
                  artifacts_dir,
                  scratch_dir,
                  manifest_path,
-                 runtime=None, **kwargs):
+                 runtime=None,
+                 osutils=None,
+                 **kwargs):
 
         super(NodejsNpmWorkflow, self).__init__(source_dir,
                                                 artifacts_dir,
@@ -29,7 +43,14 @@ def __init__(self,
                                                 runtime=runtime,
                                                 **kwargs)
 
+        if osutils is None:
+            osutils = OSUtils()
+
+        tar_dest_dir = osutils.joinpath(scratch_dir, unique_dir(manifest_path))
+        tar_package_dir = osutils.joinpath(tar_dest_dir, 'package')
+
         self.actions = [
-            NodejsNpmPackAction(artifacts_dir, scratch_dir, manifest_path, runtime),
+            NodejsNpmPackAction(tar_dest_dir, scratch_dir, manifest_path, runtime),
+            CopySourceAction(tar_package_dir, artifacts_dir, excludes=self.EXCLUDED_FILES),
             NodejsNpmInstallAction(artifacts_dir, scratch_dir, manifest_path, runtime)
         ]

From 2542cdbbcbaf4afe7ce17b1749a75804b96b1848 Mon Sep 17 00:00:00 2001
From: Gojko Adzic <gojko@gojko.com>
Date: Mon, 3 Dec 2018 20:38:30 +0100
Subject: [PATCH 14/34] - using more specific purpose, as per
 https://github.com/awslabs/aws-lambda-builders/pull/44#discussion_r238406235

---
 aws_lambda_builders/workflows/nodejs_npm/actions.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/aws_lambda_builders/workflows/nodejs_npm/actions.py b/aws_lambda_builders/workflows/nodejs_npm/actions.py
index 5aba58f39..48c334bee 100644
--- a/aws_lambda_builders/workflows/nodejs_npm/actions.py
+++ b/aws_lambda_builders/workflows/nodejs_npm/actions.py
@@ -14,7 +14,7 @@ class NodejsNpmPackAction(BaseAction):
 
     NAME = 'NpmPack'
     DESCRIPTION = "Packaging source using NPM"
-    PURPOSE = Purpose.COMPILE_SOURCE
+    PURPOSE = Purpose.COPY_SOURCE
 
     def __init__(self, artifacts_dir, scratch_dir, manifest_path, runtime, osutils=None, subprocess_npm=None):
         self.artifacts_dir = artifacts_dir

From d183932559d05f19899a6176d2d65f5b06f2abf5 Mon Sep 17 00:00:00 2001
From: Gojko Adzic <gojko@gojko.com>
Date: Mon, 3 Dec 2018 23:04:25 +0100
Subject: [PATCH 15/34] - simplify target directory naming, as per
 https://github.com/awslabs/aws-lambda-builders/pull/44#discussion_r238429922

---
 aws_lambda_builders/workflows/nodejs_npm/workflow.py | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/aws_lambda_builders/workflows/nodejs_npm/workflow.py b/aws_lambda_builders/workflows/nodejs_npm/workflow.py
index a1e64fe92..40963325b 100644
--- a/aws_lambda_builders/workflows/nodejs_npm/workflow.py
+++ b/aws_lambda_builders/workflows/nodejs_npm/workflow.py
@@ -2,8 +2,6 @@
 NodeJS NPM Workflow
 """
 
-import hashlib
-
 from aws_lambda_builders.workflow import BaseWorkflow, Capability
 from aws_lambda_builders.actions import CopySourceAction
 
@@ -11,12 +9,6 @@
 from .utils import OSUtils
 
 
-def unique_dir(manifest_path):
-    md5 = hashlib.md5()
-    md5.update(manifest_path.encode('utf8'))
-    return md5.hexdigest()
-
-
 class NodejsNpmWorkflow(BaseWorkflow):
 
     NAME = "NodejsNpmBuilder"
@@ -46,7 +38,7 @@ def __init__(self,
         if osutils is None:
             osutils = OSUtils()
 
-        tar_dest_dir = osutils.joinpath(scratch_dir, unique_dir(manifest_path))
+        tar_dest_dir = osutils.joinpath(scratch_dir, 'unpacked')
         tar_package_dir = osutils.joinpath(tar_dest_dir, 'package')
 
         self.actions = [

From 1008117d98579e68f37a9364a6e74e9f1b3b221f Mon Sep 17 00:00:00 2001
From: Gojko Adzic <gojko@gojko.com>
Date: Tue, 4 Dec 2018 00:12:52 +0100
Subject: [PATCH 16/34] initial tests for nodejs_npm actions

---
 tests/unit/workflows/nodejs_npm/__init__.py   |  0
 .../unit/workflows/nodejs_npm/test_actions.py | 87 +++++++++++++++++++
 2 files changed, 87 insertions(+)
 create mode 100644 tests/unit/workflows/nodejs_npm/__init__.py
 create mode 100644 tests/unit/workflows/nodejs_npm/test_actions.py

diff --git a/tests/unit/workflows/nodejs_npm/__init__.py b/tests/unit/workflows/nodejs_npm/__init__.py
new file mode 100644
index 000000000..e69de29bb
diff --git a/tests/unit/workflows/nodejs_npm/test_actions.py b/tests/unit/workflows/nodejs_npm/test_actions.py
new file mode 100644
index 000000000..a2d6882fe
--- /dev/null
+++ b/tests/unit/workflows/nodejs_npm/test_actions.py
@@ -0,0 +1,87 @@
+
+from unittest import TestCase
+from mock import patch
+
+from aws_lambda_builders.actions import ActionFailedError
+
+from aws_lambda_builders.workflows.nodejs_npm.actions import NodejsNpmPackAction, NodejsNpmInstallAction
+from aws_lambda_builders.workflows.nodejs_npm.npm import NpmExecutionError
+
+
+class TestNodejsNpmPackAction(TestCase):
+
+    @patch("aws_lambda_builders.workflows.nodejs_npm.utils.OSUtils")
+    @patch("aws_lambda_builders.workflows.nodejs_npm.npm.SubprocessNpm")
+    def test_tars_and_unpacks_npm_project(self, OSUtilMock, SubprocessNpmMock):
+        osutils = OSUtilMock.return_value
+        subprocess_npm = SubprocessNpmMock.return_value
+
+        action = NodejsNpmPackAction("artifacts", "scratch_dir",
+                                     "manifest", "runtime",
+                                     osutils=osutils, subprocess_npm=subprocess_npm)
+
+        osutils.dirname.side_effect = lambda value: "/dir:{}".format(value)
+        osutils.abspath.side_effect = lambda value: "/abs:{}".format(value)
+        osutils.joinpath.side_effect = lambda a, b: "{}/{}".format(a, b)
+
+        subprocess_npm.run.return_value = 'package.tar'
+
+        action.execute()
+
+        subprocess_npm.run.assert_called_with(['pack', '-q', 'file:/abs:/dir:manifest'], cwd='scratch_dir')
+        osutils.extract_tarfile.assert_called_with('scratch_dir/package.tar', 'artifacts')
+
+    @patch("aws_lambda_builders.workflows.nodejs_npm.utils.OSUtils")
+    @patch("aws_lambda_builders.workflows.nodejs_npm.npm.SubprocessNpm")
+    def test_raises_action_failed_when_npm_fails(self, OSUtilMock, SubprocessNpmMock):
+        osutils = OSUtilMock.return_value
+        subprocess_npm = SubprocessNpmMock.return_value
+
+        builder_instance = SubprocessNpmMock.return_value
+        builder_instance.run.side_effect = NpmExecutionError(message="boom!")
+
+        action = NodejsNpmPackAction("artifacts", "scratch_dir",
+                                     "manifest", "runtime",
+                                     osutils=osutils, subprocess_npm=subprocess_npm)
+
+        with self.assertRaises(ActionFailedError) as raised:
+            action.execute()
+
+        self.assertEqual(raised.exception.args[0], "NPM Failed: boom!")
+
+
+class TestNodejsNpmInstallAction(TestCase):
+
+    @patch("aws_lambda_builders.workflows.nodejs_npm.utils.OSUtils")
+    @patch("aws_lambda_builders.workflows.nodejs_npm.npm.SubprocessNpm")
+    def test_tars_and_unpacks_npm_project(self, OSUtilMock, SubprocessNpmMock):
+        osutils = OSUtilMock.return_value
+        subprocess_npm = SubprocessNpmMock.return_value
+
+        action = NodejsNpmInstallAction("artifacts", "scratch_dir",
+                                        "manifest", "runtime",
+                                        osutils=osutils, subprocess_npm=subprocess_npm)
+
+        action.execute()
+
+        expected_args = ['install', '-q', '--no-audit', '--no-save', '--production']
+
+        subprocess_npm.run.assert_called_with(expected_args, cwd='artifacts')
+
+    @patch("aws_lambda_builders.workflows.nodejs_npm.utils.OSUtils")
+    @patch("aws_lambda_builders.workflows.nodejs_npm.npm.SubprocessNpm")
+    def test_raises_action_failed_when_npm_fails(self, OSUtilMock, SubprocessNpmMock):
+        osutils = OSUtilMock.return_value
+        subprocess_npm = SubprocessNpmMock.return_value
+
+        builder_instance = SubprocessNpmMock.return_value
+        builder_instance.run.side_effect = NpmExecutionError(message="boom!")
+
+        action = NodejsNpmInstallAction("artifacts", "scratch_dir",
+                                        "manifest", "runtime",
+                                        osutils=osutils, subprocess_npm=subprocess_npm)
+
+        with self.assertRaises(ActionFailedError) as raised:
+            action.execute()
+
+        self.assertEqual(raised.exception.args[0], "NPM Failed: boom!")

From dc98d9fa2f7e20d7a528b50189e8fcf702f274ca Mon Sep 17 00:00:00 2001
From: Gojko Adzic <gojko@gojko.com>
Date: Tue, 4 Dec 2018 09:07:11 +0100
Subject: [PATCH 17/34] unit tests for the npm subprocess

---
 .../workflows/nodejs_npm/npm.py               |  3 +
 .../workflows/nodejs_npm/utils.py             | 13 ---
 tests/unit/workflows/nodejs_npm/test_npm.py   | 79 +++++++++++++++++++
 3 files changed, 82 insertions(+), 13 deletions(-)
 create mode 100644 tests/unit/workflows/nodejs_npm/test_npm.py

diff --git a/aws_lambda_builders/workflows/nodejs_npm/npm.py b/aws_lambda_builders/workflows/nodejs_npm/npm.py
index 3b0a589aa..7c4bea1ac 100644
--- a/aws_lambda_builders/workflows/nodejs_npm/npm.py
+++ b/aws_lambda_builders/workflows/nodejs_npm/npm.py
@@ -32,6 +32,9 @@ def run(self, args, cwd=None):
         if not isinstance(args, list):
             raise ValueError('args must be a list')
 
+        if not args:
+            raise ValueError('requires at least one arg')
+
         invoke_npm = [self.npm_exe] + args
 
         LOG.debug("executing NPM: %s", invoke_npm)
diff --git a/aws_lambda_builders/workflows/nodejs_npm/utils.py b/aws_lambda_builders/workflows/nodejs_npm/utils.py
index edf38b5c7..ee35048db 100644
--- a/aws_lambda_builders/workflows/nodejs_npm/utils.py
+++ b/aws_lambda_builders/workflows/nodejs_npm/utils.py
@@ -3,7 +3,6 @@
 """
 
 import os
-import shutil
 import tarfile
 import subprocess
 
@@ -26,18 +25,6 @@ def get_directory_contents(self, path):
     def joinpath(self, *args):
         return os.path.join(*args)
 
-    def copytree(self, source, destination):
-        if not os.path.exists(destination):
-            self.makedirs(destination)
-        names = self.get_directory_contents(source)
-        for name in names:
-            new_source = os.path.join(source, name)
-            new_destination = os.path.join(destination, name)
-            if os.path.isdir(new_source):
-                self.copytree(new_source, new_destination)
-            else:
-                shutil.copy2(new_source, new_destination)
-
     def popen(self, command, stdout=None, stderr=None, env=None, cwd=None):
         p = subprocess.Popen(command, stdout=stdout, stderr=stderr, env=env, cwd=cwd)
         return p
diff --git a/tests/unit/workflows/nodejs_npm/test_npm.py b/tests/unit/workflows/nodejs_npm/test_npm.py
new file mode 100644
index 000000000..3c8dc99df
--- /dev/null
+++ b/tests/unit/workflows/nodejs_npm/test_npm.py
@@ -0,0 +1,79 @@
+
+from unittest import TestCase
+from mock import patch
+
+from aws_lambda_builders.workflows.nodejs_npm.npm import SubprocessNpm, NpmExecutionError
+
+
+class FakePopen:
+    def __init__(self, out=b'out', err=b'err', retcode=0):
+        self.out = out
+        self.err = err
+        self.returncode = retcode
+
+    def communicate(self):
+        return self.out, self.err
+
+
+class TestSubprocessNpm(TestCase):
+
+    @patch("aws_lambda_builders.workflows.nodejs_npm.utils.OSUtils")
+    def setUp(self, OSUtilMock):
+
+        self.osutils = OSUtilMock.return_value
+        self.osutils.pipe = 'PIPE'
+        self.popen = FakePopen()
+        self.osutils.popen.side_effect = [self.popen]
+        self.under_test = SubprocessNpm(self.osutils)
+
+    def test_run_executes_npm_via_popen(self):
+
+        self.under_test.run(['pack', '-q'])
+
+        self.osutils.popen.assert_called_with(['npm', 'pack', '-q'], cwd=None, stderr='PIPE', stdout='PIPE')
+
+    def test_uses_custom_npm_path_if_supplied(self):
+
+        npm = SubprocessNpm(self.osutils, npm_exe="/a/b/c/npm.exe")
+
+        npm.run(['pack', '-q'])
+
+        self.osutils.popen.assert_called_with(['/a/b/c/npm.exe', 'pack', '-q'], cwd=None, stderr='PIPE', stdout='PIPE')
+
+    def test_uses_cwd_if_supplied(self):
+
+        self.under_test.run(['pack', '-q'], cwd='/a/cwd')
+
+        self.osutils.popen.assert_called_with(['npm', 'pack', '-q'], cwd='/a/cwd', stderr='PIPE', stdout='PIPE')
+
+    def test_returns_popen_out_decoded_if_retcode_is_0(self):
+
+        self.popen.out = b'some encoded text\n\n'
+
+        result = self.under_test.run(['pack'])
+
+        assert result == 'some encoded text'
+
+    def test_raises_NpmExecutionError_with_err_text_if_retcode_is_not_0(self):
+
+        self.popen.returncode = 1
+        self.popen.err = b'some error text\n\n'
+
+        with self.assertRaises(NpmExecutionError) as raised:
+            self.under_test.run(['pack'])
+
+        assert raised.exception.args[0] == "NPM Failed: some error text"
+
+    def test_raises_ValueError_if_args_not_a_list(self):
+
+        with self.assertRaises(ValueError) as raised:
+            self.under_test.run(('pack'))
+
+        assert raised.exception.args[0] == "args must be a list"
+
+    def test_raises_ValueError_if_args_empty(self):
+
+        with self.assertRaises(ValueError) as raised:
+            self.under_test.run([])
+
+        assert raised.exception.args[0] == "requires at least one arg"

From 79fc787c469ebc9ba3285d229fd9149d70fdedfa Mon Sep 17 00:00:00 2001
From: Gojko Adzic <gojko@gojko.com>
Date: Tue, 4 Dec 2018 09:34:10 +0100
Subject: [PATCH 18/34] clean up utils

---
 .../workflows/nodejs_npm/utils.py             |  9 ----
 .../workflows/nodejs_npm/test_utils.py        | 47 +++++++++++++++++--
 2 files changed, 42 insertions(+), 14 deletions(-)

diff --git a/aws_lambda_builders/workflows/nodejs_npm/utils.py b/aws_lambda_builders/workflows/nodejs_npm/utils.py
index ee35048db..0d1f49567 100644
--- a/aws_lambda_builders/workflows/nodejs_npm/utils.py
+++ b/aws_lambda_builders/workflows/nodejs_npm/utils.py
@@ -9,19 +9,10 @@
 
 class OSUtils(object):
 
-    def environ(self):
-        return os.environ
-
-    def file_exists(self, filename):
-        return os.path.isfile(filename)
-
     def extract_tarfile(self, tarfile_path, unpack_dir):
         with tarfile.open(tarfile_path, 'r:*') as tar:
             tar.extractall(unpack_dir)
 
-    def get_directory_contents(self, path):
-        return os.listdir(path)
-
     def joinpath(self, *args):
         return os.path.join(*args)
 
diff --git a/tests/functional/workflows/nodejs_npm/test_utils.py b/tests/functional/workflows/nodejs_npm/test_utils.py
index 56c1e7f41..dc077cc54 100644
--- a/tests/functional/workflows/nodejs_npm/test_utils.py
+++ b/tests/functional/workflows/nodejs_npm/test_utils.py
@@ -1,11 +1,13 @@
-import io
-
 import pytest
 
 import sys
 
 import os
 
+import shutil
+
+import tempfile
+
 from aws_lambda_builders.workflows.nodejs_npm import utils
 
 
@@ -15,12 +17,26 @@ def osutils():
 
 
 class TestOSUtils(object):
-    def test_dirname_returns_directory_for_path(self, tmpdir, osutils):
+    def test_extract_tarfile_unpacks_a_tar(self, osutils):
+
+        test_tar = os.path.join(os.path.dirname(__file__), "test_data", "test.tgz")
+
+        test_dir = tempfile.mkdtemp()
+
+        osutils.extract_tarfile(test_tar, test_dir)
+
+        output_files = set(os.listdir(test_dir))
+
+        shutil.rmtree(test_dir)
+
+        assert {"test_utils.py"} == output_files
+
+    def test_dirname_returns_directory_for_path(self, osutils):
         dirname = osutils.dirname(sys.executable)
 
         assert dirname == os.path.dirname(sys.executable)
 
-    def test_abspath_returns_absolute_path(self, tmpdir, osutils):
+    def test_abspath_returns_absolute_path(self, osutils):
 
         result = osutils.abspath('.')
 
@@ -28,7 +44,28 @@ def test_abspath_returns_absolute_path(self, tmpdir, osutils):
 
         assert result == os.path.abspath('.')
 
-    def test_popen_can_accept_cwd(self, tmpdir, osutils):
+    def test_joinpath_joins_path_components(self, osutils):
+
+        result = osutils.joinpath('a', 'b', 'c')
+
+        assert result == os.path.join('a', 'b', 'c')
+
+    def test_popen_runs_a_process_and_returns_outcome(self, osutils):
+
+        cwd_py = os.path.join(os.path.dirname(__file__), '..', '..', 'testdata', 'cwd.py')
+
+        p = osutils.popen([sys.executable, cwd_py],
+                          stdout=osutils.pipe,
+                          stderr=osutils.pipe
+                          )
+
+        out, err = p.communicate()
+
+        assert p.returncode == 0
+
+        assert out.decode('utf8').strip() == os.getcwd()
+
+    def test_popen_can_accept_cwd(self, osutils):
 
         testdata_dir = os.path.join(os.path.dirname(__file__), '..', '..', 'testdata')
 

From 77e61168a61b51abc0b89f2f761dd8040b8f6140 Mon Sep 17 00:00:00 2001
From: Gojko Adzic <gojko@gojko.com>
Date: Tue, 4 Dec 2018 09:47:17 +0100
Subject: [PATCH 19/34] clean up dependency injection

---
 .../workflows/nodejs_npm/actions.py           | 24 +++----------------
 .../workflows/nodejs_npm/npm.py               |  6 +----
 .../workflows/nodejs_npm/workflow.py          | 17 +++++++++++--
 .../unit/workflows/nodejs_npm/test_actions.py | 24 ++++++++++---------
 4 files changed, 32 insertions(+), 39 deletions(-)

diff --git a/aws_lambda_builders/workflows/nodejs_npm/actions.py b/aws_lambda_builders/workflows/nodejs_npm/actions.py
index 48c334bee..ce72f292e 100644
--- a/aws_lambda_builders/workflows/nodejs_npm/actions.py
+++ b/aws_lambda_builders/workflows/nodejs_npm/actions.py
@@ -4,8 +4,7 @@
 
 import logging
 from aws_lambda_builders.actions import BaseAction, Purpose, ActionFailedError
-from .utils import OSUtils
-from .npm import SubprocessNpm, NpmExecutionError
+from .npm import NpmExecutionError
 
 LOG = logging.getLogger(__name__)
 
@@ -16,21 +15,13 @@ class NodejsNpmPackAction(BaseAction):
     DESCRIPTION = "Packaging source using NPM"
     PURPOSE = Purpose.COPY_SOURCE
 
-    def __init__(self, artifacts_dir, scratch_dir, manifest_path, runtime, osutils=None, subprocess_npm=None):
+    def __init__(self, artifacts_dir, scratch_dir, manifest_path, osutils, subprocess_npm):
         self.artifacts_dir = artifacts_dir
         self.manifest_path = manifest_path
         self.scratch_dir = scratch_dir
-        self.runtime = runtime
-
         self.osutils = osutils
-        if osutils is None:
-            self.osutils = OSUtils()
-
         self.subprocess_npm = subprocess_npm
 
-        if self.subprocess_npm is None:
-            self.subprocess_npm = SubprocessNpm(self.osutils)
-
     def execute(self):
         try:
             package_path = "file:{}".format(self.osutils.abspath(self.osutils.dirname(self.manifest_path)))
@@ -57,21 +48,12 @@ class NodejsNpmInstallAction(BaseAction):
     DESCRIPTION = "Installing dependencies from NPM"
     PURPOSE = Purpose.RESOLVE_DEPENDENCIES
 
-    def __init__(self, artifacts_dir, scratch_dir, manifest_path, runtime, osutils=None, subprocess_npm=None):
+    def __init__(self, artifacts_dir, manifest_path, osutils, subprocess_npm):
         self.artifacts_dir = artifacts_dir
         self.manifest_path = manifest_path
-        self.scratch_dir = scratch_dir
-        self.runtime = runtime
-
         self.osutils = osutils
-        if osutils is None:
-            self.osutils = OSUtils()
-
         self.subprocess_npm = subprocess_npm
 
-        if self.subprocess_npm is None:
-            self.subprocess_npm = SubprocessNpm(self.osutils)
-
     def execute(self):
         try:
             LOG.debug("NODEJS installing in: %s from: %s", self.artifacts_dir, self.manifest_path)
diff --git a/aws_lambda_builders/workflows/nodejs_npm/npm.py b/aws_lambda_builders/workflows/nodejs_npm/npm.py
index 7c4bea1ac..496557520 100644
--- a/aws_lambda_builders/workflows/nodejs_npm/npm.py
+++ b/aws_lambda_builders/workflows/nodejs_npm/npm.py
@@ -6,8 +6,6 @@
 
 from aws_lambda_builders.exceptions import LambdaBuilderError
 
-from .utils import OSUtils
-
 LOG = logging.getLogger(__name__)
 
 
@@ -17,9 +15,7 @@ class NpmExecutionError(LambdaBuilderError):
 
 class SubprocessNpm(object):
 
-    def __init__(self, osutils=None, npm_exe=None):
-        if osutils is None:
-            osutils = OSUtils()
+    def __init__(self, osutils, npm_exe=None):
         self.osutils = osutils
 
         if npm_exe is None:
diff --git a/aws_lambda_builders/workflows/nodejs_npm/workflow.py b/aws_lambda_builders/workflows/nodejs_npm/workflow.py
index 40963325b..187558542 100644
--- a/aws_lambda_builders/workflows/nodejs_npm/workflow.py
+++ b/aws_lambda_builders/workflows/nodejs_npm/workflow.py
@@ -7,6 +7,7 @@
 
 from .actions import NodejsNpmPackAction, NodejsNpmInstallAction
 from .utils import OSUtils
+from .npm import SubprocessNpm
 
 
 class NodejsNpmWorkflow(BaseWorkflow):
@@ -38,11 +39,23 @@ def __init__(self,
         if osutils is None:
             osutils = OSUtils()
 
+        subprocess_npm = SubprocessNpm(osutils)
+
         tar_dest_dir = osutils.joinpath(scratch_dir, 'unpacked')
         tar_package_dir = osutils.joinpath(tar_dest_dir, 'package')
 
+        npm_pack = NodejsNpmPackAction(tar_dest_dir,
+                                       scratch_dir,
+                                       manifest_path,
+                                       osutils=osutils,
+                                       subprocess_npm=subprocess_npm)
+
+        npm_install = NodejsNpmInstallAction(artifacts_dir,
+                                             manifest_path,
+                                             osutils=osutils,
+                                             subprocess_npm=subprocess_npm)
         self.actions = [
-            NodejsNpmPackAction(tar_dest_dir, scratch_dir, manifest_path, runtime),
+            npm_pack,
             CopySourceAction(tar_package_dir, artifacts_dir, excludes=self.EXCLUDED_FILES),
-            NodejsNpmInstallAction(artifacts_dir, scratch_dir, manifest_path, runtime)
+            npm_install,
         ]
diff --git a/tests/unit/workflows/nodejs_npm/test_actions.py b/tests/unit/workflows/nodejs_npm/test_actions.py
index a2d6882fe..87f94241d 100644
--- a/tests/unit/workflows/nodejs_npm/test_actions.py
+++ b/tests/unit/workflows/nodejs_npm/test_actions.py
@@ -17,8 +17,9 @@ def test_tars_and_unpacks_npm_project(self, OSUtilMock, SubprocessNpmMock):
         subprocess_npm = SubprocessNpmMock.return_value
 
         action = NodejsNpmPackAction("artifacts", "scratch_dir",
-                                     "manifest", "runtime",
-                                     osutils=osutils, subprocess_npm=subprocess_npm)
+                                     "manifest",
+                                     osutils=osutils,
+                                     subprocess_npm=subprocess_npm)
 
         osutils.dirname.side_effect = lambda value: "/dir:{}".format(value)
         osutils.abspath.side_effect = lambda value: "/abs:{}".format(value)
@@ -41,13 +42,13 @@ def test_raises_action_failed_when_npm_fails(self, OSUtilMock, SubprocessNpmMock
         builder_instance.run.side_effect = NpmExecutionError(message="boom!")
 
         action = NodejsNpmPackAction("artifacts", "scratch_dir",
-                                     "manifest", "runtime",
+                                     "manifest",
                                      osutils=osutils, subprocess_npm=subprocess_npm)
 
         with self.assertRaises(ActionFailedError) as raised:
             action.execute()
 
-        self.assertEqual(raised.exception.args[0], "NPM Failed: boom!")
+        assert raised.exception.args[0] == "NPM Failed: boom!"
 
 
 class TestNodejsNpmInstallAction(TestCase):
@@ -58,9 +59,9 @@ def test_tars_and_unpacks_npm_project(self, OSUtilMock, SubprocessNpmMock):
         osutils = OSUtilMock.return_value
         subprocess_npm = SubprocessNpmMock.return_value
 
-        action = NodejsNpmInstallAction("artifacts", "scratch_dir",
-                                        "manifest", "runtime",
-                                        osutils=osutils, subprocess_npm=subprocess_npm)
+        action = NodejsNpmInstallAction("artifacts", "manifest",
+                                        osutils=osutils,
+                                        subprocess_npm=subprocess_npm)
 
         action.execute()
 
@@ -77,11 +78,12 @@ def test_raises_action_failed_when_npm_fails(self, OSUtilMock, SubprocessNpmMock
         builder_instance = SubprocessNpmMock.return_value
         builder_instance.run.side_effect = NpmExecutionError(message="boom!")
 
-        action = NodejsNpmInstallAction("artifacts", "scratch_dir",
-                                        "manifest", "runtime",
-                                        osutils=osutils, subprocess_npm=subprocess_npm)
+        action = NodejsNpmInstallAction("artifacts",
+                                        "manifest",
+                                        osutils=osutils,
+                                        subprocess_npm=subprocess_npm)
 
         with self.assertRaises(ActionFailedError) as raised:
             action.execute()
 
-        self.assertEqual(raised.exception.args[0], "NPM Failed: boom!")
+        assert raised.exception.args[0] == "NPM Failed: boom!"

From d141caf9b61c39d2aeab763367921b15a65c2568 Mon Sep 17 00:00:00 2001
From: Gojko Adzic <gojko@gojko.com>
Date: Tue, 4 Dec 2018 09:52:08 +0100
Subject: [PATCH 20/34] - better exception message interpolation, as per
 https://github.com/awslabs/aws-lambda-builders/pull/44#discussion_r238473374

---
 aws_lambda_builders/workflows/nodejs_npm/npm.py | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/aws_lambda_builders/workflows/nodejs_npm/npm.py b/aws_lambda_builders/workflows/nodejs_npm/npm.py
index 496557520..ec000548f 100644
--- a/aws_lambda_builders/workflows/nodejs_npm/npm.py
+++ b/aws_lambda_builders/workflows/nodejs_npm/npm.py
@@ -4,14 +4,15 @@
 
 import logging
 
-from aws_lambda_builders.exceptions import LambdaBuilderError
-
 LOG = logging.getLogger(__name__)
 
 
-class NpmExecutionError(LambdaBuilderError):
+class NpmExecutionError(Exception):
     MESSAGE = "NPM Failed: {message}"
 
+    def __init__(self, **kwargs):
+        Exception.__init__(self, self.MESSAGE.format(**kwargs))
+
 
 class SubprocessNpm(object):
 

From 7d5a1d0bcf45a75a41e676e35808940f071fa528 Mon Sep 17 00:00:00 2001
From: Gojko Adzic <gojko@gojko.com>
Date: Tue, 4 Dec 2018 17:28:30 +0100
Subject: [PATCH 21/34] documentation and cleanup

---
 .../workflows/nodejs_npm/actions.py           | 51 +++++++++++++++++--
 .../workflows/nodejs_npm/npm.py               | 38 ++++++++++++++
 .../workflows/nodejs_npm/utils.py             |  5 ++
 .../workflows/nodejs_npm/workflow.py          |  6 ++-
 .../unit/workflows/nodejs_npm/test_actions.py | 13 ++---
 .../workflows/nodejs_npm/test_workflow.py     | 28 ++++++++++
 6 files changed, 125 insertions(+), 16 deletions(-)
 create mode 100644 tests/unit/workflows/nodejs_npm/test_workflow.py

diff --git a/aws_lambda_builders/workflows/nodejs_npm/actions.py b/aws_lambda_builders/workflows/nodejs_npm/actions.py
index ce72f292e..08a9a76f2 100644
--- a/aws_lambda_builders/workflows/nodejs_npm/actions.py
+++ b/aws_lambda_builders/workflows/nodejs_npm/actions.py
@@ -11,11 +11,32 @@
 
 class NodejsNpmPackAction(BaseAction):
 
+    """
+    A Lambda Builder Action that packages a Node.js package using NPM to extract the source and remove test resources
+    """
+
     NAME = 'NpmPack'
     DESCRIPTION = "Packaging source using NPM"
     PURPOSE = Purpose.COPY_SOURCE
 
     def __init__(self, artifacts_dir, scratch_dir, manifest_path, osutils, subprocess_npm):
+        """
+        :type artifacts_dir: str
+        :param artifacts_dir: an existing (writable) directory where to store the output.
+            Note that the actual result will be in the 'package' subdirectory here.
+
+        :type scratch_dir: str
+        :param scratch_dir: an existing (writable) directory for temporary files
+
+        :type manifest_path: str
+        :param manifest_path: path to package.json of an NPM project with the source to pack
+
+        :type osutils: aws_lambda_builders.workflows.nodejs_npm.utils.OSUtils
+        :param osutils: An instance of OS Utilities for file manipulation
+
+        :type subprocess_npm: aws_lambda_builders.workflows.nodejs_npm.npm.SubprocessNpm
+        :param subprocess_npm: An instance of the NPM process wrapper
+        """
         self.artifacts_dir = artifacts_dir
         self.manifest_path = manifest_path
         self.scratch_dir = scratch_dir
@@ -23,6 +44,11 @@ def __init__(self, artifacts_dir, scratch_dir, manifest_path, osutils, subproces
         self.subprocess_npm = subprocess_npm
 
     def execute(self):
+        """
+        Runs the action.
+
+        :raises lambda_builders.actions.ActionFailedError: when NPM packaging fails
+        """
         try:
             package_path = "file:{}".format(self.osutils.abspath(self.osutils.dirname(self.manifest_path)))
 
@@ -44,19 +70,36 @@ def execute(self):
 
 class NodejsNpmInstallAction(BaseAction):
 
+    """
+    A Lambda Builder Action that installs NPM project dependencies
+    """
+
     NAME = 'NpmInstall'
     DESCRIPTION = "Installing dependencies from NPM"
     PURPOSE = Purpose.RESOLVE_DEPENDENCIES
 
-    def __init__(self, artifacts_dir, manifest_path, osutils, subprocess_npm):
+    def __init__(self, artifacts_dir, subprocess_npm):
+        """
+        :type artifacts_dir: str
+        :param artifacts_dir: an existing (writable) directory with project source files.
+            Dependencies will be installed in this directory.
+
+        :type subprocess_npm: aws_lambda_builders.workflows.nodejs_npm.npm.SubprocessNpm
+        :param subprocess_npm: An instance of the NPM process wrapper
+        """
+
         self.artifacts_dir = artifacts_dir
-        self.manifest_path = manifest_path
-        self.osutils = osutils
         self.subprocess_npm = subprocess_npm
 
     def execute(self):
+        """
+        Runs the action.
+
+        :raises lambda_builders.actions.ActionFailedError: when NPM execution fails
+        """
+
         try:
-            LOG.debug("NODEJS installing in: %s from: %s", self.artifacts_dir, self.manifest_path)
+            LOG.debug("NODEJS installing in: %s", self.artifacts_dir)
 
             self.subprocess_npm.run(
                     ['install', '-q', '--no-audit', '--no-save', '--production'],
diff --git a/aws_lambda_builders/workflows/nodejs_npm/npm.py b/aws_lambda_builders/workflows/nodejs_npm/npm.py
index ec000548f..5ba01a450 100644
--- a/aws_lambda_builders/workflows/nodejs_npm/npm.py
+++ b/aws_lambda_builders/workflows/nodejs_npm/npm.py
@@ -8,6 +8,12 @@
 
 
 class NpmExecutionError(Exception):
+
+    """
+    Exception raised in case NPM execution fails.
+    It will pass on the standard error output from the NPM console.
+    """
+
     MESSAGE = "NPM Failed: {message}"
 
     def __init__(self, **kwargs):
@@ -16,7 +22,20 @@ def __init__(self, **kwargs):
 
 class SubprocessNpm(object):
 
+    """
+    Wrapper around the NPM command line utility, making it
+    easy to consume execution results.
+    """
+
     def __init__(self, osutils, npm_exe=None):
+        """
+        :type osutils: aws_lambda_builders.workflows.nodejs_npm.utils.OSUtils
+        :param osutils: An instance of OS Utilities for file manipulation
+
+        :type npm_exe: str
+        :param npm_exe: Path to the NPM binary. If not set,
+            the default executable path npm will be used
+        """
         self.osutils = osutils
 
         if npm_exe is None:
@@ -26,6 +45,25 @@ def __init__(self, osutils, npm_exe=None):
 
     def run(self, args, cwd=None):
 
+        """
+        Runs the action.
+
+        :type args: list
+        :param args: Command line arguments to pass to NPM
+
+        :type cwd: str
+        :param cwd: Directory where to execute the command (defaults to current dir)
+
+        :rtype: str
+        :return: text of the standard output from the command
+
+        :raises aws_lambda_builders.workflows.nodejs_npm.npm.NpmExecutionError:
+            when the command executes with a non-zero return code. The exception will
+            contain the text of the standard error output from the command.
+
+        :raises ValueError: if arguments are not provided, or not a list
+        """
+
         if not isinstance(args, list):
             raise ValueError('args must be a list')
 
diff --git a/aws_lambda_builders/workflows/nodejs_npm/utils.py b/aws_lambda_builders/workflows/nodejs_npm/utils.py
index 0d1f49567..1878c1809 100644
--- a/aws_lambda_builders/workflows/nodejs_npm/utils.py
+++ b/aws_lambda_builders/workflows/nodejs_npm/utils.py
@@ -9,6 +9,11 @@
 
 class OSUtils(object):
 
+    """
+    Wrapper around file system functions, to make it easy to
+    unit test actions in memory
+    """
+
     def extract_tarfile(self, tarfile_path, unpack_dir):
         with tarfile.open(tarfile_path, 'r:*') as tar:
             tar.extractall(unpack_dir)
diff --git a/aws_lambda_builders/workflows/nodejs_npm/workflow.py b/aws_lambda_builders/workflows/nodejs_npm/workflow.py
index 187558542..70e27c05a 100644
--- a/aws_lambda_builders/workflows/nodejs_npm/workflow.py
+++ b/aws_lambda_builders/workflows/nodejs_npm/workflow.py
@@ -12,6 +12,10 @@
 
 class NodejsNpmWorkflow(BaseWorkflow):
 
+    """
+    A Lambda builder workflow that knows how to pack
+    NodeJS projects using NPM.
+    """
     NAME = "NodejsNpmBuilder"
 
     CAPABILITY = Capability(language="nodejs",
@@ -51,8 +55,6 @@ def __init__(self,
                                        subprocess_npm=subprocess_npm)
 
         npm_install = NodejsNpmInstallAction(artifacts_dir,
-                                             manifest_path,
-                                             osutils=osutils,
                                              subprocess_npm=subprocess_npm)
         self.actions = [
             npm_pack,
diff --git a/tests/unit/workflows/nodejs_npm/test_actions.py b/tests/unit/workflows/nodejs_npm/test_actions.py
index 87f94241d..8bd5f335f 100644
--- a/tests/unit/workflows/nodejs_npm/test_actions.py
+++ b/tests/unit/workflows/nodejs_npm/test_actions.py
@@ -53,14 +53,11 @@ def test_raises_action_failed_when_npm_fails(self, OSUtilMock, SubprocessNpmMock
 
 class TestNodejsNpmInstallAction(TestCase):
 
-    @patch("aws_lambda_builders.workflows.nodejs_npm.utils.OSUtils")
     @patch("aws_lambda_builders.workflows.nodejs_npm.npm.SubprocessNpm")
-    def test_tars_and_unpacks_npm_project(self, OSUtilMock, SubprocessNpmMock):
-        osutils = OSUtilMock.return_value
+    def test_tars_and_unpacks_npm_project(self, SubprocessNpmMock):
         subprocess_npm = SubprocessNpmMock.return_value
 
-        action = NodejsNpmInstallAction("artifacts", "manifest",
-                                        osutils=osutils,
+        action = NodejsNpmInstallAction("artifacts",
                                         subprocess_npm=subprocess_npm)
 
         action.execute()
@@ -69,18 +66,14 @@ def test_tars_and_unpacks_npm_project(self, OSUtilMock, SubprocessNpmMock):
 
         subprocess_npm.run.assert_called_with(expected_args, cwd='artifacts')
 
-    @patch("aws_lambda_builders.workflows.nodejs_npm.utils.OSUtils")
     @patch("aws_lambda_builders.workflows.nodejs_npm.npm.SubprocessNpm")
-    def test_raises_action_failed_when_npm_fails(self, OSUtilMock, SubprocessNpmMock):
-        osutils = OSUtilMock.return_value
+    def test_raises_action_failed_when_npm_fails(self, SubprocessNpmMock):
         subprocess_npm = SubprocessNpmMock.return_value
 
         builder_instance = SubprocessNpmMock.return_value
         builder_instance.run.side_effect = NpmExecutionError(message="boom!")
 
         action = NodejsNpmInstallAction("artifacts",
-                                        "manifest",
-                                        osutils=osutils,
                                         subprocess_npm=subprocess_npm)
 
         with self.assertRaises(ActionFailedError) as raised:
diff --git a/tests/unit/workflows/nodejs_npm/test_workflow.py b/tests/unit/workflows/nodejs_npm/test_workflow.py
new file mode 100644
index 000000000..f72d32a1c
--- /dev/null
+++ b/tests/unit/workflows/nodejs_npm/test_workflow.py
@@ -0,0 +1,28 @@
+
+from unittest import TestCase
+
+from aws_lambda_builders.workflows.nodejs_npm.workflow import NodejsNpmWorkflow
+
+from aws_lambda_builders.workflows.nodejs_npm.actions import NodejsNpmPackAction, NodejsNpmInstallAction
+
+from aws_lambda_builders.actions import CopySourceAction
+
+
+class TestNodejsNpmWorkflow(TestCase):
+
+    """
+    the workflow requires an external utility (npm) to run, so it is extensively tested in integration tests.
+    this is just a quick wiring test to provide fast feedback if things are badly broken
+    """
+
+    def test_workflow_sets_up_npm_actions(self):
+
+        workflow = NodejsNpmWorkflow("source", "artifacts", "scratch_dir", "manifest")
+
+        assert len(workflow.actions) == 3
+
+        assert isinstance(workflow.actions[0], NodejsNpmPackAction)
+
+        assert isinstance(workflow.actions[1], CopySourceAction)
+
+        assert isinstance(workflow.actions[2], NodejsNpmInstallAction)

From 70517bff564897aad4df00e4b9cf5b6ba8d32a67 Mon Sep 17 00:00:00 2001
From: Gojko Adzic <gojko@gojko.com>
Date: Tue, 4 Dec 2018 17:49:07 +0100
Subject: [PATCH 22/34] tar required for functional tests

---
 .../workflows/nodejs_npm/test_data/test.tgz       | Bin 0 -> 769 bytes
 1 file changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 tests/functional/workflows/nodejs_npm/test_data/test.tgz

diff --git a/tests/functional/workflows/nodejs_npm/test_data/test.tgz b/tests/functional/workflows/nodejs_npm/test_data/test.tgz
new file mode 100644
index 0000000000000000000000000000000000000000..8c2c2fc72c96f4a6e4dcddaa13768750849adf57
GIT binary patch
literal 769
zcmV+c1OEIUiwFRsI|f_;1MOB_Yuhjs?X!P{$R6x~5l)-5V-VOs*u%D`F@$1Yw@z$J
zNHS>{`|rE*hicbZMj54y={(p{bkEh%J=Y=zv|)`YN=@bI>UEGL$?bYg?ECI^)4r4I
zZf>z&tu|zJvrbm)o6YU@hTve8++C66mGn%|Xu}msxflC`xDbb7d46HNI}35|e*{G(
zg)&5*3^qa#^izG(ZY-XL`oVUJn;NKOUX&07xe^uON6kuJ?J~}GO;Ki`G(8G+$V+k5
zv=$lcHLGPsdV1S+K@fcGHm7;<Xc`4U203opu13ob0YNChmAYflPoKYbYsgJYt~L3B
z^x;>r+e2!4o=s(QGk7$Lr-q@|RxxX8$<u>o-2M`2DD!yO6R9oNqUB-)+2Sgd+&s{|
zDC%gKWQD4E1rcLyR<?}EqUB`V@I^nNm|(GdmK2$obf_S$4w)TCqp)5MeI`8`G{r8_
znA8~Am{yI({2GkaS{B-1T;vEN#=G8Csw$%ZqBBXQDr(F#N<CW~M4exlrbEX7?f<d3
z#w%y>lWeyW*}>GgzQn9+0d42#uTUqJ3&pGpGy8^(dTMK4SIfaK>~E|qq>bUb66V?*
z^U3#%RHv5jw2gLZaW2Znz<Hr=TLro)%}8jUCkkmeS2sv1G=@bTcOEt5d^*BBsncz{
zx)p8*x>d_kf!}Ejwb3sSJ|v1l9>yfx*++Wj40CnuhjotSL`or{W~#AuWm1V0G>+=b
zRU6v~MZ@pqrpJuQ(+j@lkkZgStqA*Jx9W}=cJKILa#V#@Y4mN{D{;U5_t<gfz#Z@u
zTbHPi@P}|vY7%}SdU{dB1gR)mQY>24)J4h-M02Yo?dqH24Q>rf<_*~xnt@%9!p7w9
z!zI;56?mQX2H%6hvq$F*O?l0Dnu0XU23@?2ZTb7V^Y1KtnQ?>NuWa3=a8$4zSK!UO
zoLnVSMQ{CQ#>2zI!^6YF!^6YF!^6YF!^6YF!^6YF!^6YF<3GkP9<tbs04M+e5uk_E

literal 0
HcmV?d00001


From 7eaad47750fa586b2509078816f8f7dc1dad9570 Mon Sep 17 00:00:00 2001
From: Sanath Kumar Ramesh <sanathkr@amazon.com>
Date: Tue, 4 Dec 2018 10:50:24 -0800
Subject: [PATCH 23/34] Install nodejs8.10 in travis & appveyor

---
 .appveyor.yml | 3 +++
 .travis.yml   | 5 +++++
 2 files changed, 8 insertions(+)

diff --git a/.appveyor.yml b/.appveyor.yml
index 1e458eb55..a6954b9e5 100644
--- a/.appveyor.yml
+++ b/.appveyor.yml
@@ -13,6 +13,9 @@ environment:
 build: off
 
 install:
+# To run Nodejs workflow integ tests
+- ps: Install-Product node 8.10
+
 - "set PATH=%PYTHON%\\Scripts;%PYTHON%\\bin;%PATH%"
 - "%PYTHON%\\python.exe -m pip install -r requirements/dev.txt"
 - "%PYTHON%\\python.exe -m pip install -e ."
diff --git a/.travis.yml b/.travis.yml
index 62494ef7e..c22f0aebc 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -11,6 +11,11 @@ matrix:
       dist: xenial
       sudo: true
 install:
+
+  # To run Nodejs workflow integ tests
+  - nvm install 8.10.0
+  - nvm use 8.10.0
+
   # Install the code requirements
   - make init
 script:

From bbb92a540c6b68cec6ae0c305f4aa5a032dc7197 Mon Sep 17 00:00:00 2001
From: Gojko Adzic <gojko@gojko.com>
Date: Tue, 4 Dec 2018 20:22:17 +0100
Subject: [PATCH 24/34] experiment to see if this will fix it for windows

---
 aws_lambda_builders/workflows/nodejs_npm/npm.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/aws_lambda_builders/workflows/nodejs_npm/npm.py b/aws_lambda_builders/workflows/nodejs_npm/npm.py
index 5ba01a450..1085e3df2 100644
--- a/aws_lambda_builders/workflows/nodejs_npm/npm.py
+++ b/aws_lambda_builders/workflows/nodejs_npm/npm.py
@@ -39,7 +39,7 @@ def __init__(self, osutils, npm_exe=None):
         self.osutils = osutils
 
         if npm_exe is None:
-            npm_exe = 'npm'
+            npm_exe = 'npm.cmd'
 
         self.npm_exe = npm_exe
 

From 9270a81be5edffb88e7e4ba80bf97996cf569c63 Mon Sep 17 00:00:00 2001
From: Gojko Adzic <gojko@gojko.com>
Date: Tue, 4 Dec 2018 20:31:20 +0100
Subject: [PATCH 25/34] try fixing for windows

---
 .../workflows/nodejs_npm/npm.py               |  7 ++++++-
 tests/unit/workflows/nodejs_npm/test_npm.py   | 19 +++++++++----------
 2 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/aws_lambda_builders/workflows/nodejs_npm/npm.py b/aws_lambda_builders/workflows/nodejs_npm/npm.py
index 1085e3df2..4e352870a 100644
--- a/aws_lambda_builders/workflows/nodejs_npm/npm.py
+++ b/aws_lambda_builders/workflows/nodejs_npm/npm.py
@@ -4,6 +4,8 @@
 
 import logging
 
+import platform
+
 LOG = logging.getLogger(__name__)
 
 
@@ -39,7 +41,10 @@ def __init__(self, osutils, npm_exe=None):
         self.osutils = osutils
 
         if npm_exe is None:
-            npm_exe = 'npm.cmd'
+            if platform.system() == 'Windows':
+                npm_exe = 'npm.cmd'
+            else:
+                npm_exe = 'npm'
 
         self.npm_exe = npm_exe
 
diff --git a/tests/unit/workflows/nodejs_npm/test_npm.py b/tests/unit/workflows/nodejs_npm/test_npm.py
index 3c8dc99df..373ac153b 100644
--- a/tests/unit/workflows/nodejs_npm/test_npm.py
+++ b/tests/unit/workflows/nodejs_npm/test_npm.py
@@ -24,19 +24,17 @@ def setUp(self, OSUtilMock):
         self.osutils.pipe = 'PIPE'
         self.popen = FakePopen()
         self.osutils.popen.side_effect = [self.popen]
-        self.under_test = SubprocessNpm(self.osutils)
+        self.under_test = SubprocessNpm(self.osutils, npm_exe="/a/b/c/npm.exe")
 
-    def test_run_executes_npm_via_popen(self):
-
-        self.under_test.run(['pack', '-q'])
-
-        self.osutils.popen.assert_called_with(['npm', 'pack', '-q'], cwd=None, stderr='PIPE', stdout='PIPE')
+#    def test_run_executes_npm_via_popen(self):
+#
+#        self.under_test.run(['pack', '-q'])
+#
+#        self.osutils.popen.assert_called_with(['npm', 'pack', '-q'], cwd=None, stderr='PIPE', stdout='PIPE')
 
     def test_uses_custom_npm_path_if_supplied(self):
 
-        npm = SubprocessNpm(self.osutils, npm_exe="/a/b/c/npm.exe")
-
-        npm.run(['pack', '-q'])
+        self.under_test.run(['pack', '-q'])
 
         self.osutils.popen.assert_called_with(['/a/b/c/npm.exe', 'pack', '-q'], cwd=None, stderr='PIPE', stdout='PIPE')
 
@@ -44,7 +42,8 @@ def test_uses_cwd_if_supplied(self):
 
         self.under_test.run(['pack', '-q'], cwd='/a/cwd')
 
-        self.osutils.popen.assert_called_with(['npm', 'pack', '-q'], cwd='/a/cwd', stderr='PIPE', stdout='PIPE')
+        self.osutils.popen.assert_called_with(['/a/b/c/npm.exe', 'pack', '-q'],
+                                              cwd='/a/cwd', stderr='PIPE', stdout='PIPE')
 
     def test_returns_popen_out_decoded_if_retcode_is_0(self):
 

From 6f23b113a0e5087bc82d41d22765aa3025527a84 Mon Sep 17 00:00:00 2001
From: Gojko Adzic <gojko@gojko.com>
Date: Tue, 4 Dec 2018 21:52:41 +0100
Subject: [PATCH 26/34] better testing for windows

---
 .../workflows/nodejs_npm/npm.py               |  4 +---
 .../workflows/nodejs_npm/utils.py             |  4 ++++
 tests/unit/workflows/nodejs_npm/test_npm.py   | 24 +++++++++++++++----
 3 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/aws_lambda_builders/workflows/nodejs_npm/npm.py b/aws_lambda_builders/workflows/nodejs_npm/npm.py
index 4e352870a..b737ff4d1 100644
--- a/aws_lambda_builders/workflows/nodejs_npm/npm.py
+++ b/aws_lambda_builders/workflows/nodejs_npm/npm.py
@@ -4,8 +4,6 @@
 
 import logging
 
-import platform
-
 LOG = logging.getLogger(__name__)
 
 
@@ -41,7 +39,7 @@ def __init__(self, osutils, npm_exe=None):
         self.osutils = osutils
 
         if npm_exe is None:
-            if platform.system() == 'Windows':
+            if osutils.is_windows():
                 npm_exe = 'npm.cmd'
             else:
                 npm_exe = 'npm'
diff --git a/aws_lambda_builders/workflows/nodejs_npm/utils.py b/aws_lambda_builders/workflows/nodejs_npm/utils.py
index 1878c1809..1aeebdbe6 100644
--- a/aws_lambda_builders/workflows/nodejs_npm/utils.py
+++ b/aws_lambda_builders/workflows/nodejs_npm/utils.py
@@ -5,6 +5,7 @@
 import os
 import tarfile
 import subprocess
+import sys
 
 
 class OSUtils(object):
@@ -34,3 +35,6 @@ def dirname(self, path):
 
     def abspath(self, path):
         return os.path.abspath(path)
+
+    def is_windows(self):
+        return sys.platform.lower().startswith('win')
diff --git a/tests/unit/workflows/nodejs_npm/test_npm.py b/tests/unit/workflows/nodejs_npm/test_npm.py
index 373ac153b..edaa39c2a 100644
--- a/tests/unit/workflows/nodejs_npm/test_npm.py
+++ b/tests/unit/workflows/nodejs_npm/test_npm.py
@@ -26,11 +26,25 @@ def setUp(self, OSUtilMock):
         self.osutils.popen.side_effect = [self.popen]
         self.under_test = SubprocessNpm(self.osutils, npm_exe="/a/b/c/npm.exe")
 
-#    def test_run_executes_npm_via_popen(self):
-#
-#        self.under_test.run(['pack', '-q'])
-#
-#        self.osutils.popen.assert_called_with(['npm', 'pack', '-q'], cwd=None, stderr='PIPE', stdout='PIPE')
+    def test_run_executes_npm_on_nixes(self):
+
+        self.osutils.is_windows.side_effect = [False]
+
+        self.under_test = SubprocessNpm(self.osutils)
+
+        self.under_test.run(['pack', '-q'])
+
+        self.osutils.popen.assert_called_with(['npm', 'pack', '-q'], cwd=None, stderr='PIPE', stdout='PIPE')
+
+    def test_run_executes_npm_cmd_on_windows(self):
+
+        self.osutils.is_windows.side_effect = [True]
+
+        self.under_test = SubprocessNpm(self.osutils)
+
+        self.under_test.run(['pack', '-q'])
+
+        self.osutils.popen.assert_called_with(['npm.cmd', 'pack', '-q'], cwd=None, stderr='PIPE', stdout='PIPE')
 
     def test_uses_custom_npm_path_if_supplied(self):
 

From 53f9d349da988abf445ce06b4d291240c69bbb7c Mon Sep 17 00:00:00 2001
From: Gojko Adzic <gojko@gojko.com>
Date: Wed, 5 Dec 2018 08:34:42 +0100
Subject: [PATCH 27/34] calling parent constructor for actions, as per
 https://github.com/awslabs/aws-lambda-builders/pull/44#discussion_r238843044

---
 aws_lambda_builders/workflows/nodejs_npm/actions.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/aws_lambda_builders/workflows/nodejs_npm/actions.py b/aws_lambda_builders/workflows/nodejs_npm/actions.py
index 08a9a76f2..9ab313102 100644
--- a/aws_lambda_builders/workflows/nodejs_npm/actions.py
+++ b/aws_lambda_builders/workflows/nodejs_npm/actions.py
@@ -37,6 +37,7 @@ def __init__(self, artifacts_dir, scratch_dir, manifest_path, osutils, subproces
         :type subprocess_npm: aws_lambda_builders.workflows.nodejs_npm.npm.SubprocessNpm
         :param subprocess_npm: An instance of the NPM process wrapper
         """
+        super(NodejsNpmPackAction, self).__init__()
         self.artifacts_dir = artifacts_dir
         self.manifest_path = manifest_path
         self.scratch_dir = scratch_dir
@@ -88,6 +89,7 @@ def __init__(self, artifacts_dir, subprocess_npm):
         :param subprocess_npm: An instance of the NPM process wrapper
         """
 
+        super(NodejsNpmInstallAction, self).__init__()
         self.artifacts_dir = artifacts_dir
         self.subprocess_npm = subprocess_npm
 

From 2c5d7807a95f38729ce0dc1dc4f4bf776b3b7a9c Mon Sep 17 00:00:00 2001
From: Gojko Adzic <gojko@gojko.com>
Date: Wed, 5 Dec 2018 08:44:18 +0100
Subject: [PATCH 28/34] Using platform to check for windows, to make it
 consistent with SAM CLI (as per
 https://github.com/awslabs/aws-lambda-builders/pull/44#discussion_r238848904)

---
 aws_lambda_builders/workflows/nodejs_npm/utils.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/aws_lambda_builders/workflows/nodejs_npm/utils.py b/aws_lambda_builders/workflows/nodejs_npm/utils.py
index 1aeebdbe6..dcff97272 100644
--- a/aws_lambda_builders/workflows/nodejs_npm/utils.py
+++ b/aws_lambda_builders/workflows/nodejs_npm/utils.py
@@ -3,9 +3,9 @@
 """
 
 import os
+import platform
 import tarfile
 import subprocess
-import sys
 
 
 class OSUtils(object):
@@ -37,4 +37,4 @@ def abspath(self, path):
         return os.path.abspath(path)
 
     def is_windows(self):
-        return sys.platform.lower().startswith('win')
+        return platform.system().lower() == 'windows'

From ed23979bc52ed87b6b2e1e3700d7051dcbd4b0f1 Mon Sep 17 00:00:00 2001
From: Gojko Adzic <gojko@gojko.com>
Date: Wed, 5 Dec 2018 08:56:07 +0100
Subject: [PATCH 29/34] using unittest instead of pytest (as per
 https://github.com/awslabs/aws-lambda-builders/pull/44#discussion_r238850262)

---
 .../workflows/nodejs_npm/test_utils.py        | 44 +++++++++----------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/tests/functional/workflows/nodejs_npm/test_utils.py b/tests/functional/workflows/nodejs_npm/test_utils.py
index dc077cc54..e8074e957 100644
--- a/tests/functional/workflows/nodejs_npm/test_utils.py
+++ b/tests/functional/workflows/nodejs_npm/test_utils.py
@@ -1,5 +1,3 @@
-import pytest
-
 import sys
 
 import os
@@ -8,22 +6,24 @@
 
 import tempfile
 
+from unittest import TestCase
+
 from aws_lambda_builders.workflows.nodejs_npm import utils
 
 
-@pytest.fixture
-def osutils():
-    return utils.OSUtils()
+class TestOSUtils(TestCase):
+
+    def setUp(self):
 
+        self.osutils = utils.OSUtils()
 
-class TestOSUtils(object):
-    def test_extract_tarfile_unpacks_a_tar(self, osutils):
+    def test_extract_tarfile_unpacks_a_tar(self):
 
         test_tar = os.path.join(os.path.dirname(__file__), "test_data", "test.tgz")
 
         test_dir = tempfile.mkdtemp()
 
-        osutils.extract_tarfile(test_tar, test_dir)
+        self.osutils.extract_tarfile(test_tar, test_dir)
 
         output_files = set(os.listdir(test_dir))
 
@@ -31,32 +31,32 @@ def test_extract_tarfile_unpacks_a_tar(self, osutils):
 
         assert {"test_utils.py"} == output_files
 
-    def test_dirname_returns_directory_for_path(self, osutils):
-        dirname = osutils.dirname(sys.executable)
+    def test_dirname_returns_directory_for_path(self):
+        dirname = self.osutils.dirname(sys.executable)
 
         assert dirname == os.path.dirname(sys.executable)
 
-    def test_abspath_returns_absolute_path(self, osutils):
+    def test_abspath_returns_absolute_path(self):
 
-        result = osutils.abspath('.')
+        result = self.osutils.abspath('.')
 
         assert os.path.isabs(result)
 
         assert result == os.path.abspath('.')
 
-    def test_joinpath_joins_path_components(self, osutils):
+    def test_joinpath_joins_path_components(self):
 
-        result = osutils.joinpath('a', 'b', 'c')
+        result = self.osutils.joinpath('a', 'b', 'c')
 
         assert result == os.path.join('a', 'b', 'c')
 
-    def test_popen_runs_a_process_and_returns_outcome(self, osutils):
+    def test_popen_runs_a_process_and_returns_outcome(self):
 
         cwd_py = os.path.join(os.path.dirname(__file__), '..', '..', 'testdata', 'cwd.py')
 
-        p = osutils.popen([sys.executable, cwd_py],
-                          stdout=osutils.pipe,
-                          stderr=osutils.pipe
+        p = self.osutils.popen([sys.executable, cwd_py],
+                          stdout=self.osutils.pipe,
+                          stderr=self.osutils.pipe
                           )
 
         out, err = p.communicate()
@@ -65,13 +65,13 @@ def test_popen_runs_a_process_and_returns_outcome(self, osutils):
 
         assert out.decode('utf8').strip() == os.getcwd()
 
-    def test_popen_can_accept_cwd(self, osutils):
+    def test_popen_can_accept_cwd(self):
 
         testdata_dir = os.path.join(os.path.dirname(__file__), '..', '..', 'testdata')
 
-        p = osutils.popen([sys.executable, 'cwd.py'],
-                          stdout=osutils.pipe,
-                          stderr=osutils.pipe,
+        p = self.osutils.popen([sys.executable, 'cwd.py'],
+                          stdout=self.osutils.pipe,
+                          stderr=self.osutils.pipe,
                           cwd=testdata_dir)
 
         out, err = p.communicate()

From fcd47a9753ed88e0995a26493882aa7b8fbe3402 Mon Sep 17 00:00:00 2001
From: Gojko Adzic <gojko@gojko.com>
Date: Wed, 5 Dec 2018 09:07:18 +0100
Subject: [PATCH 30/34] use self.assert* instead of assert (as per
 https://github.com/awslabs/aws-lambda-builders/pull/44#discussion_r238850817,
 https://github.com/awslabs/aws-lambda-builders/pull/44#discussion_r238850932)

---
 .../workflows/nodejs_npm/test_utils.py        | 29 +++++++++----------
 .../unit/workflows/nodejs_npm/test_actions.py |  4 +--
 tests/unit/workflows/nodejs_npm/test_npm.py   |  8 ++---
 .../workflows/nodejs_npm/test_workflow.py     |  8 ++---
 4 files changed, 24 insertions(+), 25 deletions(-)

diff --git a/tests/functional/workflows/nodejs_npm/test_utils.py b/tests/functional/workflows/nodejs_npm/test_utils.py
index e8074e957..e262107bc 100644
--- a/tests/functional/workflows/nodejs_npm/test_utils.py
+++ b/tests/functional/workflows/nodejs_npm/test_utils.py
@@ -29,53 +29,52 @@ def test_extract_tarfile_unpacks_a_tar(self):
 
         shutil.rmtree(test_dir)
 
-        assert {"test_utils.py"} == output_files
+        self.assertEqual({"test_utils.py"}, output_files)
 
     def test_dirname_returns_directory_for_path(self):
         dirname = self.osutils.dirname(sys.executable)
 
-        assert dirname == os.path.dirname(sys.executable)
+        self.assertEqual(dirname, os.path.dirname(sys.executable))
 
     def test_abspath_returns_absolute_path(self):
 
         result = self.osutils.abspath('.')
 
-        assert os.path.isabs(result)
+        self.assertTrue(os.path.isabs(result))
 
-        assert result == os.path.abspath('.')
+        self.assertEqual(result, os.path.abspath('.'))
 
     def test_joinpath_joins_path_components(self):
 
         result = self.osutils.joinpath('a', 'b', 'c')
 
-        assert result == os.path.join('a', 'b', 'c')
+        self.assertEqual(result, os.path.join('a', 'b', 'c'))
 
     def test_popen_runs_a_process_and_returns_outcome(self):
 
         cwd_py = os.path.join(os.path.dirname(__file__), '..', '..', 'testdata', 'cwd.py')
 
         p = self.osutils.popen([sys.executable, cwd_py],
-                          stdout=self.osutils.pipe,
-                          stderr=self.osutils.pipe
-                          )
+                               stdout=self.osutils.pipe,
+                               stderr=self.osutils.pipe)
 
         out, err = p.communicate()
 
-        assert p.returncode == 0
+        self.assertEqual(p.returncode, 0)
 
-        assert out.decode('utf8').strip() == os.getcwd()
+        self.assertEqual(out.decode('utf8').strip(), os.getcwd())
 
     def test_popen_can_accept_cwd(self):
 
         testdata_dir = os.path.join(os.path.dirname(__file__), '..', '..', 'testdata')
 
         p = self.osutils.popen([sys.executable, 'cwd.py'],
-                          stdout=self.osutils.pipe,
-                          stderr=self.osutils.pipe,
-                          cwd=testdata_dir)
+                               stdout=self.osutils.pipe,
+                               stderr=self.osutils.pipe,
+                               cwd=testdata_dir)
 
         out, err = p.communicate()
 
-        assert p.returncode == 0
+        self.assertEqual(p.returncode, 0)
 
-        assert out.decode('utf8').strip() == os.path.abspath(testdata_dir)
+        self.assertEqual(out.decode('utf8').strip(), os.path.abspath(testdata_dir))
diff --git a/tests/unit/workflows/nodejs_npm/test_actions.py b/tests/unit/workflows/nodejs_npm/test_actions.py
index 8bd5f335f..f3e494195 100644
--- a/tests/unit/workflows/nodejs_npm/test_actions.py
+++ b/tests/unit/workflows/nodejs_npm/test_actions.py
@@ -48,7 +48,7 @@ def test_raises_action_failed_when_npm_fails(self, OSUtilMock, SubprocessNpmMock
         with self.assertRaises(ActionFailedError) as raised:
             action.execute()
 
-        assert raised.exception.args[0] == "NPM Failed: boom!"
+        self.assertEqual(raised.exception.args[0], "NPM Failed: boom!")
 
 
 class TestNodejsNpmInstallAction(TestCase):
@@ -79,4 +79,4 @@ def test_raises_action_failed_when_npm_fails(self, SubprocessNpmMock):
         with self.assertRaises(ActionFailedError) as raised:
             action.execute()
 
-        assert raised.exception.args[0] == "NPM Failed: boom!"
+        self.assertEqual(raised.exception.args[0], "NPM Failed: boom!")
diff --git a/tests/unit/workflows/nodejs_npm/test_npm.py b/tests/unit/workflows/nodejs_npm/test_npm.py
index edaa39c2a..59ba6458f 100644
--- a/tests/unit/workflows/nodejs_npm/test_npm.py
+++ b/tests/unit/workflows/nodejs_npm/test_npm.py
@@ -65,7 +65,7 @@ def test_returns_popen_out_decoded_if_retcode_is_0(self):
 
         result = self.under_test.run(['pack'])
 
-        assert result == 'some encoded text'
+        self.assertEqual(result, 'some encoded text')
 
     def test_raises_NpmExecutionError_with_err_text_if_retcode_is_not_0(self):
 
@@ -75,18 +75,18 @@ def test_raises_NpmExecutionError_with_err_text_if_retcode_is_not_0(self):
         with self.assertRaises(NpmExecutionError) as raised:
             self.under_test.run(['pack'])
 
-        assert raised.exception.args[0] == "NPM Failed: some error text"
+        self.assertEqual(raised.exception.args[0], "NPM Failed: some error text")
 
     def test_raises_ValueError_if_args_not_a_list(self):
 
         with self.assertRaises(ValueError) as raised:
             self.under_test.run(('pack'))
 
-        assert raised.exception.args[0] == "args must be a list"
+        self.assertEqual(raised.exception.args[0], "args must be a list")
 
     def test_raises_ValueError_if_args_empty(self):
 
         with self.assertRaises(ValueError) as raised:
             self.under_test.run([])
 
-        assert raised.exception.args[0] == "requires at least one arg"
+        self.assertEqual(raised.exception.args[0], "requires at least one arg")
diff --git a/tests/unit/workflows/nodejs_npm/test_workflow.py b/tests/unit/workflows/nodejs_npm/test_workflow.py
index f72d32a1c..a7070c73f 100644
--- a/tests/unit/workflows/nodejs_npm/test_workflow.py
+++ b/tests/unit/workflows/nodejs_npm/test_workflow.py
@@ -19,10 +19,10 @@ def test_workflow_sets_up_npm_actions(self):
 
         workflow = NodejsNpmWorkflow("source", "artifacts", "scratch_dir", "manifest")
 
-        assert len(workflow.actions) == 3
+        self.assertEqual(len(workflow.actions), 3)
 
-        assert isinstance(workflow.actions[0], NodejsNpmPackAction)
+        self.assertIsInstance(workflow.actions[0], NodejsNpmPackAction)
 
-        assert isinstance(workflow.actions[1], CopySourceAction)
+        self.assertIsInstance(workflow.actions[1], CopySourceAction)
 
-        assert isinstance(workflow.actions[2], NodejsNpmInstallAction)
+        self.assertIsInstance(workflow.actions[2], NodejsNpmInstallAction)

From 09d41909bb0113f4824f562ed1d727b6cb637704 Mon Sep 17 00:00:00 2001
From: Gojko Adzic <gojko@gojko.com>
Date: Wed, 5 Dec 2018 09:52:11 +0100
Subject: [PATCH 31/34] added design document, as per
 https://github.com/awslabs/aws-lambda-builders/pull/44#issuecomment-444312410

---
 .../workflows/nodejs_npm/DESIGN.md            | 121 ++++++++++++++++++
 1 file changed, 121 insertions(+)
 create mode 100644 aws_lambda_builders/workflows/nodejs_npm/DESIGN.md

diff --git a/aws_lambda_builders/workflows/nodejs_npm/DESIGN.md b/aws_lambda_builders/workflows/nodejs_npm/DESIGN.md
new file mode 100644
index 000000000..6de6d4cd7
--- /dev/null
+++ b/aws_lambda_builders/workflows/nodejs_npm/DESIGN.md
@@ -0,0 +1,121 @@
+## NodeJS - NPM Lambda Builder
+
+### Scope
+
+This package is an effort to port the Claudia.JS packager to a library that can
+be used to handle the dependency resolution portion of packaging NodeJS code
+for use in AWS Lambda. The scope for this builder is to take an existing
+directory containing customer code, including a valid `package.json` manifest
+specifying third-party dependencies. The builder will use NPM to include
+production dependencies and exclude test resources in a way that makes them
+deployable to AWS Lambda.
+
+### Challenges
+
+NPM normally stores all dependencies in a `node_modules` subdirectory. It
+supports several dependency categories, such as development dependencies
+(usually third-party build utilities and test resources), optional dependencies
+(usually required for local execution but already available on the production
+environment, or peer-dependencies for optional third-party packages) and
+production dependencies (normally the minimum required for correct execution).
+All these dependency types are mixed in the same directory.
+
+To speed up Lambda startup time and optimise usage costs, the correct thing to
+do in most cases is just to package up production dependencies. During development 
+work we can expect that the local `node_modules` directory contains all the 
+various dependency types, and NPM does not provide a way to directly identify
+just the ones relevant for production. To identify production dependencies, 
+this packager needs to copy the source to a clean temporary directory and re-run
+dependency installation there.
+  
+It's reasonable to expect that some developers will not carefully separate
+production dependencies from test resources, so this packager will need to
+support overriding the categories of dependencies to include.
+
+NPM also provides support for running user-defined scripts as part of the build
+process, so this packager needs to support standard NPM script execution.
+
+NPM, since version 5, uses symbolic links to optimise disk space usage, so
+cross-project dependencies will just be linked to elsewhere on the local disk 
+instead of included in the `node_modules` directory. This means that just copying
+the `node_modules` directory (even if symlinks would be resolved to actual paths)
+far from optimal to create a stand-alone module. Copying would lead to significantly
+larger packages than necessary, as sub-modules might still have test resources, and
+common references from multiple projects would be duplicated.
+
+NPM also uses a locking mechanism (package-lock.json) that's in many ways more
+broken than functional, as it in some cases hard-codes locks to local disk
+paths, and gets confused by including the same package as a dependency
+throughout the project tree in different dependency categories
+(development/optional/production). Although the official tool recommends
+including this file in the version control, as a way to pin down dependency
+versions, when using on several machines with different project layout it can
+lead to uninstallable dependencies. 
+
+NPM dependencies are usually plain javascript libraries, but they may include
+native binaries precompiled for a particular platform, or require some system 
+libraries to be installed. A notable example is `sharp`, a popular image 
+manipulation library, that uses symbolic links to system libraries. Another 
+notable example is `puppeteer`, a library to control a headless Chrome browser,
+that downloads a Chromium binary for the target platform during installation.
+
+To fully deal with those cases, this packager may need to execute the
+dependency installation step on a Docker image compatible with the target
+Lambda environment.
+
+### Implementation
+
+The general algorithm for preparing a node package for use on AWS Lambda
+is as follows.
+
+#### Step 1: Prepare a clean copy of the project source files
+
+Execute `npm pack` to perform project-specific packaging using the supplied
+`package.json` manifest, which will automatically exclude temporary files, 
+test resources and other source files unnecessary for running in a production 
+environment.
+
+This will produce a `tar` archive that needs to be unpacked into the artefacts directory.
+Note that the archive will actually contain a `package` subdirectory containing the files, 
+so it's not enough to just directly unpack files. 
+
+#### Step 2: Rewrite local dependencies
+
+_(out of scope for the current version)_
+
+To optimise disk space and avoid including development dependencies from other
+locally linked packages, inspect the `package.json` manifest looking for dependencies
+referring to local file paths (can be identified as they start with `.` or `file:`),
+then for each dependency recursively execute the packaging process 
+
+Local dependencies may include other local dependencies themselves, this is a very 
+common way of sharing configuration or development utilities such as linting or testing 
+tools. This means that for each packaged local dependency this packager needs to
+recursively apply the packaging process. It also means that the packager needs to 
+track local paths and avoid re-packaging directories it already visited.
+
+NPM produces a `tar` archive while packaging that can be directly included as a
+dependency.  This will make NPM unpack and install a copy correctly. Once the
+packager produces all `tar` archives required by local dependencies, rewrite
+the manifest to point to `tar` files instead of the original location.
+
+#### Step 3: Install dependencies
+
+The packager should then run `npm install` to download an expand all dependencies to
+the local `node_modules` subdirectory. This has to be executed in the directory with
+a clean copy of the source files.
+
+Note that NPM can be configured to use proxies or local company repositories using 
+a local file, `.npmrc`. The packaging process from step 1 normally excludes this file, so it may
+need to be copied additionally before dependency installation, and then removed. 
+_(out of scope for the current version)_
+
+Some users may want to exclude optional dependencies, or even include development dependencies. 
+To avoid incompatible flags in the `sam` CLI, the packager should allow users to specify 
+options for the `npm install` command using an environment variable.
+_(out of scope for the current version)_
+
+To fully support dependencies that download or compile binaries for a target platform, this step
+needs to be executed inside a Docker image compatible with AWS Lambda. 
+_(out of scope for the current version)_
+

From 607079e4595e1e2d77ec0848d5f6cd0eb9636431 Mon Sep 17 00:00:00 2001
From: Gojko Adzic <gojko@gojko.com>
Date: Wed, 5 Dec 2018 10:05:41 +0100
Subject: [PATCH 32/34] added a note on package locking

---
 aws_lambda_builders/workflows/nodejs_npm/DESIGN.md | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/aws_lambda_builders/workflows/nodejs_npm/DESIGN.md b/aws_lambda_builders/workflows/nodejs_npm/DESIGN.md
index 6de6d4cd7..0b1814919 100644
--- a/aws_lambda_builders/workflows/nodejs_npm/DESIGN.md
+++ b/aws_lambda_builders/workflows/nodejs_npm/DESIGN.md
@@ -99,6 +99,11 @@ dependency.  This will make NPM unpack and install a copy correctly. Once the
 packager produces all `tar` archives required by local dependencies, rewrite
 the manifest to point to `tar` files instead of the original location.
 
+If the project contains a package lock file, this will cause NPM to ignore changes
+to the package.json manifest. In this case, the packager will need to remove 
+`package-lock.json` so that dependency rewrites take effect. 
+_(out of scope for the current version)_
+
 #### Step 3: Install dependencies
 
 The packager should then run `npm install` to download an expand all dependencies to

From 6863067023cbf28d5b4dfc68061a8f6ca41654b3 Mon Sep 17 00:00:00 2001
From: Gojko Adzic <gojko@gojko.com>
Date: Wed, 5 Dec 2018 17:46:19 +0100
Subject: [PATCH 33/34] group imports according to PEP8, as per
 https://github.com/awslabs/aws-lambda-builders/pull/44#discussion_r239112505

---
 aws_lambda_builders/workflows/nodejs_npm/actions.py       | 1 +
 aws_lambda_builders/workflows/nodejs_npm/workflow.py      | 1 -
 tests/functional/workflows/nodejs_npm/test_utils.py       | 5 +----
 tests/integration/workflows/nodejs_npm/test_nodejs_npm.py | 2 +-
 tests/unit/workflows/nodejs_npm/test_actions.py           | 2 --
 tests/unit/workflows/nodejs_npm/test_npm.py               | 1 -
 tests/unit/workflows/nodejs_npm/test_workflow.py          | 5 +----
 7 files changed, 4 insertions(+), 13 deletions(-)

diff --git a/aws_lambda_builders/workflows/nodejs_npm/actions.py b/aws_lambda_builders/workflows/nodejs_npm/actions.py
index 9ab313102..e3e9aad43 100644
--- a/aws_lambda_builders/workflows/nodejs_npm/actions.py
+++ b/aws_lambda_builders/workflows/nodejs_npm/actions.py
@@ -3,6 +3,7 @@
 """
 
 import logging
+
 from aws_lambda_builders.actions import BaseAction, Purpose, ActionFailedError
 from .npm import NpmExecutionError
 
diff --git a/aws_lambda_builders/workflows/nodejs_npm/workflow.py b/aws_lambda_builders/workflows/nodejs_npm/workflow.py
index 70e27c05a..b55bc9cc1 100644
--- a/aws_lambda_builders/workflows/nodejs_npm/workflow.py
+++ b/aws_lambda_builders/workflows/nodejs_npm/workflow.py
@@ -4,7 +4,6 @@
 
 from aws_lambda_builders.workflow import BaseWorkflow, Capability
 from aws_lambda_builders.actions import CopySourceAction
-
 from .actions import NodejsNpmPackAction, NodejsNpmInstallAction
 from .utils import OSUtils
 from .npm import SubprocessNpm
diff --git a/tests/functional/workflows/nodejs_npm/test_utils.py b/tests/functional/workflows/nodejs_npm/test_utils.py
index e262107bc..b84bc1e8f 100644
--- a/tests/functional/workflows/nodejs_npm/test_utils.py
+++ b/tests/functional/workflows/nodejs_npm/test_utils.py
@@ -1,9 +1,6 @@
-import sys
-
 import os
-
 import shutil
-
+import sys
 import tempfile
 
 from unittest import TestCase
diff --git a/tests/integration/workflows/nodejs_npm/test_nodejs_npm.py b/tests/integration/workflows/nodejs_npm/test_nodejs_npm.py
index fabf86c99..980281e16 100644
--- a/tests/integration/workflows/nodejs_npm/test_nodejs_npm.py
+++ b/tests/integration/workflows/nodejs_npm/test_nodejs_npm.py
@@ -1,7 +1,7 @@
-
 import os
 import shutil
 import tempfile
+
 from unittest import TestCase
 
 from aws_lambda_builders.builder import LambdaBuilder
diff --git a/tests/unit/workflows/nodejs_npm/test_actions.py b/tests/unit/workflows/nodejs_npm/test_actions.py
index f3e494195..f6d0a46fb 100644
--- a/tests/unit/workflows/nodejs_npm/test_actions.py
+++ b/tests/unit/workflows/nodejs_npm/test_actions.py
@@ -1,9 +1,7 @@
-
 from unittest import TestCase
 from mock import patch
 
 from aws_lambda_builders.actions import ActionFailedError
-
 from aws_lambda_builders.workflows.nodejs_npm.actions import NodejsNpmPackAction, NodejsNpmInstallAction
 from aws_lambda_builders.workflows.nodejs_npm.npm import NpmExecutionError
 
diff --git a/tests/unit/workflows/nodejs_npm/test_npm.py b/tests/unit/workflows/nodejs_npm/test_npm.py
index 59ba6458f..8eae867da 100644
--- a/tests/unit/workflows/nodejs_npm/test_npm.py
+++ b/tests/unit/workflows/nodejs_npm/test_npm.py
@@ -1,4 +1,3 @@
-
 from unittest import TestCase
 from mock import patch
 
diff --git a/tests/unit/workflows/nodejs_npm/test_workflow.py b/tests/unit/workflows/nodejs_npm/test_workflow.py
index a7070c73f..1d8e0d63c 100644
--- a/tests/unit/workflows/nodejs_npm/test_workflow.py
+++ b/tests/unit/workflows/nodejs_npm/test_workflow.py
@@ -1,12 +1,9 @@
-
 from unittest import TestCase
 
+from aws_lambda_builders.actions import CopySourceAction
 from aws_lambda_builders.workflows.nodejs_npm.workflow import NodejsNpmWorkflow
-
 from aws_lambda_builders.workflows.nodejs_npm.actions import NodejsNpmPackAction, NodejsNpmInstallAction
 
-from aws_lambda_builders.actions import CopySourceAction
-
 
 class TestNodejsNpmWorkflow(TestCase):
 

From fdc2707266b97add790aa7b1a653bfef832685bf Mon Sep 17 00:00:00 2001
From: Gojko Adzic <gojko@gojko.com>
Date: Thu, 6 Dec 2018 00:03:42 +0100
Subject: [PATCH 34/34] clarify the need for package customisation

---
 .../workflows/nodejs_npm/DESIGN.md            | 25 +++++++++++++------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/aws_lambda_builders/workflows/nodejs_npm/DESIGN.md b/aws_lambda_builders/workflows/nodejs_npm/DESIGN.md
index 0b1814919..1a7746756 100644
--- a/aws_lambda_builders/workflows/nodejs_npm/DESIGN.md
+++ b/aws_lambda_builders/workflows/nodejs_npm/DESIGN.md
@@ -28,9 +28,19 @@ just the ones relevant for production. To identify production dependencies,
 this packager needs to copy the source to a clean temporary directory and re-run
 dependency installation there.
   
-It's reasonable to expect that some developers will not carefully separate
-production dependencies from test resources, so this packager will need to
-support overriding the categories of dependencies to include.
+A frequently used trick to speed up NodeJS Lambda deployment is to avoid 
+bundling the `aws-sdk`, since it is already available on the Lambda VM.
+This makes deployment significantly faster for single-file lambdas, for
+example. Although this is not good from a consistency and compatibility
+perspective (as the version of the API used in production might be different
+from what was used during testing), people do this frequently enough that the
+packager should handle it in some way. A common way of marking this with ClaudiaJS
+is to include `aws-sdk` as an optional dependency, then deploy without optional
+dependencies. 
+
+Other runtimes do not have this flexibility, so instead of adding a specific
+parameter to the SAM CLI, the packager should support a flag to include or
+exclude optional dependencies through environment variables. 
 
 NPM also provides support for running user-defined scripts as part of the build
 process, so this packager needs to support standard NPM script execution.
@@ -43,7 +53,7 @@ far from optimal to create a stand-alone module. Copying would lead to significa
 larger packages than necessary, as sub-modules might still have test resources, and
 common references from multiple projects would be duplicated.
 
-NPM also uses a locking mechanism (package-lock.json) that's in many ways more
+NPM also uses a locking mechanism (`package-lock.json`) that's in many ways more
 broken than functional, as it in some cases hard-codes locks to local disk
 paths, and gets confused by including the same package as a dependency
 throughout the project tree in different dependency categories
@@ -75,9 +85,10 @@ Execute `npm pack` to perform project-specific packaging using the supplied
 test resources and other source files unnecessary for running in a production 
 environment.
 
-This will produce a `tar` archive that needs to be unpacked into the artefacts directory.
-Note that the archive will actually contain a `package` subdirectory containing the files, 
-so it's not enough to just directly unpack files. 
+This will produce a `tar` archive that needs to be unpacked into the artifacts
+directory.  Note that the archive will actually contain a `package`
+subdirectory containing the files, so it's not enough to just directly unpack
+files. 
 
 #### Step 2: Rewrite local dependencies