test: cover getPreviewData handler and config, fix ESM jest setup (VP-2467) - #19
Merged
Merged
Conversation
Implements steps 1-11 of the shopify-lp-test-coverage plan (live code only). - extract buildServer() into src/app.ts so routes are reachable through fastify.inject() without binding a port; index.ts becomes a listen shim - move jest to native ESM (ts-jest default-esm + --experimental-vm-modules) and tsconfig.test module to ESNext, which unblocks import.meta.url in controllers/index.ts - pin minimatch, @fastify/static and test-exclude via overrides so test-exclude v7 resolves and coverage runs for the first time - getPreviewData handler: happy path, product-metafields and metaobjects branches, schema 400s, render and fetch 500s, path traversal (0 -> 96.96%) - config: five env permutation files, one module lifetime each (0 -> 100%) - delete tests/environment.test.ts and tests/githubSync.test.ts, which asserted on inlined literals and imported no production code at all Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
🔒 Security Scan Results
⏱️ SLA Breach Summary
🟠 High Severity - SLA Breached Issues (with fixes)Showing 11 issue(s) that have exceeded the 30-day SLA threshold:
❌ BUILD FAILED - Security checks failed Please review and fix the security vulnerabilities before merging. |
contentstackMridul
changed the title
6 tasks
Conflict was package.json only, in the overrides block. stag (PR #20, the Snyk fix) deleted overrides entirely: the exact @fastify/static 9.1.1 pin was what held it at a vulnerable version, and swagger-ui ^6.1.1 brings @fastify/static 10 on its own. This branch had added test-exclude ^7.0.1 to that same block. Took stag's side. The test-exclude override existed only to work around the minimatch ^9 pin, which starved test-exclude@6 of a callable minimatch. With the pin gone, test-exclude@6 resolves its own minimatch@3 and --coverage works unaided, so the override is dead weight rather than something to carry forward. Scripts keep this branch's --experimental-vm-modules form; deps take stag's swagger-ui ^6.1.1. package-lock.json reset to stag verbatim; npm install reports up to date, confirming this branch introduced no dependency of its own. Verified on the merge result: 6 suites / 41 tests pass, --coverage reports the same per-file numbers as before the merge (controllers/index 96.96%, config 100%, app 90%, routes 83.33%), tsc build clean, and a boot check of dist/index.js serves /health, /documentation and all seven swagger assets under @fastify/static 10. Plugin registration order in app.ts still matches stag's index.ts. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
🔒 Security Scan Results
⏱️ SLA Breach Summary
✅ BUILD PASSED - All security checks passed |
kirtesh-cstk
reviewed
Aug 5, 2026
Contributor
Author
There was a problem hiding this comment.
Config testing was deliberate: this middleware is customer facing, and nearly everything about its runtime behaviour comes from configuration (port, host, Contentstack credentials, rate limit). Read any of that wrong and the service either won't start or misbehaves in a way that's hard to trace back.
But you're right that five files to test one config file wasn't justified. Sorted in f354933: it's now a single tests/config.test.ts with one describe block per scenario. Five files down to one, same 18 assertions, config.ts still at 100%.
kirtesh-cstk
approved these changes
Aug 5, 2026
kirtesh-cstk
left a comment
kirtesh-cstk
left a comment
There was a problem hiding this comment.
LGTM, just check that one comment
Reviewer point on PR #19: config was split across five test files (config.test, config.defaults, config.invalidNumbers, config.oversizedRateLimit, config.rateLimit), one per env permutation. The comments in those files justified the split by claiming native ESM has no module registry reset, so a fresh config.ts evaluation was only obtainable by starting a new test file. That premise is wrong. jest.resetModules() works fine here — under native ESM the `jest` object simply is not injected as a global, so the original attempt would have died on "jest is not defined". Importing it from '@jest/globals' fixes that, and resetModules() + a dynamic import re-runs config.ts against whatever env the case installed. So all five collapse into tests/config.test.ts with one describe per permutation, behind a loadConfig() helper. Same 18 assertions, no coverage change: config.ts stays at 100%, All files stays at 35%. Also refreshed tests/README.md, which still documented githubSync.test.ts, environment.test.ts and `npm run test:github` — all removed earlier in this PR — and dropped two empty leftover dirs (tests/controllers, tests/routes). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
🔒 Security Scan Results
⏱️ SLA Breach Summary
✅ BUILD PASSED - All security checks passed |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Brings the middleware's live code under test and fixes the jest setup that was blocking it. The suite goes from 3 files and 15 tests to 6 files and 41 tests, coverage runs for the first time, and the four live source files land at 94% of lines. One production change:
src/index.tssplits into abuildServer()factory insrc/app.tsplus a listen shim, because a test could not reach a route without binding a port.Type of Change
Changes Made
src/app.ts(new):buildServer()builds a fully configured fastify instance without binding a port. Plugin registration order, logger config and route wiring are lifted verbatim fromindex.ts; the one addition is aloggeroption so tests can silence output.src/index.ts: down to importingbuildServerand the existingstart(). 59 lines removed, 3 added.tsconfig.test.json: testmodulemoved fromCommonJStoESNext.jest.config.cjs:ts-jest/presets/default-esm,extensionsToTreatAsEsm,useESM: true, plus amoduleNameMapperthat strips the ESM-required.jssuffix so../src/app.jsresolves toapp.ts.src/index.tsadded to the coverage ignore list now that it is a shim.package.json: test scripts run jest throughnode --experimental-vm-modules;test-exclude: ^7.0.1added tooverrides;test:githubdropped along with the suite it pointed at.tests/setup.ts: sets the four Contentstack credential env vars, becausecontrollers/index.tsconstructs aContentstackServiceat import time and that constructor throws on an empty credential.tests/getPreviewData.test.ts(new, 23 tests): happy path, the product-metafields and metaobjects enrichment branches, a regression case pinning that the CDAschemaarray reachescreateContentTypeKeyBasedunwrapped, nine schema-layer 400s, render and fetch 500s, a path-traversal attempt run against the real liquid engine, and the unvalidatedlocalepass-through.tests/config.test.ts: rewritten, 4 tests to 7, now covering the default test env only.tests/config.defaults.test.ts,config.rateLimit.test.ts,config.invalidNumbers.test.ts,config.oversizedRateLimit.test.ts(new, 11 tests): one env permutation each.tests/environment.test.tsandtests/githubSync.test.tsdeleted (11 tests).Coverage,
% Stmtsas printed bynpm test -- --coverage:src/controllers/index.tssrc/config.tssrc/app.tssrc/routes/index.tssrc/controllers/githubSyncController.tssrc/controllers/viewsHealthController.tsLive files only, by line from
coverage/lcov.infowith the two dead controllers excluded: 94.00% (47 of 50 lines).Why These Changes
Three things were in the way, and each had to go before a single useful test could run.
tsconfig.test.jsoncompiled tests as CommonJS, butcontrollers/index.tsusesimport.meta.url, which is a syntax error under CommonJS. The controller that owns the whole preview path could not be imported by a test.The old
src/index.tsbuilt the fastify instance and calledlisten()in the same module, so importing it to reach a route meant binding port 3002. ExtractingbuildServer()makes every route reachable throughfastify.inject()with no listener running.--coveragewas broken. The pre-existingminimatch: ^9override (it is there for a Snyk fix and it stays) leaves jest resolvingtest-exclude@6, which requires minimatch as a callable, and minimatch 9's CJS entry is an object. Overridingtest-excludeto^7lines the two back up.What was there before was worse than nothing.
tests/environment.test.tsandtests/githubSync.test.tsdid not import a single production module between them: they asserted on literals pasted into the test file, so they passed no matter what the source did. Both are deleted.tests/config.test.tsdid importsrc/config, so it survives, rewritten and extended.JIRA: VP-2467
Breaking Changes
No. The public surface is unchanged: same routes, same plugin order, same logger transport, same
listen()call.buildServer()is additive, andtest-excludeis a jest-only transitive dependency so the new override has no runtime effect. Verified by booting the built output, see step 5 below.Testing
How to Test
git checkout shopify-lp-test-coverage && npm installnpm testand expect 6 suites, 41 tests passing.npm test -- --coverageand expect the per-file numbers above. The same command onstagdoes not produce a report.npm run build, clean.npm start, thenGET /healthshould return{"status":"ok",...}and/documentation/should serve the swagger UI. Both verified locally againstdist/index.json this branch.npm run lintfails on this branch and onstagalike: no ESLint config is tracked anywhere in the repo. Not introduced here.Risk Level: MEDIUM
One production file changed, and it is the service bootstrap. The move is behaviour-preserving by inspection and by the boot check in step 5, but it is the path every deploy takes.
Notes for Reviewer
The 35% overall number is misleading, so read the per-file column instead.
githubSyncController.tsandviewsHealthController.tsare 90 of the 140 measured lines and both are dead code, pending team confirmation that nobody uses/sync-github-repoor/health/viewsas a manual escape hatch. Step 12 of the plan deletes them. Excluding those two files, the live code is at 94%.security-scais red and it is not this PR. That gate is a whole-tree--fail-on=allpolicy check: 37 highs against a ceiling of 25, driven byfast-uri@3.1.2(10 of the 11 SLA breaches) andbrace-expansion@2.0.3. The same workflow ran onstagone minute earlier and reported the identical 37, andfast-uriresolves to 3.1.2 on both sides of this diff. All three delta Snyk checks pass.Two uncovered lines are worth knowing about, though I would not fix either here.
controllers/index.ts:73, thetypeof liquid_path !== 'string'guard that returns 400, is unreachable: fastify's AjvcoerceTypesdefault turns a numericliquid_pathinto a string before the handler sees it, and there is a test documenting that.routes/index.ts:20, the/healthhandler body, has no test.The config suite is split across five files deliberately.
config.tsreadsprocess.envonce at import, and native ESM has no module registry reset, so one env permutation per file is the only isolation boundary available. Each file names the permutation it owns in a header comment.Several config tests pin behaviour I would not call correct: an unparseable
PORTyieldsNaNbecause there is no|| 3002guard the wayrateLimit.maxhas one, a negativeRATE_LIMIT_MAXpasses straight through because-5is truthy, and there is no upper clamp. Each of those is marked "documented, not endorsed" in a comment so a later fix does not read the assertion as a contract.Nothing runs this suite in CI. Neither workflow has a test job, so
npm testis local-only. Worth a follow-up.The utils half of VP-2467 is contentstack/shopify-live-preview-sdk#27. It is independent of this one: no SDK version bump here, and these tests run against the published SDK.