[chore] Correct three test comments that claimed more than the tests prove - #6687
Conversation
Follow-up to #6667, comment and name only, no behaviour change. The exporter comment described the ordering backwards. The fixture sets up before the one that sets AGENTA_API_URL, so the loopback host is used; an ambient value would still win, which is why this is not exporter isolation. The fixture docstring said it owns the SDK singleton. It restores the ag.tracing alias and nothing else, and init also replaces api, async_api and the tracer and installs a provider. The abandonment test was named for a close that always happens. What it can prove is that the close is entered and that the caller does not wait for it.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@coderabbitai review |
📝 SummarySummary by CodeRabbit
WalkthroughThe pull request clarifies an abandonment-path test name and docstring. It also expands SDK singleton fixture documentation for restoration scope, initialization effects, exporter selection, and failure cleanup. ChangesTest lifecycle clarification
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: 🔵 Low · up to The fixture documentation may mislead maintainers about which API URL the test exporter uses, making future test changes harder to reason about. Clarify the ordering or declare it explicitly before merge. 🚥 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 |
Railway Preview Environment
Updated at 2026-09-08T20:33:12.401Z |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Advanced
Run ID: dc05feba-27d9-40f0-9e76-36fbd9fb2eb6
📒 Files selected for processing (2)
sdks/python/oss/tests/pytest/unit/agents/platform/test_session_context_http.pyservices/oss/tests/pytest/unit/agent/test_session_context_resolution.py
Included review availability: Your plan provides up to 8 included reviews per hour; 0 remain after this review.
|
Two function-scoped fixtures with no dependency have no guaranteed order, so a comment describing one was asserting something pytest does not promise. backend_facts now depends on sdk_singleton, which is the fixture whose environment variable the other reads. The comment no longer claims where the exporter points either. init prefers AGENTA_API_INTERNAL_URL and then AGENTA_API_URL over the host passed to it, and either can be ambient, so ordering alone settles nothing. The claim that does hold is that isolation is not needed here.
1 participant
Context
Three comments and one test name in #6667 claim more than the code does. A Codex review caught them after that PR had been approved, so they landed with it. A CodeRabbit review of this PR then found that my first attempt at one of them was wrong on two further counts. Nothing here changes behaviour, and no assertion moves.
Changes
A comment described an ordering pytest does not guarantee, and the fixtures now enforce it.
sdk_singletonandbackend_factsare both function-scoped with no dependency between them, so their relative order is unspecified. Argument order in the test signature does not decide it. One of them sets an environment variable the other reads, sobackend_factsnow depends onsdk_singletonand the order is a real dependency rather than a described one.That ordering was the smaller half.
initprefersAGENTA_API_INTERNAL_URL, thenAGENTA_API_URL, over thehostpassed to it, and derives the host from whichever is set. Either can already be in the environment, so no amount of fixture ordering settles where the exporter points. The comment no longer names a target. It says the host is a preference that either variable overrides, and keeps the claim that does hold: isolation is not needed here, because nothing reads a span and the exporter flushes off the request path, so a failed export cannot change a result.The fixture docstring claimed to own the SDK singleton. It restores the
ag.tracingalias and nothing else.initalso replaces the singleton'sapi,async_api, andtracer, and installs a provider and exporter, and those stay. The docstring now states that narrower guarantee, and notes that pytest unwinds the fixture after a failing test but not after a failure raised before the yield.A test was named for a close that always happens.
test_the_client_is_always_closed_on_the_abandonment_pathproves the context manager's exit is entered and that the caller does not wait for it. It cannot prove the close completes, and nothing can: ifaclosehangs there is no way past it. Renamed totest_the_clients_close_is_entered_on_the_abandonment_path, with the limit stated.Tests
No test logic changed. Services unit 167 passed; the SDK session-context suite 48 passed.
ruff formatandcheckclean at the CI-pinned 0.15.12.Notes
Timing on the merge, not a judgment call: these were flagged in a review round that finished after #6667 was approved, and the release manager chose not to spend another CI cycle on comment-only edits. This PR can ride the next batch or move to v0.115.4.
One thing worth carrying forward. Both wrong versions of the exporter comment came from running
pytest --setup-show, seeing an order, and writing it down as though it were guaranteed. Observing an order is not the same as pytest promising one, which is why the fix here is a dependency rather than better wording.