Skip to content

Commit 3479e6f

Browse files
juanpprietorbshop
andauthored
Enhanced cookbook recipe validation (#3238)
* test(cookbook): add vitest testing infrastructure Set up vitest for cookbook package validation tests following repo patterns: - Add vitest dependency and test scripts - Create vitest.config.ts with node environment - Add smoke test (util.test.ts) with 2 passing tests - Add cookbook to root workspaces for turbo integration Tests can be run via: - `cd cookbook && npm test` - `npx turbo run test --filter=cookbook` * feat(cookbook): enforce numeric step numbers and validate duplicate names Enforces step numbers as numbers (not strings) via Zod schema and adds runtime validation to catch duplicate step names in grouped steps. Changes: - Update Zod schema to only accept numeric step numbers - Update JSON schema to match (remove string union type) - Add validateStepNumbering() to check for duplicate names in grouped steps - Integrate validation into validateRecipe() flow before applyRecipe() - Add comprehensive test coverage (7 new tests) This catches the PR-3228 bug where recipe.yaml had step: "1" as strings and prevents duplicate step names like: step: 1, name: "Update config" step: 1, name: "Update config" ← now throws error Grouped steps with distinct names are still allowed: step: 1, name: "README.md" step: 1, name: "app/entry.server.tsx" ← valid Performance: Recipe is loaded twice (once for validation, once in applyRecipe). This duplication is acceptable as the overhead is negligible (~5ms) and keeps the implementation simple. Can be optimized later if needed. Tests: 9 passing (3 Zod validation tests, 4 duplicate name tests, 2 smoke tests) * refactor(cookbook): split validators and enforce step descriptions Refactors validation into focused, single-responsibility validators: - validateStepNames() - checks for duplicate names in grouped steps - validateStepDescriptions() - enforces descriptions are not null/empty This is a breaking change that will fail validation for 55 steps across 7 recipes (b2b, metaobjects, legacy-customer-account-flow, gtm, infinite-scroll, combined-listings, third-party-api) that currently have description: null. These will be fixed in a separate PR per PR-3228 discussion. Tests: 11 passing (4 step name tests, 2 description tests, 5 other tests) * feat(cookbook): add bidirectional file reference validation Adds validators to ensure patch and ingredient files match between recipe.yaml and filesystem in both directions - no broken references, no orphaned files. Validators added: - validatePatchFiles() - checks patches referenced in yaml exist on disk, and all patch files on disk are referenced in yaml - validateIngredientFiles() - checks ingredients referenced in yaml exist on disk, and all ingredient files on disk are referenced in yaml This catches: - Missing patch/ingredient files (broken references) - Orphaned patch/ingredient files (not referenced in recipe) - Early detection before recipe application Performance: O(n) single-pass with Set for O(1) lookups, early returns when directories don't exist. Tests: 17 passing (12 validation tests, 3 recipe tests, 2 util tests) - 3 patch validation tests - 3 ingredient validation tests - 6 existing validation tests * feat(cookbook): validate README and LLM prompt existence Adds validators to ensure rendered outputs exist: - validateReadmeExists() - checks README.md exists at recipes/{name}/README.md - validateLlmPromptExists() - checks LLM prompt exists at llms/{name}.prompt.md Uses real integration tests against actual filesystem (gtm recipe) instead of mocked tests. This provides more confidence that validation works correctly. Changes: - Add LLMS_PATH and RENDER_FILENAME_GITHUB to imports - Add two focused validators with helpful error messages - Integrate into validateRecipe() flow - Remove global fs mock, use targeted spyOn mocks only where needed - Add 4 integration tests (2 per validator) Tests: 21 passing (16 validation tests, 3 recipe tests, 2 util tests) * fix(cookbook): use numeric step fallback in generate Fixes TypeScript error caused by string step number fallback after enforcing numeric-only step numbers in recipe schema. Changed: `step: existingStep?.step ?? \`${i}\`` To: `step: existingStep?.step ?? i` This ensures generated steps always use numbers, matching the Zod schema requirement from commit c937534. * feat(cookbook): enhance validation error reporting with actual values Improves validation error messages by showing actual YAML values alongside expected types, making it immediately clear what needs to be fixed. Changes: - Add getYamlValue() to extract actual values from YAML nodes - Include actual values in Zod schema error messages Example: "Expected number, received string (actual value: "1")" - Add validator names to all error output for clarity - Refactor error collection to run all validators even with schema errors (README/LLM prompt validators now run regardless of recipe schema validity) - Remove unused functions: convertZodErrorToValidationErrors, enrichErrorsWithLineNumbers - Add comprehensive unit tests for formatValidationError - Add integration test validating actual values in error messages - Mock filesystem in validateReadmeExists and validateLlmPromptExists tests Test coverage: 26/26 tests passing * test(cookbook): mock filesystem in actual value validation test Fixes CI failure where test relied on actual filesystem paths that differ between local and CI environments. Now uses mocked YAML content instead. * test(cookbook): mock filesystem in getYamlLineNumber tests Ensures tests work in both local and CI environments by mocking fs.readFileSync instead of relying on actual recipe files. * feat(cookbook)!: migrate step field from number to string with regex validation BREAKING CHANGE: Recipe step field now requires string format instead of number. All recipe.yaml files must be updated: step: 1 → step: "1" Changes: - Update Zod schema: z.number() → z.string().regex(/^\d+(?:\.\d+)?$/) - Update JSON schema: type "number" → type "string" with pattern validation - Add comprehensive test coverage (39 tests passing) - Fix compareSteps bug: isSubstep(a) → isSubstep(step) - Remove unnecessary String() conversions in validation - Export compareSteps for testability Test improvements: - Add generate.test.ts with step ordering tests - Update all test expectations for string steps - Add edge case validation (invalid formats, substeps) - Remove unused leading zeros test - Document grouping feature and testing strategy * fix(cookbook): address PR feedback and add TypeScript strict checking Fixes issues identified in PR review and adds comprehensive TypeScript checking. Changes: - Fix type mismatch in generate.ts: use String(i) for step field fallback - Add explanatory comment for empty catch block in validate.ts - Fix test to verify actual relative order preservation in compareSteps - Add 3 edge case tests for compareSteps (leading zeros, mixed types, decimals) - Fix all Recipe test mocks: add missing llms field (12+ instances) - Fix TypeScript mock types: use Mock type with assertions - Add typecheck script and configure tsconfig with skipLibCheck - Align TypeScript version to 5.9.2 (matches packages/) Test coverage: 42/42 tests passing TypeScript: Zero errors in src/ * test(cookbook): add comprehensive grouping test coverage and cleanup test names Adds 9 new tests covering edge cases and cleans up test descriptions for clarity. New tests: - Multiple separate groups with stable sort - Interleaved groups maintaining insertion order - Interleaved main steps and substeps (validates numeric sorting) - Multiple substeps with same step number - Explicit "1.0" vs implicit "1" equivalence - Zero step numbers and zero with substeps - Very large step numbers (9999, 1000) - Multiple files in same step number (real-world scenario) Test name improvements: - Remove verbose comments and investigation tags - Simplify descriptions for contributor clarity - Focus on what behavior is being validated Coverage: 51/51 tests passing * feat(cookbook): add formatted error handling to all loadRecipe calls Ensures consistent error formatting across all commands that load recipes. Changes: - Export printValidationErrors for reuse across commands - Add handleZodErrorFromLoadRecipe helper to format Zod schema errors - Wrap loadRecipe calls in try-catch in apply.ts, render.ts, llms.ts, update.ts - Display errors with line numbers and actual values instead of raw Zod output Before: ZodError: [{ code: 'invalid_type', expected: 'string', received: 'number', path: ['steps', 0, 'step'], ... }] After: recipe.yaml:34 steps.0.step RecipeSchema: Expected string, received number (actual value: 1) All commands now have consistent, actionable error messages. * feat(cookbook): reject duplicate step numbers, enforce substep hierarchy Addresses PR feedback to prevent confusing duplicate step number headings. Changes: - Reject duplicate step numbers (e.g., two steps both labeled '1') - Detect numeric equivalence ('1' === '01' === '1.0') - Provide helpful error messages suggesting substeps - Add 4 new validation tests for duplicate detection - Remove 5 grouping tests that validated incorrect behavior - Simplify generate.test.ts to 12 focused tests - Add lint script to cookbook/package.json Validation now enforces proper hierarchy: ✅ VALID: step: '1' → step: '1.1' → step: '1.2' → step: '2' ❌ INVALID: step: '1' → step: '1' (use substeps instead) Rendering output: ### Step 1: Setup #### Step 1.1: README.md #### Step 1.2: app/root.tsx Test coverage: 46/46 tests passing --------- Co-authored-by: rbshop <[email protected]>
1 parent 2ba29bb commit 3479e6f

18 files changed

+3761
-163
lines changed

.changeset/enforce-calver-ci.js

Lines changed: 46 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ const {
2121
getBumpType,
2222
getPackagePath,
2323
readPackage,
24-
writePackage
24+
writePackage,
2525
} = require('./calver-shared.js');
2626

2727
// Read all package.json files for CalVer packages
@@ -33,20 +33,27 @@ function readPackageVersions() {
3333
// Get hydrogen's git baseline (used for major bump synchronization)
3434
let hydrogenBaselineVersion;
3535
try {
36-
const gitVersion = execSync('git show HEAD~1:packages/hydrogen/package.json 2>/dev/null || git show origin/main:packages/hydrogen/package.json', {
37-
encoding: 'utf-8',
38-
stdio: ['pipe', 'pipe', 'ignore']
39-
});
36+
const gitVersion = execSync(
37+
'git show HEAD~1:packages/hydrogen/package.json 2>/dev/null || git show origin/main:packages/hydrogen/package.json',
38+
{
39+
encoding: 'utf-8',
40+
stdio: ['pipe', 'pipe', 'ignore'],
41+
},
42+
);
4043
const versionMatch = gitVersion.match(/"version":\s*"([^"]+)"/);
4144
if (versionMatch) {
4245
hydrogenBaselineVersion = versionMatch[1];
43-
console.log(`Using hydrogen baseline from git: ${hydrogenBaselineVersion}`);
46+
console.log(
47+
`Using hydrogen baseline from git: ${hydrogenBaselineVersion}`,
48+
);
4449
}
4550
} catch (error) {
4651
const hydrogenPath = getPackagePath('@shopify/hydrogen');
4752
const hydrogenPkg = readPackage(hydrogenPath);
4853
hydrogenBaselineVersion = hydrogenPkg.version;
49-
console.log(`Using hydrogen current version as fallback: ${hydrogenBaselineVersion}`);
54+
console.log(
55+
`Using hydrogen current version as fallback: ${hydrogenBaselineVersion}`,
56+
);
5057
}
5158

5259
// Get each package's individual baseline for independent patch versioning
@@ -58,12 +65,17 @@ function readPackageVersions() {
5865
let packageOwnBaseline;
5966
try {
6067
const gitPath = pkgPath.replace(process.cwd() + '/', '');
61-
const gitVersion = execSync(`git show HEAD~1:${gitPath} 2>/dev/null || git show origin/main:${gitPath}`, {
62-
encoding: 'utf-8',
63-
stdio: ['pipe', 'pipe', 'ignore']
64-
});
68+
const gitVersion = execSync(
69+
`git show HEAD~1:${gitPath} 2>/dev/null || git show origin/main:${gitPath}`,
70+
{
71+
encoding: 'utf-8',
72+
stdio: ['pipe', 'pipe', 'ignore'],
73+
},
74+
);
6575
const versionMatch = gitVersion.match(/"version":\s*"([^"]+)"/);
66-
packageOwnBaseline = versionMatch ? versionMatch[1] : hydrogenBaselineVersion;
76+
packageOwnBaseline = versionMatch
77+
? versionMatch[1]
78+
: hydrogenBaselineVersion;
6779
} catch (error) {
6880
packageOwnBaseline = hydrogenBaselineVersion;
6981
}
@@ -141,10 +153,7 @@ function updateChangelogs(updates) {
141153
let content = fs.readFileSync(changelogPath, 'utf-8');
142154

143155
// Replace the version that changesets generated with our CalVer version
144-
const regex = new RegExp(
145-
`^## \\d+\\.\\d+\\.\\d+`,
146-
'gm'
147-
);
156+
const regex = new RegExp(`^## \\d+\\.\\d+\\.\\d+`, 'gm');
148157
content = content.replace(regex, (match) => {
149158
// Only replace if it's a recent addition (first occurrence)
150159
return content.indexOf(match) === content.search(regex)
@@ -159,41 +168,42 @@ function updateChangelogs(updates) {
159168
// Get all package.json paths from packages/ and templates/ directories
160169
function getAllPackageJsonPaths() {
161170
const paths = [];
162-
171+
163172
// Get packages directory entries
164173
const packagesDir = path.join(process.cwd(), 'packages');
165174
const packageDirs = fs.readdirSync(packagesDir);
166-
175+
167176
for (const dir of packageDirs) {
168177
const pkgPath = path.join(packagesDir, dir, 'package.json');
169178
if (fs.existsSync(pkgPath)) {
170179
paths.push(pkgPath);
171180
}
172181
}
173-
182+
174183
// Get templates directory entries
175184
const templatesDir = path.join(process.cwd(), 'templates');
176-
const templateDirs = fs.readdirSync(templatesDir)
177-
.filter(d => d !== 'TEMPLATE_GUIDELINES.md');
178-
185+
const templateDirs = fs
186+
.readdirSync(templatesDir)
187+
.filter((d) => d !== 'TEMPLATE_GUIDELINES.md');
188+
179189
for (const dir of templateDirs) {
180190
const pkgPath = path.join(templatesDir, dir, 'package.json');
181191
if (fs.existsSync(pkgPath)) {
182192
paths.push(pkgPath);
183193
}
184194
}
185-
195+
186196
return paths;
187197
}
188198

189199
// Update dependencies in a single package.json
190200
function updatePackageDependencies(pkg, versionMap) {
191201
let modified = false;
192202
const depTypes = ['dependencies', 'devDependencies', 'peerDependencies'];
193-
203+
194204
for (const depType of depTypes) {
195205
if (!pkg[depType]) continue;
196-
206+
197207
for (const [depName, depVersion] of Object.entries(pkg[depType])) {
198208
if (versionMap[depName]) {
199209
// Preserve any prefix like ^, ~, or workspace:
@@ -203,7 +213,7 @@ function updatePackageDependencies(pkg, versionMap) {
203213
}
204214
}
205215
}
206-
216+
207217
return modified;
208218
}
209219

@@ -214,15 +224,15 @@ function updateInternalDependencies(updates) {
214224
for (const update of updates) {
215225
versionMap[update.name] = update.newVersion;
216226
}
217-
227+
218228
// Get all package.json paths
219229
const packagePaths = getAllPackageJsonPaths();
220-
230+
221231
// Update each package.json file
222232
for (const pkgPath of packagePaths) {
223233
const pkg = readPackage(pkgPath);
224234
const modified = updatePackageDependencies(pkg, versionMap);
225-
235+
226236
if (modified) {
227237
writePackage(pkgPath, pkg);
228238
}
@@ -241,7 +251,9 @@ function validateCalVer(version) {
241251

242252
// Check major is a valid quarter
243253
if (!QUARTERS.includes(v.major)) {
244-
throw new Error(`Invalid quarter in version ${version}: ${v.major} not in [${QUARTERS}]`);
254+
throw new Error(
255+
`Invalid quarter in version ${version}: ${v.major} not in [${QUARTERS}]`,
256+
);
245257
}
246258

247259
return true;
@@ -263,7 +275,9 @@ function main() {
263275
}
264276

265277
if (!hasCalVerChanges) {
266-
console.log('No CalVer package changes detected. Skipping CalVer enforcement.');
278+
console.log(
279+
'No CalVer package changes detected. Skipping CalVer enforcement.',
280+
);
267281
console.log('This is a semver-only release.');
268282
return;
269283
}
@@ -298,4 +312,4 @@ if (require.main === module) {
298312
console.error('Error:', error.message);
299313
process.exit(1);
300314
}
301-
}
315+
}

0 commit comments

Comments
 (0)