test(seller-agent): add MCP-level tool tests and cover force_account_status - #475
Open
sujanchalla0510 wants to merge 1 commit into
Open
Conversation
…status reference/seller-agent already had strong backend-method-level tests for most of issue adcontextprotocol#140's acceptance list (create_media_buy, pending_creatives -> active, cancel/double-cancel, list_creatives filtering, delivery reporting, and the seed_product/seed_pricing_option/force_create_media_buy_arm custom comply_test_controller scenarios), added incrementally after adcontextprotocol#138 merged. Two real gaps remained: - forceAccountStatus (the force_account_status compliance scenario) had no test at all. - every existing test calls backend methods directly, bypassing the actual adcp.Register/adcp.AddTool/adcp.RegisterTestController wiring main() uses to serve requests — so a regression in tool names or request/response field mapping would only be caught by the npm storyboard runner. Extract newServer(b *backend) *mcp.Server out of main() (pure refactor, no behavior change) so tests can stand up the real registered server over an in-memory MCP transport, and add main_mcp_test.go covering every bullet in the issue through actual tool calls: create_media_buy with/without creative assignments, pending_creatives -> active via sync_creatives and update_media_buy, cancel + double-cancel (asserting the specific NOT_CANCELLABLE code), list_creatives filtering by creative_id and format_id, delivery simulation/reporting via comply_test_controller + get_media_buy_delivery, and the seed_product/force_create_media_buy_arm custom scenarios end to end. Verified the new tests catch real regressions, not just compile: breaking forceAccountStatus's status assignment, the double-cancel guard, and simulateDelivery's spend accumulation each failed the corresponding new test (the double-cancel and delivery breaks also exposed that the existing tests check IsError/spend but not the specific error code the new MCP test asserts), then restored and reconfirmed all tests pass. Closes adcontextprotocol#140.
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
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
Closes #140.
reference/seller-agentalready had strong, honest state-machine coverage for most of the issue's acceptance bullets — added incrementally after #138 merged (see #195, #245, and follow-on work). Before writing anything I readcmd/seller-agent/main_test.goin full to avoid duplicating that work, and cross-checked it line-by-line against every bullet in the issue:TestCreateMediaBuy_WithoutCreatives,TestCreateMediaBuy_WithCreatives)pending_creatives->activeviasync_creativesandupdate_media_buy— already coveredTestCancellation,TestDoubleCancellation)list_creativesfiltering by creative ID and format ID — already coveredcomply_test_controllerscenarios (seed_product,seed_pricing_option,force_create_media_buy_arm) — already coveredTwo real gaps remained, which this PR fixes:
force_account_statushad zero test coverage. It's one of the seller's ownTestControllerStoremethods (b.forceAccountStatus), wired intocomply_test_controller, and nothing exercised it.b.createMediaBuy(...),b.updateMediaBuy(...), etc.), which exercises the state machine but bypasses the actualadcp.Register/adcp.AddTool/adcp.RegisterTestControllerwiringmain()uses to serve requests. A regression in tool names, or in how a request/response gets JSON-mapped onto the wire, would compile fine and pass every existing test — the npm storyboard runner would be the only thing to catch it, which is exactly what the issue says shouldn't be the only safety net.What's added
newServer(b *backend) *mcp.Serverout ofmain()so tests can stand up the exact servermain()runs, over an in-memory MCP transport. Verified withgit diff -bthat this is a pure whitespace/extraction diff — no logic changed.main_mcp_test.go(new): 10 tests that call the real registered tools by name with JSON arguments (the way an MCP client, including the storyboard runner, would), covering every bullet in the issue's acceptance list end-to-end:create_media_buywith/without creative assignments,pending_creatives -> activeviasync_creativesandupdate_media_buy, cancel + double-cancel (asserting the specificNOT_CANCELLABLEcode, not just "is an error"),list_creativesfiltering bycreative_id/format_id, delivery simulation+reporting viacomply_test_controller(simulate_delivery) +get_media_buy_delivery, and theseed_product/force_create_media_buy_armcustom scenarios through the wire.main_test.go: 2 new unit tests closing the one real gap —TestForceAccountStatusandTestForceAccountStatus_NotFound.Verification that these tests catch real regressions, not just compile
Per this project's own bar for proving a test is load-bearing, I broke three separate pieces of real logic, confirmed the new tests failed for the right reason, then restored the fix and reconfirmed everything passes:
acct.Status = statusinforceAccountStatus→TestForceAccountStatusfailed (forceAccountStatus did not persist new status, got active).updateMediaBuy→TestMCP_CancelAndDoubleCancelfailed (want NOT_CANCELLABLE on double-cancel, got "INVALID_TRANSITION") — and notably, the pre-existingTestDoubleCancellationkept passing, because it only assertsIsError, not the specific code. This is a concrete example of the MCP-level tests catching something the existing suite didn't.simulateDelivery'sweightedSpendcall → both the newTestMCP_DeliverySimulationAndReportingand the pre-existingTestDeliveryReporting_SimulateDelivery/TestDeliveryReporting_SimulateDeliveryWeightsSpendByBudgetcorrectly failed.Acceptance criteria
cd reference/seller-agent && go test ./...— 44 tests, all passing, exercising every state transition in the issue (create/no-create, activation via both paths, cancel/double-cancel, filtering, delivery, and all three custom scenarios) without the npm storyboard runner.scripts/ci/run_storyboard_reference_seller.shfrom [codex] Add Go reference seller storyboard harness #138) is untouched and remains the end-to-end compliance check.Validation
All green.
git statusis clean — this PR touches exactlyreference/seller-agent/cmd/seller-agent/{main.go,main_test.go,main_mcp_test.go}.Note on scope
I did not add a general integration test for
get_media_buysaccount-scoping,sync_accounts, or the standardforce_media_buy_status/force_creative_statusscenarios beyond what already existed, since those are outside the issue's explicit bullet list and already have direct or indirect coverage (TestForceMediaBuyStatus*,TestImpairmentsRequireMaterialCreativeImpactAndNewReopenIDexercisingforceCreativeStatus). Happy to extend if maintainers want broader coverage in the same pass.