Feat: added integration tests, container tag check and layer support by qingchm · Pull Request #2780 · aws/aws-sam-cli

qingchm · 2021-03-30T21:05:23Z

Which issue(s) does this change fix?

N.A.

Why is this change necessary?

To add integration tests for new option --build-image

How does it address the issue?

N.A.

What side effects does this change have?

N.A.

Checklist

Add input/output type hints to new functions/methods
Write design document (Do I need to write a design document?)
Write unit tests
Write/update functional tests
Write/update integration tests
make pr passes
make update-reproducible-reqs if dependencies were changed
Write documentation

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

jfuss

The logic seems generally good and I like how you adding the Container option class!

Can we make sure we have unit tests for any code we added? It looks like we don't have tests for the Container class or the additional logic you added into the app_builder. Pytest should give you lines/files that are missing coverage and you can use that as a starting place to have the lines/files you added are covered.

samcli/lib/build/app_builder.py

jfuss · 2021-03-31T14:36:29Z

samcli/lib/build/app_builder.py

+                image = None
+                if self._build_images is not None:
+                    if layer_name in self._build_images:
+                        image = self._build_images[layer_name]
+                    elif None in self._build_images:
+                        image = self._build_images[None]
                self._build_function_on_container(


I am slightly confused on this. Which I think is mainly due to the nesting.

A couple questions:

Can self._build_images be None? If so, is there a way we can safely default this to an empty dict instead?

Why are we checking if the layer_name is in self._build_images?

I am not familiar with the build_images property and how it gets set. Could you describe the contents?

Something to think about for the future:
When you are writing deeply nested code, consider ways you can simplify it. There are usually ways to write the same logic with less control flows or splitting the nesting into simpler private functions. So in this case, if we could default self._build_images to an empty dict in __init__ then we safe ourselves the need to do None checks (we do this throughout the code base for this reason).

When acting on dictionaries in python, try to avoid using [] to access elements and instead use .get() method. The .get() method does the checks you want for you. So you can simplify both inner if statements by doing image = self._build_images.get(layer_name). By default, this will return None if the element (layer_name) is not in the dict.

I think I actually answered my above questions while writing the last part but will leave the in the comment to help explain my thought process/questions I would ask.

Yes it is defaulted to be none. Type hinting doesn't allow any parameter defaults to be {}, so no.

self._build_images has function names and layer names as keys and image uri's as value.

Details in feat(Public ECR): Added --build-image Options #2725, basically we take the command line argument and make a dict that stores the URI's and their related logicalId's

+1 on using get(), this is following the previous pr's pattern, will change both to get()

And we can set self._build_images to buildimages or {} during assignment, for sure this would be safer

jfuss · 2021-03-31T14:38:45Z

tests/integration/buildcmd/test_build_cmd.py

+    @parameterized.expand(
+        [
+            ("use_container", None),
+            ("use_container", "amazon/aws-sam-cli-build-image-nodejs10.x"),


Can we keep the languages our integ tests use the same as others (python I think)? This helps us in keeping things up to date across all our tests.

Sure I can change the test case to python then

jfuss · 2021-03-31T14:39:06Z

tests/integration/buildcmd/test_build_cmd.py

+
+        self.verify_docker_container_cleanedup("nodejs10.x")
+
+    def _verify_right_image_pulled(self, build_image, process_stderr):


I thought we have helper functions that do this already?

this is checking the name of the image, I don't think we have anything similar

jfuss · 2021-03-31T14:40:19Z

tests/integration/buildcmd/test_build_cmd.py

+        self.verify_docker_container_cleanedup("nodejs10.x")
+
+    def _verify_right_image_pulled(self, build_image, process_stderr):
+        image_name = build_image if build_image is not None else "public.ecr.aws/sam/build-nodejs10.x"


If you really want to stay with node (which I prefer we keep all our integ tests the same language unless really testing something lang specific), we should be using the latest node.

We have already added the support to default the tags to be latest if we don't input the tag, so this is exactly the same thing. But for the testing we can test if latest tag is included

I am not talking about the image tag here. You are using Nodejs10.x, which is the oldest of the 2 Node versions Lambda supports. My comment is to not use the oldest and instead us the latest version of Node (or any language we us in our integ tests).

+1, this can save us some work (or delay the work) when we deprecate node 10

jfuss · 2021-03-31T14:42:05Z

tests/integration/buildcmd/test_build_cmd.py

+        process = Popen(cmdlist, cwd=self.working_dir, stdout=PIPE, stderr=PIPE)
+        try:
+            stdout, stderr = process.communicate(timeout=TIMEOUT)
+            LOG.info(f"Stdout: {stdout.decode('utf-8')}")


Do we need this info always displayed in our test output?

Same goes for other LOG. statements, mainly those who will output logs of data (stderr and stdout).

We do this same logging behaviour for all of our integration tests, when we run the command @jfuss

In other build tests we do not log stdout and stderr. This comment is about removing the large outputs in our tests logs. Leaving what commands and other small information is fine, logging 30+ lines seems like overkill. If you think this info is important, then we should only output it on test failure instead of every single run.

I guess we do but it is done through the run_command:

aws-sam-cli/tests/testing_utils.py

Line 25 in 4543732

def run_command(command_list, cwd=None, env=None, timeout=TIMEOUT) -> CommandResult:

Why are we not using this here?

Because we need to do extra work that run_command does not provide, which is checking the content in the stderr

We do support checking stderr using run_command

aws-sam-cli/tests/integration/buildcmd/test_build_cmd.py

Lines 1548 to 1551 in 4543732

command_result = run_command(cmdlist, cwd=self.working_dir)

# make sure functions are deduplicated properly, in stderr they will show up in the same line.

self.assertRegex(command_result.stderr.decode("utf-8"), r"Building .+'Function2',.+LocalNestedStack/Function2")

I can try something like this, trying to use the nametuple returned

Before I had several differences between this function and run command, but seems like this run_command will return what I need

Updated to use run_command()

jfuss · 2021-03-31T14:43:29Z

tests/unit/commands/samconfig/test_samconfig.py

            LOG.debug(Path(config_path).read_text())
            runner = CliRunner()
-            result = runner.invoke(cli, [])
+            result = runner.invoke(cli, ["--use-container"])


Why are we adding --use-container to all of these? It's unclear why we need to update this.

Because otherwise as the CliRunner doesn't see this tag, it fails the invocation and showcases the ContainerTag check error

I might be missing something but my initial feeling is that seems wrong. Wouldn't this mean we are requiring --use-container now? The command shouldn't fail if we don't provided --use-container to the cli.

It should now, we are going to fail any use of container related tags if we don't provide --use-container to the cli instead of ignoring them now

jfuss · 2021-03-31T14:45:17Z

samcli/commands/build/click_container.py

+import click
+
+
+class Container(click.Option):


Should we name this something more specific like ContainerOptions? We have other Container classes and might be easier to read/understand the difference at a glance.

Sure I will change the name

aahung

How does it handle if I want to specify a build image for a function in nested stack like --build-image=ChildStack.FunctionA. It will be great if we have an integ test on that

tests/unit/commands/samconfig/test_samconfig.py

samcli/commands/build/click_container.py

qingchm · 2021-03-31T21:10:54Z

The logic seems generally good and I like how you adding the Container option class!

Can we make sure we have unit tests for any code we added? It looks like we don't have tests for the Container class or the additional logic you added into the app_builder. Pytest should give you lines/files that are missing coverage and you can use that as a starting place to have the lines/files you added are covered.

Some unit tests are already included in the previous pr. Adding additional unit tests for layer builds. As for Container class, I don't think we have tests on click_mutex class either. Are we looking to add tests for click related code?

samcli/commands/build/click_container.py

samcli/lib/build/app_builder.py

jfuss · 2021-04-01T15:39:59Z

tests/integration/buildcmd/test_build_cmd.py

+        process = Popen(cmdlist, cwd=self.working_dir, stdout=PIPE, stderr=PIPE)
+        try:
+            stdout, stderr = process.communicate(timeout=TIMEOUT)
+            LOG.info(f"Stdout: {stdout.decode('utf-8')}")


I guess we do but it is done through the run_command:

aws-sam-cli/tests/testing_utils.py

Line 25 in 4543732

def run_command(command_list, cwd=None, env=None, timeout=TIMEOUT) -> CommandResult:

Why are we not using this here?

jfuss · 2021-04-01T15:40:59Z

samcli/commands/build/click_container.py

+import click
+
+
+class ContainerOptions(click.Option):


Do we have unit tests for this class?

We don't, I don't see any tests related to https://github.com/aws/aws-sam-cli/blob/develop/samcli/commands/local/cli_common/click_mutex.py either, let me know your thoughts regarding testing on click.Option classes! One of the ways that I can think of testing it would be to create a mock CliRunner and verify that command fails when we provide container related tags without -u

Just because we do not have tests for another class, doesn't mean we should not be adding tests for new code. We always want to improve the code base through adding tests. With that logic one could say, we shouldn't be adding any tests because we have a class that doesn't have tests at all.

Please add unit tests for code we add, so we have some basic checks in place on that expectation of that code.

We shouldn't need to mock a CliRunner here or verify commands (kind of out of scope of a unit tests in my opinion). What we want to verify is that the logic you added is correct. So you can mock out methods, input, etc to help in validating what you expect to happen does happen. At the very least we should have tests to cover:

if "use_container" not in opts and self.name in opts: opt_name = self.name.replace("_", "-") msg = f"Missing required parameter, need the --use-container flag in order to use --{opt_name} flag." raise click.UsageError(msg)

An alternative way to do this is by creating a sub method and testing that, if mocking the rest of things becomes a pain.

adding two branches of tests

samcli/commands/build/click_container.py

samcli/lib/build/app_builder.py

aahung

Overall LGTM, left some small comments/questions

samcli/lib/build/app_builder.py

aahung · 2021-04-01T20:50:51Z

tests/integration/buildcmd/test_build_cmd.py

+        process = Popen(cmdlist, cwd=self.working_dir, stdout=PIPE, stderr=PIPE)
+        try:
+            stdout, stderr = process.communicate(timeout=TIMEOUT)
+            LOG.info(f"Stdout: {stdout.decode('utf-8')}")


We do support checking stderr using run_command

aws-sam-cli/tests/integration/buildcmd/test_build_cmd.py

Lines 1548 to 1551 in 4543732

command_result = run_command(cmdlist, cwd=self.working_dir)

# make sure functions are deduplicated properly, in stderr they will show up in the same line.

self.assertRegex(command_result.stderr.decode("utf-8"), r"Building .+'Function2',.+LocalNestedStack/Function2")

samcli/lib/build/app_builder.py

samcli/commands/build/click_container.py

jfuss · 2021-04-05T18:52:14Z

tests/unit/commands/buildcmd/test_command.py

+
+
+@patch("samcli.commands.build.click_container.ContainerOptions")
+class TestContainerOptionsSucceeds(TestCase):


Nit: Since this is in its own file, lets match that with our unit tests as well. This is will make it consistent with all our other tests in the repo and make it easier to know where to update tests in the future when things change.

Sounds good

jfuss

Thanks for handling all the feedback through this PR.

* feat(Public ECR): Changed Build Image Registry to Public ECR (#2708) * Changed Build Image Registry to Public ECR * Added _IMAGE_URI_PREFIX * Fixed Unit Tests * Updated Image Format * feat(Public ECR): Updated Test Image Name and Added Latest Tag Check (#2720) * Added Tests for Latest Tag * Added use_container Check for Verify Latest Tag Tests * Fixed DotNet and Go Build Tests * Added Missing use_container Check for test_build_function_and_layer * feat(Public ECR): Changed Build Image Registry to Public ECR (#2708) * Changed Build Image Registry to Public ECR * Added _IMAGE_URI_PREFIX * Fixed Unit Tests * Updated Image Format * feat(Public ECR): Updated Test Image Name and Added Latest Tag Check (#2720) * Added Tests for Latest Tag * Added use_container Check for Verify Latest Tag Tests * Fixed DotNet and Go Build Tests * Added Missing use_container Check for test_build_function_and_layer * feat(Public ECR): Added --build-image Options (#2725) * Added Latest Tag Check * Added --build-image Option * Added Unit Tests * Added Help Text and Fixed PyLint * Fixed Path Validation for Unit Tests * Updated Type Hintings * Added _parse_key_value_pair * Updated tag Handling for Image Pulls * Added Throwing Exception with Invalid Build Images * Fixed PyLint Issue * Feat: added integration tests, container tag check and layer support (#2780) * Added integration tests * Added click requirement on -u presence for container based options * Added click class that checks -u presence for container based options * Reformatting and fixing test cases * Fix for integration test failures * Added support for layers * Resolve conflict * Refactoring file location * Addressing review comments, refactoring * Reformatting logging sentence * Refactoring container check for simplicity * Clarifying test case * Added unit tests on layer builds * Improving readability * Refactoring to simplify * Removing unnecessary lines * Refactoring integration test * Shortening error message * Adjusting test behaviour * Added new unit tests * Test refactoring * Added new test class * Removed unused import * Improving help text Co-authored-by: Qingchuan Ma <69653965+qingchm@users.noreply.github.com>

* feat(Public ECR): Changed Build Image Registry to Public ECR (aws#2708) * Changed Build Image Registry to Public ECR * Added _IMAGE_URI_PREFIX * Fixed Unit Tests * Updated Image Format * feat(Public ECR): Updated Test Image Name and Added Latest Tag Check (aws#2720) * Added Tests for Latest Tag * Added use_container Check for Verify Latest Tag Tests * Fixed DotNet and Go Build Tests * Added Missing use_container Check for test_build_function_and_layer * feat(Public ECR): Changed Build Image Registry to Public ECR (aws#2708) * Changed Build Image Registry to Public ECR * Added _IMAGE_URI_PREFIX * Fixed Unit Tests * Updated Image Format * feat(Public ECR): Updated Test Image Name and Added Latest Tag Check (aws#2720) * Added Tests for Latest Tag * Added use_container Check for Verify Latest Tag Tests * Fixed DotNet and Go Build Tests * Added Missing use_container Check for test_build_function_and_layer * feat(Public ECR): Added --build-image Options (aws#2725) * Added Latest Tag Check * Added --build-image Option * Added Unit Tests * Added Help Text and Fixed PyLint * Fixed Path Validation for Unit Tests * Updated Type Hintings * Added _parse_key_value_pair * Updated tag Handling for Image Pulls * Added Throwing Exception with Invalid Build Images * Fixed PyLint Issue * Feat: added integration tests, container tag check and layer support (aws#2780) * Added integration tests * Added click requirement on -u presence for container based options * Added click class that checks -u presence for container based options * Reformatting and fixing test cases * Fix for integration test failures * Added support for layers * Resolve conflict * Refactoring file location * Addressing review comments, refactoring * Reformatting logging sentence * Refactoring container check for simplicity * Clarifying test case * Added unit tests on layer builds * Improving readability * Refactoring to simplify * Removing unnecessary lines * Refactoring integration test * Shortening error message * Adjusting test behaviour * Added new unit tests * Test refactoring * Added new test class * Removed unused import * Improving help text Co-authored-by: Qingchuan Ma <69653965+qingchm@users.noreply.github.com>

qingchm added 7 commits March 30, 2021 13:48

Added integration tests

aca8e9a

Added click requirement on -u presence for container based options

555802d

Added click class that checks -u presence for container based options

644a1da

Reformatting and fixing test cases

12c6cf2

Fix for integration test failures

4c851b7

Added support for layers

7c83d7e

Resolve conflict

f023b88

qingchm added the pr/internal label Mar 30, 2021

Refactoring file location

f38adba

jfuss suggested changes Mar 31, 2021

View reviewed changes

aahung reviewed Mar 31, 2021

View reviewed changes

tests/unit/commands/samconfig/test_samconfig.py Outdated Show resolved Hide resolved

samcli/commands/build/click_container.py Outdated Show resolved Hide resolved

samcli/commands/build/click_container.py Outdated Show resolved Hide resolved

Addressing review comments, refactoring

5e6b480

aahung reviewed Mar 31, 2021

View reviewed changes

qingchm added 4 commits March 31, 2021 17:14

Reformatting logging sentence

4337ccd

Refactoring container check for simplicity

6970d94

Clarifying test case

e37cb02

Added unit tests on layer builds

805d747

qingchm requested a review from jfuss April 1, 2021 02:10

jfuss suggested changes Apr 1, 2021

View reviewed changes

aahung reviewed Apr 1, 2021

View reviewed changes

qingchm added 2 commits April 1, 2021 11:05

Improving readability

b68e6f9

Refactoring to simplify

83b391e

aahung approved these changes Apr 1, 2021

View reviewed changes

qingchm added 4 commits April 1, 2021 14:02

Removing unnecessary lines

891250d

Refactoring integration test

694d5ab

Shortening error message

54a5da2

Adjusting test behaviour

fee15ae

qingchm requested a review from jfuss April 1, 2021 22:37

Added new unit tests

e99649d

jfuss reviewed Apr 5, 2021

View reviewed changes

qingchm added 4 commits April 5, 2021 12:07

Test refactoring

5401388

Added new test class

0a8d29a

Removed unused import

cf62ccf

Improving help text

e549915

jfuss approved these changes Apr 5, 2021

View reviewed changes

qingchm merged commit 92b1453 into aws:feat/public-ecr Apr 5, 2021


		self.verify_docker_container_cleanedup("nodejs10.x")

		def _verify_right_image_pulled(self, build_image, process_stderr):

	command_result = run_command(cmdlist, cwd=self.working_dir)

	# make sure functions are deduplicated properly, in stderr they will show up in the same line.
	self.assertRegex(command_result.stderr.decode("utf-8"), r"Building .+'Function2',.+LocalNestedStack/Function2")



		@patch("samcli.commands.build.click_container.ContainerOptions")
		class TestContainerOptionsSucceeds(TestCase):

Conversation

qingchm commented Mar 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue(s) does this change fix?

Why is this change necessary?

How does it address the issue?

What side effects does this change have?

Checklist

Uh oh!

jfuss left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qingchm Mar 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qingchm Apr 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aahung left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

qingchm commented Mar 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qingchm commented Mar 30, 2021 •

edited

Loading

qingchm Mar 31, 2021 •

edited

Loading

qingchm Apr 1, 2021 •

edited

Loading

qingchm commented Mar 31, 2021 •

edited

Loading