allow non-eip changes

This implements a few features:
- eip authors can also make changes to the assets file which will allow any changes so long as the eip passes; i.e. I can do whatever I want to assets/eip-2/.. if I (at the same time) make changes to EIPs/eip-2.md and that is eligible for auto-merge.
- if an editor approves then any non eip file will pass

I also made a few cleanups
- better error handling
- cleaner code
- more tests
- conitnuing transition to class based approach (I'm converting old style to new style whenever I make changes to that file

package.json

@@ -31,7 +31,7 @@
     "nock": "^13.1.1",
     "prettier": "^2.4.1",
     "tsconfig-paths-jest": "^0.0.1",
-    "type-fest": "^2.3.3"
+    "type-fest": "^2.5.4"
   },
   "devDependencies": {
     "@babel/preset-env": "^7.15.6",

src/__tests__/Integration.test.ts

@@ -172,14 +172,35 @@ describe("integration testing edgecases associated with editors", () => {
   });
 
   describe("Pull 3623", () => {
-    it("should pass", async() => {
+    it("should pass", async () => {
       process.env = envFactory({
         PULL_NUMBER: SavedRecord.PR3623,
         [EIPTypeOrCategoryToResolver[EIPCategory.erc]]: "@micahzoltu"
       });
 
       await __MAIN_MOCK__();
       expect(setFailedMock).not.toBeCalled();
-    })
-  })
+    });
+  });
+
+  describe("Pull 3581", () => {
+    it("should pass", async () => {
+      process.env = envFactory({
+        PULL_NUMBER: SavedRecord.PR3581,
+        [EIPTypeOrCategoryToResolver[EIPCategory.erc]]: "@lightclient"
+      });
+
+      await __MAIN_MOCK__();
+      expect(setFailedMock).not.toBeCalled();
+    });
+    it("should not pass if no editor approval", async () => {
+      process.env = envFactory({
+        PULL_NUMBER: SavedRecord.PR3581,
+        // in this case I'm not setting the approver as an editor
+      });
+
+      await __MAIN_MOCK__();
+      expect(setFailedMock).toHaveBeenCalled();
+    });
+  });
 });

src/domain/Regex.ts

@@ -6,6 +6,8 @@ export const AUTHOR_RE = /[(<]([^>)]+)[>)]/gm;
 export const EIP_NUM_RE = /eip-(\d+)\.md/;
 /** matches github handles (includes @)*/
 export const GITHUB_HANDLE = /^@[a-z\d](?:[a-z\d]|-(?=[a-z\d])){0,38}$/i;
+/** extracts the eip number of the assets folder associated */
+export const ASSETS_EIP_NUM = /(?<=^assets\/eip-)(\d+)(?=\/.*)/
 
 /**
  * This functionality is supported in es2020, but for the purposes

src/domain/Types.ts

@@ -17,7 +17,7 @@ export type Commit = PromiseValue<
 export type Files = PromiseValue<
   ReturnType<Github["pulls"]["listFiles"]>
 >["data"];
-export type File = UnArrayify<Files>;
+export type File = Files[number];
 export type CommitFiles = CompareCommits["base_commit"]["files"];
 export type CommitFile = UnArrayify<NonNullable<CommitFiles>>;
 export type Repo = PromiseValue<ReturnType<Github["repos"]["get"]>>["data"];
@@ -152,6 +152,7 @@ export function isMockMethod(method): asserts method is MockMethods {
 
 export type Results = {
   filename: string;
+  successMessage?: string;
   errors?: string[];
   mentions?: string[];
 }[];

src/domain/exceptions.ts

@@ -1,5 +1,11 @@
 import { Maybe } from "./Types";
 
+export enum Exceptions {
+  unexpectedError = "Unexpected Error",
+  requirementViolation = "Requirement Violation",
+  gracefulTermination = "Graceful Termination"
+}
+
 export class UnexpectedError {
   public readonly type = "Unexpected Error";
   constructor(public error: Maybe<string> = null, public data: Maybe<any> = null){}
@@ -9,3 +15,39 @@ export class RequirementViolation {
   public readonly type = "Requirement Violation";
   constructor(public error: Maybe<string> = null, public data: Maybe<any> = null){}
 }
+
+/**
+ *  this terminates the program gracefully, meaning that it will not be treated
+ *  as an error. This is useful in cases where an invariant violation does not
+ *  necessarily mean that the test fails.
+ * */
+export class GracefulTermination {
+  public readonly type = "Graceful Termination"
+  constructor(public error: Maybe<string> = null, public data: Maybe<any> = null){}
+}
+
+type Handlers = {[key in keyof typeof Exceptions]: (message: string) => any}
+
+/** this is written out on purpose to allow for easier changes where necessary */
+export const processError = (err: any, {
+  gracefulTermination,
+  unexpectedError,
+  requirementViolation
+}:Handlers) => {
+  if (err?.type === Exceptions.gracefulTermination) {
+    console.log(JSON.stringify(err.data, null, 2));
+    return gracefulTermination(err.error);
+  }
+
+  if (err?.type === Exceptions.requirementViolation) {
+    console.log(JSON.stringify(err.data, null, 2));
+    return requirementViolation(err.error);
+  }
+
+  if (err?.type === Exceptions.unexpectedError) {
+    console.log(JSON.stringify(err.data, null, 2));
+    return unexpectedError(err.error);
+  }
+
+  throw err
+}

src/main.ts

@@ -36,6 +36,7 @@ import {
 import { get, uniq } from "lodash";
 import { requestReviewers } from "#/approvals";
 import { getFileDiff } from "#/file";
+import { processError } from "src/domain/exceptions";
 
 const testFile = async (file: File): Promise<TestResults> => {
   // we need to define this here because the below logic can get very complicated otherwise
@@ -47,7 +48,17 @@ const testFile = async (file: File): Promise<TestResults> => {
   try {
     file = await requireFilePreexisting(file);
   } catch (err: any) {
-    errors.fileErrors.filePreexistingError = err;
+    processError(err, {
+      gracefulTermination: () => {
+        throw err
+      },
+      requirementViolation: (message) => {
+        errors.fileErrors.filePreexistingError = message;
+      },
+      unexpectedError: () => {
+        throw err
+      }
+    })
     errors.approvalErrors.isEditorApprovedError = await assertEIPEditorApproval(
       fileDiff
     );
@@ -65,7 +76,7 @@ const testFile = async (file: File): Promise<TestResults> => {
   );
   errors.approvalErrors.enoughEditorApprovalsForEIP1Error =
     await assertEIP1EditorApprovals(fileDiff);
-  errors.fileErrors.validFilenameError = assertValidFilename(file);
+  errors.fileErrors.validFilenameError = await assertValidFilename(file);
   errors.headerErrors.matchingEIPNumError =
     assertFilenameAndFileNumbersMatch(fileDiff);
   errors.headerErrors.constantEIPNumError = assertConstantEipNumber(fileDiff);
@@ -144,24 +155,25 @@ const _getMentions =
 
 const getMentions = _getMentions(getEditorMentions, getAuthorMentions);
 
-const getCommentMessage = (results: Results) => {
-  if (!results.length) return "There were no results";
+const getCommentMessage = (results: Results, header?: string) => {
+  if (!results.length) return "There were no results cc @alita-moore";
   const comment: string[] = [];
 
-  comment.push(COMMENT_HEADER);
+  comment.push(header || COMMENT_HEADER);
   comment.push("---");
-  for (const { filename, errors } of results) {
-    comment.push(`## ${filename}`);
+  for (const { filename, errors, successMessage } of results) {
     if (!errors) {
-      comment.push(`\t passed!`);
+      comment.push(`## (pass) ${filename}`);
+      const message = `\t` + (successMessage || "passed!");
+      comment.push(message);
       continue;
     }
 
+    comment.push(`## (fail) ${filename}`);
     for (const error of errors) {
-      comment.push(`- ${error}`);
+      comment.push(`\t- ${error}`);
     }
   }
-  7;
   return comment.join("\n");
 };
 
@@ -204,11 +216,42 @@ export const _main_ = async () => {
 
   // Collect the changes made in the given PR from base <-> head for eip files
   const files = await requireFiles(pr);
-  const results: Results = await Promise.all(files.map(getFileTestResults));
+  let results: Results = [];
+  for await (const file of files) {
+    try {
+      const res = await getFileTestResults(file);
+      results.push(res);
+    } catch (err: any) {
+      processError(err, {
+        gracefulTermination: (message) => {
+          results.push({
+            filename: file.filename,
+            successMessage: message
+          });
+        },
+        requirementViolation: (message) => {
+          results.push({
+            filename: file.filename,
+            errors: [message]
+          });
+        },
+        unexpectedError: (message) => {
+          results.push({
+            filename: file.filename,
+            errors: [message]
+          });
+        }
+      });
+    }
+  }
 
   if (!results.filter((res) => res.errors).length) {
-    await postComment("All tests passed; auto-merging...");
-    console.log("All tests passed; auto-merging...");
+    const commentMessage = getCommentMessage(
+      results,
+      "All tests passed; auto-merging..."
+    );
+    await postComment(commentMessage);
+    console.log(commentMessage);
     return;
   }
 
@@ -228,7 +271,7 @@ export const main = async () => {
   try {
     return await _main_();
   } catch (error: any) {
-    console.log(`An Exception Occured While Linting: \n${error}`);
+    console.log(`An unexpected exception occured while linting: \n${error}`);
     setFailed(error.message);
     if (isProd) {
       await postComment(error.message);

src/modules/assertions/Domain/types.ts

@@ -6,7 +6,17 @@ export interface IRequireEditors {
   requireEIPEditors: (fileDiff: FileDiff) => string[];
 }
 
-export type PreexistingFile = Opaque<File>
+export type PreexistingFile = Opaque<File>;
 export interface IRequireFilePreexisting {
   requireFilePreexisting: (fileDiff: File) => Promise<PreexistingFile>;
 }
+
+export interface IAssertValidFilename {
+  assertValidFilename: (file: NonNullable<File>) => Promise<string | undefined>;
+}
+
+export interface IRequireFilenameEIPNum {
+  requireFilenameEipNum: (filename: string) => Promise<number>;
+  attemptAssetGracefulTermination: (filename: string) => Promise<void>;
+  attemptEditorApprovalGracefulTermination: (filename: string) => Promise<void>;
+}

src/modules/assertions/__tests__/Assertions.test.ts

@@ -7,11 +7,9 @@ import {
   assertConstantStatus,
   assertFilenameAndFileNumbersMatch,
   assertHasAuthors,
-  assertValidFilename,
   assertValidStatus,
   requireAuthors,
   requireEvent,
-  requireFilenameEipNum,
   requireFiles,
   requirePr,
   requirePullNumber
@@ -134,19 +132,6 @@ describe("Requires", () => {
     });
   });
 
-  describe("requireFilenameEipNum", () => {
-    it("should not error if filename matches regex", () => {
-      const eipNum = requireFilenameEipNum("eip-123.md");
-      expect(eipNum).toBe(123);
-    });
-    it("should not explode if filename doesn't match", async () => {
-      await expectError(() => requireFilenameEipNum("eip-123"));
-      await expectError(() => requireFilenameEipNum("ep-123.md"));
-      await expectError(() => requireFilenameEipNum("eip-a.md"));
-      await expectError(() => requireFilenameEipNum("eip-123.js"));
-    });
-  });
-
   describe("requireFiles", () => {
     const mockFiles = [FileFactory()];
     const listFiles = jest
@@ -212,31 +197,6 @@ describe("Asserts", () => {
     });
   });
 
-  describe("assertValidFilename", () => {
-    it("should return undefined if filename is valid", () => {
-      const file = FileFactory();
-      const res = assertValidFilename(file);
-      expect(res).toBeUndefined();
-    });
-
-    it("should return defined if filename is not valid", () => {
-      const files = [
-        FileFactory({ filename: "eip-123" }),
-        FileFactory({ filename: "ep-123.md" }),
-        FileFactory({ filename: "eip-a.md" }),
-        FileFactory({ filename: "eip-123.js" })
-      ];
-      // @ts-expect-error below is an invalid type error
-      expect(assertValidFilename(files[0])).toBeDefined();
-      // @ts-expect-error below is an invalid type error
-      expect(assertValidFilename(files[1])).toBeDefined();
-      // @ts-expect-error below is an invalid type error
-      expect(assertValidFilename(files[2])).toBeDefined();
-      // @ts-expect-error below is an invalid type error
-      expect(assertValidFilename(files[3])).toBeDefined();
-    });
-  });
-
   describe("assertFilenameAndFileNumbersMatch", () => {
     it("should return undefined if the file names and headers match", () => {
       const fileDiff = FileDiffFactory();

src/modules/assertions/__tests__/assert_valid_filename.test.ts

@@ -0,0 +1,46 @@
+import { initGeneralTestEnv, mockGithubContext } from "src/tests/testutils";
+import { EVENTS } from "src/domain";
+import { FileFactory } from "src/tests/factories/fileFactory";
+import { AssertValidFilename } from "#/assertions/assert_valid_filename";
+
+describe("require_file_preexisting", () => {
+  mockGithubContext({
+    payload: { pull_request: { number: 1 } },
+    repo: { repo: "repo", owner: "owner" },
+    eventName: EVENTS.pullRequestTarget
+  });
+
+  initGeneralTestEnv();
+
+  const requireFilenameEipNum = jest.fn();
+  const _AssertValidFilename = new AssertValidFilename(
+    {requireFilenameEipNum}
+  );
+
+  beforeEach(async () => {
+    requireFilenameEipNum.mockReturnValue(Promise.resolve(1));
+  });
+
+  it("should return undefined if filename is valid", async () => {
+    const file = FileFactory();
+    const res = await _AssertValidFilename.assertValidFilename(file);
+    expect(res).toBeUndefined();
+  });
+
+  it("should return defined if filename is not valid", async () => {
+    const files = [
+      FileFactory({ filename: "eip-123" }),
+      FileFactory({ filename: "ep-123.md" }),
+      FileFactory({ filename: "eip-a.md" }),
+      FileFactory({ filename: "eip-123.js" })
+    ];
+    // @ts-expect-error below is an invalid type error
+    expect(await _AssertValidFilename.assertValidFilename(files[0])).toBeDefined();
+    // @ts-expect-error below is an invalid type error
+    expect(await _AssertValidFilename.assertValidFilename(files[1])).toBeDefined();
+    // @ts-expect-error below is an invalid type error
+    expect(await _AssertValidFilename.assertValidFilename(files[2])).toBeDefined();
+    // @ts-expect-error below is an invalid type error
+    expect(await _AssertValidFilename.assertValidFilename(files[3])).toBeDefined();
+  });
+})