Skip to content
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

test: Step functions remote invoke integ tests #5502

Merged
merged 10 commits into from
Jul 15, 2023

Conversation

hnnasit
Copy link
Contributor

@hnnasit hnnasit commented Jul 12, 2023

Why is this change necessary?

This PR adds integration tests for step functions for the remote invoke command. The tests are skipped for now as the resource is not supported yet.

  • TestSFNPriorityInvoke contains a single Step function for testing remote invoke commands when the resource_id is not provided and the resource is invoked by default.
  • Tests are added to TestMultipleResourcesInvoke and TestNestedTemplateResourcesInvoke with different combinations of commands and resources.

What side effects does this change have?

N/A

Mandatory Checklist

PRs will only be reviewed after checklist is complete

  • Add input/output type hints to new functions/methods
  • Write design document if needed (Do I need to write a design document?)
  • Write/update unit tests
  • Write/update integration tests
  • Write/update functional tests if needed
  • make pr passes
  • make update-reproducible-reqs if dependencies were changed
  • Write documentation

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

@hnnasit hnnasit requested a review from a team as a code owner July 12, 2023 21:56
@hnnasit hnnasit requested review from lucashuy and mildaniel July 12, 2023 21:56
Copy link
Contributor

@mndeveci mndeveci left a comment

Choose a reason for hiding this comment

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

LGTM in general, left couple of questions to see if we can automate test skipping.

tests/integration/remote/invoke/test_remote_invoke.py Outdated Show resolved Hide resolved
Comment on lines 399 to 400
# Skip remote invoke Step function integration tests
# ("ChildStack/HelloWorldStateMachine", '{"key1": "Hello", "key2": "world"}', "World"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Just thinking out loud, can we use the dictionary we use in factory implementation to decide whether certain test group should be run or skipped? In that sense we don't need to track to uncomment these lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, let me think about changing it to a skipIf so that it's easier to maintain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed this in the new commit, used the factory dict so that only that needs to be updated when new resources support is added.

@@ -58,6 +58,8 @@ def create_resources_and_boto_clients(cls):
}
cls.cfn_client = get_boto_client_provider_with_config()("cloudformation")
cls.lambda_client = get_boto_client_provider_with_config()("lambda")
cls.stepfunctions_client = get_boto_client_provider_with_config()("stepfunctions")
cls.xray_client = get_boto_client_provider_with_config()("xray")
Copy link
Contributor

@hawflau hawflau Jul 12, 2023

Choose a reason for hiding this comment

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

why do we need to call get_boto_client_provider_with_config() 5 times?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good callout, can create a variable and create a single call.

Co-authored-by: Wing Fung Lau <[email protected]>
'{"key1": "Hello", "key2": "serverless", "key3": "world"}',
{"key1": "Hello", "key2": "serverless", "key3": "world"},
),
# Skip remote invoke Step function integration tests
Copy link
Contributor

@hawflau hawflau Jul 12, 2023

Choose a reason for hiding this comment

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

is this not needed or will it be un-commented in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will be un-commented once step functions is supported for remote invoke. Trying to think of a way to not have to comment them and skip them based on the resource-type.

@hnnasit hnnasit requested review from hawflau and mndeveci July 13, 2023 16:44
@hnnasit hnnasit added this pull request to the merge queue Jul 14, 2023
Merged via the queue into aws:develop with commit db5b7f3 Jul 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants