Better errors (#294)

<img width="906" alt="image"
src="https://github.com/alwaysmeticulous/report-diffs-action/assets/793481/d37ec9c9-3727-4ebf-aeca-00e634976ecf">

<img width="1454" alt="image"
src="https://github.com/alwaysmeticulous/report-diffs-action/assets/793481/33bb5f90-d88c-4fd4-931f-7d72639b6a8a">

.github/workflows/test-meticulous-against-deployment.yaml

@@ -10,6 +10,7 @@ on:
   pull_request: {}
 
 permissions:
+  actions: write
   contents: read
   statuses: write
   pull-requests: write

.github/workflows/test-meticulous.yaml

@@ -10,6 +10,7 @@ on:
   pull_request: {}
 
 permissions:
+  actions: write
   contents: read
   statuses: write
   pull-requests: write

src/action.ts

@@ -1,14 +1,18 @@
 import { setFailed } from "@actions/core";
 import { context, getOctokit } from "@actions/github";
 import { applyDefaultExecutionOptionsFromProject } from "@alwaysmeticulous/client";
-import { setMeticulousLocalDataDir } from "@alwaysmeticulous/common";
+import {
+  METICULOUS_LOGGER_NAME,
+  setMeticulousLocalDataDir,
+} from "@alwaysmeticulous/common";
 import { executeTestRun } from "@alwaysmeticulous/replay-orchestrator-launcher";
 import {
   ReplayExecutionOptions,
   RunningTestRunExecution,
 } from "@alwaysmeticulous/sdk-bundles-api";
 import { initSentry } from "@alwaysmeticulous/sentry";
 import debounce from "lodash.debounce";
+import log from "loglevel";
 import { addLocalhostAliases } from "./utils/add-localhost-aliases";
 import { throwIfCannotConnectToOrigin } from "./utils/check-connection";
 import { LOGICAL_ENVIRONMENT_VERSION } from "./utils/constants";
@@ -17,7 +21,7 @@ import { getEnvironment } from "./utils/environment.utils";
 import { getBaseAndHeadCommitShas } from "./utils/get-base-and-head-commit-shas";
 import { getCodeChangeEvent } from "./utils/get-code-change-event";
 import { getInputs } from "./utils/get-inputs";
-import { initLogger, setLogLevel } from "./utils/logger.utils";
+import { initLogger, setLogLevel, shortSha } from "./utils/logger.utils";
 import { ResultsReporter } from "./utils/results-reporter";
 import { waitForDeploymentUrl } from "./utils/wait-for-deployment-url";
 
@@ -73,9 +77,10 @@ export const runMeticulousTestsAction = async (): Promise<void> => {
   const event = getCodeChangeEvent(context.eventName, payload);
   const { owner, repo } = context.repo;
   const octokit = getOctokitOrFail(githubToken);
+  const logger = log.getLogger(METICULOUS_LOGGER_NAME);
 
   if (event == null) {
-    console.warn(
+    logger.warn(
       `Running report-diffs-action is only supported for 'push', \
       'pull_request' and 'workflow_dispatch' events, but was triggered \
       on a '${context.eventName}' event. Skipping execution.`
@@ -97,19 +102,19 @@ export const runMeticulousTestsAction = async (): Promise<void> => {
   });
 
   if (shaToCompareAgainst != null && event.type === "pull_request") {
-    console.log(
+    logger.info(
       `Comparing screenshots for the commit head of this PR, ${shortSha(
         head
       )}, against ${shortSha(shaToCompareAgainst)}`
     );
   } else if (shaToCompareAgainst != null) {
-    console.log(
+    logger.info(
       `Comparing screenshots for commit ${shortSha(
         head
       )} against commit ${shortSha(shaToCompareAgainst)}}`
     );
   } else {
-    console.log(`Generating screenshots for commit ${shortSha(head)}`);
+    logger.info(`Generating screenshots for commit ${shortSha(head)}`);
   }
 
   const resultsReporter = new ResultsReporter({
@@ -218,11 +223,10 @@ const getOctokitOrFail = (githubToken: string | null) => {
   try {
     return getOctokit(githubToken);
   } catch (err) {
-    console.error(err);
+    const logger = log.getLogger(METICULOUS_LOGGER_NAME);
+    logger.error(err);
     throw new Error(
       "Error connecting to GitHub. Did you specify a valid 'github-token'?"
     );
   }
 };
-
-const shortSha = (sha: string) => sha.slice(0, 7);

src/utils/constants.ts

@@ -14,3 +14,5 @@ export const EXPECTED_PERMISSIONS_BLOCK = [
 // the environment changes in a way that would cause a replay to behave differently, e.g. upgrading to a newer
 // replay-orchestrator-launcher version, or changing the version of puppeteer.
 export const LOGICAL_ENVIRONMENT_VERSION = 2;
+
+export const DOCS_URL = "https://app.meticulous.ai/docs/github-actions-v2";

src/utils/ensure-base-exists.utils.ts

@@ -9,7 +9,11 @@ import { METICULOUS_LOGGER_NAME } from "@alwaysmeticulous/common";
 import log from "loglevel";
 import { Duration } from "luxon";
 import { CodeChangeEvent } from "../types";
-import { LOGICAL_ENVIRONMENT_VERSION } from "./constants";
+import { DOCS_URL, LOGICAL_ENVIRONMENT_VERSION } from "./constants";
+import {
+  DEFAULT_FAILED_OCTOKIT_REQUEST_MESSAGE,
+  isGithubPermissionsError,
+} from "./error.utils";
 import {
   getCurrentWorkflowId,
   getPendingWorkflowRun,
@@ -68,7 +72,7 @@ export const ensureBaseTestsExists = async ({
   });
 
   if (testRun != null) {
-    logger.log(`Tests already exist for commit ${base} (${testRun.id})`);
+    logger.info(`Tests already exist for commit ${base} (${testRun.id})`);
     return { shaToCompareAgainst: base };
   }
 
@@ -82,7 +86,7 @@ export const ensureBaseTestsExists = async ({
     octokit,
   });
   if (alreadyPending != null) {
-    logger.log(
+    logger.info(
       `Waiting on workflow run on base commit (${base}) to compare against: ${alreadyPending.html_url}`
     );
 
@@ -163,7 +167,7 @@ export const ensureBaseTestsExists = async ({
     return { shaToCompareAgainst: null };
   }
 
-  logger.log(`Waiting on workflow run: ${workflowRun.html_url}`);
+  logger.info(`Waiting on workflow run: ${workflowRun.html_url}`);
   await waitForWorkflowCompletionAndThrowIfFailed({
     owner,
     repo,
@@ -251,11 +255,26 @@ const getHeadCommitForRef = async ({
   ref: string;
   octokit: InstanceType<typeof GitHub>;
 }): Promise<string> => {
-  const result = await octokit.rest.repos.getBranch({
-    owner,
-    repo,
-    branch: ref,
-  });
-  const commitSha = result.data.commit.sha;
-  return commitSha;
+  try {
+    const result = await octokit.rest.repos.getBranch({
+      owner,
+      repo,
+      branch: ref,
+    });
+    const commitSha = result.data.commit.sha;
+    return commitSha;
+  } catch (err: unknown) {
+    if (isGithubPermissionsError(err)) {
+      // https://docs.github.com/en/rest/overview/permissions-required-for-github-apps?apiVersion=2022-11-28#repository-permissions-for-contents
+      throw new Error(
+        `Missing permission to get the head commit of the branch '${ref}'. This is required in order to correctly calculate the two commits to compare.` +
+          ` Please add the 'contents: read' permission to your workflow YAML file: see ${DOCS_URL} for the correct setup.`
+      );
+    }
+    const logger = log.getLogger(METICULOUS_LOGGER_NAME);
+    logger.error(
+      `Unable to get head commit of branch '${ref}'. This is required in order to correctly calculate the two commits to compare. ${DEFAULT_FAILED_OCTOKIT_REQUEST_MESSAGE}`
+    );
+    throw err;
+  }
 };

src/utils/error.utils.ts

@@ -0,0 +1,29 @@
+import { DOCS_URL } from "./constants";
+
+export const isGithubPermissionsError = (
+  error: MaybeOctokitRequestError | unknown
+): boolean => {
+  const message = getErrorMessage(error);
+  return (
+    message.toLowerCase().includes("resource not accessible by integration") ||
+    (error as MaybeOctokitRequestError)?.status === 403
+  );
+};
+
+export const getErrorMessage = (error: unknown) => {
+  const message = (error as MaybeOctokitRequestError)?.message ?? "";
+  return typeof message === "string" ? message : "";
+};
+
+// Octokit returns RequestErrors, which have a message and a code
+// But we can't make any strong assumptions, so we type it on the safe side
+// https://github.com/octokit/request-error.js/blob/372097e9b16f70d4ad75089003dc9154e304faa7/src/index.ts#L16
+type MaybeOctokitRequestError =
+  | {
+      message?: string | unknown;
+      status?: number | unknown;
+    }
+  | null
+  | undefined;
+
+export const DEFAULT_FAILED_OCTOKIT_REQUEST_MESSAGE = `Please check www.githubstatus.com, and that you have setup the action correctly, including with the correct permissions: see ${DOCS_URL} for the correct setup.`;

src/utils/get-base-and-head-commit-shas.ts

@@ -1,5 +1,7 @@
 import { execSync } from "child_process";
 import { context } from "@actions/github";
+import { METICULOUS_LOGGER_NAME } from "@alwaysmeticulous/common";
+import log from "loglevel";
 import { CodeChangeEvent } from "../types";
 
 interface BaseAndHeadCommitShas {
@@ -53,6 +55,8 @@ const tryGetMergeBaseOfHeadCommit = (
   pullRequestBaseSha: string,
   baseRef: string
 ): string | null => {
+  const logger = log.getLogger(METICULOUS_LOGGER_NAME);
+
   try {
     markGitDirectoryAsSafe();
     // Only a single commit is fetched by the checkout action by default
@@ -69,7 +73,7 @@ const tryGetMergeBaseOfHeadCommit = (
     if (!isValidGitSha(mergeBase)) {
       // Note: the GITHUB_SHA is always a merge commit, even if the merge is a no-op because the PR is up to date
       // So this should never happen
-      console.error(
+      logger.error(
         `Failed to get merge base of ${pullRequestHeadSha} and ${baseRef}: value returned by 'git merge-base' was not a valid git SHA ('${mergeBase}').` +
           `Using the base of the pull request instead (${pullRequestBaseSha}).`
       );
@@ -78,7 +82,7 @@ const tryGetMergeBaseOfHeadCommit = (
 
     return mergeBase;
   } catch (error) {
-    console.error(
+    logger.error(
       `Failed to get merge base of ${pullRequestHeadSha} and ${baseRef}. Error: ${error}. Using the base of the pull request instead (${pullRequestBaseSha}).`
     );
     return null;
@@ -90,9 +94,10 @@ const tryGetMergeBaseOfTemporaryMergeCommit = (
   pullRequestBaseSha: string
 ): string | null => {
   const mergeCommitSha = process.env.GITHUB_SHA;
+  const logger = log.getLogger(METICULOUS_LOGGER_NAME);
 
   if (mergeCommitSha == null) {
-    console.error(
+    logger.error(
       `No GITHUB_SHA environment var set, so can't work out true base of the merge commit. Using the base of the pull request instead (${pullRequestBaseSha}).`
     );
     return null;
@@ -104,7 +109,7 @@ const tryGetMergeBaseOfTemporaryMergeCommit = (
       .toString()
       .trim();
     if (headCommitSha !== mergeCommitSha) {
-      console.log(
+      logger.info(
         `The head commit SHA (${headCommitSha}) does not equal GITHUB_SHA environment variable (${mergeCommitSha}).
           This is likely because a custom ref has been passed to the 'actions/checkout' action. We're assuming therefore
           that the head commit SHA is not a temporary merge commit, but rather the head of the branch. Therefore we're
@@ -124,7 +129,7 @@ const tryGetMergeBaseOfTemporaryMergeCommit = (
     if (parents.length !== 2) {
       // Note: the GITHUB_SHA is always a merge commit, even if the merge is a no-op because the PR is up to date
       // So this should never happen
-      console.error(
+      logger.error(
         `GITHUB_SHA (${mergeCommitSha}) is not a merge commit, so can't work out true base of the merge commit. Using the base of the pull request instead.`
       );
       return null;
@@ -134,15 +139,15 @@ const tryGetMergeBaseOfTemporaryMergeCommit = (
     const mergeBaseSha = parents[0];
     const mergeHeadSha = parents[1];
     if (mergeHeadSha !== pullRequestHeadSha) {
-      console.error(
+      logger.error(
         `The second parent (${parents[1]}) of the GITHUB_SHA merge commit (${mergeCommitSha}) is not equal to the head of the PR (${pullRequestHeadSha}),
         so can not confidently determine the base of the merge commit to compare against. Using the base of the pull request instead (${pullRequestBaseSha}).`
       );
       return null;
     }
     return mergeBaseSha;
   } catch (e) {
-    console.error(
+    logger.error(
       `Error getting base of merge commit (${mergeCommitSha}). Using the base of the pull request instead (${pullRequestBaseSha}).`,
       e
     );

src/utils/logger.utils.ts

@@ -33,3 +33,5 @@ export const setLogLevel: (logLevel: string | undefined) => void = (
       break;
   }
 };
+
+export const shortSha = (sha: string) => sha.slice(0, 7);

src/utils/results-reporter.ts

@@ -1,10 +1,18 @@
 import { getOctokit } from "@actions/github";
 import { Project } from "@alwaysmeticulous/api";
+import { METICULOUS_LOGGER_NAME } from "@alwaysmeticulous/common";
 import {
   ExecuteTestRunResult,
   RunningTestRunExecution,
 } from "@alwaysmeticulous/sdk-bundles-api";
+import log from "loglevel";
 import { CodeChangeEvent } from "../types";
+import { DOCS_URL } from "./constants";
+import {
+  DEFAULT_FAILED_OCTOKIT_REQUEST_MESSAGE,
+  isGithubPermissionsError,
+} from "./error.utils";
+import { shortSha } from "./logger.utils";
 import { updateStatusComment } from "./update-status-comment";
 
 const SHORT_SHA_LENGTH = 7;
@@ -160,18 +168,35 @@ export class ResultsReporter {
     targetUrl?: string;
   }) {
     const { octokit, owner, repo, headSha } = this.options;
-    return octokit.rest.repos.createCommitStatus({
-      owner,
-      repo,
-      context:
-        this.options.testSuiteId != null
-          ? `Meticulous (${this.options.testSuiteId})`
-          : "Meticulous",
-      description,
-      state,
-      sha: headSha,
-      ...(targetUrl ? { target_url: targetUrl } : {}),
-    });
+    try {
+      return octokit.rest.repos.createCommitStatus({
+        owner,
+        repo,
+        context:
+          this.options.testSuiteId != null
+            ? `Meticulous (${this.options.testSuiteId})`
+            : "Meticulous",
+        description,
+        state,
+        sha: headSha,
+        ...(targetUrl ? { target_url: targetUrl } : {}),
+      });
+    } catch (err: unknown) {
+      if (isGithubPermissionsError(err)) {
+        // https://docs.github.com/en/actions/using-jobs/assigning-permissions-to-jobs
+        throw new Error(
+          `Missing permission to create and update commit statuses.` +
+            ` Please add the 'statuses: write' permission to your workflow YAML file: see ${DOCS_URL} for the correct setup.`
+        );
+      }
+      const logger = log.getLogger(METICULOUS_LOGGER_NAME);
+      logger.error(
+        `Unable to create commit status for commit '${shortSha(
+          headSha
+        )}'. ${DEFAULT_FAILED_OCTOKIT_REQUEST_MESSAGE}`
+      );
+      throw err;
+    }
   }
 
   private setStatusComment({