Tests for samconfig use with all commands #1575

sanathkr · 2019-11-22T01:46:46Z

Issue #, if available:

Description of changes:

Checklist:

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
Write documentation

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

samcli/cli/cli_config_file.py

tests/unit/commands/samconfig/test_samconfig.py

sriram-mv · 2019-11-22T05:10:30Z

since you are writing tests, could you also add version=0.1 to the toml document? when we create it.

samcli/commands/build/command.py

sriram-mv · 2019-11-22T19:04:04Z

tests/unit/commands/samconfig/test_samconfig.py

+                LOG.exception("Command failed", exc_info=result.exc_info)
+            self.assertIsNone(result.exception)
+
+            do_cli_mock.assert_called_with(


Nit: can we make assert_called_with(a=b,c=d,e=f,g=h) instead of assert_called_with(b,d,f,h). That way its explicit.

I wanted to verify the ordering as well, because that's how the do_cli method is called. Makes sense?

sriram-mv · 2019-11-22T19:12:13Z

tests/unit/commands/samconfig/test_samconfig.py

+            "manifest": "requirements.txt",
+            "docker_network": "mynetwork",
+            "skip_pull_image": True,
+            "parameter_overrides": "ParameterKey=Key,ParameterValue=Value ParameterKey=Key2,ParameterValue=Value2",


can we add parameter overrides that are "a=b c=d" as well, that way we exercise the second regex.

Yes, some of the others have this

sriram-mv · 2019-11-22T19:14:15Z

tests/unit/commands/samconfig/test_samconfig.py

+            "event": "event",
+            "no_event": False,
+            "env_vars": "envvar.json",
+            "debug_port": [1, 2, 3],


+1 glad to know multiples work well.

lol, I know! Do we document this case explicitly?

sriram-mv · 2019-11-22T19:16:27Z

tests/unit/commands/samconfig/test_samconfig.py

+            )
+
+    @patch("samcli.commands.deploy.command.do_cli")
+    def test_deploy(self, do_cli_mock):


can we add another series of tests, where we supply args and they override whats in the config file.

sriram-mv · 2019-11-22T19:35:58Z

samcli/cli/types.py

+    return f'(\\"(?:\\\\.|[^\\"\\\\]+)*\\"|(?:\\\\.|[^{delim}\\"\\\\]+)+)'
+
+
+KEY_REGEX = '([A-Za-z0-9\\"]+)'


Testing these regexes, will report back with results.

sriram-mv · 2019-11-22T19:54:25Z

samcli/cli/types.py

-
-    _pattern_1 = r"(?:ParameterKey={key},ParameterValue={value}".format(key=KEY_REGEX, value=VALUE_REGEX)
-    _pattern_2 = r"(?:(?: ){key}={value}".format(key=KEY_REGEX, value=VALUE_REGEX)
+    _pattern_1 = r"(?:ParameterKey={key},ParameterValue={value})".format(key=KEY_REGEX, value=VALUE_REGEX_SPACE_DELIM)


Ok, did some testing.
Things we may need to make a note of:
_pattern_1 works well when the entire string that is compared with the regex is under quotes.

Works:

"ParameterKey=Key,ParameterValue=Value" Groups: Key, Value
"ParameterKey=Key,ParameterValue=Value Groups: Key, Value
ParameterKey=Key,ParameterValue=Value" Groups: Key, Value
Doesnt work:

ParameterKey=Key,ParameterValue="Value" No Groups
ParameterKey="Key",ParameterValue="Value" No Groups
"ParameterKey="Key",ParameterValue="Value"" No Groups

sriram-mv · 2019-11-22T20:05:45Z

samcli/cli/types.py

-    _pattern_1 = r"(?:ParameterKey={key},ParameterValue={value}".format(key=KEY_REGEX, value=VALUE_REGEX)
-    _pattern_2 = r"(?:(?: ){key}={value}".format(key=KEY_REGEX, value=VALUE_REGEX)
+    _pattern_1 = r"(?:ParameterKey={key},ParameterValue={value})".format(key=KEY_REGEX, value=VALUE_REGEX_SPACE_DELIM)
+    _pattern_2 = r"(?:(?: ){key}={value})".format(key=KEY_REGEX, value=VALUE_REGEX_SPACE_DELIM)


Works well, Gotchas with quotes.

Works
dd=gfbfg asdf=dadsf asdasd=qwe Groups (dd,gfbfg) (asdf,dadsf) (asdasd, qwe)
" "dd"='gfbfg'" Groups ("dd",'gfbfg')

Doesn't work

' "dd"="gfbfg" "asdf"="dadsf" "asdasd"="qwed"' No Groups

I am not worried about the key not supporting quotes. that's out of scope. Also, value using single quotes is also out-of-scope.

sriram-mv

🥇 💯

This is non trivial testing, for the full surface area of the cli with configuration.

⭐️ ⭐️ ⭐️ ⭐️ ⭐️

sriram-mv · 2019-11-22T22:51:59Z

tests/unit/commands/samconfig/test_samconfig.py

+        self.scratch_dir = None
+
+    @patch("samcli.commands.local.start_lambda.cli.do_cli")
+    def test_override_with_cli_params(self, do_cli_mock):


sriram-mv · 2019-11-22T22:55:23Z

tests/unit/commands/samconfig/test_samconfig.py

+            runner = CliRunner()
+            result = runner.invoke(
+                cli,
+                env={


+1 on env overrides.

* test: Verify samconfig is accessible to all CLI commands * fix cases where comma vs space needs to be delimiter * adding few more unit tests * adding unit tests for all commands * fix linter * Adding tests for overriding args thru config, CLI args, and envvars * Fixing a minor UX issue when sam template is invalid * fixing mock imports

gdunkle · 2019-12-10T15:59:44Z

I believe this change broke tag values that actually contain spaces (i.e. MyKey=My\ Value)

sanathkr added 2 commits November 21, 2019 17:13

test: Verify samconfig is accessible to all CLI commands

51b907c

fix cases where comma vs space needs to be delimiter

88ad704

sriram-mv suggested changes Nov 22, 2019

View reviewed changes

samcli/cli/cli_config_file.py Show resolved Hide resolved

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

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

sanathkr added 3 commits November 21, 2019 23:52

adding few more unit tests

5f43d51

adding unit tests for all commands

cdf59b6

fix linter

9c2ddae

sriram-mv suggested changes Nov 22, 2019

View reviewed changes

sanathkr added 4 commits November 22, 2019 13:48

Adding tests for overriding args thru config, CLI args, and envvars

1632b82

Merge branch 'release-v0.32.0' into samconfig-integtest

cf6fcd8

Fixing a minor UX issue when sam template is invalid

8fcd4ed

fixing mock imports

026861d

sriram-mv approved these changes Nov 22, 2019

View reviewed changes

sriram-mv merged commit beb0c00 into aws:release-v0.32.0 Nov 22, 2019

		return f'(\\"(?:\\\\.\|[^\\"\\\\]+)*\\"\|(?:\\\\.\|[^{delim}\\"\\\\]+)+)'


		KEY_REGEX = '([A-Za-z0-9\\"]+)'

Tests for samconfig use with all commands #1575

Tests for samconfig use with all commands #1575

Uh oh!

Conversation

sanathkr commented Nov 22, 2019 • edited by sriram-mv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sriram-mv commented Nov 22, 2019

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!

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!

sriram-mv left a comment

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!

gdunkle commented Dec 10, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sanathkr commented Nov 22, 2019 •

edited by sriram-mv

Loading