Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ Adding env variable support so that if set to true we exit with status code 0 when any percy related error occurs. #1647

Merged
merged 6 commits into from
Jul 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions packages/cli-command/src/command.js
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,12 @@ function withBuiltIns(definition) {
}

// Helper to throw an error with an exit code and optional reason message
function exit(exitCode, reason = '') {
function exit(exitCode, reason = '', shouldOverrideExitCode = true) {
let percyExitWithZeroOnError = process.env.PERCY_EXIT_WITH_ZERO_ON_ERROR === 'true';
exitCode = percyExitWithZeroOnError && shouldOverrideExitCode ? 0 : exitCode;
let err = reason instanceof Error ? reason : new Error(reason);
// Adding additional object so that it can be used in runner function below.
err.shouldOverrideExitCode = shouldOverrideExitCode;
throw Object.assign(err, { exitCode });
}

Expand Down Expand Up @@ -155,7 +159,10 @@ export function command(name, definition, callback) {
err.message ||= `EEXIT: ${err.exitCode}`;

if (definition.exitOnError) {
process.exit(err.exitCode);
let shouldOverrideExitCode = err.shouldOverrideExitCode !== false;
let percyExitWithZeroOnError = process.env.PERCY_EXIT_WITH_ZERO_ON_ERROR === 'true';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

possible to remove duplication?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried it but when setting const above the function the tests were failing.

let exitCode = percyExitWithZeroOnError && shouldOverrideExitCode ? 0 : err.exitCode;
process.exit(exitCode);
}

// re-throw when not exiting
Expand Down
84 changes: 81 additions & 3 deletions packages/cli-command/test/command.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,22 @@ describe('Command', () => {
});

await expectAsync(test()).toBeRejected();
expect(spy).toHaveBeenCalled();
expect(spy).toHaveBeenCalledWith(1);
});

it('handles maybe exiting on unhandled errors and exits with status 0 when PERCY_EXIT_WITH_ZERO_ON_ERROR is set to true', async () => {
process.env.PERCY_EXIT_WITH_ZERO_ON_ERROR = 'true';
let spy = spyOn(process, 'exit');

let test = command('test', {
exitOnError: true
}, () => {
throw new Error();
});

await expectAsync(test()).toBeRejected();
expect(spy).toHaveBeenCalledWith(0);
delete process.env.PERCY_EXIT_WITH_ZERO_ON_ERROR;
});

it('handles graceful exit errors', async () => {
Expand All @@ -180,8 +195,13 @@ describe('Command', () => {
message = new Error(message);
}

exit(code, message);
test.never = true;
if (code > 400) {
exit(code, message, false);
test.never = true;
} else {
exit(code, message);
test.never = true;
}
});

await test(['123']).catch(e => {
Expand All @@ -197,6 +217,7 @@ describe('Command', () => {
await test(['456', 'Reason']).catch(e => {
expect(e).toHaveProperty('message', 'Reason');
expect(e).toHaveProperty('exitCode', 456);
expect(e).toHaveProperty('shouldOverrideExitCode', false);
});

expect(test).not.toHaveProperty('never');
Expand All @@ -214,6 +235,63 @@ describe('Command', () => {
]);
});

it('exits with status 0 when PERCY_EXIT_WITH_ZERO_ON_ERROR is set to true', async () => {
process.env.PERCY_EXIT_WITH_ZERO_ON_ERROR = 'true';
let test = command('test', {
args: [{
name: 'code',
parse: Number,
required: true
}, {
name: 'message'
}, {
shouldOverrideExitCode: 'shouldOverrideExitCode'
}]
}, ({ args, exit }) => {
let { code, message } = args;

// exercise coverage
if (message && code >= 1) {
message = new Error(message);
}

if (code > 400) {
exit(code, message, false);
test.never = true;
} else {
exit(code, message);
test.never = true;
}
});

await test(['123']).catch(e => {
expect(e).toHaveProperty('exitCode', 0);
expect(e).toHaveProperty('shouldOverrideExitCode', true);
});

expect(test).not.toHaveProperty('never');
expect(logger.stderr).toEqual([]);

logger.reset();

await test(['123', 'Reason']).catch(e => {
expect(e).toHaveProperty('message', 'Reason');
expect(e).toHaveProperty('exitCode', 0);
expect(e).toHaveProperty('shouldOverrideExitCode', true);
});

await test(['401', 'some reason']).catch(e => {
expect(e).toHaveProperty('message', 'some reason');
expect(e).toHaveProperty('exitCode', 401);
expect(e).toHaveProperty('shouldOverrideExitCode', false);
});

expect(test).not.toHaveProperty('never');

logger.reset();
delete process.env.PERCY_EXIT_WITH_ZERO_ON_ERROR;
});

it('handles interrupting generator actions', async () => {
let sleep = (ms, v) => new Promise(r => setTimeout(r, ms, v));

Expand Down
7 changes: 4 additions & 3 deletions packages/cli-exec/src/exec.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,14 +46,14 @@ export const exec = command('exec', {
log.error("You must supply a command to run after '--'");
log.info('Example:');
log.info(' $ percy exec -- npm test');
exit(1);
exit(1, '', false);
}

// verify the provided command exists
let { default: which } = await import('which');

if (!which.sync(command, { nothrow: true })) {
exit(127, `Command not found "${command}"`);
exit(127, `Command not found "${command}"`, false);
}

// attempt to start percy if enabled
Expand Down Expand Up @@ -90,8 +90,9 @@ export const exec = command('exec', {
// stop percy if running (force stop if there is an error);
await percy?.stop(!!error);

log.info(`Command "${[command, ...args].join(' ')}" exited with status: ${status}`);
// forward any returned status code
if (status) exit(status, error);
if (status) exit(status, error, false);

// force exit post timeout
await waitForTimeout(10000);
Expand Down
6 changes: 4 additions & 2 deletions packages/cli-exec/test/exec.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,8 @@ describe('percy exec', () => {
'[percy] Percy has started!',
'[percy] Running "node --eval "',
'[percy] Finalized build #1: https://percy.io/test/test/123',
"[percy] Build's CLI logs sent successfully. Please share this log ID with Percy team in case of any issues - random_sha"
"[percy] Build's CLI logs sent successfully. Please share this log ID with Percy team in case of any issues - random_sha",
'[percy] Command "node --eval " exited with status: 0'
]);
});

Expand All @@ -112,7 +113,8 @@ describe('percy exec', () => {
'[percy] Percy is disabled'
]);
expect(logger.stdout).toEqual([
'[percy] Running "node --eval "'
'[percy] Running "node --eval "',
'[percy] Command "node --eval " exited with status: 0'
]);
});

Expand Down
Loading