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

Expose E2E build errors #940

Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions node-src/lib/setExitCode.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@ export const exitCodes = {
STORYBOOK_START_FAILED: 22,
STORYBOOK_BROKEN: 23,

// E2E errors
E2E_BUILD_FAILED: 51,

tevanoff marked this conversation as resolved.
Show resolved Hide resolved
// Subprocess errors
GIT_NOT_CLEAN: 101,
GIT_OUT_OF_DATE: 102,
Expand Down
42 changes: 42 additions & 0 deletions node-src/tasks/build.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -166,3 +166,45 @@ describe('buildStorybook', () => {
);
});
});

describe('buildStorybook E2E', () => {
it('fails with missing dependency error when dependency not installed', async () => {
tevanoff marked this conversation as resolved.
Show resolved Hide resolved
const ctx = {
buildCommand: 'npm exec build-archive-storybook',
options: { buildScriptName: '', playwright: true },
env: { STORYBOOK_BUILD_TIMEOUT: 0 },
log: { debug: vi.fn(), error: vi.fn() },
} as any;

const missingDependencyErrorMessages = [
'Command not found: build-archive-storybook',
'Command "build-archive-storybook" not found',
'NPM error code E404\n\nMore error info',
'Command failed with exit code 1: npm exec build-archive-storybook --output-dir /tmp/chromatic--4210-0cyodqfYZabe'
];

for(const errorMessage of missingDependencyErrorMessages) {
command.mockRejectedValueOnce(new Error(errorMessage));
await expect(buildStorybook(ctx)).rejects.toThrow('Command failed');
expect(ctx.log.error).toHaveBeenCalledWith(expect.stringContaining('Failed to import `@chromatic-com/playwright`'));

ctx.log.error.mockClear();
};
});

it('fails with generic error message when not missing dependency error', async () => {
const ctx = {
buildCommand: 'npm exec build-archive-storybook',
options: { buildScriptName: '', playwright: true },
env: { STORYBOOK_BUILD_TIMEOUT: 0 },
log: { debug: vi.fn(), error: vi.fn() },
} as any;

const errorMessage = 'Command failed with exit code 1: npm exec build-archive-storybook --output-dir /tmp/chromatic--4210-0cyodqfYZabe\n\nMore error message lines\n\nAnd more';
skitterm marked this conversation as resolved.
Show resolved Hide resolved
command.mockRejectedValueOnce(new Error(errorMessage));
await expect(buildStorybook(ctx)).rejects.toThrow('Command failed');
expect(ctx.log.error).not.toHaveBeenCalledWith(expect.stringContaining('Failed to import `@chromatic-com/playwright`'));
expect(ctx.log.error).toHaveBeenCalledWith(expect.stringContaining('Failed to run `chromatic --playwright`'));
expect(ctx.log.error).toHaveBeenCalledWith(expect.stringContaining(errorMessage));
});
});
47 changes: 35 additions & 12 deletions node-src/tasks/build.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@
import buildFailed from '../ui/messages/errors/buildFailed';
import { failed, initial, pending, skipped, success } from '../ui/tasks/build';
import { getPackageManagerRunCommand } from '../lib/getPackageManager';
import { buildBinName as e2EbuildBinName, getE2EBuildCommand, isE2EBuild } from '../lib/e2e';
import { buildBinName as e2eBuildBinName, getE2EBuildCommand, isE2EBuild } from '../lib/e2e';
import e2eBuildFailed from '../ui/messages/errors/e2eBuildFailed';
import missingDependency from '../ui/messages/errors/missingDependency';

export const setSourceDir = async (ctx: Context) => {
Expand Down Expand Up @@ -60,6 +61,34 @@
const timeoutAfter = (ms) =>
new Promise((resolve, reject) => setTimeout(reject, ms, new Error(`Operation timed out`)));

function isE2EBuildCommandNotFoundError(errorMessage: string) {
// It's hard to know if this is the case as each package manager has a different type of
// error for this, but we'll try to figure it out.
const errorRegexes = ['command not found', `[\\W]?${e2eBuildBinName}[\\W]? not found`, 'code E404', 'exit code 127', `command failed.*${e2eBuildBinName}.*$`];
return errorRegexes.some((regex) => errorMessage.match(new RegExp(regex, 'gi')));

Check warning on line 68 in node-src/tasks/build.ts

View check run for this annotation

Codacy Production / Codacy Static Code Analysis

node-src/tasks/build.ts#L68

RegExp() called with a `regex` function argument, this might allow an attacker to cause a Regular Expression Denial-of-Service (ReDoS) within your application as RegExP blocks the main thread.

Check warning on line 68 in node-src/tasks/build.ts

View check run for this annotation

Codacy Production / Codacy Static Code Analysis

node-src/tasks/build.ts#L68

The `RegExp` constructor was called with a non-literal value.
Copy link
Member

@tmeasday tmeasday Mar 7, 2024

Choose a reason for hiding this comment

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

This is decent advice (be careful about dynamic regexps) from Codacy but the solution doesn't make sense to me -- I could be off though. cc @paulelliott I think the AI has jumped off the deep end a bit:

a) you can't do regex.replace('${e2eBuildBinName}',... in a previously interpolated string regex, it's already been replaced.
b) it doesn't change anything to do it at that stage anyway (say if we changed the original string :

// from
`[\\W]?${e2eBuildBinName}[\\W]? not found`

// to 
'[\\W]?${e2eBuildBinName}[\\W]? not found'

Then the string wouldn't automatically get interpolated with the var, but I don't understand why it would be better to later do a .replace() to manually interpolate.

Copy link
Member

Choose a reason for hiding this comment

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

The other piece of advice it had was to use the RE2 library from google to parse user-supplied regex in a safe way. That feels like an easy change to make and right way to resolve these issues. The initial comment it put on the PR yesterday said that and I'm not sure why it shifted to this. You can see that advice on the second warning on the Codacy app.

https://app.codacy.com/gh/chromaui/chromatic-cli/pullRequest?prid=13786760

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is good advice, however, the e2eBuildBinName that it is calling out is a simple string constant, not user-controlled input, so this is all moot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tmeasday I can't make sense of that suggestion to use .replace() either 🤔

Copy link
Member

@tmeasday tmeasday Mar 7, 2024

Choose a reason for hiding this comment

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

@paulelliott my feeling is the AI is getting really wordy here and I am glazing over and missing the key points:

  1. Be careful about dynamic strings in regexps
  2. You can comment away the warning
  3. However, be super careful if you are going to do that, and document why it's safe.

Copy link
Member

Choose a reason for hiding this comment

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

When I enabled this I just blindly turned the whole thing on. It is quite configurable in the application so if there are rules we disagree with we can easily turn them off per repo. You can also dismiss this specific warning in their interface and it keeps a log of all the specific places it was ignored.

}

function e2eBuildErrorMessage(err, workingDir: string, ctx: Context): { exitCode: number, message: string } {
const flag = ctx.options.playwright ? 'playwright' : 'cypress';
const errorMessage = err.message;

// If we tried to run the E2E package's bin directly (due to being in the action)
// and it failed, that means we couldn't find it. This probably means they haven't
// installed the right dependency or run from the right directory.
if (isE2EBuildCommandNotFoundError(errorMessage)) {
const dependencyName = `@chromatic-com/${flag}`;
return {
exitCode: exitCodes.MISSING_DEPENDENCY,
message: missingDependency({ dependencyName, flag, workingDir }),
};
}

return {
exitCode: exitCodes.E2E_BUILD_FAILED,
message: e2eBuildFailed({ flag, errorMessage }),
};
}

export const buildStorybook = async (ctx: Context) => {
let logFile = null;
if (ctx.options.storybookLogFile) {
Expand All @@ -77,7 +106,7 @@
ctx.log.debug('Runtime metadata:', JSON.stringify(ctx.runtimeMetadata, null, 2));

const subprocess = execaCommand(ctx.buildCommand, {
stdio: [null, logFile, logFile],
stdio: [null, logFile, null],
tevanoff marked this conversation as resolved.
Show resolved Hide resolved
signal,
env: { NODE_ENV: ctx.env.STORYBOOK_NODE_ENV || 'production' },
});
Expand All @@ -86,16 +115,10 @@
// If we tried to run the E2E package's bin directly (due to being in the action)
// and it failed, that means we couldn't find it. This probably means they haven't
// installed the right dependency or run from the right directory
if (
ctx.options.inAction &&
(isE2EBuild(ctx.options)) &&
e.message.match(e2EbuildBinName)
) {
const flag = ctx.options.playwright ? 'playwright' : 'cypress';
const dependencyName = `@chromatic-com/${flag}`;
ctx.log.error(missingDependency({ dependencyName, flag, workingDir: process.cwd() }));
ctx.log.debug(e);
setExitCode(ctx, exitCodes.MISSING_DEPENDENCY, true);
if (isE2EBuild(ctx.options)) {
const errorInfo = e2eBuildErrorMessage(e, process.cwd(), ctx);
ctx.log.error(errorInfo.message);
setExitCode(ctx, errorInfo.exitCode, true);
throw new Error(failed(ctx).output);
}

Expand Down
8 changes: 8 additions & 0 deletions node-src/ui/messages/errors/e2eBuildFailed.stories.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import e2eBuildFailed from './e2eBuildFailed';

export default {
title: 'CLI/Messages/Errors',
};

export const E2EBuildFailed = () =>
e2eBuildFailed({ flag: 'playwright', errorMessage: 'Error Message' });
17 changes: 17 additions & 0 deletions node-src/ui/messages/errors/e2eBuildFailed.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import chalk from 'chalk';
import { dedent } from 'ts-dedent';
import { error } from '../../components/icons';

export default ({
flag,
errorMessage,
}: {
flag: string;
errorMessage: string;
}) => {
return dedent(chalk`
${error} Failed to run \`chromatic --${flag}\`:

${errorMessage}
tevanoff marked this conversation as resolved.
Show resolved Hide resolved
`);
};