Skip to content

Conversation

ArendAMZN
Copy link
Contributor

Issue #, if available:

Description of changes:

This adds unit tests that should have been included in the previous PR
#1101

I also updated the unit test error messaging to be more readable. Previously it would output the entire output json blob and it was difficult to find the error. Now it shows a diff. See below for example of issue I was running into

E       AssertionError: assert {'Resources':...:IAM::Role'}}} == {'Resources': ...:IAM::Role'}}}
E         Differing items:
E         {'Resources': {'KitchenSinkFunction': {'Properties': {'Code': {'S3Bucket': 'sam-demo-bucket', 'S3Key': 'hello.zip'}, '...icy50'}, {'PolicyDocument': {...}, 'PolicyName': 'KitchenSinkFunctionRolePolicy51'}, ...]}, 'Type': 'AWS::IAM::Role'}}} != {'Resources': {'KitchenSinkFu
nction': {'Properties': {'Code': {'S3Bucket': 'sam-demo-bucket', 'S3Key': 'hello.zip'}, '...icy50'}, {'PolicyDocument': {...}, 'PolicyName': 'KitchenSinkFunctionRolePolicy51'}, ...]}, 'Type': 'AWS::IAM::Role'}}}
E         Use -v to get the full diff

tests/translator/test_translator.py:197: AssertionError
---------------------------------------------------------------------------------------------------------------------------------- Captured stdout call -----------------------------------------------------------------------------------------------------------------------------------
--- expected
+++ actual
@@ -17,8 +17,8 @@
         "Runtime": "python2.7", 
         "Tags": [
           {
-            "Value": "SAM", 
-            "Key": "lambda:createdBy"
+            "Key": "lambda:createdBy", 
+            "Value": "SAM"
           }
         ]
       }
@@ -221,9 +221,7 @@
                   "Resource": {
                     "Fn::Sub": [
                       {
-                        "repositoryName": {
-                          "Ref": "RepositoryName"
-                        }
+                        "repositoryName": "name"
                       }, 
                       "arn:${AWS::Partition}:codecommit:${AWS::Region}:${AWS::AccountId}:${repositoryName}"
                     ]

Description of how you validated changes:

Checklist:

  • [Y] Write/update tests
  • [Y] make pr passes
  • [n/a] Update documentation
  • [n/a] Verify transformed template deploys and application functions as expected
  • [n/a] Add/update example to examples/2016-10-31

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

@codecov-io
Copy link

codecov-io commented Aug 31, 2019

Codecov Report

Merging #1113 into develop will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #1113   +/-   ##
========================================
  Coverage    94.76%   94.76%           
========================================
  Files           71       71           
  Lines         3474     3474           
  Branches       678      678           
========================================
  Hits          3292     3292           
  Misses          93       93           
  Partials        89       89

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7b9d79a...a9844fd. Read the comment docs.

Copy link
Contributor

@jlhood jlhood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for following up on this! Only one comment about not removing printing the output file in the failure case. I'll go ahead and make that change though so we can get this merged.

@ShreyaGangishetty ShreyaGangishetty merged commit c4d7c7b into aws:develop Sep 5, 2019
@praneetap praneetap mentioned this pull request Sep 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants