-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Feat: added integration tests, container tag check and layer support #2780
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
24 commits
Select commit
Hold shift + click to select a range
aca8e9a
Added integration tests
qingchm 555802d
Added click requirement on -u presence for container based options
qingchm 644a1da
Added click class that checks -u presence for container based options
qingchm 12c6cf2
Reformatting and fixing test cases
qingchm 4c851b7
Fix for integration test failures
qingchm 7c83d7e
Added support for layers
qingchm f023b88
Resolve conflict
qingchm f38adba
Refactoring file location
qingchm 5e6b480
Addressing review comments, refactoring
qingchm 4337ccd
Reformatting logging sentence
qingchm 6970d94
Refactoring container check for simplicity
qingchm e37cb02
Clarifying test case
qingchm 805d747
Added unit tests on layer builds
qingchm b68e6f9
Improving readability
qingchm 83b391e
Refactoring to simplify
qingchm 891250d
Removing unnecessary lines
qingchm 694d5ab
Refactoring integration test
qingchm 54a5da2
Shortening error message
qingchm fee15ae
Adjusting test behaviour
qingchm e99649d
Added new unit tests
qingchm 5401388
Test refactoring
qingchm 0a8d29a
Added new test class
qingchm cf62ccf
Removed unused import
qingchm e549915
Improving help text
qingchm File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| """ | ||
| Module to check container based cli parameters | ||
| """ | ||
| import click | ||
|
|
||
|
|
||
| class ContainerOptions(click.Option): | ||
| """ | ||
| Preprocessing checks for presence of --use-container flag for container based options. | ||
| """ | ||
|
|
||
| def handle_parse_result(self, ctx, opts, args): | ||
| 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) | ||
| # To make sure no unser input prompting happens | ||
| self.prompt = None | ||
aahung marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| return super().handle_parse_result(ctx, opts, args) | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,7 @@ | |
| from unittest import skipIf | ||
| from pathlib import Path | ||
| from parameterized import parameterized, parameterized_class | ||
| from subprocess import Popen, PIPE, TimeoutExpired | ||
|
|
||
| import pytest | ||
|
|
||
|
|
@@ -1837,3 +1838,47 @@ def test_nested_build(self, use_container, cached, parallel): | |
| ("LocalNestedStack/Function2", {"pi": "3.14"}), | ||
| ], | ||
| ) | ||
|
|
||
|
|
||
| @skipIf( | ||
| ((IS_WINDOWS and RUNNING_ON_CI) and not CI_OVERRIDE), | ||
| "Skip build tests on windows when running in CI unless overridden", | ||
| ) | ||
| class TestBuildWithCustomBuildImage(BuildIntegBase): | ||
| template = "build_image_function.yaml" | ||
|
|
||
| @parameterized.expand( | ||
| [ | ||
| ("use_container", None), | ||
| ("use_container", "amazon/aws-sam-cli-build-image-python3.7:latest"), | ||
| ] | ||
| ) | ||
| @pytest.mark.flaky(reruns=3) | ||
| def test_custom_build_image_succeeds(self, use_container, build_image): | ||
| if use_container and SKIP_DOCKER_TESTS: | ||
| self.skipTest(SKIP_DOCKER_MESSAGE) | ||
|
|
||
| cmdlist = self.get_command_list(use_container=use_container, build_image=build_image) | ||
|
|
||
| command_result = run_command(cmdlist, cwd=self.working_dir) | ||
| stderr = command_result.stderr | ||
| process_stderr = stderr.strip() | ||
|
|
||
| self._verify_right_image_pulled(build_image, process_stderr) | ||
| self._verify_build_succeeds(self.default_build_dir) | ||
|
|
||
| self.verify_docker_container_cleanedup("python3.7") | ||
|
|
||
| def _verify_right_image_pulled(self, build_image, process_stderr): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought we have helper functions that do this already?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is checking the name of the image, I don't think we have anything similar |
||
| image_name = build_image if build_image is not None else "public.ecr.aws/sam/build-python3.7:latest" | ||
| processed_name = bytes(image_name, encoding="utf-8") | ||
| self.assertIn( | ||
| processed_name, | ||
| process_stderr, | ||
| ) | ||
|
|
||
| def _verify_build_succeeds(self, build_dir): | ||
| self.assertTrue(build_dir.exists(), "Build directory should be created") | ||
|
|
||
| build_dir_files = os.listdir(str(build_dir)) | ||
| self.assertIn("BuildImageFunction", build_dir_files) | ||
15 changes: 15 additions & 0 deletions
15
tests/integration/testdata/buildcmd/build_image_function.yaml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| AWSTemplateFormatVersion : '2010-09-09' | ||
| Transform: AWS::Serverless-2016-10-31 | ||
|
|
||
| Globals: | ||
| Function: | ||
| Timeout: 20 | ||
| MemorySize: 512 | ||
|
|
||
| Resources: | ||
| BuildImageFunction: | ||
| Type: AWS::Serverless::Function | ||
| Properties: | ||
| CodeUri: Python | ||
| Handler: main.handler | ||
| Runtime: python3.7 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| import click | ||
|
|
||
| from unittest import TestCase | ||
| from unittest.mock import Mock, patch, call | ||
| from samcli.commands.build.click_container import ContainerOptions | ||
|
|
||
|
|
||
| @patch("samcli.commands.build.click_container.ContainerOptions") | ||
| class TestContainerOptionsSucceeds(TestCase): | ||
| ctx_mock = Mock() | ||
| opts = {"container_env_var": ["hi=in"], "use_container": True, "resource_logical_id": None} | ||
| ContainerOptionsMock = Mock() | ||
| ContainerOptionsMock.handle_parse_result.return_value = "value" | ||
|
|
||
| def test_container_options(self, ContainerOptionsMock): | ||
| self.assertEqual(self.ContainerOptionsMock.handle_parse_result(self.ctx_mock, self.opts, []), "value") | ||
|
|
||
|
|
||
| class TestContainerOptionsFails(TestCase): | ||
| ctx_mock = Mock() | ||
| opts = {"container_env_var": ["hi=in"], "resource_logical_id": None} | ||
| args = ["--container-env-var"] | ||
| container_opt = ContainerOptions(args) | ||
|
|
||
| def test_container_options_failure(self): | ||
| with self.assertRaises(click.UsageError) as err: | ||
| self.container_opt.handle_parse_result(self.ctx_mock, self.opts, []) | ||
| self.assertEqual( | ||
| str(err.exception), | ||
| "Missing required parameter, need the --use-container flag in order to use --container-env-var flag.", | ||
| ) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have unit tests for this class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
An alternative way to do this is by creating a sub method and testing that, if mocking the rest of things becomes a pain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adding two branches of tests