test: Add simple integration tests #1797

mingkun2020 · 2020-11-14T01:00:35Z

Issue #, if available:

Description of changes:

Description of how you validated changes:

Checklist:

Write/update tests
make pr passes
Update documentation
Verify transformed template deploys and application functions as expected

Examples?

Please reach out in the comments, if you want to add an example. Examples will be
added to sam init through https://github.com/awslabs/aws-sam-cli-app-templates/

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

…st folder tests

…pected" prefix of the basic function result as we already are in the expected directory

… later into a single basic test file, added utils/stack.py

…erless-application-model into foss-integ-test

…tion. Updated the makefile to not check coverage for integration tests

…ication tests to their own file, fixed some names

…g later

…efile black commands to include the tests_integ directories

awood45

Looks like a great start! Some comments attached

tests_integ/helpers/base_test.py

tests_integ/helpers/helpers.py

tests_integ/single/test_basic_api.py

tests_integ/helpers/base_test.py

tests_integ/helpers/helpers.py

…st folder tests

…pected" prefix of the basic function result as we already are in the expected directory

…ces, added simple tests and doc for the table_print utils

… to python2, added a comment in the deployer classes ported over from sam-cli to underline it

appveyor.yml

This reverts commit 886c692.

This reverts commit bccea06.

This reverts commit 8237cbf.

codecov-io · 2021-01-11T23:50:26Z

Codecov Report

Merging #1797 (4848990) into develop (f97cdda) will decrease coverage by 0.13%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #1797      +/-   ##
===========================================
- Coverage    93.97%   93.84%   -0.14%     
===========================================
  Files           89       89              
  Lines         5826     5881      +55     
  Branches      1185     1203      +18     
===========================================
+ Hits          5475     5519      +44     
- Misses         162      166       +4     
- Partials       189      196       +7

Impacted Files	Coverage Δ
samtranslator/model/eventsources/pull.py	`79.03% <0.00%> (-5.59%)`	⬇️
samtranslator/model/s3.py	`85.71% <0.00%> (ø)`
samtranslator/model/lambda_.py	`93.10% <0.00%> (ø)`
samtranslator/region_configuration.py	`100.00% <0.00%> (ø)`
samtranslator/open_api/open_api.py	`92.56% <0.00%> (+0.19%)`	⬆️
...el/preferences/deployment_preference_collection.py	`91.56% <0.00%> (+0.20%)`	⬆️
samtranslator/model/api/http_api_generator.py	`91.06% <0.00%> (+0.28%)`	⬆️
samtranslator/translator/arn_generator.py	`93.10% <0.00%> (+5.60%)`	⬆️

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 f97cdda...4848990. Read the comment docs.

jfuss

This generally looks good. I have a few small comments about keeping consistent between our repos. The other main things for me is there is a lot of borrowed code from SAM CLI to help deploy these stacks. Which I think is ok but do we need all of it? More code here, means more to maintain. I wonder if it is worth stripping this down to reduce the amount of code needed for our integ tests.

jfuss · 2021-01-13T20:16:50Z

Makefile

-	pytest --cov samtranslator --cov-report term-missing --cov-fail-under 95 tests
+	pytest --cov samtranslator --cov-report term-missing --cov-fail-under 95 tests/*
+
+test-integ:


Nit: can we match the naming with SAM CLI? https://github.com/aws/aws-sam-cli/blob/develop/Makefile#L17 That way we are not trying to remember which project does which thing?

Updated the command name to integ-test.

jfuss · 2021-01-13T20:17:36Z

Makefile

+	pytest --cov samtranslator --cov-report term-missing --cov-fail-under 95 tests/*
+
+test-integ:
+	pytest --no-cov tests_integ/*


Same nit here: I would prefer if we standardize our naming and structure across all our repos we own.

Just so I understand correctly, does this mean moving the whole content of the current tests dir under tests/unit and moving the integration tests under tests/integration?

Just moving the tests_integration content in tests/integration will make the unit tests also run the integration tests so we may have to move everything.

I have rename the integration tests folder to have the same name with samcli. Since the unit tests are directly under tests and we don't have a unit folder for them, so currently we keep the integration folder outside the tests folder to separate them from the unit tests. If we create a unit folder for the unit tests in the future, then we can directly move the integration into tests to make them consistently with samcli style.

requirements/dev.txt

jfuss · 2021-01-13T20:20:31Z

tests_integ/helpers/base_test.py

+        cls.s3_bucket_name = S3_BUCKET_PREFIX + generate_suffix()
+        cls.session = boto3.session.Session()
+        cls.my_region = cls.session.region_name
+        cls.s3_client = boto3.client("s3")


Should we be creating clients based on the session instead? I think this is typically the better practice, though it probably doesn't impact anything here. Just caught my eye is all.

Yep this could be better indeed, to be sure we are using the same session throughout

By the boto3 documentation: "Boto3 acts as a proxy to the default session. This is created automatically when you create a low-level client." So all the clients are created by using the default session and during the tests we don't use any customized session.

jfuss · 2021-01-13T20:21:10Z

tests_integ/helpers/base_test.py

+        cls.lambda_client = boto3.client("lambda")
+        cls.iam_client = boto3.client("iam")
+        cls.api_v2_client = boto3.client("apigatewayv2")
+        cls.sfn_client = boto3.client("stepfunctions")


Do we really need each of these clients for every integ test? Shouldn't we only create the client the individual tests need?

I was thinking of this as a way to provide a simple toolbox so that every test could access any service easily, we'll check if creating all the client for each test takes too many resources (memory or cpu time)

Note that the session is not thread safe so instantiating all the clients once for the whole test suite may not be a good idea.

We could set up getters with lazy initialization.

Updated with a Client_provider class with lazy initialization.

jfuss · 2021-01-13T20:23:08Z

tests_integ/helpers/base_test.py

+        """
+        Empties and deletes the bucket used for the tests
+        """
+        s3 = boto3.resource("s3")


Why use resource here but client everywhere else? Seems like resource is a much better way to handle things in boto3 in general?

It seems resource is more high level that client, we'll check to see if we can use resource everywhere.
We needed the resource here to get the list of files in a paginated way.

client is low level and has typically maps 1:1 with the AWS service API while resource is high level but does not provide 100% API coverage of AWS services. Most of the case we use client, here we use resource since it provides a easier way to lists all the objects compared to the client.

jfuss · 2021-01-13T20:23:49Z

tests_integ/helpers/base_test.py

+            try:
+                cls.s3_client.delete_object(Key=object_summary.key, Bucket=cls.s3_bucket_name)
+            except ClientError as e:
+                LOG.error(


Should this cause a test to fail? If there is data left around, will that cause issue for the next run of the tests?

I'm not sure cleanup of a test is part of the test itself, so not sure it should fail.
What we could do is attempt to clean it again and if it fails set this resource name aside and attempt to clean it again at the end of the run.

This shouldn't cause an issue for the next run as we are adding a simple random suffix to each of the objects, stacks and buckets, that we create (see generate_suffix()). I also added this suffix so that we could run the tests in parallel but for now it's not an option as we get throttled.

jfuss · 2021-01-13T20:25:03Z

tests_integ/helpers/base_test.py

+        if cls.my_region == "us-iso-east-1":
+            return "https://s3.us-iso-east-1.c2s.ic.gov/{}/{}".format(cls.s3_bucket_name, file_name)
+        if cls.my_region == "us-isob-east-1":
+            return "https://s3.us-isob-east-1.sc2s.sgov.gov/{}/{}".format(cls.s3_bucket_name, file_name)


Is there no way to get this from S3 itself? Seems like a maintenance burden otherwise.

It seems you can only get a presigned temporary URL from the client.

jfuss · 2021-01-13T20:30:10Z

tests_integ/helpers/helpers.py

@@ -0,0 +1,147 @@
+import json


Do we have a better name then "helpers"? We should try to be explicit (single principle) and in my experience, these types of classes/files just become dumping grounds

Agree, also helpers/helpers.py is redundant.
We could maybe move the transform_template function to helpers\template.py and the rest to helpers\resource.py.

…amcli

jfuss

LGTM

* set up integration test infrastructure for SAM translator * migrated basic integration tests Co-authored-by: Mathieu Grandis <[email protected]>

mingkun2020 and others added 23 commits November 9, 2020 11:07

set up integ test folder structure

1552ea6

rename test_integ to tests_integ to keep it consistent with the unite…

a890b7c

…st folder tests

Added test-integ action to Makefile to launch integration tests

5a16b78

Added all the single templates, fixed an error in basic_function.yaml

582262c

Added test_basic_function.py from the POC, untouched; removed the "ex…

5117bf6

…pected" prefix of the basic function result as we already are in the expected directory

Added basic api inline openapi test as a standalone file to be merged…

c070cdc

… later into a single basic test file, added utils/stack.py

Renamed basic api inline openapi test with the test prefix

243edc3

fix: Reverted to range instead of enumerate

8a9efd2

set up basic code structure for sam integration test

b35d912

Merge branch 'foss-integ-test' of https://github.com/mingkun2020/serv…

eb72ddd

…erless-application-model into foss-integ-test

Updated helpers and expected basic_function file for the new verifica…

0f79f4b

…tion. Updated the makefile to not check coverage for integration tests

add expected json file and fix some bugs

090560b

remove unnecessary print statement

f7ee968

update basic function test

2bde985

Added missing expected files, updated runner

65c2ebd

Removed the ResourceStatus check for each resource

b763e7e

Refactored the test to inherit a BaseTest class, moved the basic_appl…

5a11d35

…ication tests to their own file, fixed some names

add upload zip file and resolve uri with url feature, need refactorin…

a1f583e

…g later

add extra test class and update yaml load call

d12abf4

fix bug, now allow create bucket in multi regions

d8eb9ba

Added random suffix to stack and bucket names to avoid name collisions

083942c

refactor: Moved some stuff into methods in base_test, updated the Mak…

e68b81e

…efile black commands to include the tests_integ directories

chore: Black formatting

4ae155d

awood45 suggested changes Nov 17, 2020

View reviewed changes

hoffa reviewed Nov 17, 2020

View reviewed changes

mingkun2020 and others added 5 commits November 16, 2020 17:51

set up integ test folder structure

4130f3d

rename test_integ to tests_integ to keep it consistent with the unite…

923934b

…st folder tests

Added test-integ action to Makefile to launch integration tests

67c2674

Added all the single templates, fixed an error in basic_function.yaml

ee5402e

Added test_basic_function.py from the POC, untouched; removed the "ex…

0faf6e4

…pected" prefix of the basic function result as we already are in the expected directory

mgrandis added 2 commits December 18, 2020 18:22

refactor: Using LogicalIdGenerator.HASH_LENGTH in verify_stack_resour…

40a7827

…ces, added simple tests and doc for the table_print utils

fix: Fix table_print tests syntax errors for python2

cc19689

hoffa approved these changes Dec 23, 2020

View reviewed changes

refactor: Removed the table_print tests which are complicated to port…

2fc0bb1

… to python2, added a comment in the deployer classes ported over from sam-cli to underline it

jfuss reviewed Dec 23, 2020

View reviewed changes

appveyor.yml Outdated Show resolved Hide resolved

mgrandis and others added 12 commits December 23, 2020 14:56

refactor: Revert newline removal at the end of appveyor.yml

463e1e6

for testing appveyor only, will revert later

886c692

Revert "for testing appveyor only, will revert later"

e04853f

This reverts commit 886c692.

test appveyor failure, will revert later

bccea06

Revert "test appveyor failure, will revert later"

26eed84

This reverts commit bccea06.

just for testing appveyor failure, will revert later

8711cfa

for testing only, will revert later, add ssh access to appveyor vm

35b5b09

for testing only, will revert later

8237cbf

Revert "for testing only, will revert later"

9de384e

This reverts commit 8237cbf.

revert all the previous test changes

3aaa34c

create a appveyor config file for integration test

12bc745

move integration test to a separate appveyor yml file

8f33c82

mingkun2020 added 3 commits January 11, 2021 17:50

delete extra blank line

6806e78

increase retry times to bypass Throttling

3d380b8

black reformatting

d88bddd

jfuss suggested changes Jan 13, 2021

View reviewed changes

mingkun2020 added 3 commits January 14, 2021 20:20

refactor helper.py to have meaningful class name

0bddb9a

rename folder name and make command name to have the sam style with s…

4848990

…amcli

create a client provider class with lazy initialization

d070397

jfuss approved these changes Jan 15, 2021

View reviewed changes

add comments for clients

b3930c1

mingkun2020 merged commit e29c42e into aws:develop Jan 18, 2021

jfuss mentioned this pull request Oct 25, 2021

feat: add end to end tests for features in SAM #1557

Closed

5 tasks

test: Add simple integration tests #1797

test: Add simple integration tests #1797

Uh oh!

Conversation

mingkun2020 commented Nov 14, 2020

Uh oh!

awood45 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-io commented Jan 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jfuss left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mingkun2020 Jan 15, 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!

mgrandis Jan 13, 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!

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!

mgrandis Jan 13, 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!

codecov-io commented Jan 11, 2021 •

edited

Loading

mingkun2020 Jan 15, 2021 •

edited

Loading

mgrandis Jan 13, 2021 •

edited

Loading

mgrandis Jan 13, 2021 •

edited

Loading