test: stop stubbing discovery_inquiry_flow, restore real coverage - #63
Merged
Sirajmx merged 1 commit intoSep 9, 2026
Merged
Conversation
…sue IABTechLab#60 part 1) The "pre-existing @listen() bugs with CrewAI version mismatch" stub that every ad_seller.flows-touching test file carried for discovery_inquiry_flow dates to when crewai was pinned at >=0.86.0 (commit 530df34, March 2026). crewai is >=1.14.4 now (resolves to 1.15.2) -- a major version bump, and the flow runs cleanly end to end on it: verified DiscoveryInquiryFlow.kickoff_async() across all four routing branches, both standalone and under pytest, and confirmed the production call path (the sync flow.query() wrapper -- which calls self.kickoff(), not kickoff_async() -- invoked from inside an async FastAPI handler) also works correctly. atc964 confirmed on the issue that the rationale was real at the time and just never got revisited as crewai moved forward. This wasn't just stale test hygiene: DiscoveryInquiryFlow backs a live endpoint (POST /discovery, products.py:201), so the blanket stub meant that endpoint had been running in production with zero real test coverage for as long as the stub existed. jaanijuk caught that on the issue -- grep for "DiscoveryInquiryFlow" in tests/ before this change returns nothing. execution_activation_flow stays stubbed. It has a separate, real bug (a cancel-scope leak in an MCP client cleanup path on ad-server connection failure, landing outside the code's own try/except) that's being tracked and fixed independently as issue IABTechLab#60 part 2 -- it just isn't the @listen() bug the old comment claimed either. Changes: - Removed the discovery_inquiry_flow entry from the _broken_flows stub list in all 33 test files that had it (execution_activation_flow entries are untouched). - Simplified test_deal_flow_e2e.py: its _get_deal_request_flow_class() helper existed solely to bypass flows/__init__.py via manual sys.modules surgery so importing DealRequestFlow wouldn't also trigger discovery_inquiry_flow. With the stub gone that's unnecessary -- replaced with a plain module import. - Fixed the stale docstring in test_linear_tv.py making the same claim. - Added tests/unit/test_discovery_inquiry_flow.py: the flow's own routing logic across all four response types run for real (no stub), plus POST /discovery exercised through the actual FastAPI app with the real flow (not mocked) -- closing the exact coverage gap this issue found. Full suite: 1491 passed, 28 skipped (pre-existing, unrelated), no regressions. ruff check / format clean. Part of IABTechLab#60.
Sirajmx
approved these changes
Sep 9, 2026
Sirajmx
left a comment
Sirajmx
left a comment
Contributor
There was a problem hiding this comment.
Verified: The full suite matches the stated plan exactly - 1,491 passed, 28 skipped, with Ruff clean. There are no src/ changes, so there is no direct production-code behavioral impact.
Also ran end-to-end testing against a live server with no regressions across the route surface.
Looks good to merge.
Sirajmx
pushed a commit
that referenced
this pull request
Sep 9, 2026
…ter/exit (issue #60 part 2) (#66) OpenDirect21Client.connect()/disconnect() used to split streamablehttp_client's and ClientSession's __aenter__/__aexit__ across two separate method calls. That's fine as long as the connection succeeds -- but the moment the attempt itself failed, __aenter__ never returned, so __aexit__ was never called by anything. The transport was abandoned to Python's async-generator GC finalizer, which runs in whatever task the garbage collector happens to be executing in at finalization time -- not necessarily the task that opened the connection. anyio requires a cancel scope to be entered and exited by the same task, so that mismatch crashed with "Attempted to exit cancel scope in a different task than it was entered in", and the crash propagated all the way up through kickoff_async(), taking down the whole ExecutionActivationFlow. Confirmed the root cause in isolation before touching anything: a bare `streamablehttp_client(url).__aenter__()` / manual `__aexit__()` later against an unreachable host reproduces the crash on its own, with nothing else from this codebase involved. A plain, unbroken nested `async with streamablehttp_client(...) as (r, w, _): async with ClientSession(r, w) as session: ...` against the same unreachable host fails cleanly with an ordinary ExceptionGroup -- no crash. The fix has to make connect()/disconnect() behave like that nested block, not like two independent calls. connect() now starts a background task that owns the MCP session's entire lifetime -- opening AND closing streamablehttp_client and ClientSession within one unbroken `async with`, in that one task, from start to finish. The task blocks on an asyncio.Event in between; disconnect() sets the event and awaits the task to let it unwind naturally, in the same task it opened in. This mirrors the pattern deals_api_mcp_client.py already uses for exactly this reason, cited in its own docstring: "Satisfies anyio's cancel-scope invariant by running the full streamablehttp_client lifecycle inside a single background asyncio Task." Verified: the original ExecutionActivationFlow repro from #60 no longer raises anything -- kickoff_async() now completes and the connection failure is recorded as a plain state warning, exactly the graceful-degrade behavior create_execution_order's own (now reachable) try/except was always meant to provide. Verified both execution_type paths (deal_id and io_order). Verified 5 repeated connect/disconnect cycles against an unreachable host in a row. Found along the way, not fixed here (separate bug, out of scope for this crash): with the crash no longer masking it, the resulting warning reads "'OpenDirect21Client' object has no attribute 'create_execution_order'" -- UnifiedClient.create_execution_order() calls a method OpenDirect21Client never actually defines. Worth its own issue. Tests: new tests/unit/test_opendirect21_client.py -- connection failure degrades cleanly with no raise (the regression), REST fallback via _call_tool when MCP never connected, 5 repeated cycles, disconnect without a prior connect is a no-op, and a mocked successful connection still sets up the session/tools and disconnects both context managers correctly (the happy path the rewrite must not break). Full suite: 1484 passed, 28 skipped (pre-existing, unrelated), no regressions. ruff check / format clean. Closes #60 (part 2, alongside #63 for part 1).
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
2 participants
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
Part 1 of #60 (green-lit by @atc964 on the issue).
The
"pre-existing @listen() bugs with CrewAI version mismatch"stub that everyad_seller.flows-touching test file carried fordiscovery_inquiry_flowdates to whencrewaiwas pinned at>=0.86.0(commit530df34, March 2026). It's>=1.14.4now (resolves to1.15.2) — a major version bump. Verified on the issue that the flow runs cleanly end-to-end on the current version, including the exact production call path (flow.query()'s syncself.kickoff()invoked from inside an async FastAPI handler).This wasn't just stale test hygiene —
DiscoveryInquiryFlowbacks a live endpoint (POST /discovery,products.py:201), so the blanket stub meant that endpoint had zero real test coverage for as long as the stub existed (grep -rl "DiscoveryInquiryFlow" tests/returned nothing before this PR).execution_activation_flowstays stubbed — it has a separate, real bug (a cancel-scope leak in an MCP client cleanup path, not the@listen()issue the old comment claimed) that's tracked as #60 part 2 and will be fixed independently.Changes
discovery_inquiry_flowentry from the_broken_flowsstub list in all 33 test files that had it.execution_activation_flowentries are untouched.test_deal_flow_e2e.py: its_get_deal_request_flow_class()helper existed solely to bypassflows/__init__.pyvia manualsys.modulessurgery so importingDealRequestFlowwouldn't also triggerdiscovery_inquiry_flow. With the stub gone that's unnecessary — replaced with a plain module import (net -41 lines in that file alone).test_linear_tv.pymaking the same claim.tests/unit/test_discovery_inquiry_flow.py: the flow's own routing logic across all four response types (catalog/pricing/availability/targeting) run for real, plusPOST /discoveryexercised through the actual FastAPI app with the real flow — not mocked — closing the exact coverage gap the issue found.Test plan
Full suite: 1491 passed, 28 skipped (pre-existing, unrelated to this change), no regressions.
ruff check/format --checkclean.Part of #60.