Skip to content

Assert the detection area on a node that has one - #338

Merged
Babissimo merged 1 commit into
mainfrom
fix/e2e-detection-area-placed-node
Sep 9, 2026
Merged

Babissimo merged 1 commit into
mainfrom
fix/e2e-detection-area-placed-node

Conversation

@Babissimo

Copy link
Copy Markdown
Contributor

Four detection_area assertions in the per-node analytics block failed against staging after #278 merged, gating the production deploy. The code is correct; the tests encoded the old behaviour.

Why they failed

REAL_NODE_ID registers through POST /api/radar/detections with no config at all. That route stores it deliberately unpositioned:

# unplaced until it configures itself over TCP or the v1 API.
legacy_config = canonical_config({"node_id": node_id})

Since contract 1.1.3 an unpositioned node is given no detection area, and withholding it is precisely what keeps such a node off the map. Before #278 the same node had its coordinates coerced to (0, 0) and was handed a detection area anchored on Null Island, which is the behaviour these assertions were written against.

96 tests passed; the 4 that failed all read analyticsBody.detection_area, which is now undefined.

What this changes

The four assertions move to BULK_B_NODE_ID, which registers through the bulk route with a full geo config and therefore has a footprint to describe. The block now reads two nodes, and that pair is the point: one placed, one not.

The absence on the unpositioned node is asserted directly rather than left implicit, together with the metrics and trust that must still be present — counted but unplaced is the state the feature exists to allow, so it is worth pinning rather than inferring.

n_detections reads 0 here rather than 1, because BULK_B registers with no frames. The assertion was already a non-negative check; only its comment needed correcting.

Verification

tsc --noEmit covers src only, and the Playwright job does not run on pull requests, so both halves were checked against staging, which already carries this code:

unconfigured node (e2e-auth-probe-check) returns metrics, trust, reputation, coverage_map, no detection_area
positioned node (radar3-retnode) returns detection_area with all ten keys the test requires, rx carrying lat/lon, n_detections numeric

Both match what the revised tests assert.

Follows #278. Ticket: 86cbauhxd.

🤖 Generated with Claude Code

@claude

This comment has been minimized.

@Babissimo
Babissimo force-pushed the fix/e2e-detection-area-placed-node branch from ea9a198 to 2d59685 Compare September 9, 2026 09:47
@claude

This comment has been minimized.

@Babissimo
Babissimo force-pushed the fix/e2e-detection-area-placed-node branch from 2d59685 to 8d140c6 Compare September 9, 2026 10:06
@claude

This comment has been minimized.

The per-node analytics block read REAL_NODE_ID for everything, including its
four detection_area assertions. That node registers through
POST /api/radar/detections carrying no config at all, which the route
deliberately stores as an unpositioned canonical_config, and since 1.1.3 an
unpositioned node is given no detection area: withholding it is what keeps
such a node off the map. The four assertions therefore read undefined and
failed against staging, gating the production deploy.

They move to BULK_B_NODE_ID, which registers through the bulk route with a
full geo config and so has a footprint to describe. The block now reads two
nodes, which is the point: the pair is the contract. The absence on the
unpositioned one is asserted directly rather than left implicit, alongside
the metrics and trust that must still be there, since counted-but-unplaced
is the state this feature exists to allow.

n_detections is 0 here rather than 1, because BULK_B is registered with no
frames; the assertion was already a non-negative check, only its comment
needed correcting.

Verified against staging, which carries this code: an unconfigured node
returns metrics, trust, reputation and coverage_map with no detection_area,
and a positioned node returns the block with all ten keys the test requires.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@Babissimo
Babissimo force-pushed the fix/e2e-detection-area-placed-node branch from 8d140c6 to d5f155f Compare September 9, 2026 10:12
@claude

claude Bot commented Sep 9, 2026

Copy link
Copy Markdown

Claude finished @Babissimo's task in 2m 17s —— View job


Reviewing PR #338 (new commits pushed)

  • Gather context (diff since last review, CLAUDE.md)
  • Check whether prior findings (1-7, plus the stale bulk-response-contract finding) were addressed
  • Review new/changed code
  • Post findings

What changed since the last review

A dedicated PLACED_NODE_ID fixture (bulk POST, PLACED_CONFIG, one frame) now backs the four detection_area assertions, registered in its own bulk call (nodes.spec.ts:317-332) rather than being folded into the BULK_A/BULK_B call.

This resolves the finding from the last review:

  • Bulk-response-contract test is no longer stale. Because PLACED_NODE_ID gets a separate POST /api/radar/detections/bulk call, the original call at nodes.spec.ts:301-311 still registers exactly two nodes (BULK_A, BULK_B) with empty frame lists. response: {status:'ok', nodes_registered:2, frames_queued:0} (nodes.spec.ts:358-361) is verified correct against the diff as it now stands.
  • n_detections/furthest_detections coverage — resolved. PLACED_NODE_ID carries a real frame, so the populated (1) case is reachable again, not just the empty case.
  • Registration order is correct: PLACED_NODE_ID's bulk POST is awaited inside the outer beforeAll before waitForNodes runs, and the per-node analytics beforeAll (a nested describe) only runs after that outer beforeAll resolves — no race between registration and the GET /api/radar/analytics/{PLACED_NODE_ID} read.
  • Confirmed the other count-consistency assertions in the file (total === Object.keys(nodes).length, the re-POST dedup check, Object.keys(caps).toHaveLength(0)) are all relative or scoped to a specific node_id, so the extra registered node doesn't disturb them.

Remaining minor items (carried over, non-blocking)

These were flagged in the previous review round and are unchanged by this commit:

  1. No body-shape assertion on placedBody (nodes.spec.ts:572-578) — the new "the placed node's analytics also returns HTTP 200" test checks only placedStatus, unlike its REAL_NODE_ID sibling (nodes.spec.ts:519-524), which also asserts typeof analyticsBody === "object" and not.toBeNull(). Low value to add, but would make the two paired tests symmetric.
  2. not.toHaveProperty('detection_area') isn't explicitly gated on analyticsStatus === 200 (nodes.spec.ts:620-630) — in practice a non-200 response would still fail the adjacent metrics/trust assertions in the same test, so this is more a clarity nit than a real gap.
  3. "legacy" wording (nodes.spec.ts:620) — REAL_NODE_ID is "unconfigured/unpositioned," not legacy in any versioned sense; the test title could drop that word to avoid implying a deprecated code path.

Verification limits

frontend/node_modules isn't installed in this environment, so I couldn't run tsc or the Playwright suite directly — consistent with the PR description's note that tsc --noEmit doesn't cover src here and the Playwright job doesn't run on PRs. I traced the fixture/registration/assertion wiring by hand instead (diff against origin/main, full read of the affected describe block) and it's internally consistent.

CLAUDE.md compliance

No violations: only frontend/e2e/nodes.spec.ts changed, no /v1/nodes route/model touched (no OpenAPI regen needed), no hosts/credentials/addresses introduced, no .env changes.

Summary: this revision resolves all four previously-blocking findings, including the newly-introduced bulk-response-contract regression. What remains are three low-severity wording/symmetry nits already on record — none block merge.

@Babissimo
Babissimo merged commit 8a5fe98 into main Sep 9, 2026
15 checks passed
Babissimo added a commit that referenced this pull request Sep 9, 2026
Three review notes carried over from #338, none of which justified holding
that deploy.

The placed node's status test now also asserts the body's shape, matching
its REAL_NODE_ID sibling: the two are a deliberate pair and read better
making the same claims.

The absence assertion is gated on the status. `not.toHaveProperty` holds
against an error body as readily as against a correct one, so on a failed
request the test would have gone green for the wrong reason. The metrics and
trust assertions beside it already caught that, but by accident rather than
by saying so, and an absence assertion is exactly the shape that earns an
explicit guard.

"legacy" leaves the title. POST /api/radar/detections is the current route
for a node that registers without a config, not a deprecated one, and the
word implied a dead code path a reader would go looking for and not find.

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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