Skip to content

Step Functions: Migrate v2 Test Suite to no_retry aws_client Fixture #12461

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Apr 3, 2025
Merged

Step Functions: Migrate v2 Test Suite to no_retry aws_client Fixture #12461

merged 7 commits into from
Apr 3, 2025

Conversation

MEPalma
Copy link
Contributor

@MEPalma MEPalma commented Mar 31, 2025 •
edited
Loading

Motivation

To minimize unnecessary retries of boto actions, a _no_retry variant of the aws_client boto client factory fixture was recently introduced in #12441.
These changes aim to migrate the SFN v2 test suite to use this new fixture. By doing so, we eliminate redundant retries that occur during negative snapshot tests, but also in potential flaky tests in the test suite.

Changes

  • Migrate v2 test suite from aws_client boto client factory to aws_client_no_retry

@MEPalma MEPalma added the semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases label Mar 31, 2025
@MEPalma MEPalma added this to the Playground milestone Mar 31, 2025
@MEPalma MEPalma self-assigned this Mar 31, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 31, 2025 •
edited
Loading

LocalStack Community integration with Pro

    2 files  ±    0      2 suites  ±0   32m 55s ⏱️ - 1h 16m 38s
1 467 tests  - 2 842  1 394 ✅  - 2 587  73 💤  - 255  0 ❌ ±0 
1 469 runs   - 2 842  1 394 ✅  - 2 587  75 💤  - 255  0 ❌ ±0 

Results for commit fcd2f1b. ± Comparison against base commit e1f3422.

This pull request removes 2842 tests. 
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_lambda_dynamodb
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_opensearch_crud
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_search_books
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_setup
tests.aws.scenario.kinesis_firehose.test_kinesis_firehose.TestKinesisFirehoseScenario ‑ test_kinesis_firehose_s3
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_destination_sns
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_infra
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_prefill_dynamodb_table
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input0-SUCCEEDED]
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input1-SUCCEEDED]
…

♻️ This comment has been updated with latest results.

@MEPalma MEPalma marked this pull request as ready for review April 1, 2025 12:43
@MEPalma MEPalma modified the milestones: Playground, 4.4 Apr 1, 2025
@MEPalma MEPalma changed the title Step Functions: Migrate v2 Test SuiteTo no_retry aws_client Fixture Step Functions: Migrate v2 Test Suite to no_retry aws_client Fixture Apr 1, 2025
joe4dev
joe4dev requested changes Apr 1, 2025
View reviewed changes
Copy link
Member

@joe4dev joe4dev left a comment

Choose a reason for hiding this comment

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

Good idea to pro-actively adopt the new aws_client_no_retry 🚀

However, we should avoid replacing all clients and use aws_client_no_retry only "when testing exceptions (i.e., with pytest.raises(...)) or expected errors (e.g., status code 500)" https://github.com/localstack/localstack/pull/12441/files#diff-2776b578d9cfe9e8eaed135a0f652801d1e00d4bfee6eed855d6d097fc33f8c0R76
Retries actually help to reduce flakiness, especially when running tests against AWS.

All credits go to @dfangl for noticing this :)

@MEPalma
Copy link
Contributor Author

MEPalma commented Apr 2, 2025

Thank you! I have updated these changes to only use the _no_retry variant in negative scenarios.
I understand that AWS can be flaky in positive scenarios, but I’d also consider that negative scenarios may not always be immune to flakiness—something that may be interesting to reflect on for our testing approach.
My intention was to ensure that the SFN service is exact on the first shot, given that LocalStack has no reason to behave otherwise, unlike AWS. Moving forward, I’d like the test suite to adopt this approach for LocalStack, but consider disabling the one-shot behavior when testing against AWS.

@MEPalma MEPalma requested a review from joe4dev April 2, 2025 14:00
joe4dev
joe4dev approved these changes Apr 3, 2025
View reviewed changes
Copy link
Member

@joe4dev joe4dev left a comment

Choose a reason for hiding this comment

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

This change highlights the extensive test coverage of exception cases for StepFunctions 🙌 ✨

I’d also consider that negative scenarios may not always be immune to flakiness—something that may be interesting to reflect on for our testing approach.

Very good point and something to observe while updating snapshots. Given this process is currently manual, I don't think it's a problem for now. Re-running the entire test using pytest-rerunfailures could be a suitable alternative here to a conditional client configuration.

tests/aws/services/stepfunctions/v2/test_sfn_api_logs.py
@@ -142,11 +142,8 @@ def test_invalid_logging_configuration(

sm_name = f"statemachine_{short_uid()}"

stepfunctions_client = aws_client_factory(
config=Config(parameter_validation=False)
Copy link
Member

Choose a reason for hiding this comment

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

nice catch that parameter validation disabling didn't seem necessary here 👍

@MEPalma MEPalma merged commit 997bcff into master Apr 3, 2025
31 checks passed
@MEPalma MEPalma deleted the MEP-SFN-testing_aws_client_no_retry branch April 3, 2025 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joe4dev joe4dev joe4dev approved these changes

@gregfurman gregfurman Awaiting requested review from gregfurman gregfurman is a code owner

Assignees

@MEPalma MEPalma

Labels
semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.
2 participants
@MEPalma @joe4dev