diff --git a/src/config/core.js b/src/config/core.js index 155d396..626f397 100644 --- a/src/config/core.js +++ b/src/config/core.js @@ -22,7 +22,7 @@ export const CONFIG_DEFAULTS = { batchSize: 10, timeout: 30000, }, - comparison: { threshold: 2.0 }, + comparison: { threshold: 2.0, minClusterSize: 2 }, tdd: { openReport: false }, plugins: [], }; @@ -81,6 +81,26 @@ export function validateWriteScope(scope) { // Deep Merge // ============================================================================ +function isPlainConfigObject(value) { + return value && typeof value === 'object' && !Array.isArray(value); +} + +function cloneConfigValue(value) { + if (Array.isArray(value)) { + return value.map(cloneConfigValue); + } + + if (isPlainConfigObject(value)) { + let cloned = {}; + for (let key of Object.keys(value)) { + cloned[key] = cloneConfigValue(value[key]); + } + return cloned; + } + + return value; +} + /** * Deep merge two objects * @param {Object} target - Target object @@ -88,25 +108,17 @@ export function validateWriteScope(scope) { * @returns {Object} Merged object (new object, inputs not mutated) */ export function deepMerge(target, source) { - let output = { ...target }; + let output = cloneConfigValue(target); for (let key of Object.keys(source)) { - if ( - source[key] && - typeof source[key] === 'object' && - !Array.isArray(source[key]) - ) { - if ( - target[key] && - typeof target[key] === 'object' && - !Array.isArray(target[key]) - ) { + if (isPlainConfigObject(source[key])) { + if (isPlainConfigObject(target[key])) { output[key] = deepMerge(target[key], source[key]); } else { - output[key] = source[key]; + output[key] = cloneConfigValue(source[key]); } } else { - output[key] = source[key]; + output[key] = cloneConfigValue(source[key]); } } @@ -123,12 +135,22 @@ export function deepMerge(target, source) { * @returns {Object} The value if it's an object, empty object otherwise */ function ensureObject(value) { - if (value && typeof value === 'object' && !Array.isArray(value)) { + if (isPlainConfigObject(value)) { return value; } return {}; } +function applyConfigLayer(config, sources, layer, sourceName) { + let merged = deepMerge(config, layer); + + for (let key of Object.keys(layer)) { + sources[key] = sourceName; + } + + return merged; +} + /** * Build merged config from layers with source tracking * @param {Object} options - Config layers @@ -147,37 +169,31 @@ export function buildMergedConfig({ let safeGlobalConfig = ensureObject(globalConfig); let safeEnvOverrides = ensureObject(envOverrides); - let mergedConfig = {}; + let mergedConfig = cloneConfigValue(CONFIG_DEFAULTS); let sources = {}; - // Layer 1: Defaults for (let key of Object.keys(CONFIG_DEFAULTS)) { - mergedConfig[key] = CONFIG_DEFAULTS[key]; sources[key] = 'default'; } - // Layer 2: Global config (auth, project mappings, user preferences) - if (safeGlobalConfig.auth) { - mergedConfig.auth = safeGlobalConfig.auth; - sources.auth = 'global'; - } - - if (safeGlobalConfig.projects) { - mergedConfig.projects = safeGlobalConfig.projects; - sources.projects = 'global'; - } - - // Layer 3: Project config file - for (let key of Object.keys(safeProjectConfig)) { - mergedConfig[key] = safeProjectConfig[key]; - sources[key] = 'project'; - } - - // Layer 4: Environment variables - for (let key of Object.keys(safeEnvOverrides)) { - mergedConfig[key] = safeEnvOverrides[key]; - sources[key] = 'env'; - } + mergedConfig = applyConfigLayer( + mergedConfig, + sources, + safeGlobalConfig, + 'global' + ); + mergedConfig = applyConfigLayer( + mergedConfig, + sources, + safeProjectConfig, + 'project' + ); + mergedConfig = applyConfigLayer( + mergedConfig, + sources, + safeEnvOverrides, + 'env' + ); return { config: mergedConfig, sources }; } diff --git a/src/services/config-service.js b/src/services/config-service.js index fad122e..f7fbce2 100644 --- a/src/services/config-service.js +++ b/src/services/config-service.js @@ -12,30 +12,10 @@ import { existsSync } from 'node:fs'; import { writeFile } from 'node:fs/promises'; import { join } from 'node:path'; import { cosmiconfigSync } from 'cosmiconfig'; +import { CONFIG_DEFAULTS } from '../config/core.js'; import { loadGlobalConfig, saveGlobalConfig } from '../utils/global-config.js'; import * as output from '../utils/output.js'; -/** - * Default configuration values - */ -let DEFAULT_CONFIG = { - comparison: { - threshold: 2.0, - minClusterSize: 2, - }, - server: { - port: 47392, - timeout: 30000, - }, - build: { - name: 'Build {timestamp}', - environment: 'test', - }, - tdd: { - openReport: false, - }, -}; - /** * Create a config service instance * @param {Object} options @@ -77,7 +57,7 @@ export function createConfigService({ workingDir }) { * Get merged configuration with source tracking */ async function getMergedConfig() { - let config = { ...DEFAULT_CONFIG }; + let config = cloneConfigValue(CONFIG_DEFAULTS); let sources = {}; // Layer 1: Global config @@ -102,18 +82,17 @@ export function createConfigService({ workingDir }) { // Layer 3: Environment variables if (process.env.VIZZLY_THRESHOLD) { - config.comparison.threshold = parseFloat(process.env.VIZZLY_THRESHOLD); + config.comparison.threshold = Number(process.env.VIZZLY_THRESHOLD); sources.comparison = 'env'; } if (process.env.VIZZLY_MIN_CLUSTER_SIZE) { - config.comparison.minClusterSize = parseInt( - process.env.VIZZLY_MIN_CLUSTER_SIZE, - 10 + config.comparison.minClusterSize = Number( + process.env.VIZZLY_MIN_CLUSTER_SIZE ); sources.comparison = 'env'; } if (process.env.VIZZLY_PORT) { - config.server.port = parseInt(process.env.VIZZLY_PORT, 10); + config.server.port = Number(process.env.VIZZLY_PORT); sources.server = 'env'; } @@ -240,7 +219,11 @@ export default defineConfig(${JSON.stringify(newConfig, null, 2)}); // Validate threshold if (config.comparison?.threshold !== undefined) { let threshold = config.comparison.threshold; - if (typeof threshold !== 'number' || threshold < 0) { + if ( + typeof threshold !== 'number' || + !Number.isFinite(threshold) || + threshold < 0 + ) { errors.push('comparison.threshold must be a non-negative number'); } else if (threshold > 100) { warnings.push( @@ -299,7 +282,7 @@ export default defineConfig(${JSON.stringify(newConfig, null, 2)}); * Deep merge two objects */ function mergeDeep(target, source) { - let result = { ...target }; + let result = cloneConfigValue(target); for (let key in source) { if ( @@ -308,6 +291,8 @@ function mergeDeep(target, source) { !Array.isArray(source[key]) ) { result[key] = mergeDeep(result[key] || {}, source[key]); + } else if (Array.isArray(source[key])) { + result[key] = [...source[key]]; } else { result[key] = source[key]; } @@ -316,21 +301,45 @@ function mergeDeep(target, source) { return result; } +function cloneConfigValue(value) { + if (Array.isArray(value)) { + return value.map(cloneConfigValue); + } + + if (value && typeof value === 'object') { + let clone = {}; + for (let key in value) { + clone[key] = cloneConfigValue(value[key]); + } + return clone; + } + + return value; +} + /** * Merge config with source tracking */ -function mergeWithTracking(target, source, sources, sourceName) { +function mergeWithTracking(target, source, sources, sourceName, sectionName) { for (let key in source) { + let sourceSection = sectionName || key; + if ( source[key] && typeof source[key] === 'object' && !Array.isArray(source[key]) ) { if (!target[key]) target[key] = {}; - mergeWithTracking(target[key], source[key], sources, sourceName); + mergeWithTracking( + target[key], + source[key], + sources, + sourceName, + sourceSection + ); } else { target[key] = source[key]; - sources[key] = sourceName; + sources[sourceSection] = sourceName; } } } diff --git a/src/utils/build-history.js b/src/utils/build-history.js deleted file mode 100644 index 19dd8ce..0000000 --- a/src/utils/build-history.js +++ /dev/null @@ -1,124 +0,0 @@ -import { - existsSync, - mkdirSync, - readdirSync, - rmSync, - writeFileSync, -} from 'node:fs'; -import { join } from 'node:path'; - -/** - * Archive a build to history directory - * @param {string} workingDir - Working directory - * @param {string} buildId - Build ID to archive - * @param {Array} builds - Build data - * @param {Array} comparisons - Comparison data - * @param {Object} summary - Summary stats - * @param {number} maxHistory - Maximum number of builds to keep (default: 3) - */ -export function archiveBuild( - workingDir, - buildId, - builds, - comparisons, - summary, - maxHistory = 3 -) { - const historyDir = join(workingDir, '.vizzly', 'history'); - - // Create history directory if it doesn't exist - if (!existsSync(historyDir)) { - mkdirSync(historyDir, { recursive: true }); - } - - // Save current build to history - const buildDir = join(historyDir, buildId); - if (!existsSync(buildDir)) { - mkdirSync(buildDir, { recursive: true }); - } - - const buildData = { - buildId, - timestamp: Date.now(), - builds, - comparisons, - summary, - }; - - writeFileSync( - join(buildDir, 'report.json'), - JSON.stringify(buildData, null, 2) - ); - - // Clean up old builds (keep last N) - cleanupOldBuilds(historyDir, maxHistory); -} - -/** - * Get list of archived builds - * @param {string} workingDir - Working directory - * @returns {Array} Array of build metadata - */ -export function getArchivedBuilds(workingDir) { - const historyDir = join(workingDir, '.vizzly', 'history'); - - if (!existsSync(historyDir)) { - return []; - } - - try { - const buildDirs = readdirSync(historyDir, { withFileTypes: true }) - .filter(dirent => dirent.isDirectory()) - .map(dirent => dirent.name) - .sort() - .reverse(); // Newest first - - return buildDirs - .map(buildId => { - const reportPath = join(historyDir, buildId, 'report.json'); - if (existsSync(reportPath)) { - try { - const data = JSON.parse( - require('node:fs').readFileSync(reportPath, 'utf8') - ); - return { - buildId: data.buildId, - timestamp: data.timestamp, - summary: data.summary, - }; - } catch { - return null; - } - } - return null; - }) - .filter(Boolean); - } catch { - return []; - } -} - -/** - * Remove old builds, keeping only the last N - * @private - */ -function cleanupOldBuilds(historyDir, maxHistory) { - try { - const buildDirs = readdirSync(historyDir, { withFileTypes: true }) - .filter(dirent => dirent.isDirectory()) - .map(dirent => dirent.name) - .sort() - .reverse(); // Newest first - - // Remove builds beyond maxHistory - if (buildDirs.length > maxHistory) { - const toRemove = buildDirs.slice(maxHistory); - toRemove.forEach(buildId => { - const buildDir = join(historyDir, buildId); - rmSync(buildDir, { recursive: true, force: true }); - }); - } - } catch { - // Ignore cleanup errors - } -} diff --git a/src/utils/ci-env.js b/src/utils/ci-env.js index 7777276..9535754 100644 --- a/src/utils/ci-env.js +++ b/src/utils/ci-env.js @@ -147,6 +147,15 @@ export function getCommitMessage() { ); } +function parsePullRequestNumber(value) { + if (!/^\d+$/.test(value)) { + return null; + } + + let number = Number(value); + return Number.isSafeInteger(number) && number > 0 ? number : null; +} + /** * Get the pull request number from CI environment variables * @returns {number|null} PR number or null if not available/not a PR @@ -154,7 +163,7 @@ export function getCommitMessage() { export function getPullRequestNumber() { // Check VIZZLY override first if (process.env.VIZZLY_PR_NUMBER) { - return parseInt(process.env.VIZZLY_PR_NUMBER, 10); + return parsePullRequestNumber(process.env.VIZZLY_PR_NUMBER); } // GitHub Actions - extract from GITHUB_REF @@ -162,19 +171,19 @@ export function getPullRequestNumber() { process.env.GITHUB_ACTIONS && process.env.GITHUB_EVENT_NAME === 'pull_request' ) { - const prMatch = process.env.GITHUB_REF?.match(/refs\/pull\/(\d+)\/merge/); - if (prMatch?.[1]) return parseInt(prMatch[1], 10); + let prMatch = process.env.GITHUB_REF?.match(/refs\/pull\/(\d+)\/merge/); + if (prMatch?.[1]) return parsePullRequestNumber(prMatch[1]); } // GitLab CI if (process.env.CI_MERGE_REQUEST_ID) { - return parseInt(process.env.CI_MERGE_REQUEST_ID, 10); + return parsePullRequestNumber(process.env.CI_MERGE_REQUEST_ID); } // CircleCI - extract from PR URL if (process.env.CIRCLE_PULL_REQUEST) { - const prMatch = process.env.CIRCLE_PULL_REQUEST.match(/\/pull\/(\d+)$/); - if (prMatch?.[1]) return parseInt(prMatch[1], 10); + let prMatch = process.env.CIRCLE_PULL_REQUEST.match(/\/pull\/(\d+)$/); + if (prMatch?.[1]) return parsePullRequestNumber(prMatch[1]); } // Travis CI @@ -182,7 +191,7 @@ export function getPullRequestNumber() { process.env.TRAVIS_PULL_REQUEST && process.env.TRAVIS_PULL_REQUEST !== 'false' ) { - return parseInt(process.env.TRAVIS_PULL_REQUEST, 10); + return parsePullRequestNumber(process.env.TRAVIS_PULL_REQUEST); } // Buildkite @@ -190,27 +199,27 @@ export function getPullRequestNumber() { process.env.BUILDKITE_PULL_REQUEST && process.env.BUILDKITE_PULL_REQUEST !== 'false' ) { - return parseInt(process.env.BUILDKITE_PULL_REQUEST, 10); + return parsePullRequestNumber(process.env.BUILDKITE_PULL_REQUEST); } // Drone CI if (process.env.DRONE_PULL_REQUEST) { - return parseInt(process.env.DRONE_PULL_REQUEST, 10); + return parsePullRequestNumber(process.env.DRONE_PULL_REQUEST); } // Jenkins (GitHub Pull Request Builder plugin) if (process.env.ghprbPullId) { - return parseInt(process.env.ghprbPullId, 10); + return parsePullRequestNumber(process.env.ghprbPullId); } // Azure DevOps if (process.env.SYSTEM_PULLREQUEST_PULLREQUESTID) { - return parseInt(process.env.SYSTEM_PULLREQUEST_PULLREQUESTID, 10); + return parsePullRequestNumber(process.env.SYSTEM_PULLREQUEST_PULLREQUESTID); } // AppVeyor if (process.env.APPVEYOR_PULL_REQUEST_NUMBER) { - return parseInt(process.env.APPVEYOR_PULL_REQUEST_NUMBER, 10); + return parsePullRequestNumber(process.env.APPVEYOR_PULL_REQUEST_NUMBER); } return null; diff --git a/src/utils/config-loader.js b/src/utils/config-loader.js index 211c4f1..6bb63fb 100644 --- a/src/utils/config-loader.js +++ b/src/utils/config-loader.js @@ -1,5 +1,6 @@ import { resolve } from 'node:path'; import { cosmiconfigSync } from 'cosmiconfig'; +import { CONFIG_DEFAULTS, deepMerge } from '../config/core.js'; import { validateVizzlyConfigWithDefaults } from './config-schema.js'; import { getApiToken, @@ -10,48 +11,10 @@ import { import { getAccessToken } from './global-config.js'; import * as output from './output.js'; -const DEFAULT_CONFIG = { - // API Configuration - apiKey: undefined, // Will be set from env, global config, or CLI overrides - apiUrl: getApiUrl(), - - // Server Configuration (for run command) - server: { - port: 47392, - timeout: 30000, - }, - - // Build Configuration - build: { - name: 'Build {timestamp}', - environment: 'test', - }, - - // Upload Configuration (for upload command) - upload: { - screenshotsDir: './screenshots', - batchSize: 10, - timeout: 30000, - }, - - // Comparison Configuration (CIEDE2000 Delta E: 0=exact, 1=JND, 2=recommended) - comparison: { - threshold: 2.0, - }, - - // TDD Configuration - tdd: { - openReport: false, // Whether to auto-open HTML report in browser - }, - - // Plugins - plugins: [], -}; - export async function loadConfig(configPath = null, cliOverrides = {}) { // 1. Load from config file using cosmiconfig - const explorer = cosmiconfigSync('vizzly'); - const result = configPath ? explorer.load(configPath) : explorer.search(); + let explorer = cosmiconfigSync('vizzly'); + let result = configPath ? explorer.load(configPath) : explorer.search(); let fileConfig = {}; if (result?.config) { @@ -60,18 +23,10 @@ export async function loadConfig(configPath = null, cliOverrides = {}) { } // 2. Validate config file using Zod schema - const validatedFileConfig = validateVizzlyConfigWithDefaults(fileConfig); + let validatedFileConfig = validateVizzlyConfigWithDefaults(fileConfig); // Create a proper clone of the default config to avoid shared object references - const config = { - ...DEFAULT_CONFIG, - server: { ...DEFAULT_CONFIG.server }, - build: { ...DEFAULT_CONFIG.build }, - upload: { ...DEFAULT_CONFIG.upload }, - comparison: { ...DEFAULT_CONFIG.comparison }, - tdd: { ...DEFAULT_CONFIG.tdd }, - plugins: [...DEFAULT_CONFIG.plugins], - }; + let config = deepMerge(CONFIG_DEFAULTS, {}); // Merge validated file config mergeConfig(config, validatedFileConfig); @@ -133,21 +88,24 @@ function applyCLIOverrides(config, cliOverrides = {}) { if (cliOverrides.parallelId) config.parallelId = cliOverrides.parallelId; // Server overrides - if (cliOverrides.port) config.server.port = parseInt(cliOverrides.port, 10); + if (cliOverrides.port) config.server.port = Number(cliOverrides.port); if (cliOverrides.timeout) - config.server.timeout = parseInt(cliOverrides.timeout, 10); + config.server.timeout = Number(cliOverrides.timeout); // Upload overrides if (cliOverrides.batchSize !== undefined) { - config.upload.batchSize = parseInt(cliOverrides.batchSize, 10); + config.upload.batchSize = Number(cliOverrides.batchSize); } if (cliOverrides.uploadTimeout !== undefined) { - config.upload.timeout = parseInt(cliOverrides.uploadTimeout, 10); + config.upload.timeout = Number(cliOverrides.uploadTimeout); } // Comparison overrides if (cliOverrides.threshold !== undefined) config.comparison.threshold = cliOverrides.threshold; + if (cliOverrides.minClusterSize !== undefined) { + config.comparison.minClusterSize = Number(cliOverrides.minClusterSize); + } // Baseline overrides if (cliOverrides.baselineBuild) diff --git a/src/utils/config-schema.js b/src/utils/config-schema.js index c223175..9b7043e 100644 --- a/src/utils/config-schema.js +++ b/src/utils/config-schema.js @@ -4,21 +4,22 @@ */ import { z } from 'zod'; +import { CONFIG_DEFAULTS } from '../config/core.js'; /** * Server configuration schema */ const serverSchema = z.object({ - port: z.number().int().positive().default(47392), - timeout: z.number().int().positive().default(30000), + port: z.number().int().positive().default(CONFIG_DEFAULTS.server.port), + timeout: z.number().int().positive().default(CONFIG_DEFAULTS.server.timeout), }); /** * Build configuration schema */ const buildSchema = z.object({ - name: z.string().default('Build {timestamp}'), - environment: z.string().default('test'), + name: z.string().default(CONFIG_DEFAULTS.build.name), + environment: z.string().default(CONFIG_DEFAULTS.build.environment), branch: z.string().optional(), commit: z.string().optional(), message: z.string().optional(), @@ -30,9 +31,13 @@ const buildSchema = z.object({ const uploadSchema = z.object({ screenshotsDir: z .union([z.string(), z.array(z.string())]) - .default('./screenshots'), - batchSize: z.number().int().positive().default(10), - timeout: z.number().int().positive().default(30000), + .default(CONFIG_DEFAULTS.upload.screenshotsDir), + batchSize: z + .number() + .int() + .positive() + .default(CONFIG_DEFAULTS.upload.batchSize), + timeout: z.number().int().positive().default(CONFIG_DEFAULTS.upload.timeout), }); /** @@ -41,15 +46,18 @@ const uploadSchema = z.object({ * minClusterSize: pixels (1 = exact) */ const comparisonSchema = z.object({ - threshold: z.number().min(0).default(2.0), - minClusterSize: z.int().min(1).default(2), + threshold: z.number().min(0).default(CONFIG_DEFAULTS.comparison.threshold), + minClusterSize: z + .int() + .min(1) + .default(CONFIG_DEFAULTS.comparison.minClusterSize), }); /** * TDD configuration schema */ const tddSchema = z.object({ - openReport: z.boolean().default(false), + openReport: z.boolean().default(CONFIG_DEFAULTS.tdd.openReport), }); /** @@ -60,21 +68,14 @@ export const vizzlyConfigSchema = z .object({ // Core Vizzly config apiKey: z.string().optional(), - apiUrl: z.string().url().optional(), - server: serverSchema.default({ port: 47392, timeout: 30000 }), - build: buildSchema.default({ - name: 'Build {timestamp}', - environment: 'test', - }), - upload: uploadSchema.default({ - screenshotsDir: './screenshots', - batchSize: 10, - timeout: 30000, - }), - comparison: comparisonSchema.default({ threshold: 2.0, minClusterSize: 2 }), - tdd: tddSchema.default({ openReport: false }), + apiUrl: z.string().url().default(CONFIG_DEFAULTS.apiUrl), + server: serverSchema.default(CONFIG_DEFAULTS.server), + build: buildSchema.default(CONFIG_DEFAULTS.build), + upload: uploadSchema.default(CONFIG_DEFAULTS.upload), + comparison: comparisonSchema.default(CONFIG_DEFAULTS.comparison), + tdd: tddSchema.default(CONFIG_DEFAULTS.tdd), signatureProperties: z.array(z.string()).default([]), - plugins: z.array(z.string()).default([]), + plugins: z.array(z.string()).default(CONFIG_DEFAULTS.plugins), // Additional optional fields parallelId: z.string().optional(), @@ -85,14 +86,7 @@ export const vizzlyConfigSchema = z allowNoToken: z.boolean().optional(), }) .passthrough() // Allow plugin-specific keys like `staticSite`, `storybook`, etc. - .default({ - server: { port: 47392, timeout: 30000 }, - build: { name: 'Build {timestamp}', environment: 'test' }, - upload: { screenshotsDir: './screenshots', batchSize: 10, timeout: 30000 }, - comparison: { threshold: 2.0, minClusterSize: 2 }, - tdd: { openReport: false }, - plugins: [], - }); + .default(CONFIG_DEFAULTS); /** * Validate Vizzly configuration diff --git a/src/utils/git.js b/src/utils/git.js index 63a7f1f..82b1277 100644 --- a/src/utils/git.js +++ b/src/utils/git.js @@ -1,4 +1,4 @@ -import { exec } from 'node:child_process'; +import { execFile } from 'node:child_process'; import { promisify } from 'node:util'; import { getBranch as getCIBranch, @@ -7,14 +7,16 @@ import { getPullRequestNumber, } from './ci-env.js'; -const execAsync = promisify(exec); +let execFileAsync = promisify(execFile); + +async function runGit(args, cwd = process.cwd()) { + let { stdout } = await execFileAsync('git', args, { cwd }); + return stdout.trim(); +} export async function getCommonAncestor(commit1, commit2, cwd = process.cwd()) { try { - const { stdout } = await execAsync(`git merge-base ${commit1} ${commit2}`, { - cwd, - }); - return stdout.trim(); + return await runGit(['merge-base', commit1, commit2], cwd); } catch { // If merge-base fails (e.g., no common ancestor), return null return null; @@ -23,8 +25,7 @@ export async function getCommonAncestor(commit1, commit2, cwd = process.cwd()) { export async function getCurrentCommitSha(cwd = process.cwd()) { try { - const { stdout } = await execAsync('git rev-parse HEAD', { cwd }); - return stdout.trim(); + return await runGit(['rev-parse', 'HEAD'], cwd); } catch { return null; } @@ -32,10 +33,7 @@ export async function getCurrentCommitSha(cwd = process.cwd()) { export async function getCurrentBranch(cwd = process.cwd()) { try { - const { stdout } = await execAsync('git rev-parse --abbrev-ref HEAD', { - cwd, - }); - return stdout.trim(); + return await runGit(['rev-parse', '--abbrev-ref', 'HEAD'], cwd); } catch { // Fallback strategy: use a simple, non-recursive approach // to avoid circular dependency with getDefaultBranch() @@ -49,24 +47,22 @@ export async function getCurrentBranch(cwd = process.cwd()) { */ async function getCurrentBranchFallback(cwd = process.cwd()) { // Try common default branches in order of likelihood - const commonBranches = ['main', 'master', 'develop', 'dev']; + let commonBranches = ['main', 'master', 'develop', 'dev']; - for (const branch of commonBranches) { + for (let branch of commonBranches) { try { - await execAsync(`git show-ref --verify --quiet refs/heads/${branch}`, { - cwd, - }); + await runGit( + ['show-ref', '--verify', '--quiet', `refs/heads/${branch}`], + cwd + ); return branch; } catch {} } // If none of the common branches exist, try to get any local branch try { - const { stdout } = await execAsync( - 'git branch --format="%(refname:short)"', - { cwd } - ); - const branches = stdout + let stdout = await runGit(['branch', '--format=%(refname:short)'], cwd); + let branches = stdout .trim() .split('\n') .filter(b => b.trim()); @@ -85,33 +81,31 @@ async function getCurrentBranchFallback(cwd = process.cwd()) { export async function getDefaultBranch(cwd = process.cwd()) { try { // Try to get the default branch from remote origin - const { stdout } = await execAsync( - 'git symbolic-ref refs/remotes/origin/HEAD', - { cwd } + let stdout = await runGit( + ['symbolic-ref', 'refs/remotes/origin/HEAD'], + cwd ); - const defaultBranch = stdout.trim().replace('refs/remotes/origin/', ''); + let defaultBranch = stdout.replace('refs/remotes/origin/', ''); return defaultBranch; } catch { try { // Fallback: try to get default branch from git config - const { stdout } = await execAsync( - 'git config --get init.defaultBranch', - { cwd } - ); - return stdout.trim(); + return await runGit(['config', '--get', 'init.defaultBranch'], cwd); } catch { try { // Fallback: check if main exists - await execAsync('git show-ref --verify --quiet refs/heads/main', { - cwd, - }); + await runGit( + ['show-ref', '--verify', '--quiet', 'refs/heads/main'], + cwd + ); return 'main'; } catch { try { // Fallback: check if master exists - await execAsync('git show-ref --verify --quiet refs/heads/master', { - cwd, - }); + await runGit( + ['show-ref', '--verify', '--quiet', 'refs/heads/master'], + cwd + ); return 'master'; } catch { // If we're not in a git repo or no branches exist, return null @@ -124,7 +118,7 @@ export async function getDefaultBranch(cwd = process.cwd()) { } export function generateBuildName() { - const timestamp = new Date().toISOString().replace(/[:.]/g, '-'); + let timestamp = new Date().toISOString().replace(/[:.]/g, '-'); return `Build ${timestamp}`; } @@ -135,8 +129,7 @@ export function generateBuildName() { */ export async function getCommitMessage(cwd = process.cwd()) { try { - const { stdout } = await execAsync('git log -1 --pretty=%B', { cwd }); - return stdout.trim(); + return await runGit(['log', '-1', '--pretty=%B'], cwd); } catch { return null; } @@ -155,7 +148,7 @@ export async function detectCommitMessage( if (override) return override; // Try CI environment variables first - const ciCommitMessage = getCICommitMessage(); + let ciCommitMessage = getCICommitMessage(); if (ciCommitMessage) return ciCommitMessage; // Fallback to regular git log @@ -169,7 +162,7 @@ export async function detectCommitMessage( */ export async function isGitRepository(cwd = process.cwd()) { try { - await execAsync('git rev-parse --git-dir', { cwd }); + await runGit(['rev-parse', '--git-dir'], cwd); return true; } catch { return false; @@ -183,8 +176,8 @@ export async function isGitRepository(cwd = process.cwd()) { */ export async function getGitStatus(cwd = process.cwd()) { try { - const { stdout } = await execAsync('git status --porcelain', { cwd }); - const changes = stdout + let stdout = await runGit(['status', '--porcelain'], cwd); + let changes = stdout .trim() .split('\n') .filter(line => line); @@ -211,11 +204,11 @@ export async function detectBranch(override = null, cwd = process.cwd()) { if (override) return override; // Try CI environment variables first - const ciBranch = getCIBranch(); + let ciBranch = getCIBranch(); if (ciBranch) return ciBranch; // Fallback to git command when no CI environment variables - const currentBranch = await getCurrentBranch(cwd); + let currentBranch = await getCurrentBranch(cwd); return currentBranch || 'unknown'; } @@ -229,7 +222,7 @@ export async function detectCommit(override = null, cwd = process.cwd()) { if (override) return override; // Try CI environment variables first - const ciCommit = getCICommit(); + let ciCommit = getCICommit(); if (ciCommit) return ciCommit; // Fallback to git command when no CI environment variables @@ -248,11 +241,11 @@ export async function generateBuildNameWithGit( ) { if (override) return override; - const branch = await getCurrentBranch(cwd); - const shortSha = await getCurrentCommitSha(cwd); + let branch = await getCurrentBranch(cwd); + let shortSha = await getCurrentCommitSha(cwd); if (branch && shortSha) { - const shortCommit = shortSha.substring(0, 7); + let shortCommit = shortSha.substring(0, 7); return `${branch}-${shortCommit}`; } diff --git a/src/utils/output.js b/src/utils/output.js index 0e12457..fb98516 100644 --- a/src/utils/output.js +++ b/src/utils/output.js @@ -603,15 +603,22 @@ function getComponentColor(component) { return componentColors[component] || colors.brand.info; } +export function normalizeDebugArgs(component, message, data = {}) { + if (typeof message === 'object' || message === undefined) { + return { + component: null, + message: component, + data: message || {}, + }; + } + + return { component, message, data }; +} + export function debug(component, message, data = {}) { if (!shouldLog('debug')) return; - // Handle legacy calls: debug('message') or debug('message', {data}) - if (typeof message === 'object' || message === undefined) { - data = message || {}; - message = component; - component = null; - } + ({ component, message, data } = normalizeDebugArgs(component, message, data)); let elapsed = getElapsedTime(); diff --git a/tests/config/core.test.js b/tests/config/core.test.js index c7b3572..a88f84b 100644 --- a/tests/config/core.test.js +++ b/tests/config/core.test.js @@ -26,6 +26,7 @@ describe('config/core', () => { assert.strictEqual(CONFIG_DEFAULTS.apiUrl, 'https://app.vizzly.dev'); assert.strictEqual(CONFIG_DEFAULTS.server.port, 47392); assert.strictEqual(CONFIG_DEFAULTS.comparison.threshold, 2.0); + assert.strictEqual(CONFIG_DEFAULTS.comparison.minClusterSize, 2); assert.strictEqual(CONFIG_DEFAULTS.tdd.openReport, false); assert.deepStrictEqual(CONFIG_DEFAULTS.plugins, []); }); @@ -116,6 +117,15 @@ describe('config/core', () => { assert.deepStrictEqual(result.plugins, ['c']); }); + it('does not share nested arrays with the source object', () => { + let source = { plugins: ['a'] }; + let result = deepMerge({}, source); + + result.plugins.push('b'); + + assert.deepStrictEqual(source.plugins, ['a']); + }); + it('handles null and undefined in source', () => { let target = { a: 1 }; let source = { b: null, c: undefined }; @@ -162,10 +172,37 @@ describe('config/core', () => { }, }); - assert.deepStrictEqual(config.comparison, { threshold: 5.0 }); + assert.deepStrictEqual(config.comparison, { + threshold: 5.0, + minClusterSize: CONFIG_DEFAULTS.comparison.minClusterSize, + }); assert.strictEqual(sources.comparison, 'project'); }); + it('deep merges partial global and project config with defaults', () => { + let { config, sources } = buildMergedConfig({ + globalConfig: { + server: { timeout: 45000 }, + upload: { batchSize: 25 }, + }, + projectConfig: { + server: { timeout: 60000 }, + }, + }); + + assert.deepStrictEqual(config.server, { + port: CONFIG_DEFAULTS.server.port, + timeout: 60000, + }); + assert.deepStrictEqual(config.upload, { + screenshotsDir: './screenshots', + batchSize: 25, + timeout: CONFIG_DEFAULTS.upload.timeout, + }); + assert.strictEqual(sources.server, 'project'); + assert.strictEqual(sources.upload, 'global'); + }); + it('overlays env overrides on top of project', () => { let { config, sources } = buildMergedConfig({ projectConfig: { apiUrl: 'https://project.example.com' }, @@ -187,7 +224,10 @@ describe('config/core', () => { assert.strictEqual(sources.apiUrl, 'default'); assert.deepStrictEqual(config.auth, { accessToken: 'global_token' }); assert.strictEqual(sources.auth, 'global'); - assert.deepStrictEqual(config.comparison, { threshold: 3.0 }); + assert.deepStrictEqual(config.comparison, { + threshold: 3.0, + minClusterSize: CONFIG_DEFAULTS.comparison.minClusterSize, + }); assert.strictEqual(sources.comparison, 'project'); assert.strictEqual(config.apiKey, 'env_key'); assert.strictEqual(sources.apiKey, 'env'); diff --git a/tests/services/config-service.test.js b/tests/services/config-service.test.js index e52c4e2..7fee709 100644 --- a/tests/services/config-service.test.js +++ b/tests/services/config-service.test.js @@ -77,6 +77,16 @@ describe('services/config-service', () => { assert.strictEqual(config.server.timeout, 30000); }); + it('does not share nested default objects between reads', async () => { + let service = createConfigService({ workingDir: tempDir }); + + let first = await service.getConfig('merged'); + first.config.comparison.threshold = 99; + + let second = await service.getConfig('merged'); + assert.strictEqual(second.config.comparison.threshold, 2.0); + }); + it('merges project config over defaults', async () => { // Create a vizzly.config.js in temp dir writeFileSync( @@ -88,7 +98,24 @@ describe('services/config-service', () => { let { config, sources } = await service.getConfig('merged'); assert.strictEqual(config.comparison.threshold, 5.0); - assert.strictEqual(sources.threshold, 'project'); + assert.strictEqual(sources.comparison, 'project'); + }); + + it('tracks nested global config sources by dashboard section', async () => { + let vizzlyHome = join(tempDir, '.vizzly-home'); + mkdirSync(vizzlyHome, { recursive: true }); + process.env.VIZZLY_HOME = vizzlyHome; + + writeFileSync( + join(vizzlyHome, 'config.json'), + JSON.stringify({ settings: { server: { timeout: 45000 } } }) + ); + + let service = createConfigService({ workingDir: tempDir }); + let { config, sources } = await service.getConfig('merged'); + + assert.strictEqual(config.server.timeout, 45000); + assert.strictEqual(sources.server, 'global'); }); it('applies VIZZLY_MIN_CLUSTER_SIZE env var override', async () => { @@ -116,6 +143,33 @@ describe('services/config-service', () => { assert.strictEqual(config.comparison.minClusterSize, 8); }); + + it('preserves invalid minClusterSize env values for validation', async () => { + process.env.VIZZLY_MIN_CLUSTER_SIZE = '2.5'; + + let service = createConfigService({ workingDir: tempDir }); + let { config } = await service.getConfig('merged'); + let result = await service.validateConfig(config); + + assert.strictEqual(config.comparison.minClusterSize, 2.5); + assert.strictEqual(result.valid, false); + assert.ok(result.errors.some(e => e.includes('minClusterSize'))); + }); + + it('preserves invalid numeric env values for validation', async () => { + process.env.VIZZLY_THRESHOLD = '2abc'; + process.env.VIZZLY_PORT = '47392.5'; + + let service = createConfigService({ workingDir: tempDir }); + let { config } = await service.getConfig('merged'); + let result = await service.validateConfig(config); + + assert.ok(Number.isNaN(config.comparison.threshold)); + assert.strictEqual(config.server.port, 47392.5); + assert.strictEqual(result.valid, false); + assert.ok(result.errors.some(e => e.includes('threshold'))); + assert.ok(result.errors.some(e => e.includes('port'))); + }); }); describe('getConfig - project', () => { diff --git a/tests/unit/ci-env.test.js b/tests/unit/ci-env.test.js index a2e35aa..fcc18e1 100644 --- a/tests/unit/ci-env.test.js +++ b/tests/unit/ci-env.test.js @@ -308,6 +308,13 @@ describe('CI Environment Detection', () => { assert.strictEqual(getPullRequestNumber(), 999); }); + it('should ignore malformed VIZZLY_PR_NUMBER values', () => { + process.env.VIZZLY_PR_NUMBER = '999abc'; + process.env.CI_MERGE_REQUEST_ID = '123'; + + assert.strictEqual(getPullRequestNumber(), null); + }); + it('should return GitHub Actions PR number', () => { process.env.GITHUB_ACTIONS = 'true'; process.env.GITHUB_EVENT_NAME = 'pull_request'; @@ -322,6 +329,12 @@ describe('CI Environment Detection', () => { assert.strictEqual(getPullRequestNumber(), 789); }); + it('should ignore decimal GitLab CI merge request IDs', () => { + process.env.CI_MERGE_REQUEST_ID = '789.5'; + + assert.strictEqual(getPullRequestNumber(), null); + }); + it('should return CircleCI PR number from URL', () => { process.env.CIRCLE_PULL_REQUEST = 'https://github.com/owner/repo/pull/321'; diff --git a/tests/unit/output.test.js b/tests/unit/output.test.js index c6dc3f4..64cab3f 100644 --- a/tests/unit/output.test.js +++ b/tests/unit/output.test.js @@ -7,6 +7,7 @@ import { getLogLevel, info, isVerbose, + normalizeDebugArgs, reset, warn, } from '../../src/utils/output.js'; @@ -232,6 +233,56 @@ describe('Output - Log Levels', () => { }); }); + describe('debug argument normalization', () => { + it('keeps component-scoped debug messages explicit', () => { + assert.deepStrictEqual( + normalizeDebugArgs('server', 'ready on :47392', { port: 47392 }), + { + component: 'server', + message: 'ready on :47392', + data: { port: 47392 }, + } + ); + }); + + it('preserves legacy message-only debug calls', () => { + assert.deepStrictEqual(normalizeDebugArgs('Plugin loaded'), { + component: null, + message: 'Plugin loaded', + data: {}, + }); + }); + + it('preserves legacy message plus data debug calls', () => { + assert.deepStrictEqual( + normalizeDebugArgs('Failed to read cache', { error: 'ENOENT' }), + { + component: null, + message: 'Failed to read cache', + data: { error: 'ENOENT' }, + } + ); + }); + + it('prints legacy debug data in JSON mode without losing fields', () => { + configure({ json: true, logLevel: 'debug' }); + + debug('Failed to read cache', { error: 'ENOENT' }); + + assert.strictEqual(consoleErrorMock.mock.calls.length, 1); + let line = consoleErrorMock.mock.calls[0].arguments[0]; + let data = JSON.parse(line); + assert.match(data.time, /^\d+ms$/); + delete data.time; + assert.deepStrictEqual(data, { + status: 'debug', + component: null, + message: 'Failed to read cache', + error: 'ENOENT', + }); + }); + }); + describe('priority order', () => { it('should prioritize: logLevel > verbose > env var > default', () => { process.env.VIZZLY_LOG_LEVEL = 'warn'; diff --git a/tests/utils/ci-env.test.js b/tests/utils/ci-env.test.js index 7ae0d62..b5f3682 100644 --- a/tests/utils/ci-env.test.js +++ b/tests/utils/ci-env.test.js @@ -225,6 +225,19 @@ describe('utils/ci-env', () => { assert.strictEqual(getPullRequestNumber(), 42); }); + it('returns null for malformed VIZZLY_PR_NUMBER override', () => { + process.env.VIZZLY_PR_NUMBER = '42abc'; + process.env.CI_MERGE_REQUEST_ID = '99'; + + assert.strictEqual(getPullRequestNumber(), null); + }); + + it('returns null for zero PR numbers', () => { + process.env.VIZZLY_PR_NUMBER = '0'; + + assert.strictEqual(getPullRequestNumber(), null); + }); + it('extracts PR number from GitHub Actions GITHUB_REF', () => { process.env.GITHUB_ACTIONS = 'true'; process.env.GITHUB_EVENT_NAME = 'pull_request'; @@ -247,6 +260,12 @@ describe('utils/ci-env', () => { assert.strictEqual(getPullRequestNumber(), 456); }); + it('returns null for malformed CI provider PR numbers', () => { + process.env.CI_MERGE_REQUEST_ID = '456.7'; + + assert.strictEqual(getPullRequestNumber(), null); + }); + it('extracts PR number from CircleCI CIRCLE_PULL_REQUEST URL', () => { process.env.CIRCLE_PULL_REQUEST = 'https://github.com/org/repo/pull/789'; diff --git a/tests/utils/config-loader.test.js b/tests/utils/config-loader.test.js index 93af86f..762160b 100644 --- a/tests/utils/config-loader.test.js +++ b/tests/utils/config-loader.test.js @@ -116,6 +116,7 @@ describe('utils/config-loader', () => { token: 'cli-token', port: '5000', threshold: 0.5, + minClusterSize: '4', buildName: 'My Build', environment: 'production', }); @@ -123,6 +124,7 @@ describe('utils/config-loader', () => { assert.strictEqual(config.apiKey, 'cli-token'); assert.strictEqual(config.server.port, 5000); assert.strictEqual(config.comparison.threshold, 0.5); + assert.strictEqual(config.comparison.minClusterSize, 4); assert.strictEqual(config.build.name, 'My Build'); assert.strictEqual(config.build.environment, 'production'); }); diff --git a/tests/utils/git.test.js b/tests/utils/git.test.js index 2fd206c..6f0b7c8 100644 --- a/tests/utils/git.test.js +++ b/tests/utils/git.test.js @@ -1,5 +1,10 @@ import assert from 'node:assert'; +import { execFile } from 'node:child_process'; +import { access, mkdtemp, rm, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; import { describe, it } from 'node:test'; +import { promisify } from 'node:util'; import { detectBranch, detectCommit, @@ -8,6 +13,7 @@ import { generateBuildName, generateBuildNameWithGit, getCommitMessage, + getCommonAncestor, getCurrentBranch, getCurrentCommitSha, getDefaultBranch, @@ -15,6 +21,29 @@ import { isGitRepository, } from '../../src/utils/git.js'; +let execFileAsync = promisify(execFile); + +async function runGit(cwd, args) { + await execFileAsync('git', args, { cwd }); +} + +async function withGitRepo(testFn) { + let directory = await mkdtemp(join(tmpdir(), 'vizzly-git-')); + + try { + await runGit(directory, ['init']); + await runGit(directory, ['config', 'user.email', 'test@example.com']); + await runGit(directory, ['config', 'user.name', 'Vizzly Test']); + await writeFile(join(directory, 'README.md'), 'hello\n'); + await runGit(directory, ['add', 'README.md']); + await runGit(directory, ['commit', '-m', 'Initial commit']); + + return await testFn(directory); + } finally { + await rm(directory, { recursive: true, force: true }); + } +} + describe('utils/git', () => { describe('generateBuildName', () => { it('generates build name with timestamp', () => { @@ -67,6 +96,23 @@ describe('utils/git', () => { }); }); + describe('getCommonAncestor', () => { + it('does not execute shell metacharacters from commit refs', async () => { + await withGitRepo(async directory => { + let markerPath = join(directory, 'shell-marker'); + + let ancestor = await getCommonAncestor( + `HEAD; touch ${markerPath}`, + 'HEAD', + directory + ); + + assert.strictEqual(ancestor, null); + await assert.rejects(() => access(markerPath)); + }); + }); + }); + describe('getCurrentBranch', () => { it('returns a branch name in git repository', async () => { let branch = await getCurrentBranch();