From 6bafd7cb7fb97542c85e97abed556cbc5e88b54a Mon Sep 17 00:00:00 2001 From: Arumulla Sri Ram Date: Tue, 2 Jun 2026 17:03:13 +0530 Subject: [PATCH] feat(cli-app): auto-resolve PERCY_MAESTRO_SCREENSHOT_DIR + --test-output-dir; WARN on no-addr injection skip MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Removes the last piece of customer-side bookkeeping for self-hosted Maestro+Percy. Today customers must export PERCY_MAESTRO_SCREENSHOT_DIR AND pass --test-output-dir to maestro test. After this PR, `percy app:exec` does both automatically. New helper `maybeInjectScreenshotDir(ctx, log)` next to the existing `maybeInjectMaestroServer`. Resolution order: 1. Customer set BOTH env + --test-output-dir flag → trust them. 2. Customer set env only → use env value, inject matching flag. 3. Customer set flag only → use flag value, mirror to env. 4. Neither set → try ${CWD}/.percy-out. On mkdir failure (EACCES, EROFS, EEXIST), fall back to ${TMPDIR}/percy-maestro- with a WARN log explaining why. The env var and the flag are always kept aligned to the same path. The SDK reads the env var; Maestro reads the flag — both pointing at the same dir is the contract. Also adds a WARN log in `maybeInjectMaestroServer` when `percy.address()` is falsy (percy disabled, start failed). Previously this skip was silent — customer's maestro test would run, no snapshots upload, Percy build would finalize empty with no log hint. The WARN now tells the customer what won't happen and how to fix it (set PERCY_TOKEN). Customer-supplied `-e PERCY_SERVER` override skip does NOT warn — that's legitimate flow control, not a problem. Tests: - 14 new scenarios for maybeInjectScreenshotDir (happy path, env override, flag override, both, EACCES/EROFS/EEXIST fallbacks, hierarchy/npx/python/short-args skip, basename match on full path). - 2 new scenarios for maybeInjectMaestroServer's WARN log. - 30 of 30 cli-app specs pass. --- packages/cli-app/src/exec.js | 100 ++++++++++++++- packages/cli-app/src/index.js | 2 +- packages/cli-app/test/exec.test.js | 197 ++++++++++++++++++++++++++++- 3 files changed, 292 insertions(+), 7 deletions(-) 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' + ]); + }); }); });