test(integration): port Northwind test suite to self-contained framework - #778
Merged
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Comment on lines
+2073
to
+2124
| for (const [table, pk, csv] of northwindTables) { | ||
| await client.req().send({ operation: 'create_table', schema: 'northnwd', table, primary_key: pk }).expect(200); | ||
| const csvR = await client | ||
| .req() | ||
| .send({ | ||
| operation: 'csv_file_load', | ||
| action: 'insert', | ||
| schema: 'northnwd', | ||
| table, | ||
| file_path: csvPath + csv, | ||
| }) | ||
| .expect(200); | ||
| await awaitJobCompleted(client, getJobId(csvR.body), { timeoutSeconds: 60 }); | ||
| } | ||
|
|
||
| // ── dev tables (created empty; JSON data inserted by 2_dataLoad tests) ─ | ||
| const devTables = [ | ||
| ['long_text', 'id'], | ||
| ['AttributeDropTest', 'hashid'], | ||
| ['invalid_attribute', 'id'], | ||
| ['remarks_blob', 'id'], | ||
| ['time_functions', 'id'], | ||
| ['dog', 'id'], | ||
| ['breed', 'id'], | ||
| ['owner', 'id'], | ||
| ['sql_function', 'id'], | ||
| ['leading_zero', 'id'], | ||
| ['dog_conditions', 'id'], | ||
| ['rando', 'id'], | ||
| ]; | ||
| for (const [table, pk] of devTables) { | ||
| await client.req().send({ operation: 'create_table', schema: 'dev', table, primary_key: pk }).expect(200); | ||
| } | ||
| // Large dev CSVs (books, ratings, movie, credits) — no test in 2_dataLoad re-inserts these | ||
| for (const [table, pk, csv, timeout] of [ | ||
| ['books', 'id', 'Books.csv', 30], | ||
| ['ratings', 'id', 'BooksRatings.csv', 60], | ||
| ['movie', 'id', 'movies.csv', 60], | ||
| ['credits', 'movie_id', 'credits.csv', 120], | ||
| ]) { | ||
| await client.req().send({ operation: 'create_table', schema: 'dev', table, primary_key: pk }).expect(200); | ||
| const r = await client | ||
| .req() | ||
| .send({ | ||
| operation: 'csv_file_load', | ||
| action: 'insert', | ||
| schema: 'dev', | ||
| table, | ||
| file_path: csvPath + csv, | ||
| }) | ||
| .expect(200); | ||
| await awaitJobCompleted(client, getJobId(r.body), { timeoutSeconds: timeout }); |
Contributor
There was a problem hiding this comment.
Double CSV loading makes "2. Data Load" tests hollow.
before() inserts every Northwind table via csv_file_load action: 'insert' (lines 2073-2086) and loads all four large dev CSVs (lines 2107-2124). Then the "2. Data Load" suite re-uploads the exact same files in tests 1-15.
Harper's insert path silently skips records whose primary key already exists (bulkLoad.ts:756-765, buildResponseMsg → "successfully loaded 0 of N records"). The job status is still COMPLETE, so awaitJobCompleted asserts nothing wrong — but every record is skipped. Tests 1-15 exercise only the "graceful no-op on duplicate keys" branch, not the actual insertion path.
The comment on line 2106 makes this explicit in the wrong direction: "Large dev CSVs (books, ratings, movie, credits) — no test in 2_dataLoad re-inserts these" — but tests 12-15 do exactly that.
Suggested fix: Create the tables in before() but do not load their CSV data; leave that to the "2. Data Load" suite so those tests actually exercise the insert path with empty tables. Subsequent suites depend on suite 2 running first — add a comment noting that dependency. This matches the PR description's stated goal of loading CSV data once.
Contributor
| Reviewed the 4 fix commits pushed since the initial no-blockers verdict. The fixes are correct and address the cross-model findings. One minor observation below; no blockers. --- ### 1. csvDataLoad and checkJobCompleted error-path assertion is weaker than awaitJobCompleted **File:** integrationTests/apiTests/northwind.test.mjs lines 2167 and 2229 **What:** Both shims assert notEqual(job.status, COMPLETE) when expecting an error, but awaitJobCompleted in utils/operations.mjs asserts equal(job.status, ERROR). If awaitJob times out with job still IN_PROGRESS, status !== COMPLETE passes and a timed-out job with a null message produces a confusing substring-check failure rather than a clear timeout failure. **Why it matters:** Low risk in practice because the message-substring assertion acts as a secondary guard, but the failure message would be misleading. **Suggested fix:** Replace assert.notEqual(job.status, COMPLETE) with assert.equal(job.status, ERROR) in both csvDataLoad and checkJobCompleted. This mirrors awaitJobCompleted behaviour and produces a clearer failure on timeout. --- **Deferred concerns acknowledged:** User/role cleanup in test() blocks rather than after() hooks is tracked as a follow-up per the maintainer comment. The skipped Drop bulk_load_user/Drop bulk_load_role tests carry a misleading Skip related to S3 usage comment even though they have no S3 dependency; updating to Skip: cleanup deferred to follow-up would reduce confusion. |
Member
Author
|
Cross-model review follow-up (Claude + Codex + Gemini/agy) Three commits on this branch address review findings: Commit 2 (Codex findings):
Commit 3 (Gemini/agy findings):
Remaining significant concern (not addressed — intentional scope decision): — Claude |
kriszyp
marked this pull request as ready for review
May 27, 2026 13:14
Migrates 7 legacy Northwind-dependent test files (2_dataLoad.mjs, 3_sqlTests.mjs, 4_noSqlTests.mjs, 5_noSqlRoleTesting.mjs, 6_sqlRoleTesting.mjs, 7_jobsAndJobRoleTesting.mjs, 10_otherRoleTests.mjs) into a single self-contained northwind.test.mjs. All sub-suites share one Harper instance to avoid repeating the expensive CSV load. Shim functions bridge the legacy utility API to the new createApiClient/awaitJob framework. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add dev.rando to before() devTables list (was inserted by tests but never created) - Create schema '123' and table '123.4' in before() (NoSQL tests use them) - Fix csvDataLoad shim to delegate to awaitJobCompleted so _expectedError and _expectedMessage are actually asserted rather than silently ignored Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Remove CSV loads from before() to fix duplicate-PK conflicts: Northwind and dev CSV tables are now created empty; 2_dataLoad sub-suite loads all data first, relying on sequential sub-suite execution for 3-10 - Fix checkJobCompleted shim: assert job.status !== COMPLETE when expecting an error (not just message substring match), and guard against empty body Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Fix csvPath: join(__dirname, '../data') pointed one level too high; correct path is join(__dirname, 'data') matching other test files - Apply prettier formatting (format check was failing in CI) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ailures - csvDataLoad: revert to awaitJob + manual assertions so job.message is returned as a raw object; awaitJobCompleted was stringifying it, causing TypeError when tests access errorMsg.unauthorized_access/.invalid_schema_items - Geo tests (geoLength/geoDistance/geoNear test 2): wrap deepEqual with roundFloats() to tolerate sub-ULP floating-point differences on Bun - NoSQL wildcard search tests: skip on Bun where HarperDB server returns 'finishUtf8 is not defined' (V8 internal API not available in Bun) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Use process.env.HARPER_RUNTIME === 'bun' to detect Bun: Harper runs the test process under Node.js and sets this env var rather than running tests in Bun itself, so process.versions.bun is always undefined - Increase csvDataLoad job poll timeout to 60s on Bun (was timing out at 30s with job still IN_PROGRESS) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
kriszyp
force-pushed
the
claude/api-tests-northwind
branch
from
June 8, 2026 21:57
8c870ac to
85dc194
Compare
Member
Author
CI Status ✅All integration tests for our changes pass — northwind test suite (shard 4/4) runs successfully on all platforms: Node.js v22/v24/v26, Bun, and Windows. Failing checks are pre-existing flakes not caused by this PR:
Ready for review and merge! 🎉 — Claude |
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
2_dataLoad.mjs,3_sqlTests.mjs,4_noSqlTests.mjs,5_noSqlRoleTesting.mjs,6_sqlRoleTesting.mjs,7_jobsAndJobRoleTesting.mjs,10_otherRoleTests.mjs) into a single self-containednorthwind.test.mjsbefore()csvFileUpload,csvDataLoad,csvUrlLoad,insert,searchByHash,checkJob,checkJobCompleted) close overclientto bridge the legacy utility API to the newcreateApiClient/awaitJobframework2. Data Loadsub-suite so those insert tests remain validTest plan
node --check integrationTests/apiTests/northwind.test.mjs— passesnpm run lint:required— 0 errors🤖 Generated with Claude Code
— Claude