From d6d45aae1288da0b2930cff3f7973a71bd047024 Mon Sep 17 00:00:00 2001 From: Mridul Sharma Date: Tue, 4 Aug 2026 12:31:37 +0530 Subject: [PATCH 1/2] test: cover getPreviewData handler and config, fix ESM jest setup 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 --- jest.config.cjs | 14 +- package-lock.json | 279 ++++++++++++++++++++-- package.json | 14 +- src/app.ts | 74 ++++++ src/index.ts | 62 +---- tests/config.defaults.test.ts | 58 +++++ tests/config.invalidNumbers.test.ts | 29 +++ tests/config.oversizedRateLimit.test.ts | 23 ++ tests/config.rateLimit.test.ts | 28 +++ tests/config.test.ts | 51 ++-- tests/environment.test.ts | 78 ------ tests/getPreviewData.test.ts | 300 ++++++++++++++++++++++++ tests/githubSync.test.ts | 93 -------- tests/setup.ts | 9 +- tsconfig.test.json | 2 +- 15 files changed, 836 insertions(+), 278 deletions(-) create mode 100644 src/app.ts create mode 100644 tests/config.defaults.test.ts create mode 100644 tests/config.invalidNumbers.test.ts create mode 100644 tests/config.oversizedRateLimit.test.ts create mode 100644 tests/config.rateLimit.test.ts delete mode 100644 tests/environment.test.ts create mode 100644 tests/getPreviewData.test.ts delete mode 100644 tests/githubSync.test.ts diff --git a/jest.config.cjs b/jest.config.cjs index af1dd3c..a4d83a0 100644 --- a/jest.config.cjs +++ b/jest.config.cjs @@ -1,13 +1,21 @@ +/** @type {import('ts-jest').JestConfigWithTsJest} */ module.exports = { - preset: 'ts-jest', + preset: 'ts-jest/presets/default-esm', testEnvironment: 'node', + extensionsToTreatAsEsm: ['.ts'], roots: ['/src', '/tests'], testMatch: [ '**/__tests__/**/*.ts', '**/?(*.)+(spec|test).ts' ], + // Source files import each other with the ESM-required '.js' suffix while the files + // on disk are '.ts' — strip the suffix so jest resolves them. + moduleNameMapper: { + '^(\\.{1,2}/.*)\\.js$': '$1', + }, transform: { '^.+\\.ts$': ['ts-jest', { + useESM: true, tsconfig: 'tsconfig.test.json' }] }, @@ -17,6 +25,8 @@ module.exports = { collectCoverageFrom: [ 'src/**/*.ts', '!src/**/*.d.ts', + // src/index.ts is a listen-only shim after the buildServer extraction; the real + // bootstrap logic it used to hold now lives in src/app.ts and IS measured. '!src/index.ts' ], coverageDirectory: 'coverage', @@ -27,4 +37,4 @@ module.exports = { clearMocks: true, resetMocks: true, restoreMocks: true -}; \ No newline at end of file +}; diff --git a/package-lock.json b/package-lock.json index a0a74d1..df5606c 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1428,6 +1428,109 @@ "dev": true, "license": "BSD-3-Clause" }, + "node_modules/@isaacs/cliui": { + "version": "8.0.2", + "resolved": "https://registry.npmjs.org/@isaacs/cliui/-/cliui-8.0.2.tgz", + "integrity": "sha512-O8jcjabXaleOG9DQ0+ARXWZBTfnP4WNAqzuiJK7ll44AmxGKv/J2M4TPjxjY3znBCfvBXFzucm1twdyFybFqEA==", + "dev": true, + "license": "ISC", + "dependencies": { + "string-width": "^5.1.2", + "string-width-cjs": "npm:string-width@^4.2.0", + "strip-ansi": "^7.0.1", + "strip-ansi-cjs": "npm:strip-ansi@^6.0.1", + "wrap-ansi": "^8.1.0", + "wrap-ansi-cjs": "npm:wrap-ansi@^7.0.0" + }, + "engines": { + "node": ">=12" + } + }, + "node_modules/@isaacs/cliui/node_modules/ansi-regex": { + "version": "6.2.2", + "resolved": "https://registry.npmjs.org/ansi-regex/-/ansi-regex-6.2.2.tgz", + "integrity": "sha512-Bq3SmSpyFHaWjPk8If9yc6svM8c56dB5BAtW4Qbw5jHTwwXXcTLoRMkpDJp6VL0XzlWaCHTXrkFURMYmD0sLqg==", + "dev": true, + "license": "MIT", + "engines": { + "node": ">=12" + }, + "funding": { + "url": "https://github.com/chalk/ansi-regex?sponsor=1" + } + }, + "node_modules/@isaacs/cliui/node_modules/ansi-styles": { + "version": "6.2.3", + "resolved": "https://registry.npmjs.org/ansi-styles/-/ansi-styles-6.2.3.tgz", + "integrity": "sha512-4Dj6M28JB+oAH8kFkTLUo+a2jwOFkuqb3yucU0CANcRRUbxS0cP0nZYCGjcc3BNXwRIsUVmDGgzawme7zvJHvg==", + "dev": true, + "license": "MIT", + "engines": { + "node": ">=12" + }, + "funding": { + "url": "https://github.com/chalk/ansi-styles?sponsor=1" + } + }, + "node_modules/@isaacs/cliui/node_modules/emoji-regex": { + "version": "9.2.2", + "resolved": "https://registry.npmjs.org/emoji-regex/-/emoji-regex-9.2.2.tgz", + "integrity": "sha512-L18DaJsXSUk2+42pv8mLs5jJT2hqFkFE4j21wOmgbUqsZ2hL72NsUU785g9RXgo3s0ZNgVl42TiHp3ZtOv/Vyg==", + "dev": true, + "license": "MIT" + }, + "node_modules/@isaacs/cliui/node_modules/string-width": { + "version": "5.1.2", + "resolved": "https://registry.npmjs.org/string-width/-/string-width-5.1.2.tgz", + "integrity": "sha512-HnLOCR3vjcY8beoNLtcjZ5/nxn2afmME6lhrDrebokqMap+XbeW8n9TXpPDOqdGK5qcI3oT0GKTW6wC7EMiVqA==", + "dev": true, + "license": "MIT", + "dependencies": { + "eastasianwidth": "^0.2.0", + "emoji-regex": "^9.2.2", + "strip-ansi": "^7.0.1" + }, + "engines": { + "node": ">=12" + }, + "funding": { + "url": "https://github.com/sponsors/sindresorhus" + } + }, + "node_modules/@isaacs/cliui/node_modules/strip-ansi": { + "version": "7.2.0", + "resolved": "https://registry.npmjs.org/strip-ansi/-/strip-ansi-7.2.0.tgz", + "integrity": "sha512-yDPMNjp4WyfYBkHnjIRLfca1i6KMyGCtsVgoKe/z1+6vukgaENdgGBZt+ZmKPc4gavvEZ5OgHfHdrazhgNyG7w==", + "dev": true, + "license": "MIT", + "dependencies": { + "ansi-regex": "^6.2.2" + }, + "engines": { + "node": ">=12" + }, + "funding": { + "url": "https://github.com/chalk/strip-ansi?sponsor=1" + } + }, + "node_modules/@isaacs/cliui/node_modules/wrap-ansi": { + "version": "8.1.0", + "resolved": "https://registry.npmjs.org/wrap-ansi/-/wrap-ansi-8.1.0.tgz", + "integrity": "sha512-si7QWI6zUMq56bESFvagtmzMdGOtoxfR+Sez11Mobfc7tm+VkUckk9bW2UeffTGVUbOksxmSw0AA2gs8g71NCQ==", + "dev": true, + "license": "MIT", + "dependencies": { + "ansi-styles": "^6.1.0", + "string-width": "^5.0.1", + "strip-ansi": "^7.0.1" + }, + "engines": { + "node": ">=12" + }, + "funding": { + "url": "https://github.com/chalk/wrap-ansi?sponsor=1" + } + }, "node_modules/@istanbuljs/load-nyc-config": { "version": "1.1.0", "resolved": "https://registry.npmjs.org/@istanbuljs/load-nyc-config/-/load-nyc-config-1.1.0.tgz", @@ -2169,6 +2272,17 @@ "integrity": "sha512-k2ENnmBugE/rzQfEcdWHcCY+/FM3VLzH9cYEsbdsoqrvzAKRhUZeRNhAZvB8OitQJ1TBed3yqWtdjzS6wJKBwg==", "license": "MIT" }, + "node_modules/@pkgjs/parseargs": { + "version": "0.11.0", + "resolved": "https://registry.npmjs.org/@pkgjs/parseargs/-/parseargs-0.11.0.tgz", + "integrity": "sha512-+1VkjdD0QBLPodGrJUeqarH8VAIvQODIbwh9XpP5Syisf7YoQgsJKPNFoqqLQlu+VQ/tVSshMR6loPMn8U+dPg==", + "dev": true, + "license": "MIT", + "optional": true, + "engines": { + "node": ">=14" + } + }, "node_modules/@pkgr/core": { "version": "0.2.9", "resolved": "https://registry.npmjs.org/@pkgr/core/-/core-0.2.9.tgz", @@ -3692,6 +3806,13 @@ "node": ">= 0.4" } }, + "node_modules/eastasianwidth": { + "version": "0.2.0", + "resolved": "https://registry.npmjs.org/eastasianwidth/-/eastasianwidth-0.2.0.tgz", + "integrity": "sha512-I88TYZWc9XiYHRQ4/3c5rjjfgkjhLyW2luGIheGERbNQ6OY7yTybanSpDXZa8y7VUP9YmDcYa+eyq4ca7iLqWA==", + "dev": true, + "license": "MIT" + }, "node_modules/ee-first": { "version": "1.1.1", "resolved": "https://registry.npmjs.org/ee-first/-/ee-first-1.1.1.tgz", @@ -4606,6 +4727,36 @@ } } }, + "node_modules/foreground-child": { + "version": "3.3.1", + "resolved": "https://registry.npmjs.org/foreground-child/-/foreground-child-3.3.1.tgz", + "integrity": "sha512-gIXjKqtFuWEgzFRJA9WCQeSJLZDjgJUOMCMzxtvFq/37KojM1BFGufqsCy0r4qSQmYLsZYMeyRqzIWOMup03sw==", + "dev": true, + "license": "ISC", + "dependencies": { + "cross-spawn": "^7.0.6", + "signal-exit": "^4.0.1" + }, + "engines": { + "node": ">=14" + }, + "funding": { + "url": "https://github.com/sponsors/isaacs" + } + }, + "node_modules/foreground-child/node_modules/signal-exit": { + "version": "4.1.0", + "resolved": "https://registry.npmjs.org/signal-exit/-/signal-exit-4.1.0.tgz", + "integrity": "sha512-bzyZ1e88w9O1iNJbKnOlvYTrWPDl46O1bG0D3XInv+9tkPrxrN8jUUTiFlDkkmKWgn1M6CfIA13SuGqOa9Korw==", + "dev": true, + "license": "ISC", + "engines": { + "node": ">=14" + }, + "funding": { + "url": "https://github.com/sponsors/isaacs" + } + }, "node_modules/form-data": { "version": "4.0.5", "resolved": "https://registry.npmjs.org/form-data/-/form-data-4.0.5.tgz", @@ -5331,6 +5482,22 @@ "node": ">=8" } }, + "node_modules/jackspeak": { + "version": "3.4.3", + "resolved": "https://registry.npmjs.org/jackspeak/-/jackspeak-3.4.3.tgz", + "integrity": "sha512-OGlZQpz2yfahA/Rd1Y8Cd9SIEsqvXkLVoSw/cgwhnhFMDbsQFeZYoJJ7bIZBS9BcamUW96asq/npPWugM+RQBw==", + "dev": true, + "license": "BlueOak-1.0.0", + "dependencies": { + "@isaacs/cliui": "^8.0.2" + }, + "funding": { + "url": "https://github.com/sponsors/isaacs" + }, + "optionalDependencies": { + "@pkgjs/parseargs": "^0.11.0" + } + }, "node_modules/jest": { "version": "29.7.0", "resolved": "https://registry.npmjs.org/jest/-/jest-29.7.0.tgz", @@ -6611,6 +6778,13 @@ "node": ">=6" } }, + "node_modules/package-json-from-dist": { + "version": "1.0.1", + "resolved": "https://registry.npmjs.org/package-json-from-dist/-/package-json-from-dist-1.0.1.tgz", + "integrity": "sha512-UEZIS3/by4OC8vL3P2dTXRETpebLI2NiI5vIrjaD/5UtrkFX/tNbwjTSRAGC/+7CAo2pIcBaRgWmcBBHcsaCIw==", + "dev": true, + "license": "BlueOak-1.0.0" + }, "node_modules/parent-module": { "version": "1.0.1", "resolved": "https://registry.npmjs.org/parent-module/-/parent-module-1.0.1.tgz", @@ -7742,6 +7916,22 @@ "node": ">=8" } }, + "node_modules/string-width-cjs": { + "name": "string-width", + "version": "4.2.3", + "resolved": "https://registry.npmjs.org/string-width/-/string-width-4.2.3.tgz", + "integrity": "sha512-wKyQRQpjJ0sIp62ErSZdGsjMJWsap5oRNihHhu6G7JVO/9jIB6UyevL+tXuOqrng8j/cxKTWyWUwvSTriiZz/g==", + "dev": true, + "license": "MIT", + "dependencies": { + "emoji-regex": "^8.0.0", + "is-fullwidth-code-point": "^3.0.0", + "strip-ansi": "^6.0.1" + }, + "engines": { + "node": ">=8" + } + }, "node_modules/strip-ansi": { "version": "6.0.1", "resolved": "https://registry.npmjs.org/strip-ansi/-/strip-ansi-6.0.1.tgz", @@ -7755,6 +7945,20 @@ "node": ">=8" } }, + "node_modules/strip-ansi-cjs": { + "name": "strip-ansi", + "version": "6.0.1", + "resolved": "https://registry.npmjs.org/strip-ansi/-/strip-ansi-6.0.1.tgz", + "integrity": "sha512-Y38VPSHcqkFrCpFnQ9vuSXmquuv5oXOKpGeT6aGrr3o3Gc9AlVa6JBfUSOCnbxGGZF+/0ooI7KrPuUSztUdU5A==", + "dev": true, + "license": "MIT", + "dependencies": { + "ansi-regex": "^5.0.1" + }, + "engines": { + "node": ">=8" + } + }, "node_modules/strip-bom": { "version": "4.0.0", "resolved": "https://registry.npmjs.org/strip-bom/-/strip-bom-4.0.0.tgz", @@ -7889,37 +8093,61 @@ } }, "node_modules/test-exclude": { - "version": "6.0.0", - "resolved": "https://registry.npmjs.org/test-exclude/-/test-exclude-6.0.0.tgz", - "integrity": "sha512-cAGWPIyOHU6zlmg88jwm7VRyXnMN7iV68OGAbYDk/Mh/xC/pzVPlQtY6ngoIH/5/tciuhGfvESU8GrHrcxD56w==", + "version": "7.0.2", + "resolved": "https://registry.npmjs.org/test-exclude/-/test-exclude-7.0.2.tgz", + "integrity": "sha512-u9E6A+ZDYdp7a4WnarkXPZOx8Ilz46+kby6p1yZ8zsGTz9gYa6FIS7lj2oezzNKmtdyyJNNmmXDppga5GB7kSw==", "dev": true, "license": "ISC", "dependencies": { "@istanbuljs/schema": "^0.1.2", - "glob": "^7.1.4", - "minimatch": "^3.0.4" + "glob": "^10.4.1", + "minimatch": "^10.2.2" }, "engines": { - "node": ">=8" + "node": ">=18" } }, "node_modules/test-exclude/node_modules/glob": { - "version": "7.2.3", - "resolved": "https://registry.npmjs.org/glob/-/glob-7.2.3.tgz", - "integrity": "sha512-nFR0zLpU2YCaRxwoCJvL6UvCH2JFyFVIvwTLsIf21AuHlMskA1hhTdk+LlYJtOlYt9v6dvszD2BGRqBL+iQK9Q==", + "version": "10.5.0", + "resolved": "https://registry.npmjs.org/glob/-/glob-10.5.0.tgz", + "integrity": "sha512-DfXN8DfhJ7NH3Oe7cFmu3NCu1wKbkReJ8TorzSAFbSKrlNaQSKfIzqYqVY8zlbs2NLBbWpRiU52GX2PbaBVNkg==", "deprecated": "Old versions of glob are not supported, and contain widely publicized security vulnerabilities, which have been fixed in the current version. Please update. Support for old versions may be purchased (at exorbitant rates) by contacting i@izs.me", "dev": true, "license": "ISC", "dependencies": { - "fs.realpath": "^1.0.0", - "inflight": "^1.0.4", - "inherits": "2", - "minimatch": "^3.1.1", - "once": "^1.3.0", - "path-is-absolute": "^1.0.0" + "foreground-child": "^3.1.0", + "jackspeak": "^3.1.2", + "minimatch": "^9.0.4", + "minipass": "^7.1.2", + "package-json-from-dist": "^1.0.0", + "path-scurry": "^1.11.1" + }, + "bin": { + "glob": "dist/esm/bin.mjs" + }, + "funding": { + "url": "https://github.com/sponsors/isaacs" + } + }, + "node_modules/test-exclude/node_modules/lru-cache": { + "version": "10.4.3", + "resolved": "https://registry.npmjs.org/lru-cache/-/lru-cache-10.4.3.tgz", + "integrity": "sha512-JNAzZcXrCt42VGLuYz0zfAzDfAvJWW6AfYlDBQyDV5DClI2m5sAmK+OIO7s59XfsRsWHp02jAJrRadPRGTt6SQ==", + "dev": true, + "license": "ISC" + }, + "node_modules/test-exclude/node_modules/path-scurry": { + "version": "1.11.1", + "resolved": "https://registry.npmjs.org/path-scurry/-/path-scurry-1.11.1.tgz", + "integrity": "sha512-Xa4Nw17FS9ApQFJ9umLiJS4orGjm7ZzwUrwamcGQuHSzDyth9boKDaycYdDcZDuqYATXw4HFXgaqWTctW/v1HA==", + "dev": true, + "license": "BlueOak-1.0.0", + "dependencies": { + "lru-cache": "^10.2.0", + "minipass": "^5.0.0 || ^6.0.2 || ^7.0.0" }, "engines": { - "node": "*" + "node": ">=16 || 14 >=14.18" }, "funding": { "url": "https://github.com/sponsors/isaacs" @@ -8363,6 +8591,25 @@ "url": "https://github.com/chalk/wrap-ansi?sponsor=1" } }, + "node_modules/wrap-ansi-cjs": { + "name": "wrap-ansi", + "version": "7.0.0", + "resolved": "https://registry.npmjs.org/wrap-ansi/-/wrap-ansi-7.0.0.tgz", + "integrity": "sha512-YVGIj2kamLSTxw6NsZjoBxfSwsn0ycdesmc4p+Q21c5zPuZ1pl+NfxVdxPtdHvmNVOQ6XSYG4AUtyt/Fi7D16Q==", + "dev": true, + "license": "MIT", + "dependencies": { + "ansi-styles": "^4.0.0", + "string-width": "^4.1.0", + "strip-ansi": "^6.0.0" + }, + "engines": { + "node": ">=10" + }, + "funding": { + "url": "https://github.com/chalk/wrap-ansi?sponsor=1" + } + }, "node_modules/wrappy": { "version": "1.0.2", "resolved": "https://registry.npmjs.org/wrappy/-/wrappy-1.0.2.tgz", diff --git a/package.json b/package.json index fd51e19..66f3170 100644 --- a/package.json +++ b/package.json @@ -11,12 +11,11 @@ "build": "tsc -p tsconfig.json", "start": "node dist/index.js", "dev": "tsx watch src/index.ts", - "test": "jest", - "test:watch": "jest --watch", - "test:coverage": "jest --coverage", - "test:verbose": "jest --verbose", - "test:config": "jest tests/config.test.ts", - "test:github": "jest tests/githubSync.test.ts", + "test": "node --experimental-vm-modules node_modules/jest/bin/jest.js", + "test:watch": "node --experimental-vm-modules node_modules/jest/bin/jest.js --watch", + "test:coverage": "node --experimental-vm-modules node_modules/jest/bin/jest.js --coverage", + "test:verbose": "node --experimental-vm-modules node_modules/jest/bin/jest.js --verbose", + "test:config": "node --experimental-vm-modules node_modules/jest/bin/jest.js tests/config.test.ts", "lint": "eslint . --ext .ts", "format": "prettier --write \"src/**/*.ts\"" }, @@ -51,6 +50,7 @@ }, "overrides": { "minimatch": "^9.0.7", - "@fastify/static": "9.1.1" + "@fastify/static": "9.1.1", + "test-exclude": "^7.0.1" } } diff --git a/src/app.ts b/src/app.ts new file mode 100644 index 0000000..899b0e9 --- /dev/null +++ b/src/app.ts @@ -0,0 +1,74 @@ +import { fastify, FastifyInstance, FastifyServerOptions } from 'fastify'; +import cors from '@fastify/cors'; +import rateLimit from '@fastify/rate-limit'; +import swagger from '@fastify/swagger'; +import swaggerUi from '@fastify/swagger-ui'; +import 'dotenv/config'; + +import { registerRoutes } from './routes/index.js'; +import { config } from './config.js'; + +export interface BuildServerOptions { + logger?: FastifyServerOptions['logger']; +} + +const defaultLogger: FastifyServerOptions['logger'] = { + transport: { + target: 'pino-pretty', + options: { + translateTime: 'HH:MM:ss Z', + ignore: 'pid,hostname', + }, + }, +}; + +/** + * Builds a fully configured server without binding a port, so tests can drive it + * through fastify's `.inject()` instead of standing up a real listener. + */ +export const buildServer = (options: BuildServerOptions = {}): FastifyInstance => { + const server: FastifyInstance = fastify({ + logger: options.logger === undefined ? defaultLogger : options.logger, + }); + + // Register plugins + server.register(cors, { + origin: true, + credentials: true, + }); + + // Register rate limiting to prevent resource exhaustion + server.register(rateLimit, { + max: config.contentstack.rateLimit?.max || 100, // Maximum 100 requests per window + timeWindow: config.contentstack.rateLimit?.timeWindow || '1 minute', + errorResponseBuilder: () => ({ + statusCode: 429, + error: 'Too Many Requests', + message: 'Rate limit exceeded. Please try again later.', + }), + }); + + // Register Swagger + server.register(swagger, { + swagger: { + info: { + title: 'Shopify Live Preview API', + description: 'API documentation for Shopify Live Preview Middleware', + version: '1.0.0', + }, + host: 'localhost:3002', + schemes: ['http'], + consumes: ['application/json'], + produces: ['application/json'], + }, + }); + + server.register(swaggerUi, { + routePrefix: '/documentation', + }); + + // Register routes + registerRoutes(server); + + return server; +}; diff --git a/src/index.ts b/src/index.ts index 414db04..03c787f 100644 --- a/src/index.ts +++ b/src/index.ts @@ -1,63 +1,7 @@ -import { fastify, FastifyInstance } from 'fastify'; -import cors from '@fastify/cors'; -import rateLimit from '@fastify/rate-limit'; -import swagger from '@fastify/swagger'; -import swaggerUi from '@fastify/swagger-ui'; -import 'dotenv/config'; - -import { registerRoutes } from './routes/index.js'; +import { buildServer } from './app.js'; import { config } from './config.js'; -const server: FastifyInstance = fastify({ - logger: { - transport: { - target: 'pino-pretty', - options: { - translateTime: 'HH:MM:ss Z', - ignore: 'pid,hostname', - }, - }, - }, -}); - -// Register plugins -server.register(cors, { - origin: true, - credentials: true, -}); - -// Register rate limiting to prevent resource exhaustion -server.register(rateLimit, { - max: config.contentstack.rateLimit?.max || 100, // Maximum 100 requests per window - timeWindow: config.contentstack.rateLimit?.timeWindow || '1 minute', - errorResponseBuilder: () => ({ - statusCode: 429, - error: 'Too Many Requests', - message: 'Rate limit exceeded. Please try again later.', - }), -}); - -// Register Swagger -server.register(swagger, { - swagger: { - info: { - title: 'Shopify Live Preview API', - description: 'API documentation for Shopify Live Preview Middleware', - version: '1.0.0', - }, - host: 'localhost:3002', - schemes: ['http'], - consumes: ['application/json'], - produces: ['application/json'], - }, -}); - -server.register(swaggerUi, { - routePrefix: '/documentation', -}); - -// Register routes -registerRoutes(server); +const server = buildServer(); const start = async () => { try { @@ -69,4 +13,4 @@ const start = async () => { } }; -start(); \ No newline at end of file +start(); diff --git a/tests/config.defaults.test.ts b/tests/config.defaults.test.ts new file mode 100644 index 0000000..07fd930 --- /dev/null +++ b/tests/config.defaults.test.ts @@ -0,0 +1,58 @@ +// Its own file on purpose: config.ts reads process.env once at import, and native ESM +// has no registry reset, so one env permutation per file is the isolation boundary. +// Here every var config.ts looks at is REMOVED, to pin the hardcoded fallbacks. +const originalEnv = { ...process.env }; + +delete process.env.PORT; +delete process.env.HOST; +delete process.env.NODE_ENV; +delete process.env.CONTENTSTACK_DELIVERY_TOKEN; +delete process.env.CONTENTSTACK_PREVIEW_TOKEN; +delete process.env.CONTENTSTACK_ENVIRONMENT; +delete process.env.CONTENTSTACK_API_KEY; +delete process.env.CONTENTSTACK_PREVIEW_URL; +delete process.env.RATE_LIMIT_MAX; +delete process.env.RATE_LIMIT_TIME_WINDOW; + +let config: any; + +beforeAll(async () => { + ({ config } = await import('../src/config.js')); +}); + +afterAll(() => { + process.env = originalEnv; +}); + +describe('Config (no env set)', () => { + it('defaults port to 3002', () => { + expect(config.port).toBe(3002); + }); + + it('defaults host to 0.0.0.0', () => { + expect(config.host).toBe('0.0.0.0'); + }); + + it('defaults nodeEnv to development', () => { + expect(config.nodeEnv).toBe('development'); + }); + + it('defaults the preview URL to the hosted REST preview host', () => { + expect(config.contentstack.previewUrl).toBe('https://rest-preview.contentstack.com'); + }); + + it('defaults the rate limit to 100 requests per minute', () => { + expect(config.contentstack.rateLimit).toEqual({ max: 100, timeWindow: '1 minute' }); + }); + + // Documents the empty-string fallback: config.ts does NOT throw on missing + // credentials. The failure surfaces later, when ContentstackService is constructed. + it('falls back to empty strings for missing contentstack credentials', () => { + expect(config.contentstack).toMatchObject({ + deliveryToken: '', + previewToken: '', + environment: '', + apiKey: '', + }); + }); +}); diff --git a/tests/config.invalidNumbers.test.ts b/tests/config.invalidNumbers.test.ts new file mode 100644 index 0000000..a13d7b6 --- /dev/null +++ b/tests/config.invalidNumbers.test.ts @@ -0,0 +1,29 @@ +// Own file per the one-permutation-per-file rule (see config.defaults.test.ts). +// Permutation: both numeric env vars set to unparseable strings. +const originalEnv = { ...process.env }; + +process.env.PORT = 'abc'; +process.env.RATE_LIMIT_MAX = 'abc'; + +let config: any; + +beforeAll(async () => { + ({ config } = await import('../src/config.js')); +}); + +afterAll(() => { + process.env = originalEnv; +}); + +describe('Config (unparseable numeric env values)', () => { + // parseInt('abc') is NaN and there is no `|| 3002` guard on port, unlike rateLimit.max. + // Documented, not endorsed: a typo'd PORT yields NaN rather than the default. + it('yields NaN for an unparseable PORT rather than falling back to 3002', () => { + expect(config.port).toBeNaN(); + }); + + // rateLimit.max DOES have a `|| 100` guard, so NaN is caught here. + it('falls back to a rate limit of 100 for an unparseable RATE_LIMIT_MAX', () => { + expect(config.contentstack.rateLimit.max).toBe(100); + }); +}); diff --git a/tests/config.oversizedRateLimit.test.ts b/tests/config.oversizedRateLimit.test.ts new file mode 100644 index 0000000..e0d474c --- /dev/null +++ b/tests/config.oversizedRateLimit.test.ts @@ -0,0 +1,23 @@ +// Own file per the one-permutation-per-file rule (see config.defaults.test.ts). +// Permutation: a rate limit far above any sane request volume. +const originalEnv = { ...process.env }; + +process.env.RATE_LIMIT_MAX = '999999999999'; + +let config: any; + +beforeAll(async () => { + ({ config } = await import('../src/config.js')); +}); + +afterAll(() => { + process.env = originalEnv; +}); + +describe('Config (oversized rate limit)', () => { + // Documented, not endorsed: there is no upper clamp, so an oversized max passes + // straight through and effectively disables rate limiting. + it('accepts an oversized RATE_LIMIT_MAX verbatim with no upper clamp', () => { + expect(config.contentstack.rateLimit.max).toBe(999999999999); + }); +}); diff --git a/tests/config.rateLimit.test.ts b/tests/config.rateLimit.test.ts new file mode 100644 index 0000000..ebb834f --- /dev/null +++ b/tests/config.rateLimit.test.ts @@ -0,0 +1,28 @@ +// Own file per the one-permutation-per-file rule (see config.defaults.test.ts). +// Permutation: a negative rate limit plus a custom time window. +const originalEnv = { ...process.env }; + +process.env.RATE_LIMIT_MAX = '-5'; +process.env.RATE_LIMIT_TIME_WINDOW = '30 seconds'; + +let config: any; + +beforeAll(async () => { + ({ config } = await import('../src/config.js')); +}); + +afterAll(() => { + process.env = originalEnv; +}); + +describe('Config (negative rate limit)', () => { + // Documented, not endorsed: -5 is truthy, so the `|| 100` guard does not catch it and + // a negative max reaches @fastify/rate-limit as-is. No lower bound exists today. + it('accepts a negative RATE_LIMIT_MAX verbatim', () => { + expect(config.contentstack.rateLimit.max).toBe(-5); + }); + + it('reads a custom time window from the environment', () => { + expect(config.contentstack.rateLimit.timeWindow).toBe('30 seconds'); + }); +}); diff --git a/tests/config.test.ts b/tests/config.test.ts index e33a8ff..2d7c550 100644 --- a/tests/config.test.ts +++ b/tests/config.test.ts @@ -1,34 +1,43 @@ -import { config } from '../src/config'; +import { config } from '../src/config.js'; -describe('Config', () => { - const originalEnv = process.env; +// This file covers the DEFAULT test env — the values tests/setup.ts installs. +// No per-case module reset here: config.ts reads env once at import, and under native +// ESM there is no registry reset that would re-run it. Anything needing a different +// env permutation lives in its own file (config.defaults / config.rateLimit / +// config.invalidNumbers / config.oversizedRateLimit). +describe('Config (default test env)', () => { + it('reads port from the environment as a number', () => { + expect(typeof config.port).toBe('number'); + expect(config.port).toBe(3003); // set by tests/setup.ts + }); - beforeEach(() => { - jest.resetModules(); - process.env = { ...originalEnv }; + it('parses port into a valid TCP range', () => { + expect(config.port).toBeGreaterThan(0); + expect(config.port).toBeLessThan(65536); }); - afterAll(() => { - process.env = originalEnv; + it('reads host from the environment', () => { + expect(config.host).toBe('localhost'); // set by tests/setup.ts }); - it('should have default port value', () => { - expect(config.port).toBeDefined(); - expect(typeof config.port).toBe('number'); + it('reads nodeEnv from the environment', () => { + expect(config.nodeEnv).toBe('test'); // set by tests/setup.ts }); - it('should have default host value', () => { - expect(config.host).toBeDefined(); - expect(typeof config.host).toBe('string'); + it('exposes the contentstack credentials the controller needs at import', () => { + expect(config.contentstack).toMatchObject({ + deliveryToken: 'test-delivery-token', + previewToken: 'test-preview-token', + environment: 'test-environment', + apiKey: 'test-api-key', + }); }); - it('should have default nodeEnv value', () => { - expect(config.nodeEnv).toBeDefined(); - expect(typeof config.nodeEnv).toBe('string'); + it('falls back to the hosted preview URL when CONTENTSTACK_PREVIEW_URL is unset', () => { + expect(config.contentstack.previewUrl).toBe('https://rest-preview.contentstack.com'); }); - it('should parse port as number', () => { - expect(config.port).toBeGreaterThan(0); - expect(config.port).toBeLessThan(65536); + it('falls back to a rate limit of 100 per minute', () => { + expect(config.contentstack.rateLimit).toEqual({ max: 100, timeWindow: '1 minute' }); }); -}); \ No newline at end of file +}); diff --git a/tests/environment.test.ts b/tests/environment.test.ts deleted file mode 100644 index ffc55e8..0000000 --- a/tests/environment.test.ts +++ /dev/null @@ -1,78 +0,0 @@ -describe('Environment Utilities', () => { - describe('Port parsing', () => { - it('should parse valid port numbers', () => { - const parsePort = (portStr, defaultPort) => { - if (!portStr) return defaultPort; - const parsed = parseInt(portStr, 10); - return isNaN(parsed) ? defaultPort : parsed; - }; - - expect(parsePort('3000', 8080)).toBe(3000); - expect(parsePort('8080', 3000)).toBe(8080); - expect(parsePort('80', 3000)).toBe(80); - expect(parsePort('65535', 3000)).toBe(65535); - }); - - it('should handle invalid port values', () => { - const parsePort = (portStr, defaultPort) => { - if (!portStr) return defaultPort; - const parsed = parseInt(portStr, 10); - return isNaN(parsed) ? defaultPort : parsed; - }; - - expect(parsePort('invalid', 8080)).toBe(8080); - expect(parsePort('', 8080)).toBe(8080); - expect(parsePort(undefined, 8080)).toBe(8080); - expect(parsePort(null, 8080)).toBe(8080); - expect(parsePort('abc123', 8080)).toBe(8080); - }); - - it('should handle edge cases', () => { - const parsePort = (portStr, defaultPort) => { - if (!portStr) return defaultPort; - const parsed = parseInt(portStr, 10); - return isNaN(parsed) ? defaultPort : parsed; - }; - - expect(parsePort('0', 8080)).toBe(0); - expect(parsePort('-1', 8080)).toBe(-1); - expect(parsePort('99999', 8080)).toBe(99999); - }); - }); - - describe('String validation', () => { - it('should validate non-empty strings', () => { - const isValidString = (str) => { - return typeof str === 'string' && str.length > 0; - }; - - expect(isValidString('valid')).toBe(true); - expect(isValidString('localhost')).toBe(true); - expect(isValidString('0.0.0.0')).toBe(true); - }); - - it('should reject invalid strings', () => { - const isValidString = (str) => { - return typeof str === 'string' && str.length > 0; - }; - - expect(isValidString('')).toBe(false); - expect(isValidString(null)).toBe(false); - expect(isValidString(undefined)).toBe(false); - expect(isValidString(123)).toBe(false); - }); - }); - - describe('Environment defaults', () => { - it('should provide sensible defaults', () => { - const getConfigValue = (envValue, defaultValue) => { - return envValue || defaultValue; - }; - - expect(getConfigValue(undefined, 'development')).toBe('development'); - expect(getConfigValue('', 'localhost')).toBe('localhost'); - expect(getConfigValue('production', 'development')).toBe('production'); - expect(getConfigValue('127.0.0.1', '0.0.0.0')).toBe('127.0.0.1'); - }); - }); -}); \ No newline at end of file diff --git a/tests/getPreviewData.test.ts b/tests/getPreviewData.test.ts new file mode 100644 index 0000000..544ecaf --- /dev/null +++ b/tests/getPreviewData.test.ts @@ -0,0 +1,300 @@ +import { jest } from '@jest/globals'; +import { LivePreviewShopify } from '@contentstack/shopify-live-preview-sdk'; +import type { FastifyInstance } from 'fastify'; +import { buildServer } from '../src/app.js'; + +// The controller captured this exact instance and its liquid engine at import time. +// getInstance() must be called with NO config here: passing one re-runs initialize(), +// which swaps in a fresh Liquid engine and would orphan the reference the controller +// already holds, making renderFile spies invisible to the handler. +const livePreviewShopify = LivePreviewShopify.getInstance(); +const engine = livePreviewShopify.getLiquidEngine(); + +// Captured before any spying so the passthrough below calls the real implementation. +const realCreateContentTypeKeyBased = LivePreviewShopify.prototype.createContentTypeKeyBased; + +// CDA include_schema=true returns `schema` as the field array itself — never wrapped in +// another array. Pinning that shape is the point of the regression case below. +const previewSchemaFixture = [ + { uid: 'title', data_type: 'text', display_name: 'Title' }, + { uid: 'description', data_type: 'text', display_name: 'Description' }, +]; + +const previewEntryFixture = { + uid: 'entry_123', + title: 'Updated title', + description: 'Updated description', +}; + +const buildPreviewRequestBody = (overrides: Record = {}) => ({ + live_preview: 'hash_abc123', + ctUid: 'product_ct', + entryUid: 'entry_123', + locale: 'en-us', + theme_variable: { + liquid_path: 'sections.product-template', + data_cslp: 'product_ct.entry_123.en-us.title', + payload: {}, + }, + ...overrides, +}); + +describe('POST /get-preview-data', () => { + let app: FastifyInstance; + let fetchDataSpy: any; + let keyBasedSpy: any; + let renderFileSpy: any; + let updatedMetafieldsSpy: any; + let updatedMetaobjectSpy: any; + + beforeEach(async () => { + app = buildServer({ logger: false }); + await app.ready(); + + // jest.config.cjs sets restoreMocks/resetMocks/clearMocks, so every spy has to be + // re-established here rather than once at suite level. + fetchDataSpy = jest + .spyOn(livePreviewShopify, 'fetchData') + .mockResolvedValue({ schema: previewSchemaFixture, entry: previewEntryFixture } as never); + + // Passthrough: keeps the real key-based map (so downstream branches get a real + // keyBasedCt) while still recording the argument it was handed. + keyBasedSpy = jest + .spyOn(livePreviewShopify, 'createContentTypeKeyBased') + .mockImplementation(((schema: any) => + realCreateContentTypeKeyBased.call(livePreviewShopify, schema)) as never); + + renderFileSpy = jest.spyOn(engine, 'renderFile').mockResolvedValue('
rendered
' as never); + + updatedMetafieldsSpy = jest + .spyOn(livePreviewShopify, 'getUpdatedProductMetafields') + .mockResolvedValue({ updated: 'metafields' } as never); + + updatedMetaobjectSpy = jest + .spyOn(livePreviewShopify, 'getUpdatedMetaobject') + .mockResolvedValue({ currentMetaobjects: { updated: 'metaobject' } } as never); + }); + + afterEach(async () => { + await app.close(); + }); + + const post = (payload: unknown) => + app.inject({ method: 'POST', url: '/get-preview-data', payload: payload as any }); + + describe('happy path', () => { + it('renders the liquid path with the payload and returns html', async () => { + const res = await post(buildPreviewRequestBody()); + + expect(res.statusCode).toBe(200); + expect(res.json()).toEqual({ html: '
rendered
' }); + // dots in liquid_path become slashes before hitting the engine + expect(renderFileSpy).toHaveBeenCalledWith('sections/product-template', {}); + }); + + it('forwards ctUid, entryUid, hash and locale to fetchData', async () => { + await post(buildPreviewRequestBody()); + + expect(fetchDataSpy).toHaveBeenCalledWith('product_ct', 'entry_123', 'hash_abc123', 'en-us'); + }); + + it('skips both enrichment branches when payload has no product or metaobjects', async () => { + await post(buildPreviewRequestBody()); + + expect(updatedMetafieldsSpy).not.toHaveBeenCalled(); + expect(updatedMetaobjectSpy).not.toHaveBeenCalled(); + }); + + it('still returns 200 when the CDA returns an empty schema array', async () => { + fetchDataSpy.mockResolvedValue({ schema: [], entry: previewEntryFixture } as never); + + const res = await post(buildPreviewRequestBody()); + + expect(res.statusCode).toBe(200); + expect(keyBasedSpy).toHaveBeenCalledWith([]); + }); + }); + + describe('regression: CDA schema is passed through unwrapped', () => { + it('calls createContentTypeKeyBased with the field array itself, not a wrapped array', async () => { + await post(buildPreviewRequestBody()); + + expect(keyBasedSpy).toHaveBeenCalledWith(previewSchemaFixture); + + // Guards the exact bug that was fixed: the argument must be the field array, so + // its first element is a field object — not a nested array. + const [passedSchema] = keyBasedSpy.mock.calls[0]; + expect(Array.isArray(passedSchema)).toBe(true); + expect(Array.isArray(passedSchema[0])).toBe(false); + expect(passedSchema[0]).toHaveProperty('uid', 'title'); + }); + }); + + describe('product metafields branch', () => { + const productBody = () => + buildPreviewRequestBody({ + theme_variable: { + liquid_path: 'sections.product-template', + data_cslp: 'product_ct.entry_123.en-us.title', + payload: { + product: { metafields: { contentstack_products: { existing: 'metafield' } } }, + }, + }, + }); + + it('calls getUpdatedProductMetafields with the current metafields, entry and ids', async () => { + await post(productBody()); + + expect(updatedMetafieldsSpy).toHaveBeenCalledTimes(1); + const [currentMetafields, keyBasedCt, entry, options] = updatedMetafieldsSpy.mock.calls[0]; + expect(currentMetafields).toEqual({ existing: 'metafield' }); + expect(entry).toEqual(previewEntryFixture); + expect(options).toEqual({ + ctUid: 'product_ct', + entryUid: 'entry_123', + hash: 'hash_abc123', + }); + // built by the real createContentTypeKeyBased, keyed by field uid + expect(keyBasedCt).toHaveProperty('title'); + }); + + it('assigns the resolved metafields back onto the render data', async () => { + await post(productBody()); + + const [, renderData] = renderFileSpy.mock.calls[0]; + expect(renderData.product.metafields.contentstack_products).toEqual({ updated: 'metafields' }); + }); + }); + + describe('metaobjects branch', () => { + const metaobjectBody = () => + buildPreviewRequestBody({ + theme_variable: { + liquid_path: 'sections.product-template', + data_cslp: 'product_ct.entry_123.en-us.title', + payload: { metaobjects: { existing: 'metaobject' } }, + }, + }); + + it('calls getUpdatedMetaobject with a copy of the current metaobjects and ids', async () => { + await post(metaobjectBody()); + + expect(updatedMetaobjectSpy).toHaveBeenCalledTimes(1); + const [currentMetaobjects, , entry, options] = updatedMetaobjectSpy.mock.calls[0]; + expect(currentMetaobjects).toEqual({ existing: 'metaobject' }); + expect(entry).toEqual(previewEntryFixture); + expect(options).toEqual({ ctUid: 'product_ct', hash: 'hash_abc123' }); + }); + + it('assigns result.currentMetaobjects back onto the render data', async () => { + await post(metaobjectBody()); + + const [, renderData] = renderFileSpy.mock.calls[0]; + expect(renderData.metaobjects).toEqual({ updated: 'metaobject' }); + }); + }); + + describe('schema-layer validation (400s)', () => { + it.each([ + ['live_preview', { live_preview: undefined }], + ['ctUid', { ctUid: undefined }], + ['entryUid', { entryUid: undefined }], + ['theme_variable', { theme_variable: undefined }], + ])('rejects a body missing %s with 400', async (_field, override) => { + const body: any = buildPreviewRequestBody(override as Record); + Object.keys(override as Record).forEach((k) => delete body[k]); + + const res = await post(body); + + expect(res.statusCode).toBe(400); + }); + + it.each([ + ['liquid_path', 'liquid_path'], + ['data_cslp', 'data_cslp'], + ['payload', 'payload'], + ])('rejects a theme_variable missing %s with 400', async (_name, key) => { + const body: any = buildPreviewRequestBody(); + delete body.theme_variable[key]; + + const res = await post(body); + + expect(res.statusCode).toBe(400); + }); + + it.each([ + ['an object', {}], + ['an array', []], + ])('rejects liquid_path given %s with 400', async (_name, value) => { + const body: any = buildPreviewRequestBody(); + body.theme_variable.liquid_path = value; + + const res = await post(body); + + expect(res.statusCode).toBe(400); + expect(renderFileSpy).not.toHaveBeenCalled(); + }); + + // Documents Ajv coerceTypes (a fastify default): a numeric liquid_path is coerced to + // a string at the schema layer, so the handler's own typeof guard never sees it. + it('coerces a numeric liquid_path to a string and renders it', async () => { + const body: any = buildPreviewRequestBody(); + body.theme_variable.liquid_path = 123; + + const res = await post(body); + + expect(res.statusCode).toBe(200); + expect(renderFileSpy).toHaveBeenCalledWith('123', {}); + }); + }); + + describe('failure paths', () => { + it('returns 500 with the render error message when renderFile rejects', async () => { + renderFileSpy.mockRejectedValue(new Error('template blew up') as never); + + const res = await post(buildPreviewRequestBody()); + + expect(res.statusCode).toBe(500); + expect(res.json()).toEqual({ message: 'Error rendering liquid file' }); + }); + + it('returns 500 when the upstream fetchData rejects', async () => { + fetchDataSpy.mockRejectedValue(new Error('CDA unreachable') as never); + + const res = await post(buildPreviewRequestBody()); + + expect(res.statusCode).toBe(500); + expect(renderFileSpy).not.toHaveBeenCalled(); + }); + }); + + describe('adversarial liquid_path', () => { + // Runs against the REAL engine (no renderFile spy) so the traversal attempt is + // resolved by liquidjs itself rather than by a stub. + it('does not leak file contents for a path-traversal attempt', async () => { + renderFileSpy.mockRestore(); + + const body: any = buildPreviewRequestBody(); + body.theme_variable.liquid_path = 'x/../../../../etc/passwd'; + + const res = await post(body); + + expect(res.statusCode).toBe(500); + expect(res.json()).toEqual({ message: 'Error rendering liquid file' }); + expect(res.body).not.toContain('root:'); + }); + }); + + describe('unguarded locale pass-through', () => { + // locale is absent from the route schema, so it reaches fetchData unvalidated. + it('passes undefined to fetchData when locale is omitted', async () => { + const body: any = buildPreviewRequestBody(); + delete body.locale; + + const res = await post(body); + + expect(res.statusCode).toBe(200); + expect(fetchDataSpy).toHaveBeenCalledWith('product_ct', 'entry_123', 'hash_abc123', undefined); + }); + }); +}); diff --git a/tests/githubSync.test.ts b/tests/githubSync.test.ts deleted file mode 100644 index e5fc822..0000000 --- a/tests/githubSync.test.ts +++ /dev/null @@ -1,93 +0,0 @@ -describe('GitHub Sync Logic', () => { - describe('Repository name validation', () => { - it('should validate correct repository name format', () => { - const validateRepoName = (repoName) => { - if (typeof repoName !== 'string' || repoName.length === 0) { - return false; - } - const slashIndex = repoName.indexOf('/'); - return slashIndex > 0 && slashIndex < repoName.length - 1; - }; - - expect(validateRepoName('owner/repo')).toBe(true); - expect(validateRepoName('facebook/react')).toBe(true); - expect(validateRepoName('microsoft/typescript')).toBe(true); - }); - - it('should reject invalid repository name formats', () => { - const validateRepoName = (repoName) => { - if (typeof repoName !== 'string' || repoName.length === 0) { - return false; - } - const slashIndex = repoName.indexOf('/'); - return slashIndex > 0 && slashIndex < repoName.length - 1; - }; - - expect(validateRepoName('invalid-format')).toBe(false); - expect(validateRepoName('')).toBe(false); - expect(validateRepoName('/')).toBe(false); - expect(validateRepoName('owner/')).toBe(false); - expect(validateRepoName('/repo')).toBe(false); - expect(validateRepoName(null)).toBe(false); - expect(validateRepoName(undefined)).toBe(false); - }); - }); - - describe('Repository name parsing', () => { - it('should parse repository name correctly', () => { - const parseRepoName = (repoName) => { - if (typeof repoName !== 'string' || repoName.length === 0) { - return { owner: undefined, repo: undefined }; - } - const slashIndex = repoName.indexOf('/'); - if (slashIndex > 0 && slashIndex < repoName.length - 1) { - return { - owner: repoName.substring(0, slashIndex), - repo: repoName.substring(slashIndex + 1) - }; - } - return { owner: undefined, repo: undefined }; - }; - - expect(parseRepoName('owner/repo')).toEqual({ owner: 'owner', repo: 'repo' }); - expect(parseRepoName('facebook/react')).toEqual({ owner: 'facebook', repo: 'react' }); - expect(parseRepoName('owner/repo/extra')).toEqual({ owner: 'owner', repo: 'repo/extra' }); - }); - - it('should handle invalid repository names', () => { - const parseRepoName = (repoName) => { - if (typeof repoName !== 'string' || repoName.length === 0) { - return { owner: undefined, repo: undefined }; - } - const slashIndex = repoName.indexOf('/'); - if (slashIndex > 0 && slashIndex < repoName.length - 1) { - return { - owner: repoName.substring(0, slashIndex), - repo: repoName.substring(slashIndex + 1) - }; - } - return { owner: undefined, repo: undefined }; - }; - - expect(parseRepoName('invalid')).toEqual({ owner: undefined, repo: undefined }); - expect(parseRepoName('')).toEqual({ owner: undefined, repo: undefined }); - expect(parseRepoName('/')).toEqual({ owner: undefined, repo: undefined }); - }); - }); - - describe('Error handling', () => { - it('should create appropriate error messages', () => { - const createErrorMessage = (error) => { - if (error instanceof Error) { - return `Failed to clone repository: ${error.message}`; - } - return 'Failed to clone repository: Unknown error'; - }; - - const testError = new Error('Authentication failed'); - expect(createErrorMessage(testError)).toBe('Failed to clone repository: Authentication failed'); - expect(createErrorMessage('string error')).toBe('Failed to clone repository: Unknown error'); - expect(createErrorMessage(null)).toBe('Failed to clone repository: Unknown error'); - }); - }); -}); \ No newline at end of file diff --git a/tests/setup.ts b/tests/setup.ts index 71e06e8..a8a98b7 100644 --- a/tests/setup.ts +++ b/tests/setup.ts @@ -3,4 +3,11 @@ // Mock environment variables for testing process.env.NODE_ENV = 'test'; process.env.PORT = '3003'; -process.env.HOST = 'localhost'; \ No newline at end of file +process.env.HOST = 'localhost'; + +// The controller constructs a ContentstackService at import time, and that constructor +// throws on any empty credential — so these must be set before any suite imports the app. +process.env.CONTENTSTACK_DELIVERY_TOKEN = 'test-delivery-token'; +process.env.CONTENTSTACK_PREVIEW_TOKEN = 'test-preview-token'; +process.env.CONTENTSTACK_ENVIRONMENT = 'test-environment'; +process.env.CONTENTSTACK_API_KEY = 'test-api-key'; diff --git a/tsconfig.test.json b/tsconfig.test.json index a6906a7..48567cb 100644 --- a/tsconfig.test.json +++ b/tsconfig.test.json @@ -1,7 +1,7 @@ { "extends": "./tsconfig.json", "compilerOptions": { - "module": "CommonJS", + "module": "ESNext", "target": "ES2020", "esModuleInterop": true, "allowSyntheticDefaultImports": true, From f354933ccde83d021d38c57bf2038f1377f16dbd Mon Sep 17 00:00:00 2001 From: Mridul Sharma Date: Wed, 5 Aug 2026 16:33:39 +0530 Subject: [PATCH 2/2] test: consolidate five config test files into one (VP-2467) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- tests/README.md | 26 ++-- tests/config.defaults.test.ts | 58 -------- tests/config.invalidNumbers.test.ts | 29 ---- tests/config.oversizedRateLimit.test.ts | 23 --- tests/config.rateLimit.test.ts | 28 ---- tests/config.test.ts | 190 ++++++++++++++++++++---- 6 files changed, 176 insertions(+), 178 deletions(-) delete mode 100644 tests/config.defaults.test.ts delete mode 100644 tests/config.invalidNumbers.test.ts delete mode 100644 tests/config.oversizedRateLimit.test.ts delete mode 100644 tests/config.rateLimit.test.ts diff --git a/tests/README.md b/tests/README.md index d4af918..692decd 100644 --- a/tests/README.md +++ b/tests/README.md @@ -4,10 +4,9 @@ This directory contains the test suite for the Shopify Live Preview Middleware s ## Test Structure -- `config.test.ts` - Configuration module tests -- `githubSync.test.ts` - GitHub sync logic tests -- `environment.test.ts` - Environment utilities tests -- `setup.ts` - Global test setup +- `config.test.ts` - Configuration module tests, one `describe` per env permutation +- `getPreviewData.test.ts` - `getPreviewData` handler tests, driven through `fastify.inject()` +- `setup.ts` - Global test setup (installs the env the controller needs at import time) ## Running Tests @@ -19,15 +18,14 @@ This directory contains the test suite for the Shopify Live Preview Middleware s ### Specific Tests - `npm run test:config` - Run config tests only -- `npm run test:github` - Run GitHub sync tests only ## Current Coverage -✅ **Configuration Module** - Fully tested -✅ **GitHub Sync Logic** - Business logic tested -✅ **Environment Utilities** - Utility functions tested +✅ **Configuration Module** - Fully covered, including the undefended edge cases +✅ **getPreviewData handler** - Success, validation and error paths covered +✅ **Server bootstrap** (`src/app.ts`) - Covered via `buildServer()` -⚠️ **Controllers & Routes** - Need integration testing +⚠️ **githubSyncController / viewsHealthController** - Not yet covered ## Test Patterns @@ -37,8 +35,16 @@ Tests follow Jest conventions with describe/it blocks and focus on: - Business logic - Edge cases +Two conventions worth knowing before adding tests here: + +- **Native ESM.** The `jest` object is not injected as a global, so anything needing + `jest.resetModules()` / `jest.fn()` must `import { jest } from '@jest/globals'` first. +- **Module-level env reads.** `src/config.ts` snapshots `process.env` at import time, so a test + that needs a different env has to `jest.resetModules()` and re-`import()` the module rather + than mutate `process.env` and expect the existing `config` object to change. + ## Configuration - Jest config: `jest.config.cjs` - TypeScript config: `tsconfig.test.json` -- Test setup: `tests/setup.ts` \ No newline at end of file +- Test setup: `tests/setup.ts` diff --git a/tests/config.defaults.test.ts b/tests/config.defaults.test.ts deleted file mode 100644 index 07fd930..0000000 --- a/tests/config.defaults.test.ts +++ /dev/null @@ -1,58 +0,0 @@ -// Its own file on purpose: config.ts reads process.env once at import, and native ESM -// has no registry reset, so one env permutation per file is the isolation boundary. -// Here every var config.ts looks at is REMOVED, to pin the hardcoded fallbacks. -const originalEnv = { ...process.env }; - -delete process.env.PORT; -delete process.env.HOST; -delete process.env.NODE_ENV; -delete process.env.CONTENTSTACK_DELIVERY_TOKEN; -delete process.env.CONTENTSTACK_PREVIEW_TOKEN; -delete process.env.CONTENTSTACK_ENVIRONMENT; -delete process.env.CONTENTSTACK_API_KEY; -delete process.env.CONTENTSTACK_PREVIEW_URL; -delete process.env.RATE_LIMIT_MAX; -delete process.env.RATE_LIMIT_TIME_WINDOW; - -let config: any; - -beforeAll(async () => { - ({ config } = await import('../src/config.js')); -}); - -afterAll(() => { - process.env = originalEnv; -}); - -describe('Config (no env set)', () => { - it('defaults port to 3002', () => { - expect(config.port).toBe(3002); - }); - - it('defaults host to 0.0.0.0', () => { - expect(config.host).toBe('0.0.0.0'); - }); - - it('defaults nodeEnv to development', () => { - expect(config.nodeEnv).toBe('development'); - }); - - it('defaults the preview URL to the hosted REST preview host', () => { - expect(config.contentstack.previewUrl).toBe('https://rest-preview.contentstack.com'); - }); - - it('defaults the rate limit to 100 requests per minute', () => { - expect(config.contentstack.rateLimit).toEqual({ max: 100, timeWindow: '1 minute' }); - }); - - // Documents the empty-string fallback: config.ts does NOT throw on missing - // credentials. The failure surfaces later, when ContentstackService is constructed. - it('falls back to empty strings for missing contentstack credentials', () => { - expect(config.contentstack).toMatchObject({ - deliveryToken: '', - previewToken: '', - environment: '', - apiKey: '', - }); - }); -}); diff --git a/tests/config.invalidNumbers.test.ts b/tests/config.invalidNumbers.test.ts deleted file mode 100644 index a13d7b6..0000000 --- a/tests/config.invalidNumbers.test.ts +++ /dev/null @@ -1,29 +0,0 @@ -// Own file per the one-permutation-per-file rule (see config.defaults.test.ts). -// Permutation: both numeric env vars set to unparseable strings. -const originalEnv = { ...process.env }; - -process.env.PORT = 'abc'; -process.env.RATE_LIMIT_MAX = 'abc'; - -let config: any; - -beforeAll(async () => { - ({ config } = await import('../src/config.js')); -}); - -afterAll(() => { - process.env = originalEnv; -}); - -describe('Config (unparseable numeric env values)', () => { - // parseInt('abc') is NaN and there is no `|| 3002` guard on port, unlike rateLimit.max. - // Documented, not endorsed: a typo'd PORT yields NaN rather than the default. - it('yields NaN for an unparseable PORT rather than falling back to 3002', () => { - expect(config.port).toBeNaN(); - }); - - // rateLimit.max DOES have a `|| 100` guard, so NaN is caught here. - it('falls back to a rate limit of 100 for an unparseable RATE_LIMIT_MAX', () => { - expect(config.contentstack.rateLimit.max).toBe(100); - }); -}); diff --git a/tests/config.oversizedRateLimit.test.ts b/tests/config.oversizedRateLimit.test.ts deleted file mode 100644 index e0d474c..0000000 --- a/tests/config.oversizedRateLimit.test.ts +++ /dev/null @@ -1,23 +0,0 @@ -// Own file per the one-permutation-per-file rule (see config.defaults.test.ts). -// Permutation: a rate limit far above any sane request volume. -const originalEnv = { ...process.env }; - -process.env.RATE_LIMIT_MAX = '999999999999'; - -let config: any; - -beforeAll(async () => { - ({ config } = await import('../src/config.js')); -}); - -afterAll(() => { - process.env = originalEnv; -}); - -describe('Config (oversized rate limit)', () => { - // Documented, not endorsed: there is no upper clamp, so an oversized max passes - // straight through and effectively disables rate limiting. - it('accepts an oversized RATE_LIMIT_MAX verbatim with no upper clamp', () => { - expect(config.contentstack.rateLimit.max).toBe(999999999999); - }); -}); diff --git a/tests/config.rateLimit.test.ts b/tests/config.rateLimit.test.ts deleted file mode 100644 index ebb834f..0000000 --- a/tests/config.rateLimit.test.ts +++ /dev/null @@ -1,28 +0,0 @@ -// Own file per the one-permutation-per-file rule (see config.defaults.test.ts). -// Permutation: a negative rate limit plus a custom time window. -const originalEnv = { ...process.env }; - -process.env.RATE_LIMIT_MAX = '-5'; -process.env.RATE_LIMIT_TIME_WINDOW = '30 seconds'; - -let config: any; - -beforeAll(async () => { - ({ config } = await import('../src/config.js')); -}); - -afterAll(() => { - process.env = originalEnv; -}); - -describe('Config (negative rate limit)', () => { - // Documented, not endorsed: -5 is truthy, so the `|| 100` guard does not catch it and - // a negative max reaches @fastify/rate-limit as-is. No lower bound exists today. - it('accepts a negative RATE_LIMIT_MAX verbatim', () => { - expect(config.contentstack.rateLimit.max).toBe(-5); - }); - - it('reads a custom time window from the environment', () => { - expect(config.contentstack.rateLimit.timeWindow).toBe('30 seconds'); - }); -}); diff --git a/tests/config.test.ts b/tests/config.test.ts index 2d7c550..6d5a3ed 100644 --- a/tests/config.test.ts +++ b/tests/config.test.ts @@ -1,43 +1,173 @@ -import { config } from '../src/config.js'; - -// This file covers the DEFAULT test env — the values tests/setup.ts installs. -// No per-case module reset here: config.ts reads env once at import, and under native -// ESM there is no registry reset that would re-run it. Anything needing a different -// env permutation lives in its own file (config.defaults / config.rateLimit / -// config.invalidNumbers / config.oversizedRateLimit). -describe('Config (default test env)', () => { - it('reads port from the environment as a number', () => { - expect(typeof config.port).toBe('number'); - expect(config.port).toBe(3003); // set by tests/setup.ts - }); +import { jest } from '@jest/globals'; - it('parses port into a valid TCP range', () => { - expect(config.port).toBeGreaterThan(0); - expect(config.port).toBeLessThan(65536); - }); +// config.ts snapshots process.env once, at import time, so every env permutation needs a +// fresh module instance. resetModules() drops the cached one and the dynamic import below +// re-runs the module body against whatever env the case just installed. +// +// Note the `@jest/globals` import above: under native ESM the `jest` object is not injected +// as a global, so calling jest.resetModules() without it throws "jest is not defined". +const baseEnv = { ...process.env }; + +const CONFIG_ENV_VARS = [ + 'PORT', + 'HOST', + 'NODE_ENV', + 'CONTENTSTACK_DELIVERY_TOKEN', + 'CONTENTSTACK_PREVIEW_TOKEN', + 'CONTENTSTACK_ENVIRONMENT', + 'CONTENTSTACK_API_KEY', + 'CONTENTSTACK_PREVIEW_URL', + 'RATE_LIMIT_MAX', + 'RATE_LIMIT_TIME_WINDOW', +]; + +type LoadedConfig = (typeof import('../src/config.js'))['config']; + +/** Loads config.ts fresh, with `overrides` applied on top of the env tests/setup.ts installs. */ +const loadConfig = async (overrides: Record = {}): Promise => { + process.env = { ...baseEnv, ...overrides }; + jest.resetModules(); + return (await import('../src/config.js')).config; +}; + +/** Loads config.ts fresh with every var it reads removed, to pin the hardcoded fallbacks. */ +const loadConfigWithNoEnv = async (): Promise => { + process.env = { ...baseEnv }; + for (const key of CONFIG_ENV_VARS) delete process.env[key]; + jest.resetModules(); + return (await import('../src/config.js')).config; +}; + +afterAll(() => { + process.env = baseEnv; +}); + +describe('config', () => { + describe('default test env', () => { + let config: LoadedConfig; + beforeAll(async () => { + config = await loadConfig(); + }); + + it('reads port from the environment as a number', () => { + expect(typeof config.port).toBe('number'); + expect(config.port).toBe(3003); // set by tests/setup.ts + }); - it('reads host from the environment', () => { - expect(config.host).toBe('localhost'); // set by tests/setup.ts + it('parses port into a valid TCP range', () => { + expect(config.port).toBeGreaterThan(0); + expect(config.port).toBeLessThan(65536); + }); + + it('reads host from the environment', () => { + expect(config.host).toBe('localhost'); // set by tests/setup.ts + }); + + it('reads nodeEnv from the environment', () => { + expect(config.nodeEnv).toBe('test'); // set by tests/setup.ts + }); + + it('exposes the contentstack credentials the controller needs at import', () => { + expect(config.contentstack).toMatchObject({ + deliveryToken: 'test-delivery-token', + previewToken: 'test-preview-token', + environment: 'test-environment', + apiKey: 'test-api-key', + }); + }); + + it('falls back to the hosted preview URL when CONTENTSTACK_PREVIEW_URL is unset', () => { + expect(config.contentstack.previewUrl).toBe('https://rest-preview.contentstack.com'); + }); + + it('falls back to a rate limit of 100 per minute', () => { + expect(config.contentstack.rateLimit).toEqual({ max: 100, timeWindow: '1 minute' }); + }); }); - it('reads nodeEnv from the environment', () => { - expect(config.nodeEnv).toBe('test'); // set by tests/setup.ts + describe('no env set', () => { + let config: LoadedConfig; + beforeAll(async () => { + config = await loadConfigWithNoEnv(); + }); + + it('defaults port to 3002', () => { + expect(config.port).toBe(3002); + }); + + it('defaults host to 0.0.0.0', () => { + expect(config.host).toBe('0.0.0.0'); + }); + + it('defaults nodeEnv to development', () => { + expect(config.nodeEnv).toBe('development'); + }); + + it('defaults the preview URL to the hosted REST preview host', () => { + expect(config.contentstack.previewUrl).toBe('https://rest-preview.contentstack.com'); + }); + + it('defaults the rate limit to 100 requests per minute', () => { + expect(config.contentstack.rateLimit).toEqual({ max: 100, timeWindow: '1 minute' }); + }); + + // Documents the empty-string fallback: config.ts does NOT throw on missing + // credentials. The failure surfaces later, when ContentstackService is constructed. + it('falls back to empty strings for missing contentstack credentials', () => { + expect(config.contentstack).toMatchObject({ + deliveryToken: '', + previewToken: '', + environment: '', + apiKey: '', + }); + }); }); - it('exposes the contentstack credentials the controller needs at import', () => { - expect(config.contentstack).toMatchObject({ - deliveryToken: 'test-delivery-token', - previewToken: 'test-preview-token', - environment: 'test-environment', - apiKey: 'test-api-key', + describe('unparseable numeric env values', () => { + let config: LoadedConfig; + beforeAll(async () => { + config = await loadConfig({ PORT: 'abc', RATE_LIMIT_MAX: 'abc' }); + }); + + // parseInt('abc') is NaN and there is no `|| 3002` guard on port, unlike rateLimit.max. + // Documented, not endorsed: a typo'd PORT yields NaN rather than the default. + it('yields NaN for an unparseable PORT rather than falling back to 3002', () => { + expect(config.port).toBeNaN(); + }); + + // rateLimit.max DOES have a `|| 100` guard, so NaN is caught here. + it('falls back to a rate limit of 100 for an unparseable RATE_LIMIT_MAX', () => { + expect(config.contentstack.rateLimit?.max).toBe(100); }); }); - it('falls back to the hosted preview URL when CONTENTSTACK_PREVIEW_URL is unset', () => { - expect(config.contentstack.previewUrl).toBe('https://rest-preview.contentstack.com'); + describe('negative rate limit', () => { + let config: LoadedConfig; + beforeAll(async () => { + config = await loadConfig({ RATE_LIMIT_MAX: '-5', RATE_LIMIT_TIME_WINDOW: '30 seconds' }); + }); + + // Documented, not endorsed: -5 is truthy, so the `|| 100` guard does not catch it and + // a negative max reaches @fastify/rate-limit as-is. No lower bound exists today. + it('accepts a negative RATE_LIMIT_MAX verbatim', () => { + expect(config.contentstack.rateLimit?.max).toBe(-5); + }); + + it('reads a custom time window from the environment', () => { + expect(config.contentstack.rateLimit?.timeWindow).toBe('30 seconds'); + }); }); - it('falls back to a rate limit of 100 per minute', () => { - expect(config.contentstack.rateLimit).toEqual({ max: 100, timeWindow: '1 minute' }); + describe('oversized rate limit', () => { + let config: LoadedConfig; + beforeAll(async () => { + config = await loadConfig({ RATE_LIMIT_MAX: '999999999999' }); + }); + + // Documented, not endorsed: there is no upper clamp, so an oversized max passes + // straight through and effectively disables rate limiting. + it('accepts an oversized RATE_LIMIT_MAX verbatim with no upper clamp', () => { + expect(config.contentstack.rateLimit?.max).toBe(999999999999); + }); }); });