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.

Sign up for GitHub

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

Jump to bottom

Don't abort when rule throws, but continue #87

Merged
merged 5 commits into from
Jun 27, 2024
Merged

Don't abort when rule throws, but continue #87

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ac1954b
Don't abort when rule throws, but continue
mcmire May 24, 2024
b5f2b6e
Revert output format back
mcmire Jun 5, 2024
b206993
Change name of variable back
mcmire Jun 6, 2024
6bfe0c8
Update src/main.ts
mcmire Jun 7, 2024
57881bd
Merge branch 'main' into catch-errors-in-rules
mcmire Jun 18, 2024
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
13 changes: 7 additions & 6 deletions src/build-rule-tree.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import * as dependencyGraphModule from 'dependency-graph';

import { buildRuleTree } from './build-rule-tree';
import type { Rule } from './execute-rules';
import { RuleExecutionStatus } from './execute-rules';

jest.mock('dependency-graph', () => {
return {
Expand All @@ -19,7 +20,7 @@ describe('buildRuleTree', () => {
dependencies: ['rule-2'],
execute: async () => {
return {
passed: true,
status: RuleExecutionStatus.Passed,
};
},
};
Expand All @@ -29,7 +30,7 @@ describe('buildRuleTree', () => {
dependencies: ['rule-3'],
execute: async () => {
return {
passed: true,
status: RuleExecutionStatus.Passed,
};
},
};
Expand All @@ -39,7 +40,7 @@ describe('buildRuleTree', () => {
dependencies: [],
execute: async () => {
return {
passed: true,
status: RuleExecutionStatus.Passed,
};
},
};
Expand Down Expand Up @@ -74,7 +75,7 @@ describe('buildRuleTree', () => {
dependencies: ['rule-2'],
execute: async () => {
return {
passed: false,
status: RuleExecutionStatus.Failed,
failures: [{ message: 'Oops' }],
};
},
Expand All @@ -85,7 +86,7 @@ describe('buildRuleTree', () => {
dependencies: ['rule-3'],
execute: async () => {
return {
passed: true,
status: RuleExecutionStatus.Passed,
};
},
};
Expand All @@ -95,7 +96,7 @@ describe('buildRuleTree', () => {
dependencies: ['rule-1'],
execute: async () => {
return {
passed: true,
status: RuleExecutionStatus.Passed,
};
},
};
Expand Down
14 changes: 10 additions & 4 deletions src/cli.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,13 @@ main({
cachedRepositoriesDirectoryPath: DEFAULT_CACHED_REPOSITORIES_DIRECTORY_PATH,
defaultProjectNames: DEFAULT_PROJECT_NAMES,
},
}).catch((error) => {
console.error(error);
process.exitCode = 1;
});
})
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Previously, main would set process.exitCode. However, to remain testable, I'm trying to keep main free of changing global state, so that means changing process. To accommodate this constraint, I had main return true or false, which we can then use to set process.exitCode accordingly.

.then((isSuccessful) => {
if (!isSuccessful) {
process.exitCode = 1;
}
})
.catch((error) => {
console.error(error);
process.exitCode = 1;
});
73 changes: 70 additions & 3 deletions src/execute-rules.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,14 +48,14 @@ describe('executeRules', () => {
result: {
ruleName: 'rule-2',
ruleDescription: 'Description for rule 2',
passed: true,
status: 'passed',
},
children: [
expect.objectContaining({
result: {
ruleName: 'rule-1',
ruleDescription: 'Description for rule 1',
passed: false,
status: 'failed',
failures: [{ message: 'Oops' }],
},
children: [],
Expand Down Expand Up @@ -100,7 +100,7 @@ describe('executeRules', () => {
result: {
ruleName: 'rule-2',
ruleDescription: 'Description for rule 2',
passed: false,
status: 'failed',
failures: [{ message: 'Oops' }],
},
children: [],
Expand All @@ -109,6 +109,73 @@ describe('executeRules', () => {
});
});

it('does not prevent other rules from running if a rule throws', async () => {
const rules: Rule[] = [
{
name: 'rule-1',
description: 'Description for rule 1',
dependencies: [],
execute: async ({ pass: locallyPass }) => {
return locallyPass();
},
},
{
name: 'rule-2',
description: 'Description for rule 2',
dependencies: [],
execute: async () => {
throw new Error('oops');
},
},
{
name: 'rule-3',
description: 'Description for rule 3',
dependencies: [],
execute: async ({ pass: locallyPass }) => {
return locallyPass();
},
},
];
const project = mockDeep<MetaMaskRepository>();
const template = mockDeep<MetaMaskRepository>();

const ruleExecutionResultTree = await executeRules({
rules,
project,
template,
});

expect(ruleExecutionResultTree).toMatchObject({
children: [
expect.objectContaining({
result: {
ruleName: 'rule-1',
ruleDescription: 'Description for rule 1',
status: 'passed',
},
children: [],
}),
expect.objectContaining({
result: {
ruleName: 'rule-2',
ruleDescription: 'Description for rule 2',
status: 'errored',
error: new Error('oops'),
},
children: [],
}),
expect.objectContaining({
result: {
ruleName: 'rule-3',
ruleDescription: 'Description for rule 3',
status: 'passed',
},
children: [],
}),
],
});
});

it('records the time to run each rule, exclusive and inclusive of its children', async () => {
jest.setSystemTime(new Date('2023-01-01T00:00:00Z'));

Expand Down
54 changes: 42 additions & 12 deletions src/execute-rules.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,22 @@ import { fail, pass } from './rule-helpers';

const log = createModuleLogger(projectLogger, 'establish-metamask-repository');

/**
* The status of a rule's execution. Used to determine how to present that rule
* in the ending report.
*/
export enum RuleExecutionStatus {
Passed = 'passed',
Failed = 'failed',
Errored = 'errored',
}

/**
* Represents a successfully executed rule. ("Partial" because a full result
* include the name and description of the rule.)
*/
export type SuccessfulPartialRuleExecutionResult = {
passed: true;
status: RuleExecutionStatus.Passed;
};

/**
Expand All @@ -24,21 +34,31 @@ export type RuleExecutionFailure = {
};

/**
* Represents an unsuccessfully executed rule. ("Partial" because a full result
* Represents a rule that returned a failure. ("Partial" because a full result
* include the name and description of the rule.)
*/
export type FailedPartialRuleExecutionResult = {
passed: false;
failures: RuleExecutionFailure[];
status: RuleExecutionStatus.Failed;
};

/**
* Represents a rule that threw an error while executing. ("Partial" because a
* full result include the name and description of the rule.)
*/
export type ErroredPartialRuleExecutionResult = {
error: unknown;
status: RuleExecutionStatus.Errored;
};

/**
* Represents the result of an executed rule. ("Partial" because a full result
* include the name and description of the rule.)
*/
export type PartialRuleExecutionResult =
| SuccessfulPartialRuleExecutionResult
| FailedPartialRuleExecutionResult;
| ErroredPartialRuleExecutionResult
| FailedPartialRuleExecutionResult
| SuccessfulPartialRuleExecutionResult;

/**
* All of the information that designates the result of a rule execution.
Expand Down Expand Up @@ -194,13 +214,22 @@ async function executeRule({
}): Promise<RuleExecutionResultNode> {
log('Running rule', ruleNode.rule.name);
const startDate = new Date();
let partialRuleExecutionResult: PartialRuleExecutionResult;

try {
partialRuleExecutionResult = await ruleNode.rule.execute({
project,
template,
pass,
fail,
});
} catch (error) {
partialRuleExecutionResult = {
status: RuleExecutionStatus.Errored,
error,
};
}

const partialRuleExecutionResult = await ruleNode.rule.execute({
project,
template,
pass,
fail,
});
const ruleExecutionResult: RuleExecutionResult = {
ruleName: ruleNode.rule.name,
ruleDescription: ruleNode.rule.description,
Expand All @@ -214,7 +243,8 @@ async function executeRule({
const endDateBeforeChildren = new Date();

const children: RuleExecutionResultNode[] =
ruleExecutionResult.passed && ruleNode.children.length > 0
ruleExecutionResult.status === RuleExecutionStatus.Passed &&
ruleNode.children.length > 0
? await executeRuleNodes({
ruleNodes: ruleNode.children,
project,
Expand Down
9 changes: 5 additions & 4 deletions src/lint-project.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { mockDeep } from 'jest-mock-extended';

import type { MetaMaskRepository } from './establish-metamask-repository';
import type { Rule } from './execute-rules';
import { RuleExecutionStatus } from './execute-rules';
import { lintProject } from './lint-project';
import { FakeOutputLogger } from '../tests/fake-output-logger';
import { fakeDateOnly, withinSandbox } from '../tests/helpers';
Expand Down Expand Up @@ -45,7 +46,7 @@ describe('lintProject', () => {
execute: async () => {
jest.setSystemTime(new Date('2023-01-01T00:00:02Z'));
return {
passed: false,
status: RuleExecutionStatus.Failed,
failures: [{ message: 'Oops' }],
};
},
Expand All @@ -57,7 +58,7 @@ describe('lintProject', () => {
execute: async () => {
jest.setSystemTime(new Date('2023-01-01T00:00:01Z'));
return {
passed: true,
status: RuleExecutionStatus.Passed,
};
},
},
Expand All @@ -83,14 +84,14 @@ describe('lintProject', () => {
result: {
ruleName: 'rule-2',
ruleDescription: 'Description for rule 2',
passed: true,
status: 'passed',
},
children: [
expect.objectContaining({
result: {
ruleName: 'rule-1',
ruleDescription: 'Description for rule 1',
passed: false,
status: 'failed',
failures: [{ message: 'Oops' }],
},
children: [],
Expand Down
Loading
Loading