test(telemetry): assert the listing attribute, in both directions #541

Merged

coilyco-ops merged 1 commit from qa/pin-the-listing-attribute into main

2026-08-13 15:33:52 +00:00

coilyco-ops commented

2026-08-13 15:32:15 +00:00

Member

bd31e36 replaced a duration heuristic with a stated field, which is the right call and closes #520 properly. The three tests that shipped with it all exercise needsTools — the expiry predicate, which predates the change.

The new code is the listed counter and the SetAttributes call, and nothing asserted either. An attribute whose whole purpose is to be read by a human is exactly the kind that can be silently renamed, inverted, or dropped without a test noticing.

What this asserts

One provider, opened twice against a live server:

	expected
first open — a real listing	`cached=false`, `listed=1`
second open, inside the refresh interval	`cached=true`, `listed=0`

One provider rather than two, deliberately. The attribute is only worth anything if the two cases it separates actually report differently, and two separate providers would let both directions pass while the field said the same thing twice.

Mutation checked three ways

Each fails with the offending attribute map printed, so the failure names what went wrong rather than that something did:

rename the key to mcp.tools.cachedX
  a first listing was not reported as one: map[mcp.tools.cachedX:false mcp.tools.listed:1]
  a cache hit was not reported as one:     map[mcp.tools.cachedX:true  mcp.tools.listed:0]

invert the boolean (listed != 0)
  a first listing was not reported as one: map[mcp.tools.cached:true  mcp.tools.listed:1]
  a cache hit was not reported as one:     map[mcp.tools.cached:false mcp.tools.listed:0]

always increment the counter
  a cache hit was not reported as one:     map[mcp.tools.cached:false mcp.tools.listed:1]

The third is the useful one — it passes the listing direction and fails only the hit, which is the asymmetry a single-direction test would have missed.

What this deliberately does not cover

The unreachable-server path reports cached=true for a connect that did reach the network. I found that while writing this and filed it as #540 with a runnable reproduction and a one-branch fix.

It is not in this PR on purpose. Asserting the correct behaviour would redden main, and asserting the current behaviour would pin something I am arguing is wrong. The test belongs with the fix, and 540 carries it ready to paste.

Test-only. No production change — git diff --stat against origin/main touches one new file.

Refs #520

`bd31e36` replaced a duration heuristic with a stated field, which is the right call and closes https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/issues/520 properly. The three tests that shipped with it all exercise `needsTools` — the expiry predicate, which predates the change. **The new code is the `listed` counter and the `SetAttributes` call, and nothing asserted either.** An attribute whose whole purpose is to be read by a human is exactly the kind that can be silently renamed, inverted, or dropped without a test noticing. ## What this asserts One provider, opened twice against a live server: | | expected | |---|---| | first open — a real listing | `cached=false`, `listed=1` | | second open, inside the refresh interval | `cached=true`, `listed=0` | One provider rather than two, deliberately. The attribute is only worth anything if the two cases it separates actually *report differently*, and two separate providers would let both directions pass while the field said the same thing twice. ## Mutation checked three ways Each fails with the offending attribute map printed, so the failure names what went wrong rather than that something did: ``` rename the key to mcp.tools.cachedX a first listing was not reported as one: map[mcp.tools.cachedX:false mcp.tools.listed:1] a cache hit was not reported as one: map[mcp.tools.cachedX:true mcp.tools.listed:0] invert the boolean (listed != 0) a first listing was not reported as one: map[mcp.tools.cached:true mcp.tools.listed:1] a cache hit was not reported as one: map[mcp.tools.cached:false mcp.tools.listed:0] always increment the counter a cache hit was not reported as one: map[mcp.tools.cached:false mcp.tools.listed:1] ``` The third is the useful one — it passes the listing direction and fails only the hit, which is the asymmetry a single-direction test would have missed. ## What this deliberately does not cover **The unreachable-server path reports `cached=true` for a connect that did reach the network.** I found that while writing this and filed it as https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/issues/540 with a runnable reproduction and a one-branch fix. It is not in this PR on purpose. Asserting the correct behaviour would redden `main`, and asserting the current behaviour would pin something I am arguing is wrong. The test belongs with the fix, and 540 carries it ready to paste. Test-only. No production change — `git diff --stat` against `origin/main` touches one new file. Refs https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/issues/520

coilyco-ops added 1 commit

2026-08-13 15:32:15 +00:00

test(telemetry): assert the listing attribute, in both directions

ci / image-build (pull_request) Successful in 18s

Details

ci / test (pull_request) Successful in 33s

Details

ci / publish-echo-image (pull_request) Has been skipped

Details

ci / publish-observed (pull_request) Has been skipped

Details

d8db65d0f2

bd31e36 replaced a duration heuristic with a stated field, and the three tests
that shipped with it exercise needsTools, which is the expiry predicate and
predates the change. The new code is the listed counter and the SetAttributes
call, and nothing asserted either.

This opens one provider twice against a live server. The first is a real
listing and must report cached=false, listed=1. The second is inside the
refresh interval and must report cached=true, listed=0. One provider rather
than two, because the attribute is only worth anything if the two cases it
separates actually report differently.

Mutation checked three ways, each caught with the offending map printed:

  rename the attribute key      both directions fail
  invert the boolean            both directions fail
  always increment the counter  the cache hit fails, the listing passes

This deliberately does not cover the unreachable-server path, which reports
cached=true for a connect that did reach the network. That is a product defect
rather than a missing test and is filed as sirens-echo#540 with a reproduction.
Asserting it here would either redden main or pin behaviour I am arguing is
wrong.

Refs sirens-echo#520

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Kai Siren <coilysiren@gmail.com>
Co-authored-by: Claude <noreply@anthropic.com>

coilyco-ops requested review from coilysiren

2026-08-13 15:32:15 +00:00

coilyco-ops referenced this pull request

2026-08-13 15:32:31 +00:00

The mcp.tools.list span counts lookups, so a cache hit and a network round trip are indistinguishable #520

coilyco-ops merged commit 0a91346c33 into main

2026-08-13 15:33:52 +00:00

coilyco-ops referenced this pull request from a commit

2026-08-13 15:33:53 +00:00

Merge pull request 'test(telemetry): assert the listing attribute, in both directions' (#541) from qa/pin-the-listing-attribute into main

coilyco-ops referenced this pull request

2026-08-13 16:26:14 +00:00