Conversation
…ed integration tests to catch issues like this again in future.
for more information, see https://pre-commit.ci
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
for more information, see https://pre-commit.ci
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2505 +/- ##
=======================================
Coverage 82.44% 82.45%
=======================================
Files 79 79
Lines 10142 10146 +4
Branches 1165 1165
=======================================
+ Hits 8362 8366 +4
Misses 1579 1579
Partials 201 201 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Pull request overview
Adds SQS/SNS integration coverage (using GoAws) and fixes a None handling bug in the async SQS request parameter merge logic, aligning with prior work to prevent fanout-related regressions.
Changes:
- Added a new tox integration environment for SQS backed by a GoAws Docker container.
- Added SQS integration tests (including SNS->SQS fanout) and a unit test validating
protocol_params=Nonehandling. - Refactored
AsyncSQSConnection.make_requestparameter merging into a helper that safely handlesNone.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
tox.ini |
Adds integration-sqs tox env and a goaws docker service definition. |
kombu/asynchronous/aws/sqs/connection.py |
Fixes make_request param merging to tolerate protocol_params=None via _build_make_request_params. |
t/unit/asynchronous/aws/sqs/test_connection.py |
Adds unit coverage for the new param-building helper behavior. |
t/integration/test_sqs.py |
Introduces GoAws-backed integration tests for SQS, including fanout behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…all a non-existant requirements file.
… with not using 'is_secure' in transport options config.
for more information, see https://pre-commit.ci
|
probably some suggested code did not work so integration tests are failing |
|
Hey @auvipy, Thanks for the quick review and feedback. The least intrusive change I could think of was to import the SNS class as SnsFanout, which prevents the naming collision. Best, |
|
botocore.exceptions.EndpointConnectionError: Could not connect to the endpoint URL: "http://localhost:4100/" |
This PR adds basic integration tests for the Amazon SQS & SNS transports as requested on PR #2479.
To achieve this I looked at using LocalStack, however an API key/licence was required and would have been a bit overkill for the testing needs of this project. I have used a cool project called GoAws, which runs as a Docker container and easily integrates as a drop-in replacement for AWS SQS and SNS, including testing Fanout (SNS -> SQS subscriptions).
A small fix was also made to how parameters are handled in the
AsyncSQSConnection.make_requestmethod, as it appears it should be able to handle aNonevalue for theprotocol_paramsparameter, which caused an exception. The small fix now correctly handles None values being passed, and tests have been added for validation.