diff --git a/packages/cli-app/src/exec.js b/packages/cli-app/src/exec.js index 9caa8672b..540d7b9d9 100644 --- a/packages/cli-app/src/exec.js +++ b/packages/cli-app/src/exec.js @@ -1,3 +1,5 @@ +import fs from 'fs'; +import os from 'os'; import path from 'path'; import command from '@percy/cli-command'; import * as ExecPlugin from '@percy/cli-exec'; @@ -23,6 +25,16 @@ function hasExistingPercyServerFlag(args) { return false; } +// Returns the index of the value following `--test-output-dir`, or -1 if absent. +// We return the value-index (not just a boolean) so the screenshot-dir helper +// can align PERCY_MAESTRO_SCREENSHOT_DIR with a customer-supplied flag value. +function findTestOutputDirValueIdx(args) { + for (let i = 2; i < args.length - 1; i++) { + if (args[i] === '--test-output-dir') return i + 1; + } + return -1; +} + // Maestro's GraalJS sandbox does NOT inherit the parent process's env, // so `PERCY_SERVER_ADDRESS` exported by app:exec is invisible to the // SDK. When wrapping `maestro test`, surface the CLI address through @@ -30,17 +42,91 @@ function hasExistingPercyServerFlag(args) { // healthcheck can find the local CLI without the customer having to // pair ports manually. No-op when the customer already supplied their // own `-e PERCY_SERVER=...`. -export function maybeInjectMaestroServer(ctx) { +// +// When percy?.address() is falsy (percy disabled, start failed), emit a +// WARN so the customer is not surprised by a silent zero-snapshot build. +// The customer-override skip case (their own `-e PERCY_SERVER=...` is in +// argv) does NOT warn — that's intentional flow control, not a problem. +export function maybeInjectMaestroServer(ctx, log) { const args = ctx?.argv; if (!Array.isArray(args) || args.length < 2) return; if (path.basename(args[0]) !== 'maestro') return; if (args[1] !== 'test') return; if (hasExistingPercyServerFlag(args)) return; const addr = ctx.percy?.address(); - if (!addr) return; + if (!addr) { + log?.warn( + 'app:exec did not start the Percy CLI server (percy disabled or start ' + + 'failed); -e PERCY_SERVER not injected into maestro test. Snapshots will ' + + 'NOT be uploaded. Set PERCY_TOKEN and re-run, or check the percy log above.' + ); + return; + } args.splice(2, 0, '-e', `PERCY_SERVER=${addr}`); } +// Auto-resolve the Maestro screenshot output directory so customers don't +// have to pair `export PERCY_MAESTRO_SCREENSHOT_DIR=...` with a matching +// `--test-output-dir ` in their maestro test command. +// +// Resolution order: +// 1. Customer set BOTH process.env.PERCY_MAESTRO_SCREENSHOT_DIR and +// --test-output-dir in argv → trust them, do nothing. +// 2. Customer set PERCY_MAESTRO_SCREENSHOT_DIR only → use it, inject +// `--test-output-dir ` into argv. +// 3. Customer set --test-output-dir only → use that value, mirror it +// into process.env.PERCY_MAESTRO_SCREENSHOT_DIR (so the SDK + +// CLI relay see the same path). +// 4. Neither set → pick `${process.cwd()}/.percy-out`. On any mkdir +// failure (read-only CWD, EACCES, EEXIST as a file), fall back to +// `${os.tmpdir()}/percy-maestro-` with a WARN log. +// +// The env-var update and argv splice always keep both sources of truth +// (SDK reads env var; Maestro reads the flag) aligned to the same path. +export function maybeInjectScreenshotDir(ctx, log) { + const args = ctx?.argv; + if (!Array.isArray(args) || args.length < 2) return; + if (path.basename(args[0]) !== 'maestro') return; + if (args[1] !== 'test') return; + + const envSet = !!process.env.PERCY_MAESTRO_SCREENSHOT_DIR; + const flagValueIdx = findTestOutputDirValueIdx(args); + const flagSet = flagValueIdx > 0; + + // Fully customer-controlled — nothing to do. + if (envSet && flagSet) return; + + let resolved; + if (envSet) { + resolved = process.env.PERCY_MAESTRO_SCREENSHOT_DIR; + } else if (flagSet) { + resolved = args[flagValueIdx]; + } else { + const preferred = path.join(process.cwd(), '.percy-out'); + try { + fs.mkdirSync(preferred, { recursive: true }); + resolved = preferred; + } catch (err) { + const fallback = path.join(os.tmpdir(), `percy-maestro-${process.pid}`); + try { + fs.mkdirSync(fallback, { recursive: true }); + } catch (_) { + // tmpdir mkdir failure is exceedingly rare; fall through and let + // downstream code surface a clearer error than this helper can. + } + resolved = fallback; + log?.warn( + `Could not create ${preferred} (${err.code || err.message}); ` + + `falling back to ${fallback}. Set PERCY_MAESTRO_SCREENSHOT_DIR to ` + + 'pick a specific location.' + ); + } + } + + if (!envSet) process.env.PERCY_MAESTRO_SCREENSHOT_DIR = resolved; + if (!flagSet) args.splice(2, 0, '--test-output-dir', resolved); +} + export const exec = command('exec', { description: 'Start and stop Percy around a supplied command for native apps', usage: '[options] -- ', @@ -56,7 +142,15 @@ export const exec = command('exec', { skipDiscovery: true } }, async function*(ctx) { - maybeInjectMaestroServer(ctx); + // Order matters: maestro-server splice goes in at index 2 first, then + // screenshot-dir splice goes in at index 2 too (pushing the server flags + // to index 4). Resulting argv for `maestro test flow.yaml` becomes: + // maestro test --test-output-dir -e PERCY_SERVER= flow.yaml + // Both flag groups land between `test` and the flow file, which Maestro + // accepts. Reversing the order would still work; this order is chosen + // for log readability. + maybeInjectMaestroServer(ctx, ctx.log); + maybeInjectScreenshotDir(ctx, ctx.log); yield* ExecPlugin.default.callback(ctx); }); diff --git a/packages/cli-app/src/index.js b/packages/cli-app/src/index.js index 4f95c7b7d..d1c8d4ffd 100644 --- a/packages/cli-app/src/index.js +++ b/packages/cli-app/src/index.js @@ -1,2 +1,2 @@ export { default, app } from './app.js'; -export { exec, start, stop, ping, maybeInjectMaestroServer } from './exec.js'; +export { exec, start, stop, ping, maybeInjectMaestroServer, maybeInjectScreenshotDir } from './exec.js'; diff --git a/packages/cli-app/test/exec.test.js b/packages/cli-app/test/exec.test.js index 7b859c8c4..be32f1ee9 100644 --- a/packages/cli-app/test/exec.test.js +++ b/packages/cli-app/test/exec.test.js @@ -1,6 +1,12 @@ +import fs from 'fs'; +import os from 'os'; +import path from 'path'; import { setupTest } from '@percy/cli-command/test/helpers'; import * as ExecPlugin from '@percy/cli-exec'; -import { exec, start, stop, ping, maybeInjectMaestroServer } from '@percy/cli-app'; +import { + exec, start, stop, ping, + maybeInjectMaestroServer, maybeInjectScreenshotDir +} from '@percy/cli-app'; describe('percy app:exec', () => { beforeEach(async () => { @@ -9,8 +15,8 @@ describe('percy app:exec', () => { it('wraps cli-exec callbacks while preserving differing definitions', async () => { // exec.callback wraps cli-exec's callback (auto-injects -e PERCY_SERVER - // for `maestro test`), so it is no longer reference-equal — but start, - // stop, and ping remain straight delegations. + // and --test-output-dir for `maestro test`), so it is no longer reference- + // equal — but start, stop, and ping remain straight delegations. expect(typeof exec.callback).toBe('function'); expect(exec.callback).not.toEqual(ExecPlugin.default.callback); expect(exec.definition).not.toEqual(ExecPlugin.default.definition); @@ -158,5 +164,190 @@ describe('percy app:exec', () => { expect(a.argv).not.toContain('PERCY_SERVER=http://localhost:5339'); expect(b.argv).not.toContain('PERCY_SERVER=http://localhost:5338'); }); + + it('emits WARN log when percy address is falsy and there is no customer override', () => { + const log = { warn: jasmine.createSpy('warn') }; + const ctx = { argv: ['maestro', 'test', 'flow.yaml'], percy: { address: () => undefined } }; + maybeInjectMaestroServer(ctx, log); + expect(log.warn).toHaveBeenCalledTimes(1); + expect(log.warn.calls.argsFor(0)[0]).toContain('-e PERCY_SERVER not injected'); + }); + + it('does NOT emit WARN when customer-supplied -e PERCY_SERVER override is present', () => { + const log = { warn: jasmine.createSpy('warn') }; + const ctx = { + argv: ['maestro', 'test', '-e', 'PERCY_SERVER=http://custom:9999', 'flow.yaml'], + percy: { address: () => undefined } + }; + maybeInjectMaestroServer(ctx, log); + expect(log.warn).not.toHaveBeenCalled(); + }); + }); + + describe('maybeInjectScreenshotDir', () => { + let originalEnvValue; + + beforeEach(() => { + originalEnvValue = process.env.PERCY_MAESTRO_SCREENSHOT_DIR; + delete process.env.PERCY_MAESTRO_SCREENSHOT_DIR; + }); + + afterEach(() => { + if (originalEnvValue === undefined) { + delete process.env.PERCY_MAESTRO_SCREENSHOT_DIR; + } else { + process.env.PERCY_MAESTRO_SCREENSHOT_DIR = originalEnvValue; + } + }); + + function ctxFor(argv) { + return { argv: [...argv] }; + } + + it('injects --test-output-dir and sets env var to /.percy-out on happy path', () => { + const mkdir = spyOn(fs, 'mkdirSync').and.callFake(() => {}); + const expectedDir = path.join(process.cwd(), '.percy-out'); + const ctx = ctxFor(['maestro', 'test', 'flow.yaml']); + maybeInjectScreenshotDir(ctx); + expect(mkdir).toHaveBeenCalledWith(expectedDir, { recursive: true }); + expect(process.env.PERCY_MAESTRO_SCREENSHOT_DIR).toBe(expectedDir); + expect(ctx.argv).toEqual([ + 'maestro', 'test', '--test-output-dir', expectedDir, 'flow.yaml' + ]); + }); + + it('honors customer-set PERCY_MAESTRO_SCREENSHOT_DIR and injects flag aligned to it', () => { + process.env.PERCY_MAESTRO_SCREENSHOT_DIR = '/custom/screenshot/dir'; + const mkdir = spyOn(fs, 'mkdirSync').and.callFake(() => {}); + const ctx = ctxFor(['maestro', 'test', 'flow.yaml']); + maybeInjectScreenshotDir(ctx); + expect(mkdir).not.toHaveBeenCalled(); + expect(process.env.PERCY_MAESTRO_SCREENSHOT_DIR).toBe('/custom/screenshot/dir'); + expect(ctx.argv).toEqual([ + 'maestro', 'test', '--test-output-dir', '/custom/screenshot/dir', 'flow.yaml' + ]); + }); + + it('honors customer-supplied --test-output-dir and mirrors value into env var', () => { + const mkdir = spyOn(fs, 'mkdirSync').and.callFake(() => {}); + const ctx = ctxFor(['maestro', 'test', '--test-output-dir', '/custom/path', 'flow.yaml']); + maybeInjectScreenshotDir(ctx); + expect(mkdir).not.toHaveBeenCalled(); + expect(process.env.PERCY_MAESTRO_SCREENSHOT_DIR).toBe('/custom/path'); + // argv unchanged — customer's flag stays where they put it + expect(ctx.argv).toEqual(['maestro', 'test', '--test-output-dir', '/custom/path', 'flow.yaml']); + }); + + it('skips entirely when both env var and --test-output-dir are customer-set', () => { + process.env.PERCY_MAESTRO_SCREENSHOT_DIR = '/from/env'; + const mkdir = spyOn(fs, 'mkdirSync').and.callFake(() => {}); + const argv = ['maestro', 'test', '--test-output-dir', '/from/flag', 'flow.yaml']; + const ctx = ctxFor(argv); + maybeInjectScreenshotDir(ctx); + expect(mkdir).not.toHaveBeenCalled(); + expect(process.env.PERCY_MAESTRO_SCREENSHOT_DIR).toBe('/from/env'); + expect(ctx.argv).toEqual(argv); + }); + + it('falls back to /percy-maestro- on EACCES and emits WARN', () => { + const mkdir = spyOn(fs, 'mkdirSync').and.callFake((dirPath) => { + if (dirPath === path.join(process.cwd(), '.percy-out')) { + const err = new Error('EACCES'); err.code = 'EACCES'; + throw err; + } + }); + const log = { warn: jasmine.createSpy('warn') }; + const ctx = ctxFor(['maestro', 'test', 'flow.yaml']); + maybeInjectScreenshotDir(ctx, log); + const fallback = path.join(os.tmpdir(), `percy-maestro-${process.pid}`); + expect(mkdir).toHaveBeenCalledWith(fallback, { recursive: true }); + expect(process.env.PERCY_MAESTRO_SCREENSHOT_DIR).toBe(fallback); + expect(ctx.argv).toEqual([ + 'maestro', 'test', '--test-output-dir', fallback, 'flow.yaml' + ]); + expect(log.warn).toHaveBeenCalledTimes(1); + expect(log.warn.calls.argsFor(0)[0]).toContain('EACCES'); + expect(log.warn.calls.argsFor(0)[0]).toContain(fallback); + }); + + it('falls back on EROFS (read-only filesystem)', () => { + spyOn(fs, 'mkdirSync').and.callFake((dirPath) => { + if (dirPath === path.join(process.cwd(), '.percy-out')) { + const err = new Error('EROFS'); err.code = 'EROFS'; + throw err; + } + }); + const log = { warn: jasmine.createSpy('warn') }; + const ctx = ctxFor(['maestro', 'test', 'flow.yaml']); + maybeInjectScreenshotDir(ctx, log); + const fallback = path.join(os.tmpdir(), `percy-maestro-${process.pid}`); + expect(process.env.PERCY_MAESTRO_SCREENSHOT_DIR).toBe(fallback); + expect(log.warn).toHaveBeenCalled(); + }); + + it('falls back on EEXIST (.percy-out collides with a non-directory)', () => { + spyOn(fs, 'mkdirSync').and.callFake((dirPath) => { + if (dirPath === path.join(process.cwd(), '.percy-out')) { + const err = new Error('EEXIST'); err.code = 'EEXIST'; + throw err; + } + }); + const log = { warn: jasmine.createSpy('warn') }; + const ctx = ctxFor(['maestro', 'test', 'flow.yaml']); + maybeInjectScreenshotDir(ctx, log); + const fallback = path.join(os.tmpdir(), `percy-maestro-${process.pid}`); + expect(process.env.PERCY_MAESTRO_SCREENSHOT_DIR).toBe(fallback); + expect(log.warn).toHaveBeenCalled(); + }); + + it('skips for `maestro hierarchy` (not a test command)', () => { + const mkdir = spyOn(fs, 'mkdirSync').and.callFake(() => {}); + const argv = ['maestro', 'hierarchy', '--udid', 'X']; + const ctx = ctxFor(argv); + maybeInjectScreenshotDir(ctx); + expect(mkdir).not.toHaveBeenCalled(); + expect(process.env.PERCY_MAESTRO_SCREENSHOT_DIR).toBeUndefined(); + expect(ctx.argv).toEqual(argv); + }); + + it('skips for `npx maestro test` (argv[0] is npx, not maestro)', () => { + const mkdir = spyOn(fs, 'mkdirSync').and.callFake(() => {}); + const argv = ['npx', 'maestro', 'test', 'flow.yaml']; + const ctx = ctxFor(argv); + maybeInjectScreenshotDir(ctx); + expect(mkdir).not.toHaveBeenCalled(); + expect(ctx.argv).toEqual(argv); + }); + + it('skips for non-maestro commands', () => { + const mkdir = spyOn(fs, 'mkdirSync').and.callFake(() => {}); + const argv = ['python', 'test.py']; + const ctx = ctxFor(argv); + maybeInjectScreenshotDir(ctx); + expect(mkdir).not.toHaveBeenCalled(); + expect(ctx.argv).toEqual(argv); + }); + + it('skips when args has fewer than two elements', () => { + const mkdir = spyOn(fs, 'mkdirSync').and.callFake(() => {}); + const argv = ['maestro']; + const ctx = ctxFor(argv); + maybeInjectScreenshotDir(ctx); + expect(mkdir).not.toHaveBeenCalled(); + expect(ctx.argv).toEqual(argv); + }); + + it('matches by basename when the command is an absolute path', () => { + const mkdir = spyOn(fs, 'mkdirSync').and.callFake(() => {}); + const expectedDir = path.join(process.cwd(), '.percy-out'); + const ctx = ctxFor(['/Users/foo/.maestro/bin/maestro', 'test', 'flow.yaml']); + maybeInjectScreenshotDir(ctx); + expect(mkdir).toHaveBeenCalledWith(expectedDir, { recursive: true }); + expect(ctx.argv).toEqual([ + '/Users/foo/.maestro/bin/maestro', 'test', + '--test-output-dir', expectedDir, + 'flow.yaml' + ]); + }); }); });