diff --git a/src/commands/api.js b/src/commands/api.js index 8180718..4fe82e6 100644 --- a/src/commands/api.js +++ b/src/commands/api.js @@ -6,6 +6,132 @@ import { createApiClient as defaultCreateApiClient } from '../api/index.js'; import { loadConfig as defaultLoadConfig } from '../utils/config-loader.js'; import * as defaultOutput from '../utils/output.js'; +let ALLOWED_POST_ENDPOINTS = [ + /^\/api\/sdk\/comparisons\/[^/]+\/approve$/, + /^\/api\/sdk\/comparisons\/[^/]+\/reject$/, + /^\/api\/sdk\/builds\/[^/]+\/comments$/, +]; + +function createApiCommandDeps(deps = {}) { + return { + loadConfig: deps.loadConfig || defaultLoadConfig, + createApiClient: deps.createApiClient || defaultCreateApiClient, + output: deps.output || defaultOutput, + exit: deps.exit || (code => process.exit(code)), + }; +} + +function configureOutput(output, globalOptions) { + output.configure({ + json: globalOptions.json, + verbose: globalOptions.verbose, + color: !globalOptions.noColor, + }); +} + +export function normalizeApiEndpoint(endpoint) { + let normalizedEndpoint = endpoint.startsWith('/') ? endpoint : `/${endpoint}`; + + if (!normalizedEndpoint.startsWith('/api/')) { + normalizedEndpoint = `/api${normalizedEndpoint}`; + } + + return normalizedEndpoint; +} + +export function normalizeApiMethod(method = 'GET') { + return method.toUpperCase(); +} + +export function isAllowedPostEndpoint(endpoint) { + return ALLOWED_POST_ENDPOINTS.some(pattern => pattern.test(endpoint)); +} + +export function parseApiHeaders(headerOption) { + let headers = {}; + let headerList = Array.isArray(headerOption) ? headerOption : [headerOption]; + + for (let header of headerList.filter(Boolean)) { + let [key, ...valueParts] = header.split(':'); + if (key && valueParts.length > 0) { + headers[key.trim()] = valueParts.join(':').trim(); + } + } + + return headers; +} + +export function appendApiQuery(endpoint, queryOption) { + if (!queryOption) { + return endpoint; + } + + let params = new URLSearchParams(); + let queryList = Array.isArray(queryOption) ? queryOption : [queryOption]; + + for (let query of queryList) { + let [key, ...valueParts] = query.split('='); + if (key && valueParts.length > 0) { + params.append(key.trim(), valueParts.join('=').trim()); + } + } + + let queryString = params.toString(); + if (!queryString) { + return endpoint; + } + + return endpoint + (endpoint.includes('?') ? '&' : '?') + queryString; +} + +export function validateApiRequest({ endpoint, method }) { + let errors = []; + + if (method !== 'GET' && method !== 'POST') { + errors.push( + `Method ${method} not allowed. Use GET for queries or POST for approve/reject/comment.` + ); + return errors; + } + + if (method === 'POST' && !isAllowedPostEndpoint(endpoint)) { + errors.push( + `POST not allowed for ${endpoint}. Only approve, reject, and comment endpoints support POST.` + ); + } + + return errors; +} + +export function buildApiRequest({ endpoint, options = {} }) { + let normalizedEndpoint = normalizeApiEndpoint(endpoint); + let method = normalizeApiMethod(options.method || 'GET'); + let errors = validateApiRequest({ endpoint: normalizedEndpoint, method }); + + if (errors.length > 0) { + return { errors, method, normalizedEndpoint, requestOptions: null }; + } + + let headers = parseApiHeaders(options.header); + let requestOptions = { method }; + + if (options.data && method === 'POST') { + headers['Content-Type'] = headers['Content-Type'] || 'application/json'; + requestOptions.body = options.data; + } + + if (Object.keys(headers).length > 0) { + requestOptions.headers = headers; + } + + return { + errors: [], + method, + normalizedEndpoint: appendApiQuery(normalizedEndpoint, options.query), + requestOptions, + }; +} + /** * API command - make raw API requests * @param {string} endpoint - API endpoint (e.g., /sdk/builds) @@ -19,18 +145,12 @@ export async function apiCommand( globalOptions = {}, deps = {} ) { - let { - loadConfig = defaultLoadConfig, - createApiClient = defaultCreateApiClient, - output = defaultOutput, - exit = code => process.exit(code), - } = deps; + let { loadConfig, createApiClient, output, exit } = + createApiCommandDeps(deps); + let displayEndpoint = normalizeApiEndpoint(endpoint); + let displayMethod = normalizeApiMethod(options.method || 'GET'); - output.configure({ - json: globalOptions.json, - verbose: globalOptions.verbose, - color: !globalOptions.noColor, - }); + configureOutput(output, globalOptions); try { // Load configuration @@ -42,84 +162,32 @@ export async function apiCommand( output.error( 'API token required. Use --token or set VIZZLY_TOKEN environment variable' ); + output.cleanup(); exit(1); return; } - // Normalize endpoint - let normalizedEndpoint = endpoint.startsWith('/') - ? endpoint - : `/${endpoint}`; - if (!normalizedEndpoint.startsWith('/api/')) { - normalizedEndpoint = `/api${normalizedEndpoint}`; - } + let { errors, method, normalizedEndpoint, requestOptions } = + buildApiRequest({ endpoint, options }); - // Build request options - let method = (options.method || 'GET').toUpperCase(); + displayEndpoint = normalizedEndpoint; + displayMethod = method; - // Validate method and endpoint combination - if (method === 'POST' && !isAllowedPostEndpoint(normalizedEndpoint)) { - output.error( - `POST not allowed for ${normalizedEndpoint}. Only approve, reject, and comment endpoints support POST.` - ); + if (errors.length > 0) { + output.error(errors[0]); + if (method === 'POST') { + output.hint( + 'Use GET for queries, or use dedicated commands (vizzly approve, vizzly reject, vizzly comment)' + ); + } output.hint( - 'Use GET for queries, or use dedicated commands (vizzly approve, vizzly reject, vizzly comment)' + 'Most raw API use should stay read-only; prefer dedicated commands for mutations.' ); + output.cleanup(); exit(1); return; } - if (method !== 'GET' && method !== 'POST') { - output.error(`Method ${method} not allowed. Use GET for queries.`); - exit(1); - return; - } - - let requestOptions = { method }; - - // Add headers - let headers = {}; - if (options.header) { - let headerList = Array.isArray(options.header) - ? options.header - : [options.header]; - for (let h of headerList) { - let [key, ...valueParts] = h.split(':'); - if (key && valueParts.length > 0) { - headers[key.trim()] = valueParts.join(':').trim(); - } - } - } - - // Add body for POST/PUT/PATCH - if (options.data && ['POST', 'PUT', 'PATCH'].includes(method)) { - headers['Content-Type'] = headers['Content-Type'] || 'application/json'; - requestOptions.body = options.data; - } - - if (Object.keys(headers).length > 0) { - requestOptions.headers = headers; - } - - // Add query parameters - if (options.query) { - let params = new URLSearchParams(); - let queryList = Array.isArray(options.query) - ? options.query - : [options.query]; - for (let q of queryList) { - let [key, ...valueParts] = q.split('='); - if (key && valueParts.length > 0) { - params.append(key.trim(), valueParts.join('=').trim()); - } - } - let queryString = params.toString(); - if (queryString) { - normalizedEndpoint += - (normalizedEndpoint.includes('?') ? '&' : '?') + queryString; - } - } - // Make the request output.startSpinner(`${method} ${normalizedEndpoint}`); @@ -162,8 +230,8 @@ export async function apiCommand( if (globalOptions.json) { output.data({ - endpoint, - method: options.method || 'GET', + endpoint: displayEndpoint, + method: displayMethod, error: { message: error.message, code: error.code, @@ -181,23 +249,6 @@ export async function apiCommand( } } -/** - * Allowed POST endpoints (whitelist for mutations) - * Most mutations should use dedicated commands, but these are allowed for raw API access - */ -const ALLOWED_POST_ENDPOINTS = [ - /^\/api\/sdk\/comparisons\/[^/]+\/approve$/, - /^\/api\/sdk\/comparisons\/[^/]+\/reject$/, - /^\/api\/sdk\/builds\/[^/]+\/comments$/, -]; - -/** - * Check if a POST endpoint is allowed - */ -function isAllowedPostEndpoint(endpoint) { - return ALLOWED_POST_ENDPOINTS.some(pattern => pattern.test(endpoint)); -} - /** * Validate API command options */ @@ -208,15 +259,13 @@ export function validateApiOptions(endpoint, options = {}) { errors.push('Endpoint is required'); } - let method = (options.method || 'GET').toUpperCase(); - - // Only GET is allowed by default - // POST is allowed only for whitelisted endpoints - if (method !== 'GET' && method !== 'POST') { - errors.push( - `Method ${method} not allowed. Use GET for queries or POST for approve/reject/comment.` - ); + if (!endpoint || endpoint.trim() === '') { + return errors; } + let normalizedEndpoint = normalizeApiEndpoint(endpoint); + let method = normalizeApiMethod(options.method || 'GET'); + errors.push(...validateApiRequest({ endpoint: normalizedEndpoint, method })); + return errors; } diff --git a/src/commands/baselines.js b/src/commands/baselines.js index db75187..2e4e9ab 100644 --- a/src/commands/baselines.js +++ b/src/commands/baselines.js @@ -2,15 +2,20 @@ * Baselines command - query local TDD baselines */ -import { existsSync, readdirSync, statSync } from 'node:fs'; +import { + existsSync as defaultExistsSync, + readdirSync as defaultReaddirSync, + statSync as defaultStatSync, +} from 'node:fs'; import { basename, join, resolve } from 'node:path'; -import { loadBaselineMetadata } from '../tdd/metadata/baseline-metadata.js'; +import { loadBaselineMetadata as defaultLoadBaselineMetadata } from '../tdd/metadata/baseline-metadata.js'; import * as defaultOutput from '../utils/output.js'; +import { createWildcardMatcher } from '../utils/patterns.js'; /** * Extract filename from a path */ -function getFilename(screenshot) { +export function getFilename(screenshot) { if (screenshot.filename) return screenshot.filename; if (screenshot.path) return basename(screenshot.path); return null; @@ -19,7 +24,7 @@ function getFilename(screenshot) { /** * Extract viewport from screenshot properties */ -function getViewport(screenshot) { +export function getViewport(screenshot) { if (screenshot.viewport) return screenshot.viewport; if ( screenshot.properties?.viewport_width && @@ -33,6 +38,72 @@ function getViewport(screenshot) { return null; } +export function createBaselineNameMatcher(pattern) { + return createWildcardMatcher(pattern); +} + +export function getBaselineFiles(baselinesDir, fs = {}) { + let { + existsSync = defaultExistsSync, + readdirSync = defaultReaddirSync, + statSync = defaultStatSync, + } = fs; + + if (!existsSync(baselinesDir)) { + return []; + } + + return readdirSync(baselinesDir) + .filter(filename => filename.endsWith('.png')) + .map(filename => { + let filePath = join(baselinesDir, filename); + try { + let stat = statSync(filePath); + return { + filename, + path: filePath, + size: stat.size, + modifiedAt: stat.mtime.toISOString(), + }; + } catch { + // File was deleted between readdir and stat, skip it. + return null; + } + }) + .filter(Boolean); +} + +export function createBaselineList({ + baselinesDir, + baselineFiles, + screenshots, +}) { + return screenshots.map(screenshot => { + let filename = getFilename(screenshot); + let viewport = getViewport(screenshot); + let file = baselineFiles.find(candidate => candidate.filename === filename); + + return { + name: screenshot.name, + signature: screenshot.signature, + filename, + path: + screenshot.path || + file?.path || + getFallbackPath(baselinesDir, filename), + sha256: screenshot.sha256, + viewport, + browser: screenshot.browser || null, + createdAt: screenshot.createdAt, + fileSize: file?.size, + }; + }); +} + +function getFallbackPath(baselinesDir, filename) { + return filename ? join(baselinesDir, filename) : null; +} + /** * Baselines command - list and query local TDD baselines * @param {Object} options - Command options @@ -48,6 +119,10 @@ export async function baselinesCommand( output = defaultOutput, exit = code => process.exit(code), cwd = process.cwd, + existsSync = defaultExistsSync, + loadBaselineMetadata = defaultLoadBaselineMetadata, + readdirSync = defaultReaddirSync, + statSync = defaultStatSync, } = deps; output.configure({ @@ -82,36 +157,17 @@ export async function baselinesCommand( // Load metadata let metadata = loadBaselineMetadata(baselinesDir); let screenshots = metadata?.screenshots || []; - - // Get actual baseline files - let baselineFiles = []; - if (existsSync(baselinesDir)) { - baselineFiles = readdirSync(baselinesDir) - .filter(f => f.endsWith('.png')) - .map(f => { - let filePath = join(baselinesDir, f); - try { - let stat = statSync(filePath); - return { - filename: f, - path: filePath, - size: stat.size, - modifiedAt: stat.mtime.toISOString(), - }; - } catch { - // File was deleted between readdir and stat, skip it - return null; - } - }) - .filter(Boolean); - } + let baselineFiles = getBaselineFiles(baselinesDir, { + existsSync, + readdirSync, + statSync, + }); // Filter by name if provided if (options.name) { - let pattern = options.name.replace(/\*/g, '.*'); - let regex = new RegExp(pattern, 'i'); - screenshots = screenshots.filter(s => regex.test(s.name)); - baselineFiles = baselineFiles.filter(f => regex.test(f.filename)); + let matchesName = createBaselineNameMatcher(options.name); + screenshots = screenshots.filter(s => matchesName(s.name)); + baselineFiles = baselineFiles.filter(f => matchesName(f.filename)); } // Get specific baseline info @@ -141,7 +197,10 @@ export async function baselinesCommand( name: screenshot.name, signature: screenshot.signature, filename, - path: screenshot.path || file?.path || join(baselinesDir, filename), + path: + screenshot.path || + file?.path || + getFallbackPath(baselinesDir, filename), sha256: screenshot.sha256, viewport, browser: screenshot.browser || null, @@ -184,21 +243,10 @@ export async function baselinesCommand( // JSON output for list if (globalOptions.json) { - let baselines = screenshots.map(s => { - let filename = getFilename(s); - let viewport = getViewport(s); - let file = baselineFiles.find(f => f.filename === filename); - return { - name: s.name, - signature: s.signature, - filename, - path: s.path || file?.path || join(baselinesDir, filename), - sha256: s.sha256, - viewport, - browser: s.browser || null, - createdAt: s.createdAt, - fileSize: file?.size, - }; + let baselines = createBaselineList({ + baselinesDir, + baselineFiles, + screenshots, }); output.data({ @@ -237,7 +285,7 @@ export async function baselinesCommand( if (metadata.branch && metadata.branch !== 'local') { output.labelValue('Branch', metadata.branch); } - output.labelValue('Threshold', `${metadata.threshold || 2.0}%`); + output.labelValue('Threshold', `${metadata.threshold ?? 2.0} CIEDE2000`); output.blank(); } @@ -296,7 +344,7 @@ export async function baselinesCommand( /** * Format bytes to human readable */ -function formatBytes(bytes) { +export function formatBytes(bytes) { if (bytes < 1024) return `${bytes} B`; if (bytes < 1024 * 1024) return `${(bytes / 1024).toFixed(1)} KB`; return `${(bytes / (1024 * 1024)).toFixed(1)} MB`; diff --git a/src/commands/builds.js b/src/commands/builds.js index ce3ae3b..58b6085 100644 --- a/src/commands/builds.js +++ b/src/commands/builds.js @@ -403,14 +403,19 @@ export function validateBuildsOptions(options = {}) { let errors = []; if ( - options.limit && - (Number.isNaN(options.limit) || options.limit < 1 || options.limit > 250) + options.limit !== undefined && + (!Number.isInteger(options.limit) || + options.limit < 1 || + options.limit > 250) ) { - errors.push('--limit must be a number between 1 and 250'); + errors.push('--limit must be an integer between 1 and 250'); } - if (options.offset && (Number.isNaN(options.offset) || options.offset < 0)) { - errors.push('--offset must be a non-negative number'); + if ( + options.offset !== undefined && + (!Number.isInteger(options.offset) || options.offset < 0) + ) { + errors.push('--offset must be a non-negative integer'); } return errors; diff --git a/src/commands/comparisons.js b/src/commands/comparisons.js index e145752..dd9a7af 100644 --- a/src/commands/comparisons.js +++ b/src/commands/comparisons.js @@ -10,6 +10,7 @@ import { } from '../api/index.js'; import { loadConfig as defaultLoadConfig } from '../utils/config-loader.js'; import * as defaultOutput from '../utils/output.js'; +import { createWildcardMatcher } from '../utils/patterns.js'; /** * Comparisons command - query comparisons with various filters @@ -91,9 +92,8 @@ export async function comparisonsCommand( // Apply name filter if provided if (options.name) { - let pattern = options.name.replace(/\*/g, '.*'); - let regex = new RegExp(pattern, 'i'); - comparisons = comparisons.filter(c => regex.test(c.name)); + let matchesName = createWildcardMatcher(options.name); + comparisons = comparisons.filter(c => matchesName(c.name)); } if (globalOptions.json) { @@ -257,8 +257,6 @@ function formatComparisonForJson(comparison) { * Display a single comparison in detail */ function displayComparison(output, comparison, verbose) { - let _colors = output.getColors(); - output.header('comparison', comparison.status); output.keyValue({ @@ -505,14 +503,19 @@ export function validateComparisonsOptions(options = {}) { let errors = []; if ( - options.limit && - (Number.isNaN(options.limit) || options.limit < 1 || options.limit > 250) + options.limit !== undefined && + (!Number.isInteger(options.limit) || + options.limit < 1 || + options.limit > 250) ) { - errors.push('--limit must be a number between 1 and 250'); + errors.push('--limit must be an integer between 1 and 250'); } - if (options.offset && (Number.isNaN(options.offset) || options.offset < 0)) { - errors.push('--offset must be a non-negative number'); + if ( + options.offset !== undefined && + (!Number.isInteger(options.offset) || options.offset < 0) + ) { + errors.push('--offset must be a non-negative integer'); } return errors; diff --git a/src/commands/projects.js b/src/commands/projects.js index 683c1f3..3863f28 100644 --- a/src/commands/projects.js +++ b/src/commands/projects.js @@ -134,6 +134,24 @@ export async function projectsCommand( * @param {Object} _options - Command options * @returns {string[]} Validation errors */ -export function validateProjectsOptions(_options = {}) { - return []; +export function validateProjectsOptions(options = {}) { + let errors = []; + + if ( + options.limit !== undefined && + (!Number.isInteger(options.limit) || + options.limit < 1 || + options.limit > 250) + ) { + errors.push('--limit must be an integer between 1 and 250'); + } + + if ( + options.offset !== undefined && + (!Number.isInteger(options.offset) || options.offset < 0) + ) { + errors.push('--offset must be a non-negative integer'); + } + + return errors; } diff --git a/src/commands/review.js b/src/commands/review.js index c325838..b93c9db 100644 --- a/src/commands/review.js +++ b/src/commands/review.js @@ -6,202 +6,222 @@ import { createApiClient as defaultCreateApiClient } from '../api/index.js'; import { loadConfig as defaultLoadConfig } from '../utils/config-loader.js'; import * as defaultOutput from '../utils/output.js'; -/** - * Approve a comparison - * @param {string} comparisonId - Comparison ID to approve - * @param {Object} options - Command options - * @param {Object} globalOptions - Global CLI options - * @param {Object} deps - Dependencies for testing - */ -export async function approveCommand( - comparisonId, - options = {}, - globalOptions = {}, - deps = {} -) { - let { - loadConfig = defaultLoadConfig, - createApiClient = defaultCreateApiClient, - output = defaultOutput, - exit = code => process.exit(code), - } = deps; +function createReviewDeps(deps = {}) { + return { + loadConfig: deps.loadConfig || defaultLoadConfig, + createApiClient: deps.createApiClient || defaultCreateApiClient, + output: deps.output || defaultOutput, + exit: deps.exit || (code => process.exit(code)), + }; +} +function configureOutput(output, globalOptions) { output.configure({ json: globalOptions.json, verbose: globalOptions.verbose, color: !globalOptions.noColor, }); +} + +async function loadReviewConfig({ loadConfig, options, globalOptions }) { + let allOptions = { ...globalOptions, ...options }; + return await loadConfig(globalOptions.config, allOptions); +} + +function createReviewClient({ createApiClient, config, command }) { + return createApiClient({ + baseUrl: config.apiUrl, + token: config.apiKey, + command, + }); +} + +function jsonBody(body) { + if (!body || Object.keys(body).length === 0) { + return {}; + } + + return { + body: JSON.stringify(body), + headers: { 'Content-Type': 'application/json' }, + }; +} + +async function runReviewMutation({ + command, + endpoint, + failureMessage, + globalOptions, + options, + requestBody, + spinnerMessage, + writeJsonError, + writeJsonSuccess, + writeHumanSuccess, + deps, + configure = true, +}) { + let { loadConfig, createApiClient, output, exit } = createReviewDeps(deps); + + if (configure) { + configureOutput(output, globalOptions); + } try { - let allOptions = { ...globalOptions, ...options }; - let config = await loadConfig(globalOptions.config, allOptions); + let config = await loadReviewConfig({ loadConfig, options, globalOptions }); if (!config.apiKey) { output.error('API token required'); output.hint('Use --token or set VIZZLY_TOKEN environment variable'); + output.cleanup(); exit(1); return; } - output.startSpinner('Approving comparison...'); + output.startSpinner(spinnerMessage); - let client = createApiClient({ - baseUrl: config.apiUrl, - token: config.apiKey, - command: 'approve', + let client = createReviewClient({ createApiClient, config, command }); + let response = await client.request(endpoint, { + method: 'POST', + ...jsonBody(requestBody), }); - let body = {}; - if (options.comment) { - body.comment = options.comment; - } - - let response = await client.request( - `/api/sdk/comparisons/${comparisonId}/approve`, - { - method: 'POST', - body: Object.keys(body).length > 0 ? JSON.stringify(body) : undefined, - headers: - Object.keys(body).length > 0 - ? { 'Content-Type': 'application/json' } - : undefined, - } - ); - output.stopSpinner(); if (globalOptions.json) { - output.data({ - approved: true, - comparisonId, - comparison: response.comparison, - }); + writeJsonSuccess(output, response); output.cleanup(); return; } - output.complete(`Comparison ${comparisonId} approved`); - if (options.comment) { - output.hint(`Comment: "${options.comment}"`); - } - + writeHumanSuccess(output, response); output.cleanup(); } catch (error) { output.stopSpinner(); if (globalOptions.json) { - output.data({ - approved: false, - comparisonId, - error: { message: error.message, code: error.code }, - }); + writeJsonError(output, error); output.cleanup(); exit(1); return; } - output.error('Failed to approve comparison', error); + output.error(failureMessage, error); output.cleanup(); exit(1); } } +export function createApprovalBody(options = {}) { + return options.comment ? { comment: options.comment } : {}; +} + +export function createRejectionBody(options = {}) { + return { reason: options.reason }; +} + +export function createCommentBody(message, options = {}) { + return { + content: message, + type: options.type || 'general', + }; +} + /** - * Reject a comparison - * @param {string} comparisonId - Comparison ID to reject + * Approve a comparison + * @param {string} comparisonId - Comparison ID to approve * @param {Object} options - Command options * @param {Object} globalOptions - Global CLI options * @param {Object} deps - Dependencies for testing */ -export async function rejectCommand( +export async function approveCommand( comparisonId, options = {}, globalOptions = {}, deps = {} ) { - let { - loadConfig = defaultLoadConfig, - createApiClient = defaultCreateApiClient, - output = defaultOutput, - exit = code => process.exit(code), - } = deps; - - output.configure({ - json: globalOptions.json, - verbose: globalOptions.verbose, - color: !globalOptions.noColor, - }); - - try { - let allOptions = { ...globalOptions, ...options }; - let config = await loadConfig(globalOptions.config, allOptions); - - if (!config.apiKey) { - output.error('API token required'); - output.hint('Use --token or set VIZZLY_TOKEN environment variable'); - exit(1); - return; - } - - if (!options.reason) { - output.error('Reason required when rejecting'); - output.hint('Use --reason "explanation" to provide a reason'); - exit(1); - return; - } - - output.startSpinner('Rejecting comparison...'); - - let client = createApiClient({ - baseUrl: config.apiUrl, - token: config.apiKey, - command: 'reject', - }); - - let response = await client.request( - `/api/sdk/comparisons/${comparisonId}/reject`, - { - method: 'POST', - body: JSON.stringify({ reason: options.reason }), - headers: { 'Content-Type': 'application/json' }, - } - ); - - output.stopSpinner(); - - if (globalOptions.json) { + return await runReviewMutation({ + command: 'approve', + endpoint: `/api/sdk/comparisons/${comparisonId}/approve`, + failureMessage: 'Failed to approve comparison', + globalOptions, + options, + requestBody: createApprovalBody(options), + spinnerMessage: 'Approving comparison...', + writeJsonError: (output, error) => output.data({ - rejected: true, + approved: false, + comparisonId, + error: { message: error.message, code: error.code }, + }), + writeJsonSuccess: (output, response) => + output.data({ + approved: true, comparisonId, - reason: options.reason, comparison: response.comparison, - }); - output.cleanup(); - return; - } + }), + writeHumanSuccess: output => { + output.complete(`Comparison ${comparisonId} approved`); + if (options.comment) { + output.hint(`Comment: "${options.comment}"`); + } + }, + deps, + }); +} - output.complete(`Comparison ${comparisonId} rejected`); - output.hint(`Reason: "${options.reason}"`); +/** + * Reject a comparison + * @param {string} comparisonId - Comparison ID to reject + * @param {Object} options - Command options + * @param {Object} globalOptions - Global CLI options + * @param {Object} deps - Dependencies for testing + */ +export async function rejectCommand( + comparisonId, + options = {}, + globalOptions = {}, + deps = {} +) { + let { output, exit } = createReviewDeps(deps); + configureOutput(output, globalOptions); + if (!options.reason) { + output.error('Reason required when rejecting'); + output.hint('Use --reason "explanation" to provide a reason'); output.cleanup(); - } catch (error) { - output.stopSpinner(); + exit(1); + return; + } - if (globalOptions.json) { + return await runReviewMutation({ + command: 'reject', + endpoint: `/api/sdk/comparisons/${comparisonId}/reject`, + failureMessage: 'Failed to reject comparison', + globalOptions, + options, + requestBody: createRejectionBody(options), + spinnerMessage: 'Rejecting comparison...', + writeJsonError: (output, error) => output.data({ rejected: false, comparisonId, error: { message: error.message, code: error.code }, - }); - output.cleanup(); - exit(1); - return; - } - - output.error('Failed to reject comparison', error); - output.cleanup(); - exit(1); - } + }), + writeJsonSuccess: (output, response) => + output.data({ + rejected: true, + comparisonId, + reason: options.reason, + comparison: response.comparison, + }), + writeHumanSuccess: output => { + output.complete(`Comparison ${comparisonId} rejected`); + output.hint(`Reason: "${options.reason}"`); + }, + deps, + configure: false, + }); } /** @@ -219,90 +239,44 @@ export async function commentCommand( globalOptions = {}, deps = {} ) { - let { - loadConfig = defaultLoadConfig, - createApiClient = defaultCreateApiClient, - output = defaultOutput, - exit = code => process.exit(code), - } = deps; - - output.configure({ - json: globalOptions.json, - verbose: globalOptions.verbose, - color: !globalOptions.noColor, - }); - - try { - let allOptions = { ...globalOptions, ...options }; - let config = await loadConfig(globalOptions.config, allOptions); - - if (!config.apiKey) { - output.error('API token required'); - output.hint('Use --token or set VIZZLY_TOKEN environment variable'); - exit(1); - return; - } - - if (!message || message.trim() === '') { - output.error('Comment message required'); - exit(1); - return; - } - - output.startSpinner('Adding comment...'); - - let client = createApiClient({ - baseUrl: config.apiUrl, - token: config.apiKey, - command: 'comment', - }); - - let body = { - content: message, - type: options.type || 'general', - }; - - let response = await client.request(`/api/sdk/builds/${buildId}/comments`, { - method: 'POST', - body: JSON.stringify(body), - headers: { 'Content-Type': 'application/json' }, - }); - - output.stopSpinner(); - - if (globalOptions.json) { - output.data({ - created: true, - buildId, - comment: response.comment, - }); - output.cleanup(); - return; - } - - output.complete('Comment added'); - output.labelValue('Build', buildId); - output.labelValue('Message', message); + let { output, exit } = createReviewDeps(deps); + configureOutput(output, globalOptions); + if (!message || message.trim() === '') { + output.error('Comment message required'); output.cleanup(); - } catch (error) { - output.stopSpinner(); + exit(1); + return; + } - if (globalOptions.json) { + return await runReviewMutation({ + command: 'comment', + endpoint: `/api/sdk/builds/${buildId}/comments`, + failureMessage: 'Failed to add comment', + globalOptions, + options, + requestBody: createCommentBody(message, options), + spinnerMessage: 'Adding comment...', + writeJsonError: (output, error) => output.data({ created: false, buildId, error: { message: error.message, code: error.code }, - }); - output.cleanup(); - exit(1); - return; - } - - output.error('Failed to add comment', error); - output.cleanup(); - exit(1); - } + }), + writeJsonSuccess: (output, response) => + output.data({ + created: true, + buildId, + comment: response.comment, + }), + writeHumanSuccess: output => { + output.complete('Comment added'); + output.labelValue('Build', buildId); + output.labelValue('Message', message); + }, + deps, + configure: false, + }); } /** diff --git a/src/commands/upload.js b/src/commands/upload.js index 25e7f66..024a29e 100644 --- a/src/commands/upload.js +++ b/src/commands/upload.js @@ -4,6 +4,7 @@ import { getTokenContext as defaultGetTokenContext, } from '../api/index.js'; import { createUploader as defaultCreateUploader } from '../services/uploader.js'; +import { getAppBaseUrl } from '../utils/api-url.js'; import { loadConfig as defaultLoadConfig } from '../utils/config-loader.js'; import { detectBranch as defaultDetectBranch, @@ -38,7 +39,7 @@ export async function constructBuildUrl(buildId, apiUrl, apiToken, deps = {}) { }); let tokenContext = await getTokenContext(client); - let baseUrl = apiUrl.replace(/\/api.*$/, ''); + let baseUrl = getAppBaseUrl(apiUrl); if (tokenContext.organization?.slug && tokenContext.project?.slug) { return `${baseUrl}/${tokenContext.organization.slug}/${tokenContext.project.slug}/builds/${buildId}`; @@ -51,7 +52,7 @@ export async function constructBuildUrl(buildId, apiUrl, apiToken, deps = {}) { } // Fallback URL construction - let baseUrl = apiUrl.replace(/\/api.*$/, ''); + let baseUrl = getAppBaseUrl(apiUrl); return `${baseUrl}/builds/${buildId}`; } @@ -422,8 +423,8 @@ export function validateUploadOptions(screenshotsPath, options) { } if (options.threshold !== undefined) { - const threshold = parseFloat(options.threshold); - if (Number.isNaN(threshold) || threshold < 0) { + let threshold = Number(options.threshold); + if (!Number.isFinite(threshold) || threshold < 0) { errors.push( 'Threshold must be a non-negative number (CIEDE2000 Delta E)' ); @@ -431,15 +432,15 @@ export function validateUploadOptions(screenshotsPath, options) { } if (options.batchSize !== undefined) { - const n = parseInt(options.batchSize, 10); - if (!Number.isFinite(n) || n <= 0) { + let n = Number(options.batchSize); + if (!Number.isInteger(n) || n <= 0) { errors.push('Batch size must be a positive integer'); } } if (options.uploadTimeout !== undefined) { - const n = parseInt(options.uploadTimeout, 10); - if (!Number.isFinite(n) || n <= 0) { + let n = Number(options.uploadTimeout); + if (!Number.isInteger(n) || n <= 0) { errors.push('Upload timeout must be a positive integer (milliseconds)'); } } diff --git a/tests/commands/api.test.js b/tests/commands/api.test.js new file mode 100644 index 0000000..9269f1d --- /dev/null +++ b/tests/commands/api.test.js @@ -0,0 +1,357 @@ +import assert from 'node:assert/strict'; +import { describe, it } from 'node:test'; +import { + apiCommand, + appendApiQuery, + buildApiRequest, + isAllowedPostEndpoint, + normalizeApiEndpoint, + normalizeApiMethod, + parseApiHeaders, + validateApiOptions, + validateApiRequest, +} from '../../src/commands/api.js'; + +function createMockOutput() { + let calls = []; + return { + calls, + configure: opts => calls.push({ method: 'configure', args: [opts] }), + data: value => calls.push({ method: 'data', args: [value] }), + error: (message, error) => + calls.push({ method: 'error', args: [message, error] }), + hint: message => calls.push({ method: 'hint', args: [message] }), + header: command => calls.push({ method: 'header', args: [command] }), + labelValue: (label, value) => + calls.push({ method: 'labelValue', args: [label, value] }), + blank: () => calls.push({ method: 'blank', args: [] }), + print: message => calls.push({ method: 'print', args: [message] }), + startSpinner: message => + calls.push({ method: 'startSpinner', args: [message] }), + stopSpinner: () => calls.push({ method: 'stopSpinner', args: [] }), + cleanup: () => calls.push({ method: 'cleanup', args: [] }), + }; +} + +function createApiHarness(response = { ok: true }) { + let output = createMockOutput(); + let clientConfig = null; + let request = null; + let exitCode = null; + + return { + output, + get clientConfig() { + return clientConfig; + }, + get request() { + return request; + }, + get exitCode() { + return exitCode; + }, + deps: { + loadConfig: async () => ({ + apiKey: 'token-123', + apiUrl: 'https://api.example.test', + }), + createApiClient: config => { + clientConfig = config; + return { + request: async (endpoint, options) => { + request = { endpoint, options }; + return response; + }, + }; + }, + output, + exit: code => { + exitCode = code; + }, + }, + }; +} + +describe('commands/api', () => { + describe('request helpers', () => { + it('normalizes endpoint and method inputs', () => { + assert.strictEqual(normalizeApiEndpoint('sdk/builds'), '/api/sdk/builds'); + assert.strictEqual( + normalizeApiEndpoint('/api/sdk/builds'), + '/api/sdk/builds' + ); + assert.strictEqual(normalizeApiMethod('post'), 'POST'); + assert.strictEqual(normalizeApiMethod(), 'GET'); + }); + + it('parses headers and query parameters without losing separators', () => { + assert.deepStrictEqual( + parseApiHeaders(['X-Test: alpha:beta', 'Accept: application/json']), + { + 'X-Test': 'alpha:beta', + Accept: 'application/json', + } + ); + assert.strictEqual( + appendApiQuery('/api/sdk/builds?existing=1', [ + 'branch=feature/a=b', + 'limit=5', + ]), + '/api/sdk/builds?existing=1&branch=feature%2Fa%3Db&limit=5' + ); + }); + + it('allows only selected POST endpoints', () => { + assert.strictEqual( + isAllowedPostEndpoint('/api/sdk/comparisons/cmp-1/approve'), + true + ); + assert.strictEqual( + isAllowedPostEndpoint('/api/sdk/comparisons/cmp-1/reject'), + true + ); + assert.strictEqual( + isAllowedPostEndpoint('/api/sdk/builds/build-1/comments'), + true + ); + assert.strictEqual(isAllowedPostEndpoint('/api/sdk/builds'), false); + }); + + it('builds GET and POST request options', () => { + assert.deepStrictEqual( + buildApiRequest({ + endpoint: 'sdk/builds', + options: { + query: ['limit=5'], + header: 'X-Test: yes', + }, + }), + { + errors: [], + method: 'GET', + normalizedEndpoint: '/api/sdk/builds?limit=5', + requestOptions: { + method: 'GET', + headers: { 'X-Test': 'yes' }, + }, + } + ); + + assert.deepStrictEqual( + buildApiRequest({ + endpoint: '/api/sdk/comparisons/cmp-1/approve', + options: { method: 'POST', data: '{"ok":true}' }, + }), + { + errors: [], + method: 'POST', + normalizedEndpoint: '/api/sdk/comparisons/cmp-1/approve', + requestOptions: { + method: 'POST', + body: '{"ok":true}', + headers: { 'Content-Type': 'application/json' }, + }, + } + ); + }); + + it('reports unsafe API requests', () => { + assert.deepStrictEqual( + validateApiRequest({ + endpoint: '/api/sdk/builds', + method: 'POST', + }), + [ + 'POST not allowed for /api/sdk/builds. Only approve, reject, and comment endpoints support POST.', + ] + ); + assert.deepStrictEqual( + validateApiRequest({ + endpoint: '/api/sdk/builds', + method: 'DELETE', + }), + [ + 'Method DELETE not allowed. Use GET for queries or POST for approve/reject/comment.', + ] + ); + }); + }); + + describe('validateApiOptions', () => { + it('validates endpoint and method options', () => { + assert.deepStrictEqual(validateApiOptions('/api/sdk/builds'), []); + assert.deepStrictEqual(validateApiOptions(''), ['Endpoint is required']); + assert.deepStrictEqual(validateApiOptions(' '), [ + 'Endpoint is required', + ]); + assert.deepStrictEqual( + validateApiOptions('/api/sdk/builds', { method: 'POST' }), + [ + 'POST not allowed for /api/sdk/builds. Only approve, reject, and comment endpoints support POST.', + ] + ); + assert.deepStrictEqual( + validateApiOptions('/api/sdk/builds', { method: 'PATCH' }), + [ + 'Method PATCH not allowed. Use GET for queries or POST for approve/reject/comment.', + ] + ); + }); + }); + + describe('apiCommand', () => { + it('performs a GET request and returns JSON output', async () => { + let harness = createApiHarness({ builds: [] }); + + await apiCommand( + 'sdk/builds', + { query: ['limit=5'] }, + { json: true }, + harness.deps + ); + + assert.deepStrictEqual(harness.clientConfig, { + baseUrl: 'https://api.example.test', + token: 'token-123', + command: 'api', + }); + assert.deepStrictEqual(harness.request, { + endpoint: '/api/sdk/builds?limit=5', + options: { method: 'GET' }, + }); + + let dataCall = harness.output.calls.find(call => call.method === 'data'); + assert.deepStrictEqual(dataCall.args[0], { + endpoint: '/api/sdk/builds?limit=5', + method: 'GET', + response: { builds: [] }, + }); + }); + + it('performs an allowed POST request with headers and body', async () => { + let harness = createApiHarness({ comparison: { id: 'cmp-1' } }); + + await apiCommand( + '/api/sdk/comparisons/cmp-1/approve', + { + method: 'POST', + data: '{"comment":"LGTM"}', + header: 'X-Trace: trace-1', + }, + {}, + harness.deps + ); + + assert.deepStrictEqual(harness.request, { + endpoint: '/api/sdk/comparisons/cmp-1/approve', + options: { + method: 'POST', + body: '{"comment":"LGTM"}', + headers: { + 'X-Trace': 'trace-1', + 'Content-Type': 'application/json', + }, + }, + }); + assert.ok( + harness.output.calls.some( + call => call.method === 'labelValue' && call.args[0] === 'Endpoint' + ) + ); + }); + + it('cleans up and exits when no API token is configured', async () => { + let output = createMockOutput(); + let exitCode = null; + + await apiCommand( + '/api/sdk/builds', + {}, + {}, + { + loadConfig: async () => ({ apiUrl: 'https://api.example.test' }), + output, + exit: code => { + exitCode = code; + }, + } + ); + + assert.strictEqual(exitCode, 1); + assert.ok(output.calls.some(call => call.method === 'error')); + assert.ok(output.calls.some(call => call.method === 'cleanup')); + }); + + it('blocks unsafe POST requests before creating a client', async () => { + let output = createMockOutput(); + let exitCode = null; + let createdClient = false; + + await apiCommand( + '/api/sdk/builds', + { method: 'POST', data: '{}' }, + {}, + { + loadConfig: async () => ({ + apiKey: 'token-123', + apiUrl: 'https://api.example.test', + }), + createApiClient: () => { + createdClient = true; + return {}; + }, + output, + exit: code => { + exitCode = code; + }, + } + ); + + assert.strictEqual(exitCode, 1); + assert.strictEqual(createdClient, false); + assert.ok(output.calls.some(call => call.method === 'error')); + assert.ok(output.calls.some(call => call.method === 'cleanup')); + }); + + it('returns JSON failure details using normalized endpoint and method', async () => { + let output = createMockOutput(); + let exitCode = null; + let error = new Error('network failed'); + error.code = 'network_error'; + error.context = { status: 503 }; + + await apiCommand( + 'sdk/builds', + {}, + { json: true }, + { + loadConfig: async () => ({ + apiKey: 'token-123', + apiUrl: 'https://api.example.test', + }), + createApiClient: () => ({ + request: async () => { + throw error; + }, + }), + output, + exit: code => { + exitCode = code; + }, + } + ); + + assert.strictEqual(exitCode, 1); + let dataCall = output.calls.find(call => call.method === 'data'); + assert.deepStrictEqual(dataCall.args[0], { + endpoint: '/api/sdk/builds', + method: 'GET', + error: { + message: 'network failed', + code: 'network_error', + status: 503, + }, + }); + }); + }); +}); diff --git a/tests/commands/baselines.test.js b/tests/commands/baselines.test.js new file mode 100644 index 0000000..4a7ea24 --- /dev/null +++ b/tests/commands/baselines.test.js @@ -0,0 +1,332 @@ +import assert from 'node:assert/strict'; +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { describe, it } from 'node:test'; +import { + baselinesCommand, + createBaselineList, + createBaselineNameMatcher, + formatBytes, + getFilename, + getViewport, + validateBaselinesOptions, +} from '../../src/commands/baselines.js'; + +function createMockOutput() { + let calls = []; + return { + calls, + configure: opts => calls.push({ method: 'configure', args: [opts] }), + data: obj => calls.push({ method: 'data', args: [obj] }), + header: command => calls.push({ method: 'header', args: [command] }), + keyValue: values => calls.push({ method: 'keyValue', args: [values] }), + labelValue: (label, value) => + calls.push({ method: 'labelValue', args: [label, value] }), + print: message => calls.push({ method: 'print', args: [message] }), + blank: () => calls.push({ method: 'blank', args: [] }), + hint: message => calls.push({ method: 'hint', args: [message] }), + warn: message => calls.push({ method: 'warn', args: [message] }), + error: (message, error) => + calls.push({ method: 'error', args: [message, error] }), + cleanup: () => calls.push({ method: 'cleanup', args: [] }), + getColors: () => ({ + dim: value => value, + brand: { + success: value => value, + warning: value => value, + }, + }), + }; +} + +function createWorkspace() { + let root = mkdtempSync(join(tmpdir(), 'vizzly-baselines-')); + let vizzlyDir = join(root, '.vizzly'); + let baselinesDir = join(vizzlyDir, 'baselines'); + mkdirSync(baselinesDir, { recursive: true }); + + return { + root, + baselinesDir, + dispose: () => rmSync(root, { recursive: true, force: true }), + }; +} + +function writeBaselineFixture(workspace) { + writeFileSync(join(workspace.baselinesDir, 'button-home.png'), 'png'); + writeFileSync(join(workspace.baselinesDir, 'dialog-[special].png'), 'png'); + + writeFileSync( + join(workspace.baselinesDir, 'metadata.json'), + JSON.stringify({ + buildId: 'build-1', + buildName: 'Local Build', + branch: 'feature/baselines', + threshold: 1.5, + createdAt: '2026-05-18T00:00:00.000Z', + screenshots: [ + { + name: 'Button Home', + signature: 'sig-button', + filename: 'button-home.png', + sha256: 'abc1234567890abcdef', + viewport: { width: 1280, height: 720 }, + browser: 'firefox', + createdAt: '2026-05-18T00:00:01.000Z', + }, + { + name: 'Dialog [Special]', + signature: 'sig-dialog', + filename: 'dialog-[special].png', + properties: { viewport_width: 390, viewport_height: 844 }, + }, + ], + }) + ); +} + +describe('commands/baselines', () => { + describe('validateBaselinesOptions', () => { + it('returns no errors', () => { + assert.deepStrictEqual(validateBaselinesOptions({}), []); + }); + }); + + describe('baseline helpers', () => { + it('extracts filenames and viewport data from metadata variants', () => { + assert.strictEqual( + getFilename({ path: '/tmp/homepage.png' }), + 'homepage.png' + ); + assert.deepStrictEqual( + getViewport({ + properties: { viewport_width: 390, viewport_height: 844 }, + }), + { width: 390, height: 844 } + ); + }); + + it('matches wildcard names without treating other characters as regex', () => { + let wildcardMatcher = createBaselineNameMatcher('Button*'); + let bracketMatcher = createBaselineNameMatcher('[Special]'); + + assert.equal(wildcardMatcher('Button Home'), true); + assert.equal(wildcardMatcher('Dialog Home'), false); + assert.equal(bracketMatcher('Dialog [Special]'), true); + }); + + it('builds JSON baseline entries with file details', () => { + let baselines = createBaselineList({ + baselinesDir: '/tmp/baselines', + baselineFiles: [ + { + filename: 'home.png', + path: '/tmp/baselines/home.png', + size: 128, + }, + ], + screenshots: [ + { name: 'Home', signature: 'sig-home', filename: 'home.png' }, + ], + }); + + assert.deepStrictEqual(baselines, [ + { + name: 'Home', + signature: 'sig-home', + filename: 'home.png', + path: '/tmp/baselines/home.png', + sha256: undefined, + viewport: null, + browser: null, + createdAt: undefined, + fileSize: 128, + }, + ]); + }); + + it('keeps metadata-only screenshots without filenames readable', () => { + let baselines = createBaselineList({ + baselinesDir: '/tmp/baselines', + baselineFiles: [], + screenshots: [{ name: 'Metadata Only', signature: 'sig-metadata' }], + }); + + assert.strictEqual(baselines[0].filename, null); + assert.strictEqual(baselines[0].path, null); + }); + + it('formats file sizes for users', () => { + assert.strictEqual(formatBytes(512), '512 B'); + assert.strictEqual(formatBytes(1536), '1.5 KB'); + assert.strictEqual(formatBytes(2 * 1024 * 1024), '2.0 MB'); + }); + }); + + describe('baselinesCommand', () => { + it('returns empty JSON when no local baselines exist', async () => { + let workspace = mkdtempSync(join(tmpdir(), 'vizzly-baselines-empty-')); + let output = createMockOutput(); + + try { + await baselinesCommand( + {}, + { json: true }, + { + cwd: () => workspace, + output, + } + ); + } finally { + rmSync(workspace, { recursive: true, force: true }); + } + + let dataCall = output.calls.find(call => call.method === 'data'); + assert.deepStrictEqual(dataCall.args[0], { + baselines: [], + count: 0, + error: 'No .vizzly directory found', + }); + }); + + it('lists local baselines as JSON with metadata and file sizes', async () => { + let workspace = createWorkspace(); + let output = createMockOutput(); + writeBaselineFixture(workspace); + + try { + await baselinesCommand( + {}, + { json: true }, + { + cwd: () => workspace.root, + output, + } + ); + } finally { + workspace.dispose(); + } + + let dataCall = output.calls.find(call => call.method === 'data'); + assert.strictEqual(dataCall.args[0].count, 2); + assert.strictEqual(dataCall.args[0].metadata.buildName, 'Local Build'); + assert.strictEqual(dataCall.args[0].baselines[0].name, 'Button Home'); + assert.strictEqual(dataCall.args[0].baselines[0].fileSize, 3); + }); + + it('filters baselines by literal wildcard pattern', async () => { + let workspace = createWorkspace(); + let output = createMockOutput(); + writeBaselineFixture(workspace); + + try { + await baselinesCommand( + { name: '[Special]' }, + { json: true }, + { + cwd: () => workspace.root, + output, + } + ); + } finally { + workspace.dispose(); + } + + let dataCall = output.calls.find(call => call.method === 'data'); + assert.strictEqual(dataCall.args[0].count, 1); + assert.strictEqual( + dataCall.args[0].baselines[0].name, + 'Dialog [Special]' + ); + }); + + it('prints exact-match threshold metadata without falling back to default', async () => { + let workspace = createWorkspace(); + let output = createMockOutput(); + writeBaselineFixture(workspace); + writeFileSync( + join(workspace.baselinesDir, 'metadata.json'), + JSON.stringify({ + buildId: 'build-1', + buildName: 'Local Build', + branch: 'local', + threshold: 0, + screenshots: [], + }) + ); + + try { + await baselinesCommand( + {}, + {}, + { + cwd: () => workspace.root, + output, + } + ); + } finally { + workspace.dispose(); + } + + let thresholdCall = output.calls.find( + call => call.method === 'labelValue' && call.args[0] === 'Threshold' + ); + assert.deepStrictEqual(thresholdCall.args, ['Threshold', '0 CIEDE2000']); + }); + + it('returns detailed baseline info by name', async () => { + let workspace = createWorkspace(); + let output = createMockOutput(); + writeBaselineFixture(workspace); + + try { + await baselinesCommand( + { info: 'Button Home' }, + { json: true }, + { + cwd: () => workspace.root, + output, + } + ); + } finally { + workspace.dispose(); + } + + let dataCall = output.calls.find(call => call.method === 'data'); + assert.strictEqual(dataCall.args[0].name, 'Button Home'); + assert.deepStrictEqual(dataCall.args[0].viewport, { + width: 1280, + height: 720, + }); + assert.strictEqual(dataCall.args[0].fileSize, 3); + }); + + it('exits with status 1 when requested baseline info is missing', async () => { + let workspace = createWorkspace(); + let output = createMockOutput(); + let exitCode = null; + writeBaselineFixture(workspace); + + try { + await baselinesCommand( + { info: 'Missing' }, + {}, + { + cwd: () => workspace.root, + output, + exit: code => { + exitCode = code; + }, + } + ); + } finally { + workspace.dispose(); + } + + assert.strictEqual(exitCode, 1); + assert.ok(output.calls.some(call => call.method === 'error')); + assert.ok(output.calls.some(call => call.method === 'cleanup')); + }); + }); +}); diff --git a/tests/commands/builds.test.js b/tests/commands/builds.test.js index 5b19ac0..8666acd 100644 --- a/tests/commands/builds.test.js +++ b/tests/commands/builds.test.js @@ -48,12 +48,40 @@ describe('commands/builds', () => { it('returns error for invalid limit', () => { let errors = validateBuildsOptions({ limit: 500 }); - assert.ok(errors.includes('--limit must be a number between 1 and 250')); + assert.ok( + errors.includes('--limit must be an integer between 1 and 250') + ); + }); + + it('returns error for malformed and decimal limits', () => { + assert.ok( + validateBuildsOptions({ limit: Number('20abc') }).includes( + '--limit must be an integer between 1 and 250' + ) + ); + assert.ok( + validateBuildsOptions({ limit: 20.5 }).includes( + '--limit must be an integer between 1 and 250' + ) + ); }); it('returns error for negative offset', () => { let errors = validateBuildsOptions({ offset: -1 }); - assert.ok(errors.includes('--offset must be a non-negative number')); + assert.ok(errors.includes('--offset must be a non-negative integer')); + }); + + it('returns error for malformed and decimal offsets', () => { + assert.ok( + validateBuildsOptions({ offset: Number('1abc') }).includes( + '--offset must be a non-negative integer' + ) + ); + assert.ok( + validateBuildsOptions({ offset: 1.5 }).includes( + '--offset must be a non-negative integer' + ) + ); }); }); diff --git a/tests/commands/comparisons.test.js b/tests/commands/comparisons.test.js index 8c7ccd4..da5f3f2 100644 --- a/tests/commands/comparisons.test.js +++ b/tests/commands/comparisons.test.js @@ -48,12 +48,40 @@ describe('commands/comparisons', () => { it('returns error for invalid limit', () => { let errors = validateComparisonsOptions({ limit: 500 }); - assert.ok(errors.includes('--limit must be a number between 1 and 250')); + assert.ok( + errors.includes('--limit must be an integer between 1 and 250') + ); + }); + + it('returns error for malformed and decimal limits', () => { + assert.ok( + validateComparisonsOptions({ limit: Number('20abc') }).includes( + '--limit must be an integer between 1 and 250' + ) + ); + assert.ok( + validateComparisonsOptions({ limit: 20.5 }).includes( + '--limit must be an integer between 1 and 250' + ) + ); }); it('returns error for negative offset', () => { let errors = validateComparisonsOptions({ offset: -1 }); - assert.ok(errors.includes('--offset must be a non-negative number')); + assert.ok(errors.includes('--offset must be a non-negative integer')); + }); + + it('returns error for malformed and decimal offsets', () => { + assert.ok( + validateComparisonsOptions({ offset: Number('1abc') }).includes( + '--offset must be a non-negative integer' + ) + ); + assert.ok( + validateComparisonsOptions({ offset: 1.5 }).includes( + '--offset must be a non-negative integer' + ) + ); }); }); @@ -229,6 +257,37 @@ describe('commands/comparisons', () => { ); }); + it('filters build comparisons with literal regex characters in names', async () => { + let output = createMockOutput(); + let mockBuild = { + id: 'build-1', + name: 'Build 1', + comparisons: [ + { id: 'comp-1', name: 'card[primary].png', status: 'changed' }, + { id: 'comp-2', name: 'cardp.png', status: 'changed' }, + ], + }; + + await comparisonsCommand( + { build: 'build-1', name: 'card[primary].png' }, + { json: true }, + { + loadConfig: async () => ({ apiKey: 'test-token' }), + createApiClient: () => ({}), + getBuild: async () => ({ build: mockBuild }), + output, + exit: () => {}, + } + ); + + let dataCall = output.calls.find(c => c.method === 'data'); + assert.ok(dataCall); + assert.deepStrictEqual( + dataCall.args[0].comparisons.map(comparison => comparison.name), + ['card[primary].png'] + ); + }); + it('passes project filter to search', async () => { let output = createMockOutput(); let capturedFilters = null; diff --git a/tests/commands/projects.test.js b/tests/commands/projects.test.js index 8fe049e..22e3c7a 100644 --- a/tests/commands/projects.test.js +++ b/tests/commands/projects.test.js @@ -60,6 +60,42 @@ describe('commands/projects', () => { let errors = validateProjectsOptions({}); assert.deepStrictEqual(errors, []); }); + + it('rejects malformed and out-of-range limits', () => { + assert.ok( + validateProjectsOptions({ limit: Number('20abc') }).includes( + '--limit must be an integer between 1 and 250' + ) + ); + assert.ok( + validateProjectsOptions({ limit: 251 }).includes( + '--limit must be an integer between 1 and 250' + ) + ); + assert.ok( + validateProjectsOptions({ limit: 1.5 }).includes( + '--limit must be an integer between 1 and 250' + ) + ); + }); + + it('rejects malformed and negative offsets', () => { + assert.ok( + validateProjectsOptions({ offset: Number('1abc') }).includes( + '--offset must be a non-negative integer' + ) + ); + assert.ok( + validateProjectsOptions({ offset: -1 }).includes( + '--offset must be a non-negative integer' + ) + ); + assert.ok( + validateProjectsOptions({ offset: 1.5 }).includes( + '--offset must be a non-negative integer' + ) + ); + }); }); describe('projectsCommand', () => { diff --git a/tests/commands/review.test.js b/tests/commands/review.test.js new file mode 100644 index 0000000..c06dc45 --- /dev/null +++ b/tests/commands/review.test.js @@ -0,0 +1,353 @@ +import assert from 'node:assert/strict'; +import { describe, it } from 'node:test'; +import { + approveCommand, + commentCommand, + createApprovalBody, + createCommentBody, + createRejectionBody, + rejectCommand, + validateApproveOptions, + validateCommentOptions, + validateRejectOptions, +} from '../../src/commands/review.js'; + +function createMockOutput() { + let calls = []; + return { + calls, + configure: opts => calls.push({ method: 'configure', args: [opts] }), + data: obj => calls.push({ method: 'data', args: [obj] }), + labelValue: (label, value) => + calls.push({ method: 'labelValue', args: [label, value] }), + hint: message => calls.push({ method: 'hint', args: [message] }), + complete: message => calls.push({ method: 'complete', args: [message] }), + error: (message, error) => + calls.push({ method: 'error', args: [message, error] }), + startSpinner: message => + calls.push({ method: 'startSpinner', args: [message] }), + stopSpinner: () => calls.push({ method: 'stopSpinner', args: [] }), + cleanup: () => calls.push({ method: 'cleanup', args: [] }), + }; +} + +function createReviewHarness(response = {}) { + let output = createMockOutput(); + let clientCalls = []; + let clientConfig = null; + let exitCode = null; + + return { + output, + get clientCalls() { + return clientCalls; + }, + get clientConfig() { + return clientConfig; + }, + get exitCode() { + return exitCode; + }, + deps: { + loadConfig: async () => ({ + apiKey: 'token-123', + apiUrl: 'https://api.example.test', + }), + createApiClient: config => { + clientConfig = config; + return { + request: async (endpoint, options) => { + clientCalls.push({ endpoint, options }); + return response; + }, + }; + }, + output, + exit: code => { + exitCode = code; + }, + }, + }; +} + +describe('commands/review', () => { + describe('validation', () => { + it('validates approve inputs', () => { + assert.deepStrictEqual(validateApproveOptions('comparison-1'), []); + assert.deepStrictEqual(validateApproveOptions(' '), [ + 'Comparison ID is required', + ]); + }); + + it('validates reject inputs', () => { + assert.deepStrictEqual( + validateRejectOptions('comparison-1', { reason: 'Regression' }), + [] + ); + assert.deepStrictEqual(validateRejectOptions('', {}), [ + 'Comparison ID is required', + '--reason is required when rejecting', + ]); + }); + + it('validates comment inputs', () => { + assert.deepStrictEqual( + validateCommentOptions('build-1', 'Looks good', { type: 'approval' }), + [] + ); + assert.deepStrictEqual( + validateCommentOptions('', '', { type: 'unknown' }), + [ + 'Build ID is required', + 'Comment message is required', + '--type must be one of: general, approval, rejection', + ] + ); + }); + }); + + describe('payload helpers', () => { + it('creates review payloads from command options', () => { + assert.deepStrictEqual(createApprovalBody({}), {}); + assert.deepStrictEqual(createApprovalBody({ comment: 'LGTM' }), { + comment: 'LGTM', + }); + assert.deepStrictEqual(createRejectionBody({ reason: 'Regression' }), { + reason: 'Regression', + }); + assert.deepStrictEqual(createCommentBody('Needs follow-up', {}), { + content: 'Needs follow-up', + type: 'general', + }); + assert.deepStrictEqual( + createCommentBody('Approved', { type: 'approval' }), + { + content: 'Approved', + type: 'approval', + } + ); + }); + }); + + describe('approveCommand', () => { + it('approves a comparison without sending an empty JSON body', async () => { + let harness = createReviewHarness({ + comparison: { id: 'comparison-1', status: 'approved' }, + }); + + await approveCommand('comparison-1', {}, { json: true }, harness.deps); + + assert.deepStrictEqual(harness.clientConfig, { + baseUrl: 'https://api.example.test', + token: 'token-123', + command: 'approve', + }); + assert.deepStrictEqual(harness.clientCalls, [ + { + endpoint: '/api/sdk/comparisons/comparison-1/approve', + options: { method: 'POST' }, + }, + ]); + + let dataCall = harness.output.calls.find(call => call.method === 'data'); + assert.deepStrictEqual(dataCall.args[0], { + approved: true, + comparisonId: 'comparison-1', + comparison: { id: 'comparison-1', status: 'approved' }, + }); + }); + + it('sends an approval comment when provided', async () => { + let harness = createReviewHarness({ comparison: { id: 'comparison-1' } }); + + await approveCommand( + 'comparison-1', + { comment: 'LGTM' }, + {}, + harness.deps + ); + + assert.deepStrictEqual(harness.clientCalls[0].options, { + method: 'POST', + body: JSON.stringify({ comment: 'LGTM' }), + headers: { 'Content-Type': 'application/json' }, + }); + assert.ok( + harness.output.calls.some( + call => + call.method === 'complete' && call.args[0].includes('approved') + ) + ); + assert.ok( + harness.output.calls.some( + call => call.method === 'hint' && call.args[0] === 'Comment: "LGTM"' + ) + ); + }); + }); + + describe('rejectCommand', () => { + it('rejects a comparison with a required reason', async () => { + let harness = createReviewHarness({ + comparison: { id: 'comparison-1', status: 'rejected' }, + }); + + await rejectCommand( + 'comparison-1', + { reason: 'Visual regression' }, + { json: true }, + harness.deps + ); + + assert.deepStrictEqual(harness.clientCalls, [ + { + endpoint: '/api/sdk/comparisons/comparison-1/reject', + options: { + method: 'POST', + body: JSON.stringify({ reason: 'Visual regression' }), + headers: { 'Content-Type': 'application/json' }, + }, + }, + ]); + + let dataCall = harness.output.calls.find(call => call.method === 'data'); + assert.deepStrictEqual(dataCall.args[0], { + rejected: true, + comparisonId: 'comparison-1', + reason: 'Visual regression', + comparison: { id: 'comparison-1', status: 'rejected' }, + }); + }); + + it('exits before API work when reason is missing', async () => { + let harness = createReviewHarness(); + + await rejectCommand('comparison-1', {}, {}, harness.deps); + + assert.strictEqual(harness.exitCode, 1); + assert.deepStrictEqual(harness.clientCalls, []); + assert.ok(harness.output.calls.some(call => call.method === 'cleanup')); + }); + }); + + describe('commentCommand', () => { + it('creates a build comment and prints human-readable output', async () => { + let harness = createReviewHarness({ + comment: { id: 'comment-1', content: 'Looks good' }, + }); + + await commentCommand( + 'build-1', + 'Looks good', + { type: 'approval' }, + {}, + harness.deps + ); + + assert.deepStrictEqual(harness.clientConfig, { + baseUrl: 'https://api.example.test', + token: 'token-123', + command: 'comment', + }); + assert.deepStrictEqual(harness.clientCalls, [ + { + endpoint: '/api/sdk/builds/build-1/comments', + options: { + method: 'POST', + body: JSON.stringify({ + content: 'Looks good', + type: 'approval', + }), + headers: { 'Content-Type': 'application/json' }, + }, + }, + ]); + assert.ok( + harness.output.calls.some( + call => call.method === 'complete' && call.args[0] === 'Comment added' + ) + ); + assert.ok( + harness.output.calls.some( + call => + call.method === 'labelValue' && + call.args[0] === 'Message' && + call.args[1] === 'Looks good' + ) + ); + }); + + it('exits before API work when the message is blank', async () => { + let harness = createReviewHarness(); + + await commentCommand('build-1', ' ', {}, {}, harness.deps); + + assert.strictEqual(harness.exitCode, 1); + assert.deepStrictEqual(harness.clientCalls, []); + assert.ok(harness.output.calls.some(call => call.method === 'cleanup')); + }); + }); + + describe('error handling', () => { + it('cleans up and exits when no API token is configured', async () => { + let output = createMockOutput(); + let exitCode = null; + + await approveCommand( + 'comparison-1', + {}, + {}, + { + loadConfig: async () => ({ apiUrl: 'https://api.example.test' }), + output, + exit: code => { + exitCode = code; + }, + } + ); + + assert.strictEqual(exitCode, 1); + assert.ok(output.calls.some(call => call.method === 'error')); + assert.ok(output.calls.some(call => call.method === 'hint')); + assert.ok(output.calls.some(call => call.method === 'cleanup')); + }); + + it('returns JSON failure details when an API request fails', async () => { + let output = createMockOutput(); + let exitCode = null; + let error = new Error('API failed'); + error.code = 'server_error'; + + await commentCommand( + 'build-1', + 'Looks good', + {}, + { json: true }, + { + loadConfig: async () => ({ + apiKey: 'token-123', + apiUrl: 'https://api.example.test', + }), + createApiClient: () => ({ + request: async () => { + throw error; + }, + }), + output, + exit: code => { + exitCode = code; + }, + } + ); + + assert.strictEqual(exitCode, 1); + let dataCall = output.calls.find(call => call.method === 'data'); + assert.deepStrictEqual(dataCall.args[0], { + created: false, + buildId: 'build-1', + error: { message: 'API failed', code: 'server_error' }, + }); + assert.ok(output.calls.some(call => call.method === 'cleanup')); + }); + }); +}); diff --git a/tests/commands/upload.test.js b/tests/commands/upload.test.js index c627d72..d5bfcd4 100644 --- a/tests/commands/upload.test.js +++ b/tests/commands/upload.test.js @@ -111,6 +111,17 @@ describe('validateUploadOptions', () => { ); }); + it('should fail when threshold has trailing text', () => { + let errors = validateUploadOptions('./screenshots', { + threshold: '2abc', + }); + assert.ok( + errors.includes( + 'Threshold must be a non-negative number (CIEDE2000 Delta E)' + ) + ); + }); + it('should fail with threshold below 0', () => { let errors = validateUploadOptions('./screenshots', { threshold: '-0.1', @@ -145,6 +156,13 @@ describe('validateUploadOptions', () => { assert.ok(errors.includes('Batch size must be a positive integer')); }); + it('should fail with decimal batch size', () => { + let errors = validateUploadOptions('./screenshots', { + batchSize: '2.5', + }); + assert.ok(errors.includes('Batch size must be a positive integer')); + }); + it('should fail with zero batch size', () => { let errors = validateUploadOptions('./screenshots', { batchSize: '0' }); assert.ok(errors.includes('Batch size must be a positive integer')); @@ -177,6 +195,17 @@ describe('validateUploadOptions', () => { ); }); + it('should fail with decimal upload timeout', () => { + let errors = validateUploadOptions('./screenshots', { + uploadTimeout: '2500.5', + }); + assert.ok( + errors.includes( + 'Upload timeout must be a positive integer (milliseconds)' + ) + ); + }); + it('should fail with zero upload timeout', () => { let errors = validateUploadOptions('./screenshots', { uploadTimeout: '0', @@ -287,6 +316,21 @@ describe('constructBuildUrl', () => { assert.strictEqual(url, 'https://custom.example.com/builds/build-123'); }); + + it('does not strip api from hostnames when building fallback URLs', async () => { + let url = await constructBuildUrl( + 'build-123', + 'https://api.example.com/api/v1', + 'test-token', + { + createApiClient: () => ({}), + getTokenContext: async () => ({}), + output: createMockOutput(), + } + ); + + assert.strictEqual(url, 'https://api.example.com/builds/build-123'); + }); }); describe('uploadCommand', () => { diff --git a/tests/helpers/http-mocks.js b/tests/helpers/http-mocks.js new file mode 100644 index 0000000..cb8a628 --- /dev/null +++ b/tests/helpers/http-mocks.js @@ -0,0 +1,58 @@ +import { EventEmitter } from 'node:events'; + +export function createMockRequest(method = 'GET', body = null) { + let req = new EventEmitter(); + req.method = method; + + if (body !== null) { + let didEmitBody = false; + let originalOn = req.on.bind(req); + + req.on = (eventName, listener) => { + let result = originalOn(eventName, listener); + + if (eventName === 'end' && !didEmitBody) { + didEmitBody = true; + req.emit('data', JSON.stringify(body)); + req.emit('end'); + } + + return result; + }; + } + + return req; +} + +export function createMockResponse() { + let headers = {}; + let statusCode = null; + let body = null; + + return { + get statusCode() { + return statusCode; + }, + set statusCode(code) { + statusCode = code; + }, + setHeader(name, value) { + headers[name] = value; + }, + getHeader(name) { + return headers[name]; + }, + end(content) { + body = content; + }, + get headers() { + return headers; + }, + get body() { + return body; + }, + getParsedBody() { + return body && typeof body === 'string' ? JSON.parse(body) : body; + }, + }; +}