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.
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