feat(test): add "test export" / "test import" to round-trip test DEFINITIONS for version control - #265
feat(test): add "test export" / "test import" to round-trip test DEFINITIONS for version control#265Andy00L wants to merge 4 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughTest definition export/import support is added to the test CLI, including deterministic JSON definitions, backend and frontend export behavior, validated create/update imports, concurrency and idempotency handling, file safety, and comprehensive command coverage. ChangesTest definition round-trip
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant runExport
participant runImport
participant TestAPI
participant DefinitionFile
CLI->>runExport: export test definition
runExport->>TestAPI: fetch metadata and code
TestAPI-->>runExport: return test data and codeVersion
runExport->>DefinitionFile: write JSON definition
CLI->>runImport: import JSON definition
runImport->>DefinitionFile: read and validate definition
runImport->>TestAPI: create or update metadata
runImport->>TestAPI: upload code with If-Match
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/commands/test.test.ts (1)
3265-3411: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSolid core coverage; a few critical paths are untested.
The suite covers export composition, create/update import, and schemaVersion rejection well. Missing cases that exercise other new branches in
runExport/runImport:
--out/--forcefile-write path (including the "already exists"VALIDATION_ERRORwhen--forceis omitted).--dry-runfor both commands (banner emission, canned sample shape).- The
NOT_FOUNDcode-fetch fallback (a fresh test with no code yet →codeomitted from the definition).- Required-field validation errors for
projectId/type/name(onlyschemaVersionis currently exercised).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/test.test.ts` around lines 3265 - 3411, Extend the runExport/runImport test suite to cover the missing branches: verify --out writes a file and rejects an existing file with VALIDATION_ERROR unless --force is set; verify --dry-run for both commands emits its banner and returns the expected canned sample shape; mock a NOT_FOUND code-fetch response and assert export omits code; and add import cases asserting missing projectId, type, or name produce field-level VALIDATION_ERROR results.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands/test.ts`:
- Around line 4252-4265: Align the create path with the validation used by the
update branch: only include the code POST request when def.code is defined and
def.code.body is a string. Update the create-branch condition around the POST
payload while preserving the existing behavior and metadata for valid code
bodies.
- Around line 4243-4280: Update the test import create and update flows around
the POST /tests and PUT /tests/{testId}[/code] calls to accept and reuse a
controllable idempotency key, rather than generating unrelated keys for each
request; when keys are auto-generated, log them to stderr following the existing
create/run behavior, including under JSON output. Expose the key through the
import command’s existing option/configuration path and ensure retries can
replay the same key for both update PUT requests and the create POST.
---
Nitpick comments:
In `@src/commands/test.test.ts`:
- Around line 3265-3411: Extend the runExport/runImport test suite to cover the
missing branches: verify --out writes a file and rejects an existing file with
VALIDATION_ERROR unless --force is set; verify --dry-run for both commands emits
its banner and returns the expected canned sample shape; mock a NOT_FOUND
code-fetch response and assert export omits code; and add import cases asserting
missing projectId, type, or name produce field-level VALIDATION_ERROR results.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c0a9f1ff-1d93-4f37-979c-4281b31c5e5c
⛔ Files ignored due to path filters (1)
test/__snapshots__/help.snapshot.test.ts.snapis excluded by!**/*.snap,!**/*.snap
📒 Files selected for processing (2)
src/commands/test.test.tssrc/commands/test.ts
|
Maintainer review: request changes — solid concept, five concrete gaps, plus a scope note.
Scope note: issue #125 was triaged deferred on 07-03 (schema-freeze vs in-flight server-side test-entity work) and that hold hasn't formally lifted — we're re-evaluating it now that the wire contract is stability-committed, and will record the outcome on #125. Fixing the five items above is worthwhile regardless of timing; landing waits on that call. |
…Match *, --out tests, error split
|
All five points addressed:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands/test.test.ts`:
- Around line 3415-3424: Replace the literal BOM character at the start of the
template literal passed to writeFileSync with the escaped \uFEFF sequence,
preserving the generated file’s BOM while allowing lint to pass.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b3435cc0-cbbc-485e-ac76-75fdfddbdf58
⛔ Files ignored due to path filters (1)
test/__snapshots__/help.snapshot.test.ts.snapis excluded by!**/*.snap,!**/*.snap
📒 Files selected for processing (2)
src/commands/test.test.tssrc/commands/test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/commands/test.ts
|
Lint red fixed: the BOM fixture sat in a template literal, which no-irregular-whitespace rejects (plain strings are exempt, which is why the existing BOM fixtures pass). Moved to a concatenated string; eslint and the suite are green. |
|
Adopted the suggested \uFEFF escape form for the BOM fixture (the interim concatenated-string fix already unblocked lint; this matches the proposed diff exactly). |
|
Re-reviewed properly. All five defects from the July review are fixed — BOM stripping on import, the idempotency key on the write path, What's blocking is on me, not the code. #125 was deferred on 2026-07-03 because an export format would freeze a test-entity shape we were actively changing; I promised on 07-23 to record whether that deferral was lifted, and never did. I'm posting the analysis on #125 now and will land the decision there rather than leaving it implicit here. Two practical notes meanwhile:
|
|
Thanks for the patience — this sat 19 days after you addressed CodeRabbit's review, and that silence is on us. Now that #125's deferral is lifted, here's what's needed to land it. 1. The conflict is a false alarm, not competing logic. Both this branch and Resolution: rebase onto current 2. Missing 3. Non-atomicity in I don't think this needs to hold up the PR. A doc-comment note on Separately: see the note on #125 about whether the import path's file read can reuse |
zeshi-du
left a comment
zeshi-du
left a comment
There was a problem hiding this comment.
Approving the direction.
All five defects from the July 23 review are fixed — checked each against the current diff, not just your word for it:
- BOM handling —
runImportnow doesstripBom(readFileSync(absolute, 'utf8'))beforeJSON.parse, matching every other JSON read in this file, plus a BOM-prefixed test case. - Idempotency parity —
--idempotency-keyflag, auto-mint with a stderr echo, and:meta/:codekeys derived from one base key so a retry after an ambiguous failure replays the same wire keys instead of minting new ones. codeVersion: nullno longer dropsIf-Matchsilently — it now sendsIf-Match: *with a stderr notice, matchingcode put's handling of a legacy row, with a test covering it.--out/--forceis covered: "export --out writes the file, refuses to overwrite without --force, overwrites with it."- ENOENT vs malformed JSON are two distinct messages now ("file not found: …" vs "is not valid JSON: …").
That's a complete response to review feedback.
The conflict is one real spot, and it's an adjacent insertion, not competing logic. Both branches insert new top-level code at the identical seam in src/commands/test.ts, right before the pre-existing test lint doc comment: main added a toLintIssue() helper (with its own doc comment) there after this branch was cut; you add CliTestDefinition / ExportOptions / runExport / ImportOptions / runImport at the same point. Neither side touches a line the other wrote. Resolution: keep both blocks; the order between them doesn't matter, since they're self-contained top-level declarations with no cross-references. src/commands/test.test.ts and test/__snapshots__/help.snapshot.test.ts.snap both changed on both sides too, but auto-merge without conflict markers — run the snapshot test after rebasing to confirm the merged help text is exact.
Rebase
git fetch upstream main
git rebase upstream/main
# only src/commands/test.ts should show conflict markers — keep both inserted
# blocks (main's toLintIssue() and your CliTestDefinition/runExport/runImport),
# in either order
git add src/commands/test.ts
git rebase --continue
npm run lint && npm run typecheck && npm test
git push --force-with-lease
CI is green, but that run is from 2026-07-23 — before both v0.5.0 (08-05) and v0.6.0 (08-12) — so treat it as informative, not current; the post-rebase run is the real gate.
Scope, as a condition of merge, not a formality — this is a file format we own forever once it ships, so before I merge, confirm in this thread which of these you've verified:
- The export is a projection of the API's existing shapes, not a new schema with its own versioning story. Right now
def.schemaVersion !== 1gatesimport, andschemaVersion: 1is stamped on everyexport— that's a fixed compatibility check today, but it's also exactly the shape of theformatVersionfield I flagged as the signal we're inventing a format. Tell me which one it is, and why — on its face it reads like the latter, not the former. - Round-tripping is lossless or explicitly lossy, never silently lossy.
- This is single-definition version control, not a suite/grouping concept — Test Lists own that, and export/import shouldn't grow toward it.
One item stays on #125 rather than repeating here in full: whether runImport's file read can reuse readSecretFileGuarded() (src/lib/secret-file.ts:40). One correction to that ask, since it changes what "reuse" means: that helper's .trim() already strips a leading BOM (U+FEFF is ECMAScript whitespace — its own doc comment says so), so adopting it would make your separate stripBom() call redundant, not something you'd still need on top of it. The stat → ENOENT/EACCES/EISDIR → VALIDATION_ERROR mapping it does is still the same contract you hand-rolled for defect 5.
Missing before merge: a CHANGELOG.md entry under ## [Unreleased] for test export / test import.
zeshi-du
left a comment
zeshi-du
left a comment
There was a problem hiding this comment.
I need to withdraw the approval I gave on 2026-08-19, and explain properly why, because you have turned around every round of review we've asked for and this is our error rather than yours.
What went wrong on our side. That approval was given on the basis that the five defects from the July review were fixed and CI was green — both true. It was not given against the three merge conditions we wrote down internally when we lifted the deferral on this feature, which is the check that should have happened before an approval, not after. Having now done it properly, two of the three are not met.
1. The export must be a projection of shapes the API already returns.
The export carries schemaVersion: 1, and the import side has a test that rejects a wrong schemaVersion. That is us inventing and versioning a file format, which is the specific outcome that condition exists to prevent — once a schemaVersion: 1 ships, we owe the world a migration story for 2, forever, for a convenience feature.
2. Round-trip must be lossless, or explicitly lossy — never silently lossy.
Two places break this, and the first is the one that actually worries me:
GET /tests/{id}/codereturns a presigned URL rather than a body once the code exceeds roughly 100KB.test code getdereferences that URL before showing it to the user. This PR stores the response verbatim as the test's source. So for any test above that threshold, the exported file contains a presigned URL where the source should be — and on import, once that URL has expired, the test's source becomes that dead URL string. Large tests are silently corrupted, and the corruption only becomes visible later, on a run.isPresignedCodeUrlappears zero times in the diff, so there is no branch that could be catching this.- The export drops
priority,produces,consumesandcategory, while the test that covers it is named "backend: lossless".
What unblocks a merge, concretely:
- Drop
schemaVersionand project the existing wire envelope instead. - Detect a presigned code URL and either dereference it into bounded source, or refuse to export that test with a clear error naming the test. Silently writing the URL is the one option that isn't acceptable.
- Either carry
priority/produces/consumes/category, or declare the loss — in the exported file itself and in--help— and rename the test so it doesn't claim losslessness it doesn't have.
Separately: main has shipped four releases since this branch was cut (v0.8.0 → v0.11.0), so src/commands/test.ts now conflicts. Rebase whenever you pick this back up — no rush, and there's no need to touch it until you've decided whether you want to take the three points above.
I'm sorry this took four weeks to say. The feature is one we want; the shape isn't right yet.
2 participants
Round-trip a test DEFINITION (metadata + code with
codeVersionprovenance) to a versionable JSON file, so definitions can be reviewed, backed up, and migrated.test export <test-id>writes the definition (--out <file>+--force, stdout by default). Frontend plans are write-only on the API, so FE exports carryplanUnavailable: trueand a stderr note.test import <file>creates or updates from a definition file: atestIdin the file selects update (code PUT replays the recordedcodeVersionasIf-Match, so a drifted server copy fails loudly with the existing 412 contract); notestIdcreates.testsurface test and help snapshot updated for the two new subcommands.Closes #125
Discord: interferon0
Summary by CodeRabbit
New Features
test exportto generate deterministic, versioned test definition JSON to stdout or--out(supports--dry-runand prevents overwrites unless--force).test importto create or update tests from a definition file (validates schema fields, supports--dry-run, and reportscreated/updated).planUnavailableand emit a write-only warning.Tests