Skip to content

test: assert API error responses through a shared helper - #3558

Merged
freeqaz merged 2 commits into
mainfrom
test-dry-1025-api-errors
Oct 25, 2026
Merged

freeqaz merged 2 commits into
mainfrom
test-dry-1025-api-errors

Conversation

@freeqaz

@freeqaz freeqaz commented Oct 25, 2026

Copy link
Copy Markdown
Collaborator

Problem

Tests check API refusals with the same two lines over and over: assert.equal(x.status, 4xx, ...) and then assert.equal(x.body.error.code, "CODE", ...). A survey of tests/ on main found 181 such pairs in 19 files (96 in occ-api.test.mjs). The failure messages differ by site: some print the body, some print only a label, and some print nothing, so a refusal that answered differently can fail without showing what came back.

Change

  • New tests/helpers/api-error.mjs with assertApiError(response, status, code, label). It asserts response.status and response.body.error.code with the same strict assert.equal the call sites used. Each failure message is the response body as JSON, after label when given.
  • 159 pairs in 15 files now call it. Each site keeps its status, its code and its label (the label the site passed to either assertion; a ${label}: ${JSON.stringify(body)} message becomes label). Sites that printed x.text now print the parsed body.
  • Left alone:

Test-only. No product code, assertion values or test names change.

Verification

Risks

Low. The helper asserts the same two values with the same strict equality; the messages now include the body where some sites had none. A response without body.error still fails with a TypeError, as before.

@freeqaz
freeqaz requested a review from a team as a code owner October 25, 2026 00:44
@clawsweeper

clawsweeper Bot commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Oct 25, 2026
@clawsweeper

clawsweeper Bot commented Oct 25, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge.

What this changes

This PR replaces 159 repeated API error assertion pairs with a shared helper that includes the response body in failure messages.

Example: An API refusal test expects 403 FORBIDDEN but receives 500 INTERNAL_ERROR.

  • Before: Some assertion failures show only the status mismatch or a label.
  • After: The assertion failure includes the JSON response body and any existing label.

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused test refactor with preserved expectations, proportionate reported validation, successful exact-head checks, and no actionable findings.
Proof confidence 🐚 platinum hermit (4/6) This maintainer-authored PR changes only test assertions; reported caller-suite and mutation runs support the extraction, and no shipped product behavior requires separate live proof.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Product

Kind: Test only · Worth it: Yes · Fix scope: Complete
User problem: Contributors diagnosing failing API refusal tests can receive inconsistent messages that omit the response body.
Reason: The maintainer-authored direction improves diagnostics while reducing repetition and preserving test coverage.

Merge readiness

✅ Ready for maintainer review

Keep open: this maintainer-authored PR provides useful test cleanup, preserves the existing expectations, and has no actionable findings.

Priority: P3
Reviewed head: 1e16c04188fc1df09359fcf47d20d82a878cce1b

Before merge

None.

Findings

None.

Agent review details

How this fits together

Integration and conformance tests pass parsed controller responses into the helper, which checks HTTP status and error code and produces assertion diagnostics.

flowchart LR
  A[Test scenario] --> B[Controller API]
  B --> C[Parsed response]
  C --> D[Shared error assertion]
  D --> E[Test result and diagnostics]
Loading

Technical review

Best possible solution:

Retain the small shared assertion helper and its behavior-preserving caller replacements.

Do we have a high-confidence way to reproduce the issue?

No product bug is claimed; the pinned base assertions directly establish the repeated checks and inconsistent diagnostic messages.

Is this the best way to solve the issue?

The helper is a proportionate solution: it preserves strict comparisons, keeps expectations at each call site, and leaves richer assertions in place.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against eef2a181350f.

Provenance checked

  • API status and error-code expectations keeps the original intent (1814d57: Earlier integration-test consolidation retained explicit API refusal checks; this PR preserves their independent expected values.)
  • Console authorization refusals keeps the original intent (feat: add read-only platform console #12: Console API tests enforce exact IAM boundaries and canonical API errors; only their repeated assertion syntax changes.)
  • Runtime-log request bounds keeps the original intent (f2a4902: Recent coverage pins request-schema bounds and error details; the helper retains status/code checks and leaves detail assertions untouched.)

Testing

Proof path: in-process harness.

Security

None.

Evidence

What I checked:

  • Introduced changes preserve assertions: The pinned introduction diff contains only the helper, its imports, and replacements of status/code assertion pairs; expected values and surrounding message, detail, audit, and side-effect assertions remain intact. (tests/helpers/api-error.mjs:8, 1e16c04188fc)
  • Response boundary matches helper: The console fixture obtains real HTTP responses, parses JSON, and returns the status/body shape consumed by the helper; serialization therefore operates on parsed JSON rather than arbitrary application objects. (tests/helpers/console-app.mjs:591, 1e16c04188fc)
  • Current main and related work: Fetched main has no api-error helper, and repository searches found no competing assertion-helper PR; test: open and close test postgresql pools through a shared helper #3560 concerns PostgreSQL pool teardown and does not supersede this change. (tests/helpers/api-error.mjs, eef2a181350f)
  • Validation on reviewed head: GitHub reports 32 successful and seven skipped checks on the reviewed head, including successful CI Required, conformance, and PostgreSQL Platform checks; the body also records caller-suite and mutation runs. Leaf-log retrieval redirected to a blocked Azure storage host, so detailed logs were not inspected. (1e16c04188fc)
  • Applicable policy and discussion: The complete root policy was applied; no nested tests policy, maintainer-notes directory, or pr-simplify skill was present. REST reads confirmed collaborator authorship and only automated status comments, with no substantive review objections. (AGENTS.md, 1e16c04188fc)

Likely related people:

  • Free Wortley: Raw commit f2a4902 adds tests/conformance/occ-api-security.test.mjs:1796 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: f2a4902f68af; files: tests/conformance/occ-api-security.test.mjs)
  • kevinlin-openai: Raw commit 3474bb7 adds tests/integration/console-api.test.mjs:92 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: medium; commits: 3474bb717fff; files: tests/integration/console-api.test.mjs)
  • Russell Bryant: Raw commit 3935a45 adds tests/integration/console-api.test.mjs:65 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: low; commits: 3935a4545342; files: tests/integration/console-api.test.mjs)

Review metrics

Metric Value Why it matters
Change scope Production +0/-0; tests and helpers +188/-320 The 13-line helper removes repeated assertions without adding test cases or production machinery.

Labels

Label changes:

  • add P3: This is low-risk test maintenance that improves failure diagnostics without changing product behavior.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR.

Label justifications:

  • P3: This is low-risk test maintenance that improves failure diagnostics without changing product behavior.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR.

Rating scale

6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior.

Workflow

ClawSweeper edits this one comment on every review. Comment @clawsweeper re-review for a fresh review only. Repair and merge execution lanes are retired.

Reviewed October 24, 2026, 8:58 PM ET / October 25, 2026, 00:58 UTC.

@freeqaz
freeqaz merged commit 5f94137 into main Oct 25, 2026
57 checks passed
@freeqaz
freeqaz deleted the test-dry-1025-api-errors branch October 25, 2026 01:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant