Skip to content

test(mcp): drop two tests that others already cover - #323

Merged
gustmrg merged 1 commit into
mainfrom
test/drop-duplicated-mcp-tool-tests
Oct 31, 2026
Merged

gustmrg merged 1 commit into
mainfrom
test/drop-duplicated-mcp-tool-tests

Conversation

@gustmrg

@gustmrg gustmrg commented Oct 31, 2026 •

Copy link
Copy Markdown
Owner

An audit of the MCP and tool-policy tests listed every test next to the behavior it protects. Two tests protected nothing another test does not, so they are removed. A first version of this PR removed two more, but breaking the code they cover showed each was the only test catching a real bug, so they stay. Nothing else changes.

  • tests/mcp-bridge.test.ts: removed "returns needs_sign_in for an unauthorized oauth server without opening a browser". Its fixture (provider: {}) cannot open a browser, so its title's promise is only asserted by "never opens a browser for background OAuth connections".
  • tests/tool-authorization-broker.test.ts: removed "stores a blocked MCP tool call as an integration rule". Giving integration_call blocks the wrong scope in blockCategory still fails two kept tests.
  • tests/mcp-service.test.ts: renamed "discards sign-in results when the connection changed during the flow" to "says the connection changed when that interrupts its sign-in before it reaches the server". It does not reach the commit re-check: it is the only test of signInFailure naming the change instead of reporting a bare cancellation.
  • tests/mcp-service.test.ts: kept "discards credentials exchanged for a connection that was removed before they were committed". It is the only test of the not_found branch at commit, and of a removed connection's credentials being deleted when remove() fails to do it.

Testing

  • npm run lint: clean (two pre-existing warnings in src/components/storage-*.tsx, untouched).
  • npm run format:check, npm run typecheck (all four tsconfigs), and npm run build: clean.
  • npm test on Node 22: 2437 passed. The earlier note about 19 failures from this machine's SSH keys was wrong: they appear only on Node 26, and the three SSH test files pass on Node 22.
  • Each removal was checked by breaking the code it covers and running the tests left; each kept test above was checked the same way.
  • Built app: not checked; the change touches no production code.

Not in this PR

Two gaps this audit found are covered in a separate PR, test(mcp): cover revoking access and deleting secrets for a connection.

An audit of the MCP and tool-policy tests found two tests that protect
nothing another test does not, each checked by breaking the code they
cover:

- McpConnection's "returns needs_sign_in for an unauthorized oauth
  server without opening a browser" had no browser in its fixture, so
  its title's promise is only asserted by "never opens a browser for
  background OAuth connections".
- ToolAuthorizationBroker's "stores a blocked MCP tool call as an
  integration rule" fails only for bugs that "remembers an
  always-allowed MCP tool through its integration and never over a
  Block rule" and the risk mode test also catch.

"discards sign-in results when the connection changed during the flow"
looked like a third, but it is the only test of the error shown when a
change interrupts a sign-in before it reaches the server. It stays,
with a title that says so.
@gustmrg
gustmrg force-pushed the test/drop-duplicated-mcp-tool-tests branch from 43cba5d to fcc491c Compare October 31, 2026 19:21
@gustmrg gustmrg changed the title test(mcp): drop tests that duplicate a stricter one test(mcp): drop two tests that others already cover Oct 31, 2026
@gustmrg
gustmrg merged commit 7895bbf into main Oct 31, 2026
1 check passed
@gustmrg
gustmrg deleted the test/drop-duplicated-mcp-tool-tests branch October 31, 2026 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant