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

If a project is being linted and a rule throws an error — perhaps it
relies on a file to be in a certain format, or some other reason — then
linting for that project will halt, and the lint report for that project
will only show that error. An error like this signifies a bug in
`module-lint`, but sometimes we can't fix that bug right away, and in
the meantime this makes the report useless, since we don't know whether
any of the other rules passed or not.

This commit fixes this problem by catching errors inside of rules and
displaying them as such. To implement this change, we needed to convert
the binary status of a rule to a trinary status ("passed", "failed",
"errored"). We also updated the output of the lint report to display
this error status and distinguish it from a failed status.

---------

Co-authored-by: Kanthesha Devaramane <kanthesha.devaramane@consensys.net>

src/build-rule-tree.test.ts

@@ -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 {
@@ -19,7 +20,7 @@ describe('buildRuleTree', () => {
       dependencies: ['rule-2'],
       execute: async () => {
         return {
-          passed: true,
+          status: RuleExecutionStatus.Passed,
         };
       },
     };
@@ -29,7 +30,7 @@ describe('buildRuleTree', () => {
       dependencies: ['rule-3'],
       execute: async () => {
         return {
-          passed: true,
+          status: RuleExecutionStatus.Passed,
         };
       },
     };
@@ -39,7 +40,7 @@ describe('buildRuleTree', () => {
       dependencies: [],
       execute: async () => {
         return {
-          passed: true,
+          status: RuleExecutionStatus.Passed,
         };
       },
     };
@@ -74,7 +75,7 @@ describe('buildRuleTree', () => {
       dependencies: ['rule-2'],
       execute: async () => {
         return {
-          passed: false,
+          status: RuleExecutionStatus.Failed,
           failures: [{ message: 'Oops' }],
         };
       },
@@ -85,7 +86,7 @@ describe('buildRuleTree', () => {
       dependencies: ['rule-3'],
       execute: async () => {
         return {
-          passed: true,
+          status: RuleExecutionStatus.Passed,
         };
       },
     };
@@ -95,7 +96,7 @@ describe('buildRuleTree', () => {
       dependencies: ['rule-1'],
       execute: async () => {
         return {
-          passed: true,
+          status: RuleExecutionStatus.Passed,
         };
       },
     };

src/cli.ts

@@ -14,7 +14,13 @@ main({
     cachedRepositoriesDirectoryPath: DEFAULT_CACHED_REPOSITORIES_DIRECTORY_PATH,
     defaultProjectNames: DEFAULT_PROJECT_NAMES,
   },
-}).catch((error) => {
-  console.error(error);
-  process.exitCode = 1;
-});
+})
+  .then((isSuccessful) => {
+    if (!isSuccessful) {
+      process.exitCode = 1;
+    }
+  })
+  .catch((error) => {
+    console.error(error);
+    process.exitCode = 1;
+  });

src/execute-rules.test.ts

@@ -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: [],
@@ -100,7 +100,7 @@ describe('executeRules', () => {
           result: {
             ruleName: 'rule-2',
             ruleDescription: 'Description for rule 2',
-            passed: false,
+            status: 'failed',
             failures: [{ message: 'Oops' }],
           },
           children: [],
@@ -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'));
 

src/execute-rules.ts

@@ -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;
 };
 
 /**
@@ -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.
@@ -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,
@@ -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,

src/lint-project.test.ts

@@ -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';
@@ -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' }],
             };
           },
@@ -57,7 +58,7 @@ describe('lintProject', () => {
           execute: async () => {
             jest.setSystemTime(new Date('2023-01-01T00:00:01Z'));
             return {
-              passed: true,
+              status: RuleExecutionStatus.Passed,
             };
           },
         },
@@ -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: [],