feat(ztd-cli): add test-kind support for feature tests scaffold#772
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 47 minutes and 55 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR implements design and support for a feature test "lane" selection mechanism in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ztd-cli/src/commands/featureTests.ts (1)
459-463:⚠️ Potential issue | 🟡 MinorGenerate lane-specific DB hints for the traditional artifacts.
buildDbScenarioHints()always emits ZTD guidance (“fixed app-level harness”, “keep the ZTD path thin”), soanalysis.traditional.jsonandTEST_PLAN.traditional.mdcurrently tell users to implement the traditional lane with ZTD-specific instructions. Please threadtestKindinto this hint builder and swap those messages for traditional-specific guidance.Also applies to: 497-513
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ztd-cli/src/commands/featureTests.ts` around lines 459 - 463, The DB scenario hint generator is currently ZTD-biased because buildDbScenarioHints is called without the test kind; thread params.testKind into buildDbScenarioHints calls (e.g., replace buildDbScenarioHints(writesTables, params.queryLayout.queryName, fixtureCandidateTables) with buildDbScenarioHints(params.testKind, writesTables, params.queryLayout.queryName, fixtureCandidateTables) at both the shown call and the other occurrence around 497-513) and update the buildDbScenarioHints implementation to branch on the incoming testKind (e.g., if testKind === 'traditional' emit traditional-specific guidance like implementing the traditional lane and non-ZTD harness notes; if testKind indicates ZTD keep the existing ZTD-specific hints), ensuring the function signature and any callers are adjusted accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/guide/ztd-cli-traditional-lane-followup-plan.md`:
- Around line 8-10: Update the guide header to reflect the shipped scaffold:
change "Implementation status: `not done`" to `done` (so both "Current status"
and "Implementation status" indicate completion) and replace all occurrences of
the old filenames `TEST_PLAN.ztd.md` and `analysis.ztd.json` with the scaffold's
actual paths `generated/TEST_PLAN.md` and `generated/analysis.json` in
docs/guide/ztd-cli-traditional-lane-followup-plan.md (also apply the same
replacements where referenced in lines ~72-75).
In `@packages/ztd-cli/src/commands/featureTests.ts`:
- Around line 345-375: The generated vitest entrypoint currently emits a
non-runnable test.skip scaffold for the "traditional" kind (see
vitestEntrypointFile and the test.skip line) and also leaves fixedVerifierPath
as a TODO; either wire this scaffold to the real library traditional-mode
adapter or block/flag the kind: add a guard where the CLI/templating chooses
test kind (check the code that sets params.testKind / fixedVerifierPath) to
either (A) replace the test.skip scaffold with an actual call to the library
traditional adapter (use the real adapter import instead of the TODO
fixedVerifierPath and remove test.skip), or (B) fail-fast / mark as experimental
by throwing a clear error or returning a template that logs "traditional mode
not supported yet" when params.testKind === 'traditional' so we never generate a
permanent skipped test; update any references to fixedVerifierPath and the
vitestEntrypointFile generation to reflect the chosen behavior.
---
Outside diff comments:
In `@packages/ztd-cli/src/commands/featureTests.ts`:
- Around line 459-463: The DB scenario hint generator is currently ZTD-biased
because buildDbScenarioHints is called without the test kind; thread
params.testKind into buildDbScenarioHints calls (e.g., replace
buildDbScenarioHints(writesTables, params.queryLayout.queryName,
fixtureCandidateTables) with buildDbScenarioHints(params.testKind, writesTables,
params.queryLayout.queryName, fixtureCandidateTables) at both the shown call and
the other occurrence around 497-513) and update the buildDbScenarioHints
implementation to branch on the incoming testKind (e.g., if testKind ===
'traditional' emit traditional-specific guidance like implementing the
traditional lane and non-ZTD harness notes; if testKind indicates ZTD keep the
existing ZTD-specific hints), ensuring the function signature and any callers
are adjusted accordingly.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 46894cb3-3efa-42ca-a130-dc88379409f5
📒 Files selected for processing (6)
.changeset/fair-apes-tie.mddocs/guide/ztd-cli-traditional-lane-followup-plan.mdpackages/ztd-cli/README.mdpackages/ztd-cli/src/commands/featureTests.tspackages/ztd-cli/tests/cliCommands.test.tspackages/ztd-cli/tests/featureTestsScaffold.unit.test.ts
| - Current status: `done` (planning artifact for Issue #767) | ||
| - Implementation status: `not done` (separate issues/PRs are expected) | ||
|
|
There was a problem hiding this comment.
Align this guide with the shipped scaffold layout.
This doc is linked from README as a user guide, but it still says the implementation is “not done” and shows TEST_PLAN.ztd.md / analysis.ztd.json. The current scaffold code already ships --test-kind and keeps the ZTD lane on generated/TEST_PLAN.md and generated/analysis.json, so readers will follow the wrong filenames and infer the feature is unavailable.
Also applies to: 72-75
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/guide/ztd-cli-traditional-lane-followup-plan.md` around lines 8 - 10,
Update the guide header to reflect the shipped scaffold: change "Implementation
status: `not done`" to `done` (so both "Current status" and "Implementation
status" indicate completion) and replace all occurrences of the old filenames
`TEST_PLAN.ztd.md` and `analysis.ztd.json` with the scaffold's actual paths
`generated/TEST_PLAN.md` and `generated/analysis.json` in
docs/guide/ztd-cli-traditional-lane-followup-plan.md (also apply the same
replacements where referenced in lines ~72-75).
| const vitestEntrypointFile = isZtdLane | ||
| ? [ | ||
| `import { expect, test } from 'vitest';`, | ||
| '', | ||
| `import { runQuerySpecZtdCases } from '${harnessImportPath}';`, | ||
| `import { ${executorName} } from '${querySpecImportPath}';`, | ||
| `import cases from '${casesImportPath}';`, | ||
| `import type { ${queryCaseTypeName} } from '${queryTypesImportPath}';`, | ||
| '', | ||
| `test('${params.featureName}/${params.queryName} boundary ZTD cases run through the fixed app-level harness', async () => {`, | ||
| ' expect(cases.length).toBeGreaterThan(0);', | ||
| ` const evidence = await runQuerySpecZtdCases(cases, ${executorName});`, | ||
| " expect(evidence.every((entry) => entry.mode === 'ztd')).toBe(true);", | ||
| ' expect(evidence.every((entry) => entry.physicalSetupUsed === false)).toBe(true);', | ||
| '});', | ||
| '' | ||
| ].join('\n') | ||
| : [ | ||
| `import { expect, test } from 'vitest';`, | ||
| '', | ||
| `import { ${executorName} } from '${querySpecImportPath}';`, | ||
| `import cases from '${casesImportPath}';`, | ||
| `import type { ${queryCaseTypeName} } from '${queryTypesImportPath}';`, | ||
| '', | ||
| `test.skip('${params.featureName}/${params.queryName} boundary traditional lane scaffold placeholder', async () => {`, | ||
| ' expect(cases.length).toBeGreaterThan(0);', | ||
| ' // TODO(issue-767 follow-up): wire this lane to the library traditional mode API.', | ||
| ` void ${executorName};`, | ||
| '});', | ||
| '' | ||
| ].join('\n'); |
There was a problem hiding this comment.
Don't ship traditional as a supported kind until the scaffold is executable.
Right now --test-kind traditional generates a permanent test.skip(...) placeholder and a fixedVerifierPath of TODO: library traditional mode API adapter. That means the new public CLI option produces a non-runnable lane even though the help text, README link, and changeset present it as supported. Either wire this scaffold to the library adapter now or fail fast / mark the kind as experimental until that adapter exists.
Also applies to: 476-478
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ztd-cli/src/commands/featureTests.ts` around lines 345 - 375, The
generated vitest entrypoint currently emits a non-runnable test.skip scaffold
for the "traditional" kind (see vitestEntrypointFile and the test.skip line) and
also leaves fixedVerifierPath as a TODO; either wire this scaffold to the real
library traditional-mode adapter or block/flag the kind: add a guard where the
CLI/templating chooses test kind (check the code that sets params.testKind /
fixedVerifierPath) to either (A) replace the test.skip scaffold with an actual
call to the library traditional adapter (use the real adapter import instead of
the TODO fixedVerifierPath and remove test.skip), or (B) fail-fast / mark as
experimental by throwing a clear error or returning a template that logs
"traditional mode not supported yet" when params.testKind === 'traditional' so
we never generate a permanent skipped test; update any references to
fixedVerifierPath and the vitestEntrypointFile generation to reflect the chosen
behavior.
mk3008
deleted the
767-ztd-cli-traditional-test-kind-と-ztdtraditional-併存-scaffold-を後続拡張として設計する
branch
1 participant
Summary
--test-kind <kind>toztd feature tests scaffoldwithztdas the default.ztdandtraditionalso both can coexist under the same query tests directory.--test-kindfail-fast behavior.Verification
pnpm --filter @rawsql-ts/ztd-cli buildpnpm --filter @rawsql-ts/ztd-cli test -- featureTestsScaffold.unit.test.ts cliCommands.test.tspre-commitgate suite passed during commit (includestest:essential, build, lint)Merge Readiness
Tracking issue:
Scoped checks run:
Why full baseline is not required:
CLI Surface Migration
No-migration rationale:
Upgrade note:
ztd feature tests scaffoldnow accepts--test-kind ztd|traditional. Default remainsztd.Deprecation/removal plan or issue: None. No existing flag or behavior was removed.
Docs/help/examples updated: Yes. CLI help coverage updated in
packages/ztd-cli/tests/cliCommands.test.ts; follow-up plan linked frompackages/ztd-cli/README.md.Release/changeset wording: Added
.changeset/fair-apes-tie.mdas@rawsql-ts/ztd-cli minor.Scaffold Contract Proof
No-proof rationale:
Non-edit assertion: Existing ZTD scaffold assets are preserved (
<query>.boundary.ztd.test.ts,generated/TEST_PLAN.md,generated/analysis.json) and traditional lane files are generated as additional, lane-qualified artifacts.Fail-fast input-contract proof: Unsupported
--test-kindvalues fail fast with an explicit error (Feature test kind supports only ztd, traditional.), covered by unit test.Generated-output viability proof: Traditional lane now generates
*.boundary.traditional.test.ts,boundary-traditional-types.ts,cases/basic.traditional.case.ts,generated/TEST_PLAN.traditional.md, andgenerated/analysis.traditional.json, covered by unit test.Closes #767
Summary by CodeRabbit
New Features
--test-kindCLI option forztd feature tests scaffold, allowing users to select betweenztdandtraditionaltest scaffolding approaches. Defaults to ZTD for backward compatibility, with each mode generating appropriate scaffold outputs.Documentation