test: fix CLI test typing errors - #15
Conversation
📝 WalkthroughWalkthroughCLI tests now use a shared ChangesCLI test migration
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #15 +/- ##
=======================================
Coverage 99.26% 99.26%
=======================================
Files 34 34
Lines 4079 4081 +2
Branches 241 241
=======================================
+ Hits 4049 4051 +2
Misses 19 19
Partials 11 11 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR updates the CLI/provider test scaffolding to satisfy stricter typing (Settings dataclass instances and concrete httpx.AsyncClient), and exposes the OpenAI-compatible provider base URL so tests no longer need to inspect private state.
Changes:
- Expose
base_urlonOpenAICompatibleChatCompletionsProviderand use it when constructing the request URL. - Narrow the CLI LLM provider factory typing and adjust imports accordingly.
- Refactor CLI tests to build real
Settingsinstances viadataclasses.replaceand introduce anhttpx.AsyncClient-typed test double.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| weather_briefing/llm.py | Makes provider base URL accessible and uses it when constructing the chat completions endpoint. |
| weather_briefing/cli.py | Updates the LLM provider factory typing/imports in support of the provider/test changes. |
| tests/test_cli.py | Replaces structural fakes with real Settings instances and a typed httpx.AsyncClient test double; updates assertions to use base_url. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Expose the endpoint on the OpenAI-compatible adapter while keeping the provider factory typed to the general LLMProvider protocol. Adapter-focused tests narrow the returned value before inspecting the endpoint.
Use a real httpx.AsyncClient in provider factory tests and close it during fixture teardown. This satisfies the factory annotations without relying on casts or leaking client resources.
Replace the SimpleNamespace-based _make_fake_settings with a module-level _DEFAULT_SETTINGS constant and dataclasses.replace(). Parameters are explicitly typed; locations uses tuple[LocationSpec, ...] instead of Any. Three integration tests refactored to put LocationSpec in settings.locations and return pre-made ResolvedLocation from the resolver.
IceCodeNew
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_cli.py (1)
381-387: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
async_clientteardown relies on httpx's internal close-state guard rather than an idiomatic async fixture.The
async with httpx.AsyncClient(...) as client:block insiderun()(seeweather_briefing/cli.py) will transition the shared fixture client to its closed state during the test itself whenhttpx.AsyncClientis monkeypatched to returnasync_client. The teardown'sasyncio.run(client.aclose())then runs a second, effectively no-op close (httpx guards against re-closing) in a brand-new event loop. This works today, but it depends on an implementation detail ofhttpx.AsyncClient(idempotentaclose()/ "cannot reopen once closed" state machine) rather than a documented public contract, and it doesn't match idiomatic pytest-asyncio fixture patterns already used elsewhere in this suite (the file already has nativeasync def test_...functions).Since the project clearly already supports async tests, a plain async generator fixture would be more idiomatic and avoid coupling to httpx internals:
♻️ Suggested idiomatic async fixture
-@pytest.fixture -def async_client() -> Iterator[httpx.AsyncClient]: - client = httpx.AsyncClient() - try: - yield client - finally: - asyncio.run(client.aclose()) +@pytest.fixture +async def async_client() -> AsyncIterator[httpx.AsyncClient]: + async with httpx.AsyncClient() as client: + yield clientPlease confirm the pytest-asyncio mode (
asyncio_mode) configured for this project (e.g., inpyproject.toml/pytest.ini/conftest.py) supports plain@pytest.fixtureasync generators, since that determines whether this simplification is a drop-in change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_cli.py` around lines 381 - 387, Update the async_client fixture to use an async generator with async def and await client.aclose() in teardown, matching the suite’s existing native async tests. Before applying this change, verify the project’s pytest-asyncio asyncio_mode configuration supports plain `@pytest.fixture` async generators; preserve the fixture’s shared httpx.AsyncClient behavior used by the CLI tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/test_cli.py`:
- Around line 381-387: Update the async_client fixture to use an async generator
with async def and await client.aclose() in teardown, matching the suite’s
existing native async tests. Before applying this change, verify the project’s
pytest-asyncio asyncio_mode configuration supports plain `@pytest.fixture` async
generators; preserve the fixture’s shared httpx.AsyncClient behavior used by the
CLI tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 90cf7eb7-a8a0-41ce-9a9f-a19ae388e466
📒 Files selected for processing (2)
tests/test_cli.pyweather_briefing/llm.py
Use an AnyIO async fixture for provider factory tests and let CLI run tests construct their own clients. Each client is now closed exactly once by the context that owns it.
|
Addressed the async client lifecycle review in 12e63fe. Provider factory tests now use an AnyIO async fixture, while CLI run tests let run() construct and close its own client, so no client is closed twice. Validation: 57 focused tests passed with ResourceWarning treated as an error, all 312 tests passed with branch coverage, and ty, Ruff, and prek passed. |
Summary
Why
The CLI tests passed structurally similar objects where provider factories require concrete Settings and httpx.AsyncClient values. Adapter tests also inspected a private field on a value returned as the general LLMProvider protocol. These changes make the tests satisfy the real boundaries without casts or broader production typing, and ensure each test client is closed exactly once by its owning async context.
Validation