Skip to content

test(integration): add scale correctness suite (#1192) - #1229

Merged
kriszyp merged 2 commits into
mainfrom
kris/integration-test-scale
Jun 25, 2026
Merged

test(integration): add scale correctness suite (#1192)#1229
kriszyp merged 2 commits into
mainfrom
kris/integration-test-scale

Conversation

@kriszyp

@kriszyp kriszyp commented Jun 10, 2026

Copy link
Copy Markdown
Member

Summary

  • Adds integrationTests/server/scale.test.ts with five correctness tests at scale (up to 100 K records), exercising large indexed tables on every PR run — the gap identified in Promote scale tests to PR CI: 100K–1M record subset + 1.44M multi-index table correctness #1192.
  • Adds integrationTests/fixtures/scale-test/ fixture with Product (category indexed) and Attribute (productId + key indexed) tables.
  • Uses setupHarperWithFixture + sendOperation / SQL COUNT patterns consistent with existing server tests; REST pagination uses Harper's limit(N) query syntax.

Tests

  1. 100K insert: paginated read + indexed search — SQL count, two non-overlapping REST pages, exact category-A count (20 K), and search_by_conditions result purity.
  2. 10K correctness — total-count growth and pagination non-overlap. (Full 1M tracked in nightly YCSB.)
  3. Multi-condition indexed search — 5 K records; SQL count + search_by_conditions verify category='A' filter purity.
  4. Bulk delete correctness — 1 K insert then bulk-delete; point-check + total-count delta confirm record-level correctness.
  5. EAV at scale — 1 K entity rows + 5 K attribute rows; search_by_conditions on indexed productId returns exactly 5 per entity.

Notes

  • value is a reserved field name in Harper's condition schema; the Attribute.attrValue rename avoids the clash.
  • REST pagination: ?limit=N&offset=M is treated as search conditions; correct syntax is ?limit(N,end).
  • All tests pass locally (5/5); CI gates before marking ready.

Test plan

  • CI integration-test job runs integrationTests/server/scale.test.ts and passes (5/5).
  • No regressions in adjacent server tests.

Closes #1192

Generated with Claude Code

Five tests verify record-level correctness at scale (up to 100K records) against a
fixture-deployed Product/Attribute schema: 100K paginated read + indexed search,
10K count + pagination non-overlap, multi-condition indexed search, bulk
insert→delete correctness, and 1K EAV entity→attribute join via indexed productId.
Uses Harper's `limit(N)` query syntax for REST pagination; full 1M / YCSB coverage
remains in nightly CI.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
@kriszyp
kriszyp requested review from Ethan-Arrowood and heskew June 10, 2026 15:23

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces scale correctness integration tests, including a configuration file, a GraphQL schema, and a test suite (scale.test.ts) to verify record-level correctness at scale (up to 100K records). The review feedback correctly identifies several instances where JSON responses from external API calls are parsed and cast directly without verifying if they are arrays. It is recommended to validate these responses using Array.isArray before iterating or mapping over them to prevent potential runtime errors.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread integrationTests/server/scale.test.ts Outdated
headers: { Authorization: auth },
});
strictEqual(pageResp.status, 200, `REST page GET failed: ${pageResp.status}`);
const page = (await pageResp.json()) as unknown[];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The response from the external API is parsed and mapped directly. Under the repository guidelines, responses from external APIs must be validated with Array.isArray before iterating or mapping to prevent runtime errors if the API returns an unexpected error object.

Suggested change
const page = (await pageResp.json()) as unknown[];
const page = await pageResp.json();
ok(Array.isArray(page), 'Expected page to be an array');
References
  1. When handling responses from peer nodes or external APIs, explicitly verify that expected array fields are actually arrays (e.g., using Array.isArray) before iterating over them, to prevent malformed responses from causing runtime errors.

Comment thread integrationTests/server/scale.test.ts Outdated
headers: { Authorization: auth },
});
strictEqual(page2Resp.status, 200, `REST page2 GET failed: ${page2Resp.status}`);
const page2 = (await page2Resp.json()) as Array<{ id: string }>;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The response from the external API is parsed and iterated directly. Under the repository guidelines, responses from external APIs must be validated with Array.isArray before iterating or mapping to prevent runtime errors if the API returns an unexpected error object.

Suggested change
const page2 = (await page2Resp.json()) as Array<{ id: string }>;
const page2 = await page2Resp.json();
ok(Array.isArray(page2), 'Expected page2 to be an array');
References
  1. When handling responses from peer nodes or external APIs, explicitly verify that expected array fields are actually arrays (e.g., using Array.isArray) before iterating over them, to prevent malformed responses from causing runtime errors.

Comment thread integrationTests/server/scale.test.ts Outdated
Comment on lines +181 to +182
const ids1 = new Set(((await p1Resp.json()) as Array<{ id: string }>).map((r) => r.id));
const ids2 = ((await p2Resp.json()) as Array<{ id: string }>).map((r) => r.id);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The responses from p1Resp and p2Resp are parsed and mapped directly. Under the repository guidelines, responses from external APIs must be validated with Array.isArray before iterating or mapping to prevent runtime errors if the API returns an unexpected error object.

Suggested change
const ids1 = new Set(((await p1Resp.json()) as Array<{ id: string }>).map((r) => r.id));
const ids2 = ((await p2Resp.json()) as Array<{ id: string }>).map((r) => r.id);
const p1Data = await p1Resp.json();
ok(Array.isArray(p1Data), 'Expected page 1 response to be an array');
const ids1 = new Set((p1Data as Array<{ id: string }>).map((r) => r.id));
const p2Data = await p2Resp.json();
ok(Array.isArray(p2Data), 'Expected page 2 response to be an array');
const ids2 = (p2Data as Array<{ id: string }>).map((r) => r.id);
References
  1. When handling responses from peer nodes or external APIs, explicitly verify that expected array fields are actually arrays (e.g., using Array.isArray) before iterating over them, to prevent malformed responses from causing runtime errors.

// Suite
// ──────────────────────────────────────────────────────────────────────────────

suite('Scale correctness', (ctx: ContextWithHarper) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocker: suite callback argument is node:test's TestContext, not ContextWithHarper.

In node:test, the function passed to suite(name, fn) receives a node:test TestContext as its sole argument — not the Harper context. Every other integration test in this repo declares ctx as an outer let/const binding and relies on before() to populate it via setupHarperWithFixture(ctx, …). By naming the suite callback parameter ctx: ContextWithHarper, this code shadows that outer binding, so ctx.harper inside every test() callback resolves to the node:test TestContext object — not the running Harper instance. All five tests will fail at runtime with property-access errors.

The fix is to match the repo's established pattern:

Suggested change
suite('Scale correctness', (ctx: ContextWithHarper) => {
suite('Scale correctness', () => {
const ctx = {} as ContextWithHarper;

Then before/after/test bodies close over the outer ctx, exactly as in every other test file in this repo.

Comment thread integrationTests/server/scale.test.ts Outdated
// Verify via total growth instead.
const totalAfter = await countRows(ctx, 'Product');
// Total should have grown by exactly TOTAL (IDs are unique, no overlap with Test 1).
const totalTest1 = 100_000;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocker: Test 2 hardcodes Test 1's exact record count.

const totalTest1 = 100_000; makes Test 2 structurally dependent on Test 1 running first and inserting exactly 100 K records in the same table. If Test 1 is skipped, re-ordered, or its TOTAL changes, this assertion silently produces a wrong expected value — or fails with a misleading message that hides the real issue.

Use the actual pre-insert count to compute the delta instead:

Suggested change
const totalTest1 = 100_000;
// Record the count before this test's inserts so the delta is self-contained.
const countBefore = await countRows(ctx, 'Product');
for (let start = 0; start < TOTAL; start += BATCH) {

Then assert strictEqual(totalAfter, countBefore + TOTAL, …) using the locally-measured baseline. This removes the hard coupling to Test 1's implementation.

Comment thread integrationTests/server/scale.test.ts Outdated
headers: { Authorization: auth },
});
strictEqual(pageResp.status, 200, `REST page GET failed: ${pageResp.status}`);
const page = (await pageResp.json()) as unknown[];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unguarded array assumption on REST response.

pageResp.json() is cast directly to unknown[] and then .length is accessed without verifying the response is actually an array. If the server returns an error envelope { error: "..." } the cast silently succeeds and page.length will be undefined, turning the subsequent strictEqual into a confusing "Expected 100, got undefined" rather than a clear failure.

Add a guard before using the value:

Suggested change
const page = (await pageResp.json()) as unknown[];
const page = (await pageResp.json()) as unknown[];
ok(Array.isArray(page), `Expected array response from /Product, got: ${JSON.stringify(page)}`);

Same pattern applies to page2 (line 113), p1Resp/p2Resp (lines 181–182).

Comment thread integrationTests/server/scale.test.ts Outdated
`category='A' AND inStock=true AND id >= 'prod-${ID_OFFSET}' AND id < 'prod-${ID_OFFSET + TOTAL}'`
);
// Allow a small tolerance for lexicographic edge effects on string-range predicates.
// The exact count via search_by_conditions is the gold standard; SQL gives us a lower bound.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Near-no-op assertion undermines Test 3's value.

The comment on line 212 concedes that the SQL range predicate has "lexicographic edge effects" and then falls back to ok(sqlCount > 0, …). This is effectively a liveness check, not a correctness check — sqlCount = 1 would pass. The main purpose of Test 3 is to verify multi-condition indexed-search correctness, but the SQL side of the verification is so weakened that it adds almost no signal.

Options:

  1. Drop the SQL COUNT entirely and rely solely on the search_by_conditions purity loop below (which is the real assertion).
  2. Use a numeric ID scheme for Test 3's ID_OFFSET so lexicographic order matches numeric order, enabling a precise SQL count assertion.

@claude

claude Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Reviewed; no blockers found.

(Prior finding #1suite callback param — was a false positive: crl-verification.test.ts and custom-resources.test.ts use the same (ctx: ContextWithHarper) pattern; setupHarperWithFixture mutates the TestContext in-place. Findings #2 and #3 are fixed in 55e9293.)

- Snapshot Product count before Test 2's inserts instead of hardcoding
  Test 1's 100K, removing the hidden inter-test dependency.
- Guard all REST pagination responses with Array.isArray before reading
  .length / mapping, per repo external-response handling guidelines.
- Drop Test 3's weak SQL string-range count (lexicographic ordering made
  it a near-no-op liveness check); rely on the search_by_conditions
  purity loop as the real multi-condition correctness assertion.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@kriszyp kriszyp added this to the v5.1 milestone Jun 14, 2026
@kriszyp
kriszyp marked this pull request as ready for review June 14, 2026 22:34

@Ethan-Arrowood Ethan-Arrowood left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — solid correctness coverage for the scale gap from #1192. The one red check is just a stale build-artifact/infra failure (not the tests), so a re-run should green it up. Good to go.

sent with Claude Opus 4.8

@kriszyp
kriszyp merged commit 2a64b96 into main Jun 25, 2026
252 of 258 checks passed
@kriszyp
kriszyp deleted the kris/integration-test-scale branch June 25, 2026 17:24
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.

Promote scale tests to PR CI: 100K–1M record subset + 1.44M multi-index table correctness

2 participants