Skip to content

StepFunctions: Clean-up legacy JSONata unit tests #11921

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 4 commits into from
Jan 24, 2025
Merged

StepFunctions: Clean-up legacy JSONata unit tests #11921

merged 4 commits into from
Jan 24, 2025

Conversation

MEPalma
Copy link
Contributor

@MEPalma MEPalma commented Nov 25, 2024 •
edited
Loading

Motivation

Given the extensive number of JSONata library integration tests now included in the v2 integration test suite, along with the intention to continue expanding them, these changes remove most of the legacy unit tests for JSONata integration. The legacy tests were previously skipped due to undesired imports from the integration test library, which created an import antipattern. Additionally, duplicate test cases were identified and removed from the analytics test suite.

Changes

  • remove legacy SFN-JSONata test cases
  • resolve imports from integration tests module

@MEPalma MEPalma added the semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases label Nov 25, 2024
@MEPalma MEPalma self-assigned this Nov 25, 2024
@MEPalma MEPalma added semver: patch Non-breaking changes which can be included in patch releases and removed semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases labels Nov 25, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 25, 2024 •
edited
Loading

LocalStack Community integration with Pro

    2 files  ±    0      2 suites  ±0   33m 33s ⏱️ - 1h 17m 46s
1 391 tests  - 2 592  1 321 ✅  - 2 346  70 💤  - 246  0 ❌ ±0 
1 393 runs   - 2 592  1 321 ✅  - 2 346  72 💤  - 246  0 ❌ ±0 

Results for commit ddf5905. ± Comparison against base commit 6fea92c.

This pull request removes 2592 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.

dominikschubert
dominikschubert reviewed Nov 25, 2024
View reviewed changes
tests/unit/services/stepfunctions/test_arguments.py Outdated
Comment on lines 8 to 10
from tests.aws.services.stepfunctions.templates.querylanguage.query_language_templates import (
QueryLanguageTemplate,
)
Copy link
Member

Choose a reason for hiding this comment

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

I would actually still heavily discourage importing from the integration tests here in the unit tests. Duplicating the state machine definition here is absolutely fine for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. I was considering converting these to snapshot tests instead for the scenarios we don't cover there

octavio12345300

This comment was marked as spam.

Sign in to view

@MEPalma MEPalma changed the title SFN: Enable Unit Tests for JSONata Integration StepFunctions: Cleanup Legacy JSONata unit tests Jan 22, 2025
@MEPalma MEPalma changed the title StepFunctions: Cleanup Legacy JSONata unit tests StepFunctions: Clean-up Legacy JSONata unit tests Jan 22, 2025
@MEPalma MEPalma marked this pull request as ready for review January 22, 2025 10:30
@MEPalma MEPalma changed the title StepFunctions: Clean-up Legacy JSONata unit tests StepFunctions: Clean-up legacy JSONata unit tests Jan 22, 2025
@MEPalma MEPalma added semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases and removed semver: patch Non-breaking changes which can be included in patch releases labels Jan 22, 2025
joe4dev
joe4dev approved these changes Jan 24, 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.

Coverage overlap is hard to judge without a deep understanding, but LGTM as far as I can judge.
Favoring aws-validated tests (e.g., tests/aws/services/stepfunctions/v2/evaluate_jsonata) over potentially drifting unit test makes sense here.

🧹 🧹🧹

tests/aws/services/stepfunctions/v2/evaluate_jsonata/test_base_evaluate_expressions.py
reason="Single-quote esacped JSONata expressions are not yet supported",
),
),
{"Items": EJT.JSONATA_ARRAY_ELEMENT_EXPRESSION},
Copy link
Member

Choose a reason for hiding this comment

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

🥳

tests/unit/services/stepfunctions/test_usage_metrics_static_analyser.py
@@ -5,92 +5,134 @@
from localstack.services.stepfunctions.asl.static_analyser.usage_metrics_static_analyser import (
UsageMetricsStaticAnalyser,
)
from tests.aws.services.stepfunctions.templates.assign.assign_templates import (
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for getting rid of such cross-test imports 🧹

@MEPalma MEPalma merged commit 0a5cd13 into master Jan 24, 2025
44 checks passed
@MEPalma MEPalma deleted the MEP-sfn-sfn_unit_tests_uncomment branch January 24, 2025 16: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

@dominikschubert dominikschubert Awaiting requested review from dominikschubert

+1 more reviewer

@octavio12345300 octavio12345300 octavio12345300 left review comments

Reviewers whose approvals may not affect merge requirements
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
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@MEPalma @dominikschubert @joe4dev @octavio12345300