feat(template): add real-broker worker integration tests - #49
Merged
Conversation
Extract the testing patterns proven in litestar-faststream and litestar-events#9 into the worker component, and fix the systemic coverage-gate failures the work surfaced (ADR-008). Worker testing: - Replace the module-level broker singleton with a build_broker(url=None) factory that registers the subscriber/publisher and resolves the connection at call time, giving tests an injectable seam. - Rewrite unit tests to the collector-subscriber pattern (assert on observable behavior, not internal wiring). - Add a testcontainers-backed integration test (marked `integration`, deselected by default) that round-trips a message through the real broker driver, plus an on-demand `tox -e integration` env and a conditional Ubuntu-only `worker-integration` CI job gating `check`. Coverage gate (fail_under = 99), systemic fixes: - Exclude untestable process/UI/server entrypoints and the PackageNotFoundError fallback via exclude_lines; pragma the CLI launcher subcommands, GUI/TUI display helpers, worker lifecycle hooks, and the c-extension import fallback. - Anchor coverage omit globs on filenames (*/_version.py) so they also match tox's site-packages installs (src/** silently missed them). - Remap [tool.coverage.paths] to */site-packages/<pkg> so multi-env `coverage combine` unifies per-interpreter data instead of counting version-gated lines as missed. Validated for all four brokers (kafka/nats/rabbitmq/redis): full style suite, multi-env tox with 100% coverage, and real-container integration round-trips. Documented in docs/adr/008-worker-broker-testing-strategy.md, CLAUDE.md, and the generated README.
Contributor
Reviewer's GuideIntroduces a broker factory-based worker design to enable real-broker integration testing with testcontainers, adds a dedicated tox env and CI job for integration tests, and fixes systemic coverage configuration issues (paths, omit patterns, and untestable entrypoints) across the template while updating worker/CLI/GUI/TUI code and docs accordingly. Sequence diagram for the worker broker factory and integration testsequenceDiagram
actor Tester
participant IntegrationTest
participant TestcontainersBroker
participant build_broker
participant Broker
participant handle_version_request
participant Publisher
Tester->>IntegrationTest: run pytest -m integration
IntegrationTest->>TestcontainersBroker: start() # from testcontainers
TestcontainersBroker-->>IntegrationTest: return broker_url
IntegrationTest->>build_broker: build_broker(url)
build_broker-->>Broker: return configured broker
build_broker->>Broker: broker.publisher("version-responses")
build_broker->>Broker: broker.subscriber("version-requests")
Broker-->>handle_version_request: register handler via decorator
IntegrationTest->>Broker: [send VersionRequest]
Broker-->>handle_version_request: invoke with VersionRequest
handle_version_request->>Publisher: publisher.publish(VersionResponse)
Publisher-->>IntegrationTest: [collector receives VersionResponse]
IntegrationTest-->>Tester: assert on collected VersionResponse
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
hasansezertasan
added
the
no-issue
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The
build_brokerfactory currently relies on a nestedhandle_version_requestclosure and an_ = handle_version_requestreference to appease static analysis; consider exposing the handler as a named module-level function or adding a targeted vulture/ruff ignore instead of this indirection to keep the worker wiring easier to reason about and reuse. - The
integrationtox env relaxes deprecation warnings globally via-W default::DeprecationWarning, which can mask deprecations in your own code; you may want to narrow this to specific third-party modules (e.g., testcontainers and broker drivers) to keep template code honest while still preventing external noise from failing the run.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `build_broker` factory currently relies on a nested `handle_version_request` closure and an `_ = handle_version_request` reference to appease static analysis; consider exposing the handler as a named module-level function or adding a targeted vulture/ruff ignore instead of this indirection to keep the worker wiring easier to reason about and reuse.
- The `integration` tox env relaxes deprecation warnings globally via `-W default::DeprecationWarning`, which can mask deprecations in your own code; you may want to narrow this to specific third-party modules (e.g., testcontainers and broker drivers) to keep template code honest while still preventing external noise from failing the run.
## Individual Comments
### Comment 1
<location path="template/pyproject.toml.jinja" line_range="304" />
<code_context>
+# separate copy per env and a version-gated line covered in one interpreter but not
+# another is wrongly counted as missed — silently failing the `fail_under` gate.
+{{github_repo_name}} = ["src/{{github_repo_name}}", "*/site-packages/{{github_repo_name}}"]
+tests = ["tests", "*/site-packages/tests"]
</code_context>
<issue_to_address>
**suggestion (testing):** Reconsider mapping `tests` to `*/site-packages/tests` in `coverage.paths`.
The `tests` package is typically not installed into site-packages, so this mapping is likely unused and could unintentionally merge coverage from an unrelated installed `tests` package. Since `tests` is already listed as a source package and mapped to the repo-local `tests` directory, consider removing the `*/site-packages/tests` entry unless you have a specific case where your own tests are installed there.
Suggested implementation:
```
# Unify coverage data across environments. tox installs the built wheel/sdist into
# each env's site-packages, so the same source file is measured under a different
# `.tox/<env>/.../site-packages/{{github_repo_name}}` path per interpreter. Without
# remapping those back to the canonical `src/` tree, `coverage combine` keeps a
# separate copy per env and a version-gated line covered in one interpreter but not
# another is wrongly counted as missed — silently failing the `fail_under` gate.
{{github_repo_name}} = ["src/{{github_repo_name}}", "*/site-packages/{{github_repo_name}}"]
```
If `tests` needs to be treated as a source package for coverage, ensure it is listed under `[tool.coverage.run]` (e.g. `source = ["{{github_repo_name}}", "tests"]`) elsewhere in the template, without adding any `coverage.paths` remapping to `*/site-packages/tests`. This keeps coverage focused on the repo-local tests directory while avoiding accidental merging with unrelated installed `tests` packages.
</issue_to_address>
### Comment 2
<location path="template/pyproject.toml.jinja" line_range="279" />
<code_context>
+ # don't let `error` promote it to a collection/test failure (ADR-008). The
+ # integration module is imported during default collection even though its tests
+ # are deselected by the `integration` marker.
+ "ignore:The @wait_container_is_ready decorator is deprecated:DeprecationWarning",
+{%- endif %}
]
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Narrow the deprecation warning filter to just the testcontainers import site if possible.
This filter will silence any `DeprecationWarning` with that message, regardless of where it comes from. If testcontainers emits it from a consistent module path, consider using the full `action:message:category:module` form scoped to `testcontainers.*` to avoid hiding unrelated deprecations while still ignoring this specific library noise.
Suggested implementation:
```
# testcontainers applies its own deprecated @wait_container_is_ready decorator at
# import time. The warning is internal to the library and not actionable here, so
# don't let `error` promote it to a collection/test failure (ADR-008). The
# integration module is imported during default collection even though its tests
# are deselected by the `integration` marker.
+ "ignore:The @wait_container_is_ready decorator is deprecated:DeprecationWarning:testcontainers.*",
```
```
"error",
# It is harmless for tests, so don't let `error` promote it to a failure.
"ignore:Using `httpx` with `starlette.testclient` is deprecated:Warning",
{%- endif %}
{%- if include_worker %}
# testcontainers applies its own deprecated @wait_container_is_ready decorator at
# import time. The warning is internal to the library and not actionable here, so
# don't let `error` promote it to a collection/test failure (ADR-008). The
# integration module is imported during default collection even though its tests
# are deselected by the `integration` marker.
"ignore:The @wait_container_is_ready decorator is deprecated:DeprecationWarning:testcontainers.*",
docs = [
```
If the actual module path for the deprecation is known (e.g. `testcontainers.core.waiting_utils`), you can replace `testcontainers.*` with the more precise module name to further narrow the filter:
`"ignore:The @wait_container_is_ready decorator is deprecated:DeprecationWarning:testcontainers.core.waiting_utils"`.
</issue_to_address>
### Comment 3
<location path="docs/adr/008-worker-broker-testing-strategy.md" line_range="8" />
<code_context>
+Accepted (2026-06). Implemented.
+
+> **Revision (during implementation).** The original draft proposed *lazy broker
+> imports* as item 1. Implementation showed that buys nothing for this worker:
+> it has exactly one broker, `faststream[<broker>]` is a mandatory dependency
+> (so the driver is always installed), and the broker is constructed at module
</code_context>
<issue_to_address>
**issue (typo):** Sentence "Implementation showed that buys nothing for this worker" is missing a word.
Consider rephrasing to make the sentence grammatical, e.g. "Implementation showed that this buys nothing for this worker" or "Implementation showed that it buys nothing for this worker."
```suggestion
> imports* as item 1. Implementation showed that this buys nothing for this worker:
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Review fixes from the multi-agent review pass and Sourcery
(sourcery-ai review 4630116841):
Rendered-output bugs:
- README: render TestRabbitBroker (not the nonexistent TestRabbitmqBroker)
for the rabbitmq broker via the same rabbit mapping used for the
faststream extra.
- Worker unit tests: restore the parenthesized multi-line import so repo
names longer than 11 chars stay under ruff's 88-char limit (the
single-line form broke the generated style gate).
Coverage honesty (replaces blanket exclude_lines with per-site pragmas):
- Drop the 'def main\(', 'async def run_server', and
'except PackageNotFoundError' exclude_lines patterns: a regex matching a
def line excludes the whole function body, which silently un-measured the
tested GUI/TUI entrypoints and the web 503 handlers.
- Add targeted `# pragma: no cover` to the genuinely blocking entrypoints
(web/MCP/worker main, MCP run_server, __main__ dispatchers) and the
worker's module-level metadata fallback (now with a warning log).
- Test the reachable metadata-failure paths instead: web /version + /info
503s (fastapi + litestar), CLI exit-code-1 contract, GUI/TUI
"Version: unknown" degradation, MCP error text — via a
_MissingDistribution monkeypatch stub and importlib module handles (the
cli/web/worker __init__ re-export `app`, shadowing the submodule).
- Cover the GUI/TUI main() success paths (previously masked by the blanket
pattern) with stubbed display helpers.
Worker tests:
- Assert build_broker actually forwards the explicit URL (constructor spy)
and add the env-var fallback test — previously a dropped url parameter
would only surface in the Docker-only CI job.
- Add a malformed-request negative test (ValidationError, no response).
- Drop the tautological test_broker_exists.
- Replace the `_ = handle_version_request` indirection with a targeted
pyright ignore naming the tool (basedpyright reportUnusedFunction).
Integration test hardening:
- Publish inside the poll loop: kafka/redis/nats subscriptions are
at-most-once from a cold consumer, so a single up-front publish was a
built-in race on the only real-driver CI job.
- Bound broker.start() with asyncio.wait_for and each test with
pytest-timeout (--timeout=300) so hangs fail with a traceback instead of
the 15-min CI job kill.
- Give kafka a larger poll window (consumer-group join on cold runners).
- Assert the full response payload, not just the correlation id.
Config cleanups:
- Anchor the integration omit glob to */worker/test_integration.py.
- Drop the dead tests site-packages remap in coverage.paths.
- Drop the no-op rabbitmq conditional in the testcontainers extra.
- Scope the wait_container_is_ready filterwarnings entry to the
testcontainers module (ini filters are regexes). The integration env's
-W demotion stays broad: CLI -W module matching is exact and drivers
attribute warnings via stacklevel to arbitrary caller modules (verified
empirically with the redis driver).
Docs:
- ADR-008: fix the faststream dependency claim (extra, not mandatory),
the Context/Decision contradiction about the unit-test rewrite, the
url-or code sample, and document the pragma-over-blanket rationale and
the -W scoping rejection; grammar fix from Sourcery.
- CLAUDE.md: rewrite the coverage exclusion convention for the per-site
pragma approach.
Verified: unit suites for all four brokers, real-container integration
round-trips for all four brokers, 100% coverage + green style gate on the
all-components (rabbitmq, 20-char name), litestar-web, and base projects.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Extracts the testing patterns from litestar-faststream and litestar-events#9 into the worker component, and fixes the systemic coverage-gate failures the work surfaced. Design of record:
docs/adr/008-worker-broker-testing-strategy.md.Worker testing
build_broker(url=None)factory replaces the module-level broker singleton: registers the subscriber/publisher and resolves the connection at call time, so tests can point a fresh, self-contained broker at a throwaway instance without import-order/caching pitfalls. The module still exposesbroker = build_broker()for the entry point.urlparameter would only surface in the Docker-only CI job) and the malformed-request path (validation error, no response published).tests/worker/test_integration.py, markedintegration, deselected from the default suite) starts a real broker via testcontainers and round-trips a message through the actual driver, asserting the full response payload. Hardened against the real world: the request is (re)published inside the poll loop because kafka/redis/nats subscriptions are at-most-once from a cold consumer (a single up-front publish was a built-in race);broker.start()is bounded byasyncio.wait_forand each test by pytest-timeout, so a wedged container fails with a traceback instead of hanging until the CI job kill; kafka gets a larger poll window for cold-runner consumer-group joins.tox -e integrationenv (Docker required) and a conditional, Ubuntu-onlyworker-integrationCI job that gates thecheckaggregation job. The env demotes DeprecationWarnings broadly (not per-module — CLI-Wmodule matching is exact and drivers attribute warnings viastacklevelto arbitrary caller modules, verified empirically with the redis driver); the default suite stays strict.Systemic coverage fixes (
fail_under = 99)exclude_lines. An earlier iteration excludeddef main(/async def run_server/except PackageNotFoundErrorvia[tool.coverage.report] exclude_lines; that was rejected during review — a regex matching adefline excludes the entire function body, which silently un-measured the tested GUI/TUI entrypoints and the web 503 handlers. Now only the genuinely blocking entrypoints carry# pragma: no cover(web/MCP/workermain(), MCPrun_server, the__main__dispatchers, the worker's module-level metadata fallback — which now logs)./version+/info503 responses (FastAPI and Litestar), the CLI metadata-failure exit-code-1 contract, the GUI/TUI "Version: unknown" degradation, the MCP error-text response, and the GUI/TUImain()success paths (a coverage hole the blanket pattern had been hiding). Patch targets go throughimportlib.import_modulebecause the cli/web/worker__init__re-exportapp, shadowing the submodule on any attribute-based import.omitglobs are filename-anchored (*/_version.py) — the previoussrc/**pattern silently missed tox's site-packages installs and reported_version.pyat 0% for every generated project. The integration omit is additionally directory-anchored (*/worker/test_integration.py) so a futuretest_integration.pyin another component stays measured.[tool.coverage.paths]now remaps*/site-packages/<pkg>back tosrc/<pkg>, so multi-envcoverage combineunifies per-interpreter data instead of counting version-gated lines as missed (base project fell to ~92% across five envs without this).Validation
No linked issue — template maintenance extracted from external-repo analysis (label:
no-issue).