Convert manual integration tests to pytest tests #148

ghukill · 2023-08-31T19:54:54Z

What does this PR do?

This PR adds some pytest integration tests to replace the need for running the manual tests previously defined in the application README.

As noted in the updated README, these new tests have some limitations:

tests are limited to Dev1 environment
AWS AWSAdministratorAccess role credentials must be set on developer machine
environment variable WORKSPACE=dev must be set, but no other environment variables are required, these are all retrieved from deployed context

A new pytest marker integration has been created that identifies these tests as "integration" tests, which can be run separately.

These tests rely on some fixtures that allow the calling environment to utilize AWS credentials set in the developer machine to make boto3 calls to retrieve actual, deployed environment variables, and check the results of actions performed. These fixtures also upload files to S3 for PPOD and TIMDEX, such that those StepFunction invocations will have something to work with.

Helpful background context

Previously, the README provided some instructions on how to manually test the deployed webhook handling lambda. However, it was somewhat difficult to set the required environment variables and ensure proper artifacts were present in S3, making these tests potentially cumbersome to run.

How can a reviewer manually see the effects of these changes?

Set WORKSPACE=dev either as a command line environment variable or via the .env file.
Run integration tests:

make test-integration

Observe that tests pass and step functions were invoked:

PPOD
TIMDEX

Includes new or updated dependencies?

NO

What are the relevant tickets?

Closes Convert manual tests from README into pytest tests #147

Developer

All new ENV is documented in README
Stakeholder approval has been confirmed (or is not needed)

Code Reviewer

The commit message is clear and follows our guidelines (not just this pull request message)
There are appropriate tests covering any new functionality
The documentation has been updated or is unnecessary
The changes have been verified
New dependencies are appropriate or there were no changes

Why these changes are being introduced: Previously, the README provided some instructions on how to manually test the deployed webhook handling lambda. However, it was somewhat difficult to set the required environment variables and ensure proper artifacts were present in S3. How this addresses that need: A new pytest marker `integration` was created that identifies some tests as "integration" tests, which can be run separately. These tests rely on some fixtures that allow the calling environment to utilize AWS credentials set in the developer machine to make calls to AWS to retrieve environment variables, and check the results of actions performed. These fixtures also upload files to S3 for PPOD and TIMDEX, such that those StepFunction invocations will have something to work with. This allows for running 'make test-integration' locally to perform these tests without the manual work previously required. Side effects of this change: * Nonex, except some testing fixtures were shuffled around to support the more nuanced handling of environment variables. Relevant ticket(s): #147 Closes #147

github-actions · 2023-08-31T20:04:12Z

Pull Request Test Coverage Report for Build 6085878947

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 100.0%

Totals
Change from base Build 6015788420:	0.0%
Covered Lines:	139
Relevant Lines:	139

💛 - Coveralls

ehanson8

Works as expected, a few comments on structure and optional suggestions

ehanson8 · 2023-09-01T14:35:26Z

tests/conftest.py

+
+
+@pytest.fixture
+def _set_integration_tests_env_vars() -> None:


While the comments help, this is a bit involved and long so you might consider breaking this up into smaller fixtures that are then called by this fixture for readability

Thanks, that's a good suggestion. This prompted utilizing a contextmanager to temporarily reset the environment, for each integration test, which gaurantees it's torn down correctly as well.

Also broke the lambda config and SMS retrievals into their own functions. Hopefully you can look at this fixture now (changes coming in a new commit) and see that it's:

using a context manager to temporarily reset the environment

call some functions that get data from AWS and set to environment

release all those changes when that test completes

tests/conftest.py

ehanson8 · 2023-09-01T14:55:24Z

tests/conftest.py

+    """
+    s3 = boto3.client("s3")
+
+    def check_and_upload_file(bucket, key):


I'm not sure of the best practice on something like this but this design pattern feels weird (defining a function inside a fixture). It seems possible to just stick this at the top:

# Specify your bucket and key fixtures = [ ( "dev-sftp-shared", "exlibris/pod/POD_ALMA_EXPORT_20230815_220844[016]_new.tar.gz", ), ( "dev-sftp-shared", "exlibris/timdex/TIMDEX_ALMA_EXPORT_DAILY_20230815_220844[016]_new.tar.gz", ), ] for bucket, key in fixtures:

But I don't have enough experience with this type of integration tests to have much confidence in commenting on this

Good call! Much easier to call via a loop, and can tighten up that S3 file check/upload code as well.

ehanson8 · 2023-09-01T14:57:42Z

tests/conftest.py

+
+    https://docs.pytest.org/en/7.1.x/reference/reference.html#pytest.hookspec.pytest_collection_modifyitems
+    """
+    # skip integration tests if WORKSPACE is not 'dev'


I'd move this into the docstring, I like this function

Normally I'd agree, but this hook is a built-in function for pytests, and is called just by virtue of being named pytest_collection_modifyitems. Therefore, if we want to add other logic that should fire during this hook, it would also get added here.

But now that you mention it.. this behavior could be put into another function that this hook calls. Then, that function's docstring can explain what it does, and future logic / functions could be added to this hook if needed.

Thanks for flagging!

tests/integration/test_webhook.py

ehanson8

Looks good, easier to parse now! I'd encourage @jonavellecuerdo to run these tests as well to surface any other questions as we start down the path of these types of integration tests

ghukill · 2023-09-01T18:41:29Z

Looks good, easier to parse now! I'd encourage @jonavellecuerdo to run these tests as well to surface any other questions as we start down the path of these types of integration tests

Agreed. I'm still not convinced this pattern is a good one, but it feels like it moves in the right direction of converting those manual tests into something programmattic.

tests/integration/test_webhook.py

Makefile

jonavellecuerdo · 2023-09-05T13:58:23Z

README.md


-#### GET request example
+#### Steps to run integration tests


Can we use numbering to enumerate these steps? I think it would help with the readability! :)

README.md

jonavellecuerdo · 2023-09-05T14:04:38Z

@ghukill I added some minor comments, but overall, I think this looks great! It is a significant improvement from the manual tests that preceded these updates. I don't have suggestions for improvement but will continue to reflect on these changes. Awesome work. 🚀

ghukill requested review from ehanson8 and jonavellecuerdo August 31, 2023 19:55

reinstate coveralls Make command

26295d6

ehanson8 reviewed Sep 1, 2023

View reviewed changes

PR revisions

7a7f51c

ghukill requested a review from ehanson8 September 1, 2023 16:49

ehanson8 approved these changes Sep 1, 2023

View reviewed changes

jonavellecuerdo reviewed Sep 5, 2023

View reviewed changes

tests/integration/test_webhook.py Show resolved Hide resolved

jonavellecuerdo reviewed Sep 5, 2023

View reviewed changes

Makefile Show resolved Hide resolved

jonavellecuerdo reviewed Sep 5, 2023

View reviewed changes

README.md Show resolved Hide resolved

update readme based on PR comments

ee2382f

ghukill merged commit b702ff3 into main Sep 5, 2023
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert manual integration tests to pytest tests #148

Convert manual integration tests to pytest tests #148

ghukill commented Aug 31, 2023 •

edited

github-actions bot commented Aug 31, 2023 •

edited

ehanson8 left a comment

ehanson8 Sep 1, 2023

ghukill Sep 1, 2023

ehanson8 Sep 1, 2023

ghukill Sep 1, 2023

ehanson8 Sep 1, 2023

ghukill Sep 1, 2023 •

edited

ehanson8 left a comment

ghukill commented Sep 1, 2023

jonavellecuerdo Sep 5, 2023

jonavellecuerdo commented Sep 5, 2023



		@pytest.fixture
		def _set_integration_tests_env_vars() -> None:

Convert manual integration tests to pytest tests #148

Convert manual integration tests to pytest tests #148

Conversation

ghukill commented Aug 31, 2023 • edited

What does this PR do?

Helpful background context

How can a reviewer manually see the effects of these changes?

Includes new or updated dependencies?

What are the relevant tickets?

Developer

Code Reviewer

github-actions bot commented Aug 31, 2023 • edited

Pull Request Test Coverage Report for Build 6085878947

💛 - Coveralls

ehanson8 left a comment

Choose a reason for hiding this comment

ehanson8 Sep 1, 2023

Choose a reason for hiding this comment

ghukill Sep 1, 2023

Choose a reason for hiding this comment

ehanson8 Sep 1, 2023

Choose a reason for hiding this comment

ghukill Sep 1, 2023

Choose a reason for hiding this comment

ehanson8 Sep 1, 2023

Choose a reason for hiding this comment

ghukill Sep 1, 2023 • edited

Choose a reason for hiding this comment

ehanson8 left a comment

Choose a reason for hiding this comment

ghukill commented Sep 1, 2023

jonavellecuerdo Sep 5, 2023

Choose a reason for hiding this comment

jonavellecuerdo commented Sep 5, 2023

ghukill commented Aug 31, 2023 •

edited

github-actions bot commented Aug 31, 2023 •

edited

ghukill Sep 1, 2023 •

edited