Skip to content

APIGW NG fix last tests #11198

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
Jul 16, 2024
Merged

APIGW NG fix last tests #11198

merged 7 commits into from
Jul 16, 2024

Conversation

bentsku
Copy link
Contributor

@bentsku bentsku commented Jul 13, 2024 •
edited
Loading

Motivation

Fix the last following tests in order to be able to enable a new CI step to test the next gen APIGW invocation logic.

FAILED tests/aws/scenario/note_taking/test_note_taking.py::TestNoteTakingScenario::test_notes_rest_api
FAILED tests/aws/services/stepfunctions/v2/services/test_apigetway_task_service.py::TestTaskApiGateway::test_invoke_base
FAILED tests/aws/services/stepfunctions/v2/services/test_apigetway_task_service.py::TestTaskApiGateway::test_invoke_with_body_post[None]
FAILED tests/aws/services/stepfunctions/v2/services/test_apigetway_task_service.py::TestTaskApiGateway::test_invoke_with_body_post[]
FAILED tests/aws/services/stepfunctions/v2/services/test_apigetway_task_service.py::TestTaskApiGateway::test_invoke_with_body_post[HelloWorld]
FAILED tests/aws/services/stepfunctions/v2/services/test_apigetway_task_service.py::TestTaskApiGateway::test_invoke_with_body_post[request_body3]
FAILED tests/aws/services/stepfunctions/v2/services/test_apigetway_task_service.py::TestTaskApiGateway::test_invoke_with_query_parameters
FAILED tests/aws/test_multiregion.py::TestMultiRegion::test_multi_region_api_gateway

Changes

  • fix the tests, only a few changes needed

Testing

Successful run with the flag enabled: https://app.circleci.com/jobs/github/localstack/localstack/227232

TODO

What's left to do:

  • remove the forced flag (sorry @thrau you're asked for review because of the forced flag change in config, will remove once the base PR for this is merged 😅)

@bentsku bentsku added aws:apigateway Amazon API Gateway semver: patch Non-breaking changes which can be included in patch releases labels Jul 13, 2024
@bentsku bentsku self-assigned this Jul 13, 2024
@bentsku bentsku mentioned this pull request Jul 13, 2024
Merged
@bentsku bentsku force-pushed the apigw-ng-fix-tests branch from c0f49ae to 9aa346d Compare July 13, 2024 00:48
@github-actions GitHub Actions
Copy link

github-actions bot commented Jul 13, 2024 •
edited
Loading

S3 Image Test Results (AMD64 / ARM64)

  2 files    2 suites   3m 31s ⏱️
404 tests 352 ✅  52 💤 0 ❌
808 runs  704 ✅ 104 💤 0 ❌

Results for commit 3597b1d.

♻️ This comment has been updated with latest results.

@bentsku bentsku mentioned this pull request Jul 13, 2024
Merged
2 tasks
@bentsku bentsku added this to the 3.6 milestone Jul 13, 2024
@bentsku bentsku marked this pull request as ready for review July 13, 2024 02:14
@bentsku bentsku removed the request for review from thrau July 15, 2024 16:41
cloutierMat
cloutierMat approved these changes Jul 15, 2024
View reviewed changes
Copy link
Contributor

@cloutierMat cloutierMat left a comment

Choose a reason for hiding this comment

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

🎉 🎈 🎉 All the test! 👀 Congrats!

I only left a comment about the testing configuration function in the helper file 😕, but nothing blocking, this can easily be tackled in a follow up

...ck-core/localstack/services/apigateway/next_gen/execute_api/handlers/integration_response.py Outdated
@@ -110,19 +110,18 @@ def __call__(
headers[f"x-amzn-remapped-{header}"] = value
headers.remove(header)

# Those headers are passed through from the response headers, there might be more?
passthrough_headers = ("connection", "content-length")
# # Those headers are passed through from the response headers, there might be more?
Copy link
Contributor

Choose a reason for hiding this comment

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

Can probably remove that ;)

Suggested change
# # Those headers are passed through from the response headers, there might be more?
# Those headers are passed through from the response headers, there might be more?

localstack-core/localstack/services/apigateway/helpers.py
Comment on lines +877 to +879
"requestParameters": {
"integration.request.header.Content-Type": "'application/x-www-form-urlencoded'"
},
Copy link
Contributor

Choose a reason for hiding this comment

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

We seem to have a few "testing only" helpers in this file. Should we make a plan to move them to the tests files?

Copy link
Contributor Author

@bentsku bentsku Jul 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes please 😭 We really should, I believe once we migrate the current implementation into a legacy folder, we can find all imports to legacy and refactor all of it. This function for example makes no sense and is not even that right...
Ultimately the goal would be to remove this whole file all together and find its usages and refactor it or keep it in the "next-gen" helper file.

Base automatically changed from apigw-ng-aws-integration to master July 16, 2024 09:59
@bentsku bentsku force-pushed the apigw-ng-fix-tests branch from 3597b1d to 352ebf2 Compare July 16, 2024 10:07
@github-actions GitHub Actions
Copy link

LocalStack Community integration with Pro

  2 files    2 suites   24m 34s ⏱️
683 tests 637 ✅ 46 💤 0 ❌
685 runs  637 ✅ 48 💤 0 ❌

Results for commit 352ebf2.

@bentsku bentsku merged commit 446c876 into master Jul 16, 2024
36 checks passed
@bentsku bentsku deleted the apigw-ng-fix-tests branch July 16, 2024 10:33
@cloutierMat cloutierMat mentioned this pull request Jul 18, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cloutierMat cloutierMat cloutierMat approved these changes

Assignees

@bentsku bentsku

Labels
aws:apigateway Amazon API Gateway semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Milestone
3.6
Development

Successfully merging this pull request may close these issues.
2 participants
@bentsku @cloutierMat