Skip to content

Say why the unconfigured-node assertion cannot pass vacuously - #341

Merged
Babissimo merged 1 commit into
mainfrom
fix/e2e-review-nits
Sep 9, 2026
Merged

Babissimo merged 1 commit into
mainfrom
fix/e2e-review-nits

Conversation

@Babissimo

Copy link
Copy Markdown
Contributor

The three minor notes the review carried over from #338. All were non-blocking, and production was a release behind at the time, so they were left rather than holding that deploy.

The one that matters

not.toHaveProperty("detection_area") was not gated on the response status. An absence assertion holds against an error body as readily as against a correct one, so a failed request would have taken the test green for the wrong reason — the exact shape of vacuous pass that is worth guarding explicitly.

The metrics and trust assertions beside it did already catch that case, which is why the reviewer rated it clarity rather than a gap. But they caught it as a side effect rather than by intent, and a future edit could remove them without anyone noticing what else they were holding up. The status check now says it outright.

The two smaller ones

Symmetry. The placed node's status test now also asserts the body is a non-null object, matching its REAL_NODE_ID sibling. The two are a deliberate pair; they read better making the same claims.

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

Verification

No behaviour change: three assertions added, one word removed. The Playwright job does not run on pull requests, only on deploy, so CI here cannot exercise this file — the same limitation noted on #338. The assertions themselves were checked against staging while preparing that PR: an unconfigured node returns metrics, trust, reputation and coverage_map with no detection_area, and the endpoint answers 200 for both nodes in the pair.

Follows #338. Ticket: 86cbauhxd (closed).

🤖 Generated with Claude Code

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>
@claude

claude Bot commented Sep 9, 2026

Copy link
Copy Markdown

Claude finished @Babissimo's task in 1m 15s —— View job


Review complete

  • Gather context (PR diff, related file)
  • Run code review
  • Post findings

Findings

None. This is a tiny, self-contained e2e-test hardening change (8/-1 lines, no production code) and it does what the description says:

  • expect(analyticsStatus).toBe(200) (frontend/e2e/nodes.spec.ts:~638) now gates the not.toHaveProperty("detection_area") absence check, so a failed request can no longer make the assertion pass vacuously — the metrics/trust checks right below it already caught that case as a side effect, but this makes the intent explicit.
  • expect(typeof placedBody).toBe("object") / expect(placedBody).not.toBeNull() added to the placed-node test mirror the existing pair used on its REAL_NODE_ID sibling — correct pairing (typeof null === "object" in JS, hence the separate not.toBeNull()), consistent with the pattern already in use nearby.
  • "legacy" dropped from the test title — string-only change, no assertions affected.

No correctness issues, no removed behavior, nothing touching /v1/nodes contracts or other CLAUDE.md-gated concerns (this is Playwright e2e only, which per the PR description doesn't run in CI on pull requests). Diff matches the PR description precisely.

@Babissimo
Babissimo merged commit 1785d32 into main Sep 9, 2026
15 checks passed
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