diff --git a/aws_lambda_builders/workflows/nodejs_npm/DESIGN.md b/aws_lambda_builders/workflows/nodejs_npm/DESIGN.md index 803052ed7..381c53a44 100644 --- a/aws_lambda_builders/workflows/nodejs_npm/DESIGN.md +++ b/aws_lambda_builders/workflows/nodejs_npm/DESIGN.md @@ -21,14 +21,14 @@ 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 +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, +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. -A frequently used trick to speed up NodeJS Lambda deployment is to avoid +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 @@ -40,7 +40,7 @@ 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. +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. @@ -63,9 +63,9 @@ 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 +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. @@ -81,29 +81,28 @@ 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 +`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 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. +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 +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 +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. +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 @@ -111,9 +110,8 @@ 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)_ +to the package.json manifest. In this case, the packager will need to remove +`package-lock.json` so that dependency rewrites take effect. #### Step 3: Install dependencies @@ -121,16 +119,17 @@ The packager should then run `npm install` to download an expand all dependencie 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 needs -to be copied before dependency installation, and then removed. +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 needs +to be copied before dependency installation, and then removed. -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 +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)_ +needs to be executed inside a Docker image compatible with AWS Lambda. +_(out of scope for the current version)_ diff --git a/aws_lambda_builders/workflows/nodejs_npm/actions.py b/aws_lambda_builders/workflows/nodejs_npm/actions.py index 43259bd8c..6d900e5de 100644 --- a/aws_lambda_builders/workflows/nodejs_npm/actions.py +++ b/aws_lambda_builders/workflows/nodejs_npm/actions.py @@ -6,6 +6,7 @@ from aws_lambda_builders.actions import BaseAction, Purpose, ActionFailedError from .npm import NpmExecutionError +from .utils import DependencyUtils LOG = logging.getLogger(__name__) @@ -47,25 +48,21 @@ def __init__(self, artifacts_dir, scratch_dir, manifest_path, osutils, subproces def execute(self): """ - Runs the action. + Runs the action - :raises lambda_builders.actions.ActionFailedError: when NPM packaging fails + Raises + ------ + ActionFailedError + When NPM packaging fails """ try: - 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) - - tarfile_name = self.subprocess_npm.run(["pack", "-q", package_path], cwd=self.scratch_dir).splitlines()[-1] - - LOG.debug("NODEJS packed to %s", tarfile_name) - - tarfile_path = self.osutils.joinpath(self.scratch_dir, tarfile_name) - - LOG.debug("NODEJS extracting to %s", self.artifacts_dir) + tar_path = DependencyUtils.package_dependencies( + self.manifest_path, self.scratch_dir, {}, self.osutils, self.subprocess_npm + ) - self.osutils.extract_tarfile(tarfile_path, self.artifacts_dir) + LOG.debug("NODEJS extracting final %s to artifacts dir %s", tar_path, self.artifacts_dir) + self.osutils.extract_tarfile(tar_path, self.artifacts_dir) except NpmExecutionError as ex: raise ActionFailedError(str(ex)) @@ -115,7 +112,7 @@ def execute(self): class NodejsNpmrcCopyAction(BaseAction): """ - A Lambda Builder Action that copies NPM config file .npmrc + Lambda Builder Action that copies NPM config file .npmrc """ NAME = "CopyNpmrc" @@ -157,13 +154,14 @@ def execute(self): raise ActionFailedError(str(ex)) -class NodejsNpmrcCleanUpAction(BaseAction): - +class NodejsCleanUpAction(BaseAction): """ - A Lambda Builder Action that cleans NPM config file .npmrc + Lambda Builder Action that cleans up after install: + - Removes NPM config file .npmrc + - Restores modified package.json files from their backup """ - NAME = "CleanUpNpmrc" + NAME = "CleanUp" DESCRIPTION = "Cleans artifacts dir" PURPOSE = Purpose.COPY_SOURCE @@ -177,22 +175,45 @@ def __init__(self, artifacts_dir, osutils): :param osutils: An instance of OS Utilities for file manipulation """ - super(NodejsNpmrcCleanUpAction, self).__init__() + super(NodejsCleanUpAction, self).__init__() self.artifacts_dir = artifacts_dir self.osutils = osutils def execute(self): """ - Runs the action. + Runs the actions - :raises lambda_builders.actions.ActionFailedError: when .npmrc copying fails + Raises + ------ + ActionFailedError + When .npmrc removing or package.json restoring fails """ try: - npmrc_path = self.osutils.joinpath(self.artifacts_dir, ".npmrc") - if self.osutils.file_exists(npmrc_path): - LOG.debug(".npmrc cleanup in: %s", self.artifacts_dir) - self.osutils.remove_file(npmrc_path) - + self._remove_npmrc() + self._restore_package_json() except OSError as ex: raise ActionFailedError(str(ex)) + + def _remove_npmrc(self): + """ + Removes the .npmrc file + """ + npmrc_path = self.osutils.joinpath(self.artifacts_dir, ".npmrc") + + if self.osutils.file_exists(npmrc_path): + LOG.debug(".npmrc cleanup in: %s", self.artifacts_dir) + self.osutils.remove_file(npmrc_path) + + def _restore_package_json(self): + """ + Restores the original package.json from its backup + """ + org_file = "package.json" + bak_file = org_file + ".bak" + + for (root, _, files) in self.osutils.walk(self.artifacts_dir): + if bak_file in files: + bak_path = self.osutils.joinpath(root, bak_file) + self.osutils.copy_file(bak_path, self.osutils.joinpath(root, org_file)) + self.osutils.remove_file(bak_path) diff --git a/aws_lambda_builders/workflows/nodejs_npm/utils.py b/aws_lambda_builders/workflows/nodejs_npm/utils.py index ad92cfd23..a82b0d73c 100644 --- a/aws_lambda_builders/workflows/nodejs_npm/utils.py +++ b/aws_lambda_builders/workflows/nodejs_npm/utils.py @@ -2,15 +2,19 @@ Commonly used utilities """ +import io +import json +import logging import os import platform import tarfile import subprocess import shutil +LOG = logging.getLogger(__name__) -class OSUtils(object): +class OSUtils(object): """ Wrapper around file system functions, to make it easy to unit test actions in memory @@ -19,6 +23,9 @@ class OSUtils(object): def copy_file(self, file_path, destination_path): return shutil.copy2(file_path, destination_path) + def dir_exists(self, directory): + return os.path.isdir(directory) + def extract_tarfile(self, tarfile_path, unpack_dir): with tarfile.open(tarfile_path, "r:*") as tar: tar.extractall(unpack_dir) @@ -29,6 +36,18 @@ def file_exists(self, filename): def joinpath(self, *args): return os.path.join(*args) + def mkdir(self, path): + return os.mkdir(path) + + def makedirs(self, path): + return os.makedirs(path) + + def normpath(self, *args): + return os.path.normpath(*args) + + def open_file(self, filename, mode="r", encoding="utf-8"): + return io.open(filename, mode, encoding=encoding) + 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 @@ -48,3 +67,229 @@ def abspath(self, path): def is_windows(self): return platform.system().lower() == "windows" + + def walk(self, dir, topdown=True): + return os.walk(dir, topdown) + + +class DependencyUtils(object): + """ + Collection of helper functions for managing local NPM dependencies + """ + + @staticmethod + def package(package_path, from_dir, subprocess_npm): + """ + Packages a module + + Parameters + ---------- + package_path : str + Package path + from_dir : str + Origin directory path + subprocess_npm : SubprocessNpm + NPM process + + Returns + ------- + str + Name of the package tar file + """ + return subprocess_npm.run(["pack", "-q", package_path], cwd=from_dir).splitlines()[-1] + + @staticmethod + def ship_module(package_path, from_dir, to_dir, osutils, subprocess_npm): + """ + Packages a module and extracts it to a destination directory + + Parameters + ---------- + package_path : str + Package path + from_dir : str + Origin directory path + to_dir : str + Destination directory path + osutils : OSUtils + OS utils + subprocess_npm : SubprocessNpm + NPM process + """ + tarfile_name = DependencyUtils.package(package_path, from_dir, subprocess_npm) + + LOG.debug("NODEJS packed to %s", tarfile_name) + + tarfile_path = osutils.joinpath(from_dir, tarfile_name) + + LOG.debug("NODEJS extracting to %s", to_dir) + + osutils.extract_tarfile(tarfile_path, to_dir) + + @staticmethod + def get_local_dependencies(manifest_path, osutils): + """ + Extracts all local dependencies in a package.json manifest + + Parameters + ---------- + manifest_path : str + Manifest path + osutils : OSUtils + OS utils + + Returns + ------- + dict + Dict of local dependencies key->value if any, empty otherwise + """ + with osutils.open_file(manifest_path) as manifest_file: + manifest = json.loads(manifest_file.read()) + + if "dependencies" in manifest: + return dict( + (k, DependencyUtils.strip_file_prefix(v)) + for (k, v) in manifest["dependencies"].items() + if DependencyUtils.is_local_dependency(v) + ) + + return {} + + @staticmethod + def is_local_dependency(path): + """ + Checks if a dependency is a local package + + Parameters + ---------- + path : str + Path to check + + Returns + ------- + boolean + True if a local dependency, False otherwise + """ + try: + return path.startswith("file:") or path.startswith(".") + except AttributeError: + return False + + @staticmethod + def strip_file_prefix(dependency): + """ + Strips the "file:" prefix off a local dependency + NPM local dependencies can be defined as "./mydep" or "file:./mydep" + "file:" should be stripped to build an absolute path to the dependency + + Parameters + ---------- + dependency : str + Local dependency + + Returns + ------- + str + Local dependency stripped of the "file:" prefix + """ + if dependency.startswith("file:"): + dependency = dependency[5:].strip() + return dependency + + @staticmethod + def package_dependencies(manifest_path, scratch_dir, packed_manifests, osutils, subprocess_npm): + """ + Recursively packages NPM dependencies, including local ones. + Handles circular dependencies and skips already processed ones. + + Parameters + ---------- + manifest_path : str + Path to the manifest to package + scratch_dir : str + Path to the scratch directory + packed_manifests : dict + Dict of hashes pointing to packed manifest tar used to avoid + osutils : OSUtils + OS utils + subprocess_npm : SubprocessNpm + NPM process + + Returns + ------- + string + Path to the packed tar file + """ + manifest_path = osutils.normpath(manifest_path) + LOG.debug("NODEJS processing %s", manifest_path) + manifest_hash = str(abs(hash(manifest_path))) + if manifest_hash in packed_manifests: + # Already processed or circular dependency + # If empty, it will be removed from the manifest + return packed_manifests[manifest_hash] + packed_manifests[manifest_hash] = "" + manifest_dir = osutils.dirname(manifest_path) + manifest_scratch_dir = osutils.joinpath(scratch_dir, manifest_hash) + manifest_scratch_package_dir = osutils.joinpath(manifest_scratch_dir, "package") + osutils.makedirs(manifest_scratch_package_dir) + + # Pack and copy module to scratch so that we don't update the customers files in place + DependencyUtils.ship_module(manifest_dir, scratch_dir, manifest_scratch_dir, osutils, subprocess_npm) + + # Process local children dependencies + local_dependencies = DependencyUtils.get_local_dependencies(manifest_path, osutils) + + for (dep_name, dep_path) in local_dependencies.items(): + dep_manifest = osutils.joinpath(manifest_dir, dep_path, "package.json") + + tar_path = DependencyUtils.package_dependencies( + dep_manifest, scratch_dir, packed_manifests, osutils, subprocess_npm + ) + + manifest_scratch_path = osutils.joinpath(manifest_scratch_package_dir, "package.json") + + # Make a backup we will use to restore in the final build folder + osutils.copy_file(manifest_scratch_path, manifest_scratch_path + ".bak") + + DependencyUtils.update_manifest(manifest_scratch_path, dep_name, tar_path, osutils) + + # Pack the current dependency + tarfile_name = DependencyUtils.package(manifest_scratch_package_dir, scratch_dir, subprocess_npm) + + packed_manifests[manifest_hash] = osutils.joinpath(scratch_dir, tarfile_name) + + LOG.debug("NODEJS %s packed to %s", manifest_scratch_package_dir, packed_manifests[manifest_hash]) + + return packed_manifests[manifest_hash] + + @staticmethod + def update_manifest(manifest_path, dep_name, dependency_tarfile_path, osutils): + """ + Helper function to update dependency path to localized tar + + Parameters + ---------- + manifest_path : str + Manifest path to update + dep_name : str + Dependency name + dependency_tarfile_path : str + Packed dependency tar file path + osutils : OSUtils + OS utils + """ + with osutils.open_file(manifest_path, "r") as manifest_read_file: + manifest = json.loads(manifest_read_file.read()) + + if "dependencies" in manifest and dep_name in manifest["dependencies"]: + if not dependency_tarfile_path: + LOG.debug("NODEJS removing dep '%s' from '%s'", dep_name, manifest_path) + manifest["dependencies"].pop(dep_name) + else: + LOG.debug( + "NODEJS updating dep '%s' of '%s' with '%s'", dep_name, manifest_path, dependency_tarfile_path + ) + manifest["dependencies"][dep_name] = "file:{}".format(dependency_tarfile_path) + + with osutils.open_file(manifest_path, "w") as manifest_write_file: + manifest_write_file.write(json.dumps(manifest, indent=4, ensure_ascii=False)) diff --git a/aws_lambda_builders/workflows/nodejs_npm/workflow.py b/aws_lambda_builders/workflows/nodejs_npm/workflow.py index b48ea4430..cbbe825f9 100644 --- a/aws_lambda_builders/workflows/nodejs_npm/workflow.py +++ b/aws_lambda_builders/workflows/nodejs_npm/workflow.py @@ -4,7 +4,12 @@ from aws_lambda_builders.path_resolver import PathResolver from aws_lambda_builders.workflow import BaseWorkflow, Capability from aws_lambda_builders.actions import CopySourceAction -from .actions import NodejsNpmPackAction, NodejsNpmInstallAction, NodejsNpmrcCopyAction, NodejsNpmrcCleanUpAction +from .actions import ( + NodejsNpmPackAction, + NodejsNpmInstallAction, + NodejsNpmrcCopyAction, + NodejsCleanUpAction, +) from .utils import OSUtils from .npm import SubprocessNpm @@ -49,7 +54,7 @@ def __init__(self, source_dir, artifacts_dir, scratch_dir, manifest_path, runtim npm_copy_npmrc, CopySourceAction(tar_package_dir, artifacts_dir, excludes=self.EXCLUDED_FILES), npm_install, - NodejsNpmrcCleanUpAction(artifacts_dir, osutils=osutils), + NodejsCleanUpAction(artifacts_dir, osutils=osutils), ] def get_resolvers(self): diff --git a/tests/functional/workflows/nodejs_npm/test_utils.py b/tests/functional/workflows/nodejs_npm/test_utils.py index bd39e0ff3..9f42361b7 100644 --- a/tests/functional/workflows/nodejs_npm/test_utils.py +++ b/tests/functional/workflows/nodejs_npm/test_utils.py @@ -10,45 +10,32 @@ class TestOSUtils(TestCase): def setUp(self): - self.osutils = utils.OSUtils() def test_copy_file_copies_existing_file_into_a_dir(self): - test_file = os.path.join(os.path.dirname(__file__), "test_data", "test.tgz") - test_dir = tempfile.mkdtemp() self.osutils.copy_file(test_file, test_dir) output_files = set(os.listdir(test_dir)) - shutil.rmtree(test_dir) - self.assertEqual({"test.tgz"}, output_files) def test_copy_file_copies_existing_file_into_a_file(self): - test_file = os.path.join(os.path.dirname(__file__), "test_data", "test.tgz") - test_dir = tempfile.mkdtemp() self.osutils.copy_file(test_file, os.path.join(test_dir, "copied_test.tgz")) output_files = set(os.listdir(test_dir)) - shutil.rmtree(test_dir) - self.assertEqual({"copied_test.tgz"}, output_files) def test_remove_file_removes_existing_file(self): - test_file = os.path.join(os.path.dirname(__file__), "test_data", "test.tgz") - test_dir = tempfile.mkdtemp() - copied_file = os.path.join(test_dir, "copied_test.tgz") - shutil.copy(test_file, copied_file) self.osutils.remove_file(copied_file) @@ -56,27 +43,20 @@ def test_remove_file_removes_existing_file(self): self.assertFalse(os.path.isfile(copied_file)) def test_file_exists_checking_if_file_exists_in_a_dir(self): - existing_file = os.path.join(os.path.dirname(__file__), "test_data", "test.tgz") - nonexisting_file = os.path.join(os.path.dirname(__file__), "test_data", "nonexisting.tgz") self.assertTrue(self.osutils.file_exists(existing_file)) - self.assertFalse(self.osutils.file_exists(nonexisting_file)) 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() self.osutils.extract_tarfile(test_tar, test_dir) output_files = set(os.listdir(test_dir)) - shutil.rmtree(test_dir) - self.assertEqual({"test_utils.py"}, output_files) def test_dirname_returns_directory_for_path(self): @@ -85,33 +65,26 @@ def test_dirname_returns_directory_for_path(self): self.assertEqual(dirname, os.path.dirname(sys.executable)) def test_abspath_returns_absolute_path(self): - result = self.osutils.abspath(".") self.assertTrue(os.path.isabs(result)) - self.assertEqual(result, os.path.abspath(".")) def test_joinpath_joins_path_components(self): - result = self.osutils.joinpath("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) out, err = p.communicate() - self.assertEqual(p.returncode, 0) - 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( @@ -119,7 +92,96 @@ def test_popen_can_accept_cwd(self): ) out, err = p.communicate() - self.assertEqual(p.returncode, 0) - self.assertEqual(out.decode("utf8").strip(), os.path.abspath(testdata_dir)) + + def test_dir_exists(self): + self.assertFalse(self.osutils.dir_exists("20201210_some_directory_that_should_not_exist")) + + temp_dir = tempfile.mkdtemp() + + self.assertTrue(self.osutils.dir_exists(temp_dir)) + + shutil.rmtree(temp_dir) + + def test_mkdir_makes_directory(self): + dir_to_create = os.path.join(tempfile.gettempdir(), "20201210_some_directory_that_should_not_exist") + self.assertFalse(os.path.isdir(dir_to_create)) + + self.osutils.mkdir(dir_to_create) + + self.assertTrue(os.path.isdir(dir_to_create)) + + shutil.rmtree(dir_to_create) + + def test_mkdirs_makes_directory_and_subdir(self): + dir_to_create = os.path.join(tempfile.gettempdir(), "20201210_some_directory_that_should_not_exist") + subdir_to_create = os.path.join(dir_to_create, "subdirectory_that_should_not_exist") + self.assertFalse(os.path.isdir(dir_to_create)) + self.assertFalse(os.path.isdir(subdir_to_create)) + + self.osutils.makedirs(subdir_to_create) + + self.assertTrue(os.path.isdir(dir_to_create)) + self.assertTrue(os.path.isdir(subdir_to_create)) + shutil.rmtree(dir_to_create) + + def test_normpath_normalizes_paths(self): + result = os.path.normpath("/my/path/with/without/../package.json") + + self.assertEqual(result, self.osutils.normpath("/my/path/with/without/../package.json")) + + def test_open_file_opens_file_for_reading(self): + temp_dir = tempfile.mkdtemp() + + file_to_open = os.path.join(temp_dir, "test_open.txt") + + with open(file_to_open, "w") as fid: + fid.write(u"this is text") + + with self.osutils.open_file(file_to_open) as fid: + content = fid.read() + + self.assertEqual("this is text", content) + + shutil.rmtree(temp_dir) + + def test_open_file_opens_file_for_writing(self): + temp_dir = tempfile.mkdtemp() + + file_to_open = os.path.join(temp_dir, "test_open.txt") + + with self.osutils.open_file(file_to_open, "w") as fid: + fid.write(u"this is some other text") + + with self.osutils.open_file(file_to_open) as fid: + content = fid.read() + + self.assertEqual("this is some other text", content) + + shutil.rmtree(temp_dir) + + def test_walk_walks_tree(self): + all_files = [] + + for (_, _, files) in self.osutils.walk(os.path.dirname(__file__)): + for f in files: + all_files.append(f) + + # Only testing those two as OSes enjoy adding other hidden files (like .DS_Store) + self.assertTrue("test_utils.py" in all_files) + self.assertTrue("test.tgz" in all_files) + + +class TestDependencyUtils(TestCase): + def test_is_local_dependency_file_prefix(self): + self.assertTrue(utils.DependencyUtils.is_local_dependency("file:./local/dep")) + + def test_is_local_dependency_dot_prefix(self): + self.assertTrue(utils.DependencyUtils.is_local_dependency("./local/dep")) + + def test_is_local_dependency_package_name(self): + self.assertFalse(utils.DependencyUtils.is_local_dependency("typescript")) + + def test_is_local_dependency_invalid(self): + self.assertFalse(utils.DependencyUtils.is_local_dependency(None)) diff --git a/tests/integration/workflows/nodejs_npm/test_nodejs_npm.py b/tests/integration/workflows/nodejs_npm/test_nodejs_npm.py index 83a8de6e0..ffe4ed956 100644 --- a/tests/integration/workflows/nodejs_npm/test_nodejs_npm.py +++ b/tests/integration/workflows/nodejs_npm/test_nodejs_npm.py @@ -77,6 +77,29 @@ def test_builds_project_with_remote_dependencies(self): output_modules = set(os.listdir(os.path.join(self.artifacts_dir, "node_modules"))) self.assertEqual(expected_modules, output_modules) + def test_builds_project_with_local_dependencies(self): + source_dir = os.path.join(self.TEST_DATA_FOLDER, "local-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.assertEqual(expected_files, output_files) + + expected_modules = {"@mockcompany"} + output_modules = set(os.listdir(os.path.join(self.artifacts_dir, "node_modules"))) + self.assertEqual(expected_modules, output_modules) + + expected_sub_modules = {"module-a", "module-b", "module-c"} + output_sub_modules = set(os.listdir(os.path.join(self.artifacts_dir, "node_modules", "@mockcompany"))) + self.assertEqual(expected_sub_modules, output_sub_modules) + def test_builds_project_with_npmrc(self): source_dir = os.path.join(self.TEST_DATA_FOLDER, "npmrc") @@ -98,7 +121,6 @@ def test_builds_project_with_npmrc(self): self.assertEqual(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: @@ -113,7 +135,6 @@ def test_fails_if_npm_cannot_resolve_dependencies(self): 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: diff --git a/tests/integration/workflows/nodejs_npm/testdata/local-deps/excluded.js b/tests/integration/workflows/nodejs_npm/testdata/local-deps/excluded.js new file mode 100644 index 000000000..8bf8be437 --- /dev/null +++ b/tests/integration/workflows/nodejs_npm/testdata/local-deps/excluded.js @@ -0,0 +1,2 @@ +//excluded +const x = 1; diff --git a/tests/integration/workflows/nodejs_npm/testdata/local-deps/included.js b/tests/integration/workflows/nodejs_npm/testdata/local-deps/included.js new file mode 100644 index 000000000..872ecacbb --- /dev/null +++ b/tests/integration/workflows/nodejs_npm/testdata/local-deps/included.js @@ -0,0 +1,6 @@ +const module_a = require('@mockcompany/module-a') + +//included +const x = 1; + +const greetings = module_a.sayHello(); diff --git a/tests/integration/workflows/nodejs_npm/testdata/local-deps/package.json b/tests/integration/workflows/nodejs_npm/testdata/local-deps/package.json new file mode 100644 index 000000000..47df45240 --- /dev/null +++ b/tests/integration/workflows/nodejs_npm/testdata/local-deps/package.json @@ -0,0 +1,12 @@ +{ + "name": "localdeps", + "version": "1.0.0", + "description": "", + "files": ["included.js"], + "keywords": [], + "author": "", + "license": "APACHE2.0", + "dependencies": { + "@mockcompany/module-a": "file:../modules/module_a" + } +} diff --git a/tests/integration/workflows/nodejs_npm/testdata/modules/module_a/index.js b/tests/integration/workflows/nodejs_npm/testdata/modules/module_a/index.js new file mode 100644 index 000000000..5594a5346 --- /dev/null +++ b/tests/integration/workflows/nodejs_npm/testdata/modules/module_a/index.js @@ -0,0 +1,5 @@ +const module_b = require('@mockcompany/module-b'); + +exports.sayHello = function() { + return 'hello from module a! module b says: ' + module_b.sayHello(); +} \ No newline at end of file diff --git a/tests/integration/workflows/nodejs_npm/testdata/modules/module_a/package.json b/tests/integration/workflows/nodejs_npm/testdata/modules/module_a/package.json new file mode 100644 index 000000000..84952331b --- /dev/null +++ b/tests/integration/workflows/nodejs_npm/testdata/modules/module_a/package.json @@ -0,0 +1,12 @@ +{ + "name": "@mockcompany/module-a", + "version": "1.0.0", + "description": "", + "files": ["index.js"], + "keywords": [], + "author": "", + "license": "APACHE2.0", + "dependencies": { + "@mockcompany/module-b": "file:../module_b" + } +} \ No newline at end of file diff --git a/tests/integration/workflows/nodejs_npm/testdata/modules/module_b/index.js b/tests/integration/workflows/nodejs_npm/testdata/modules/module_b/index.js new file mode 100644 index 000000000..e99abb1ac --- /dev/null +++ b/tests/integration/workflows/nodejs_npm/testdata/modules/module_b/index.js @@ -0,0 +1,5 @@ +const module_c = require('@mockcompany/module-c'); + +exports.sayHello = function() { + return 'hello from module b! module-c says: ' + module_c.sayHello(); +} \ No newline at end of file diff --git a/tests/integration/workflows/nodejs_npm/testdata/modules/module_b/package.json b/tests/integration/workflows/nodejs_npm/testdata/modules/module_b/package.json new file mode 100644 index 000000000..780db8f14 --- /dev/null +++ b/tests/integration/workflows/nodejs_npm/testdata/modules/module_b/package.json @@ -0,0 +1,15 @@ +{ + "name": "@mockcompany/module-b", + "version": "1.0.0", + "description": "", + "files": [ + "index.js" + ], + "keywords": [], + "author": "", + "license": "APACHE2.0", + "dependencies": { + "@mockcompany/module-a": "file:../module_a", + "@mockcompany/module-c": "file:../module_c" + } +} \ No newline at end of file diff --git a/tests/integration/workflows/nodejs_npm/testdata/modules/module_c/index.js b/tests/integration/workflows/nodejs_npm/testdata/modules/module_c/index.js new file mode 100644 index 000000000..9162b06fb --- /dev/null +++ b/tests/integration/workflows/nodejs_npm/testdata/modules/module_c/index.js @@ -0,0 +1,3 @@ +exports.sayHello = function() { + return 'hello from module c! I have no news... :('; +} \ No newline at end of file diff --git a/tests/integration/workflows/nodejs_npm/testdata/modules/module_c/package.json b/tests/integration/workflows/nodejs_npm/testdata/modules/module_c/package.json new file mode 100644 index 000000000..d20ca19ec --- /dev/null +++ b/tests/integration/workflows/nodejs_npm/testdata/modules/module_c/package.json @@ -0,0 +1,9 @@ +{ + "name": "@mockcompany/module-c", + "version": "1.0.0", + "description": "", + "files": ["index.js"], + "keywords": [], + "author": "", + "license": "APACHE2.0" +} \ No newline at end of file diff --git a/tests/unit/workflows/nodejs_npm/test_actions.py b/tests/unit/workflows/nodejs_npm/test_actions.py index 39007a59c..0612278a1 100644 --- a/tests/unit/workflows/nodejs_npm/test_actions.py +++ b/tests/unit/workflows/nodejs_npm/test_actions.py @@ -1,16 +1,83 @@ -from unittest import TestCase +from json import dumps from mock import patch +from os.path import basename, normpath +from unittest import TestCase +from re import match from aws_lambda_builders.actions import ActionFailedError from aws_lambda_builders.workflows.nodejs_npm.actions import ( NodejsNpmPackAction, NodejsNpmInstallAction, NodejsNpmrcCopyAction, - NodejsNpmrcCleanUpAction, + NodejsCleanUpAction, ) from aws_lambda_builders.workflows.nodejs_npm.npm import NpmExecutionError +class MockOpener: + calls = -1 + + @property + def content(self): + return self._contents[min(self.calls, len(self._contents) - 1)] + + def __init__(self, contents=None): + if contents is None: + self._contents = ["{}"] + else: + self._contents = contents + + def open(self, filename, mode="r"): + self.calls += 1 + return FakeFileObject(filename, mode, self.content) + + +class FakeFileObject(object): + def __init__(self, filename, mode="r", content="{}"): + self.filename = filename + self.mode = mode + + self.content = content + + def __enter__(self): + return self + + def __exit__(self, type, value, traceback): + pass + + def read(self): + if self.mode.startswith("r"): + return self.content + elif (self.mode.startswith == "w" or self.mode.startswith == "a") and self.mode.endswith("+"): + return "" + else: + raise IOError("file not open for reading") + + def write(self, data): + if self.mode.startswith("w") or self.mode.startswith("a") or self.mode.endswith("+"): + return self.content + else: + raise IOError("file not open for writing") + + +class RegexMatch: + expressions = [] + + def __init__(self, expressions): + self.expressions = expressions + + def __repr__(self): + return str(self.expressions) + + def __eq__(self, b): + _b = b if isinstance(b, list) else [b] + + if len(self.expressions) != len(_b): + return False + + return all([(match(self.expressions[inx], _b[inx]) is not None) for inx in range(len(self.expressions))]) + + class TestNodejsNpmPackAction(TestCase): @patch("aws_lambda_builders.workflows.nodejs_npm.utils.OSUtils") @patch("aws_lambda_builders.workflows.nodejs_npm.npm.SubprocessNpm") @@ -22,15 +89,19 @@ def test_tars_and_unpacks_npm_project(self, OSUtilMock, SubprocessNpmMock): "artifacts", "scratch_dir", "manifest", osutils=osutils, subprocess_npm=subprocess_npm ) + osutils.normpath.side_effect = lambda value: normpath(value) 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) + osutils.joinpath.side_effect = lambda *args: "/".join(args) + osutils.open_file.side_effect = MockOpener().open subprocess_npm.run.return_value = "package.tar" action.execute() - subprocess_npm.run.assert_called_with(["pack", "-q", "file:/abs:/dir:manifest"], cwd="scratch_dir") + subprocess_npm.run.assert_called_with( + RegexMatch(["pack", "-q", r"scratch_dir/\d+?/package$"]), cwd="scratch_dir" + ) osutils.extract_tarfile.assert_called_with("scratch_dir/package.tar", "artifacts") @patch("aws_lambda_builders.workflows.nodejs_npm.utils.OSUtils") @@ -51,6 +122,35 @@ def test_raises_action_failed_when_npm_fails(self, OSUtilMock, SubprocessNpmMock self.assertEqual(raised.exception.args[0], "NPM Failed: boom!") + @patch("aws_lambda_builders.workflows.nodejs_npm.utils.OSUtils") + @patch("aws_lambda_builders.workflows.nodejs_npm.npm.SubprocessNpm") + def test_tars_and_unpacks_local_dependencies(self, OSUtilMock, SubprocessNpmMock): + osutils = OSUtilMock.return_value + subprocess_npm = SubprocessNpmMock.return_value + + action = NodejsNpmPackAction( + "artifacts", "scratch_dir", "manifest", osutils=osutils, subprocess_npm=subprocess_npm + ) + + file_open_responses = [ + dumps({"dependencies": {"dep_1": "file:./local/path"}}), + dumps({"dependencies": {"dep_2": "file:local/path"}}), + "{}", + ] + + osutils.dirname.side_effect = lambda value: "/dir:{}".format(value) + osutils.abspath.side_effect = lambda value: "/abs:{}".format(value) + osutils.joinpath.side_effect = lambda *args: "/".join(args) + osutils.open_file.side_effect = MockOpener(file_open_responses).open + + subprocess_npm.run.return_value = "package.tar" + + action.execute() + + # pack is called once for top level package, then twice for each local dependency + self.assertEqual(subprocess_npm.run.call_count, 2) + subprocess_npm.run.assert_any_call(RegexMatch(["pack", "-q", r"scratch_dir/\d+?/package$"]), cwd="scratch_dir") + class TestNodejsNpmInstallAction(TestCase): @patch("aws_lambda_builders.workflows.nodejs_npm.npm.SubprocessNpm") @@ -118,13 +218,13 @@ def test_raises_action_failed_when_copying_fails(self, OSUtilMock): action.execute() -class TestNodejsNpmrcCleanUpAction(TestCase): +class TestNodejsCleanUpAction(TestCase): @patch("aws_lambda_builders.workflows.nodejs_npm.utils.OSUtils") def test_removes_npmrc_if_npmrc_exists(self, OSUtilMock): osutils = OSUtilMock.return_value osutils.joinpath.side_effect = lambda a, b: "{}/{}".format(a, b) - action = NodejsNpmrcCleanUpAction("artifacts", osutils=osutils) + action = NodejsCleanUpAction("artifacts", osutils=osutils) osutils.file_exists.side_effect = [True] action.execute() @@ -135,7 +235,7 @@ def test_skips_npmrc_removal_if_npmrc_doesnt_exist(self, OSUtilMock): osutils = OSUtilMock.return_value osutils.joinpath.side_effect = lambda a, b: "{}/{}".format(a, b) - action = NodejsNpmrcCleanUpAction("artifacts", osutils=osutils) + action = NodejsCleanUpAction("artifacts", osutils=osutils) osutils.file_exists.side_effect = [False] action.execute() diff --git a/tests/unit/workflows/nodejs_npm/test_utils.py b/tests/unit/workflows/nodejs_npm/test_utils.py new file mode 100644 index 000000000..353bdd98e --- /dev/null +++ b/tests/unit/workflows/nodejs_npm/test_utils.py @@ -0,0 +1,75 @@ +import os +import shutil +import tempfile +from unittest import TestCase + +from aws_lambda_builders.workflows.nodejs_npm.utils import OSUtils, DependencyUtils +from aws_lambda_builders.workflows.nodejs_npm.npm import SubprocessNpm + + +class TestDependencyUtils(TestCase): + TEST_DATA_FOLDER = os.path.join(os.path.dirname(__file__), "testdata") + + def setUp(self): + self.osutils = OSUtils() + self.subprocess_npm = SubprocessNpm(self.osutils) + self.uut = DependencyUtils() + self.artifacts_dir = tempfile.mkdtemp() + self.scratch_dir = tempfile.mkdtemp() + + def tearDown(self): + shutil.rmtree(self.artifacts_dir) + shutil.rmtree(self.scratch_dir) + + def test_ship_module(self): + self.uut.ship_module( + os.path.join(self.TEST_DATA_FOLDER, "module_1"), + self.scratch_dir, + self.artifacts_dir, + self.osutils, + self.subprocess_npm, + ) + + expected_files = {"package.json", "included.js"} + output_files = set(os.listdir(os.path.join(self.artifacts_dir, "package"))) + self.assertEqual(output_files, expected_files) + + def test_get_local_dependencies_with_none(self): + dependencies = self.uut.get_local_dependencies( + os.path.join(self.TEST_DATA_FOLDER, "module_1", "package.json"), + self.osutils, + ) + + self.assertEqual(dependencies, {}) + + def test_get_local_dependencies_with_some(self): + dependencies = self.uut.get_local_dependencies( + os.path.join(self.TEST_DATA_FOLDER, "module_2", "package.json"), + self.osutils, + ) + + self.assertEqual(dependencies, {"@mockcompany/module-a": "../modules/module_a"}) + + def test_is_local_dependency_when_is(self): + self.assertEqual(self.uut.is_local_dependency("file:../modules/module_a"), True) + + def test_is_local_dependency_when_is_not(self): + self.assertEqual(self.uut.is_local_dependency("1.2.3"), False) + + def test_is_local_dependency_when_is_not_string(self): + self.assertEqual(self.uut.is_local_dependency(None), False) + + def test_package_dependencies(self): + package = self.uut.package_dependencies( + os.path.join(self.TEST_DATA_FOLDER, "module_1", "package.json"), + self.scratch_dir, + {}, + self.osutils, + self.subprocess_npm, + ) + self.assertEqual(os.path.basename(package), "nodejs_npm_unit_tests_module_1-1.0.0.tgz") + + def test_update_manifest(self): + scratch_manifest = os.path.join(self.scratch_dir, "scratch_package.json") + self.osutils.copy_file(os.path.join(self.TEST_DATA_FOLDER, "module_2", "package.json"), scratch_manifest) + self.uut.update_manifest(scratch_manifest, "@mockcompany/module-a", "interim/path.tgz", self.osutils) diff --git a/tests/unit/workflows/nodejs_npm/test_workflow.py b/tests/unit/workflows/nodejs_npm/test_workflow.py index 3dac6d791..d2250824b 100644 --- a/tests/unit/workflows/nodejs_npm/test_workflow.py +++ b/tests/unit/workflows/nodejs_npm/test_workflow.py @@ -6,7 +6,7 @@ NodejsNpmPackAction, NodejsNpmInstallAction, NodejsNpmrcCopyAction, - NodejsNpmrcCleanUpAction, + NodejsCleanUpAction, ) @@ -31,4 +31,4 @@ def test_workflow_sets_up_npm_actions(self): self.assertIsInstance(workflow.actions[3], NodejsNpmInstallAction) - self.assertIsInstance(workflow.actions[4], NodejsNpmrcCleanUpAction) + self.assertIsInstance(workflow.actions[4], NodejsCleanUpAction) diff --git a/tests/unit/workflows/nodejs_npm/testdata/module_1/included.js b/tests/unit/workflows/nodejs_npm/testdata/module_1/included.js new file mode 100644 index 000000000..0d4401a11 --- /dev/null +++ b/tests/unit/workflows/nodejs_npm/testdata/module_1/included.js @@ -0,0 +1 @@ +console.log('blank') \ No newline at end of file diff --git a/tests/unit/workflows/nodejs_npm/testdata/module_1/package.json b/tests/unit/workflows/nodejs_npm/testdata/module_1/package.json new file mode 100644 index 000000000..3183772d9 --- /dev/null +++ b/tests/unit/workflows/nodejs_npm/testdata/module_1/package.json @@ -0,0 +1,9 @@ +{ + "name": "nodejs_npm_unit_tests_module_1", + "version": "1.0.0", + "description": "", + "files": ["included.js"], + "keywords": [], + "author": "", + "license": "APACHE2.0" +} \ No newline at end of file diff --git a/tests/unit/workflows/nodejs_npm/testdata/module_2/included.js b/tests/unit/workflows/nodejs_npm/testdata/module_2/included.js new file mode 100644 index 000000000..0d4401a11 --- /dev/null +++ b/tests/unit/workflows/nodejs_npm/testdata/module_2/included.js @@ -0,0 +1 @@ +console.log('blank') \ No newline at end of file diff --git a/tests/unit/workflows/nodejs_npm/testdata/module_2/package.json b/tests/unit/workflows/nodejs_npm/testdata/module_2/package.json new file mode 100644 index 000000000..4b8ea150f --- /dev/null +++ b/tests/unit/workflows/nodejs_npm/testdata/module_2/package.json @@ -0,0 +1,13 @@ +{ + "name": "nodejs_npm_unit_tests_module_1", + "version": "1.0.0", + "description": "", + "files": ["included.js"], + "keywords": [], + "author": "", + "license": "APACHE2.0", + "dependencies": { + "@mockcompany/module-a": "file:../modules/module_a", + "npm_dep": "1.2.3" + } +} \ No newline at end of file