Skip to content

test(mcp): isolate the invalid-action trace test from on-disk state - #87

Closed
basil-k-aji-dev wants to merge 1 commit into
google:mainfrom
basil-k-aji-dev:test/isolate-inspect-trace-invalid-action
Closed

basil-k-aji-dev wants to merge 1 commit into
google:mainfrom
basil-k-aji-dev:test/isolate-inspect-trace-invalid-action

Conversation

@basil-k-aji-dev

Copy link
Copy Markdown

Closes #62.

Problem

test_mobile_inspect_trace_invalid_action does not request the temp_trace_env fixture the rest of the module uses, so it runs against whatever trace_store.TRACES_DIR resolves to. mobile_inspect_trace looks for data_engine.db there, falls back to <project_root>/traces/data_engine.db, and returns "Database not found" before it validates action at all — so the unsupported-action branch the test names is never reached either way.

The result therefore reports leftover disk state, not the logic under test.

Reproduction on this host

The project-root database exists only because running the suite creates it, which is exactly what makes this self-concealing — it passes on the second run and fails on a clean checkout:

$ ls traces/data_engine.db
traces/data_engine.db
$ uv run pytest -q tests/unit/mcp/test_mcp_tools.py::test_mobile_inspect_trace_invalid_action
1 passed

$ mv traces /tmp/traces_bak          # simulate a clean checkout
$ uv run pytest -q tests/unit/mcp/test_mcp_tools.py::test_mobile_inspect_trace_invalid_action
AssertionError: assert 'not supported' in 'Data engine database does not exist at .../traces/data_engine.db yet.'
1 failed

I hit this accidentally mid-session: a full-suite run overlapped with the window in which I had moved traces/ aside, and this single test flipped to failing in that run and back to passing afterwards, with no code change in between.

Change

Give the test the same isolated trace directory the other tests in the module use, and seed an empty data_engine.db in it. An unknown action never reads the database — the only gate before the final else is os.path.exists(db_path) — so an empty file is enough to get past it and into the branch under test. Seeding the temp database also stops the <project_root>/traces fallback from being consulted at all, which is what made temp_trace_env alone insufficient here.

Also assert that the offending action name is echoed back, since that is the part of the message a caller acts on.

Scope note

I deliberately did not move action validation ahead of the database check in mcp_server/tools/inspect_trace.py. It would arguably be a better error for an unknown action with no database present, but it changes the tool's response for a real caller and is a separate decision from making this test honest. Happy to follow up if you'd prefer that.

Verification

The decisive property is that the outcome no longer depends on the project-root database:

traces/data_engine.db present absent
this branch passed passed
main passed failed
Command Result
uv run pytest -q tests/unit/mcp/test_mcp_tools.py 25 passed
uv run pytest -q (full deterministic suite) 2096 passed, same 89 pre-existing failures as main — 0 new
uv run ruff check / ruff format --check on the touched file clean

make typecheck could not be run in my environment — the pyright wheel downloads a Node runtime on first use and my sandbox blocks it. The touched file is not in pyright-core.json.

`test_mobile_inspect_trace_invalid_action` did not request `temp_trace_env`, so
it ran against whatever `trace_store.TRACES_DIR` resolves to, defaulting to
`<project_root>/traces`. `mobile_inspect_trace` checks for `data_engine.db`
there -- and falls back to `<project_root>/traces/data_engine.db` -- and returns
"Database not found" before it validates `action` at all.

The result therefore tracked leftover disk state rather than the branch the
test names: it passed on a machine with a database from an earlier run and
failed on a clean checkout, and in neither case reached the unsupported-action
branch it asserts on.

Give it the same isolated trace directory the other tests in the module use and
seed an empty database there. An unknown action never reads the database, so an
empty file is enough to get past the gate and into the branch under test. Also
assert that the offending action name is echoed back, which is the part of the
message a caller acts on.

The test now passes both with and without a database under the project root,
and fails on main without one.

Closes google#62.
@RohanOnKeys

Copy link
Copy Markdown

close this pr, a solution is already on way (#66 .)
please read thru the codebase before you commit anything

@basil-k-aji-dev

Copy link
Copy Markdown
Author

Closing as a duplicate. #66 by @RohanOnKeys already covers issue #62 and was opened first — that one should get the review, not this.

My mistake: I didn't check the open PR queue for an existing claim before sending this. Sorry for the noise.

giveUphope added a commit to giveUphope/autoGamer that referenced this pull request Oct 14, 2026
…p opened as G92

The ledger records the reversal (google#87 deferred OCR, the user ruled it built-in),
the numbers measured while landing it, and the two places where the plan was
wrong rather than merely incomplete: the confidence floor is judged on the words
a match actually covers, not on the line or the whole frame the plan quoted, and
the packaging spec does load the real model, because "no install, no network, no
leftover files" cannot be proved through an injected fake.

G92 is opened instead of papered over: on a live desktop capture the packaged
model reads only 2 of 24 text-bearing nodes, and the failures are unread glyphs,
not the threshold refusing them. Candidate fixes are listed, none chosen.
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.

test_mobile_inspect_trace_invalid_action passes or fails based on unrelated on-disk state

2 participants