fix(action): stop skipping status update for job re-runs

.github/workflows/github-pages.yml

@@ -28,6 +28,12 @@ jobs:
       - name: Setup pnpm
         uses: pnpm/action-setup@v2
 
+      - name: Setup node and pnpm cache
+        uses: actions/setup-node@v3
+        with:
+          node-version-file: .nvmrc
+          cache: pnpm
+
       - name: Install
         run: pnpm install --prod --ignore-scripts
 

.github/workflows/publish.yml

@@ -45,6 +45,12 @@ jobs:
       - name: Setup pnpm
         uses: pnpm/action-setup@v2
 
+      - name: Setup node and pnpm cache
+        uses: actions/setup-node@v3
+        with:
+          node-version-file: .nvmrc
+          cache: pnpm
+
       - name: Install
         run: pnpm --filter comparadise-utils install
 

.github/workflows/test.yml

@@ -27,6 +27,12 @@ jobs:
       - name: Setup pnpm
         uses: pnpm/action-setup@v2
 
+      - name: Setup node and pnpm cache
+        uses: actions/setup-node@v3
+        with:
+          node-version-file: .nvmrc
+          cache: pnpm
+
       - name: Install
         run: pnpm install
 

.npmrc

@@ -1,3 +1,2 @@
 enable-pre-post-scripts=true
 registry=https://registry.npmjs.org
-use-node-version=18.16.0

.nvmrc

@@ -0,0 +1 @@
+20.5.0

Dockerfile

@@ -1,10 +1,9 @@
-FROM node:18.16.0-slim
-
+FROM node:20-slim
+ENV PNPM_HOME="/pnpm"
+ENV PATH="$PNPM_HOME:$PATH"
+RUN corepack enable
 WORKDIR /app
 
-RUN apt-get update && apt-get -qq -y install curl
-RUN curl -f https://get.pnpm.io/v6.16.js | node - add --global pnpm
-
 RUN useradd -ms /bin/sh admin
 RUN chown -R admin .
 COPY --chown=admin . .

action/dist/index.js

@@ -12003,7 +12003,8 @@ const run = async () => {
     if (diffFileCount === 0 && newFileCount === 0) {
         (0, core_1.info)('All visual tests passed, and no diffs found!');
         const latestVisualRegressionStatus = await (0, get_latest_visual_regression_status_1.getLatestVisualRegressionStatus)(commitHash);
-        if (latestVisualRegressionStatus?.state === 'failure') {
+        if (latestVisualRegressionStatus?.state === 'failure' &&
+            github_1.context.runNumber === 1) {
             (0, core_1.info)('Visual Regression status has already been set to failed, so skipping status update.');
             return;
         }
@@ -12017,7 +12018,9 @@ const run = async () => {
     }
     const latestVisualRegressionStatus = await (0, get_latest_visual_regression_status_1.getLatestVisualRegressionStatus)(commitHash);
     if (latestVisualRegressionStatus?.state === 'failure' &&
-        latestVisualRegressionStatus?.description === shared_1.VISUAL_TESTS_FAILED_TO_EXECUTE) {
+        latestVisualRegressionStatus?.description ===
+            shared_1.VISUAL_TESTS_FAILED_TO_EXECUTE &&
+        github_1.context.runNumber === 1) {
         (0, core_1.warning)('Some other Visual Regression tests failed to execute successfully, so skipping status update and comment.');
         return;
     }
@@ -12081,13 +12084,12 @@ exports.uploadBaseImages = uploadBaseImages;
 "use strict";
 
 Object.defineProperty(exports, "__esModule", ({ value: true }));
-exports.VISUAL_TESTS_FAILED_TO_EXECUTE = exports.UPDATE_BASE_IMAGES_ERROR_MESSAGE = exports.NEW_IMAGE_NAME = exports.DIFF_IMAGE_NAME = exports.BASE_IMAGE_NAME = exports.BASE_IMAGES_DIRECTORY = exports.VISUAL_REGRESSION_CONTEXT = void 0;
+exports.VISUAL_TESTS_FAILED_TO_EXECUTE = exports.NEW_IMAGE_NAME = exports.DIFF_IMAGE_NAME = exports.BASE_IMAGE_NAME = exports.BASE_IMAGES_DIRECTORY = exports.VISUAL_REGRESSION_CONTEXT = void 0;
 exports.VISUAL_REGRESSION_CONTEXT = 'Visual Regression';
 exports.BASE_IMAGES_DIRECTORY = 'base-images';
 exports.BASE_IMAGE_NAME = 'base';
 exports.DIFF_IMAGE_NAME = 'diff';
 exports.NEW_IMAGE_NAME = 'new';
-exports.UPDATE_BASE_IMAGES_ERROR_MESSAGE = 'At least one non-visual status check has not passed on your PR. Please ensure all other checks have passed before updating base images!';
 exports.VISUAL_TESTS_FAILED_TO_EXECUTE = 'Visual tests failed to execute successfully.';
 
 

action/src/run.ts

@@ -58,7 +58,10 @@ export const run = async () => {
     const latestVisualRegressionStatus = await getLatestVisualRegressionStatus(
       commitHash
     );
-    if (latestVisualRegressionStatus?.state === 'failure') {
+    if (
+      latestVisualRegressionStatus?.state === 'failure' &&
+      context.runNumber === 1
+    ) {
       info(
         'Visual Regression status has already been set to failed, so skipping status update.'
       );
@@ -79,7 +82,9 @@ export const run = async () => {
   );
   if (
     latestVisualRegressionStatus?.state === 'failure' &&
-    latestVisualRegressionStatus?.description === VISUAL_TESTS_FAILED_TO_EXECUTE
+    latestVisualRegressionStatus?.description ===
+      VISUAL_TESTS_FAILED_TO_EXECUTE &&
+    context.runNumber === 1
   ) {
     warning(
       'Some other Visual Regression tests failed to execute successfully, so skipping status update and comment.'

action/test/run.test.ts

@@ -1,5 +1,6 @@
 import { run } from '../src/run';
 import { exec } from '@actions/exec';
+import { context } from '@actions/github';
 import { getInput, getMultilineInput, setFailed } from '@actions/core';
 import { octokit } from '../src/octokit';
 import { sync } from 'glob';
@@ -13,7 +14,7 @@ jest.mock('glob');
 jest.mock('@actions/core');
 jest.mock('@actions/exec');
 jest.mock('@actions/github', () => ({
-  context: { repo: { repo: 'repo', owner: 'owner' } },
+  context: { repo: { repo: 'repo', owner: 'owner' }, runNumber: 1 },
   getOctokit: jest.fn(() => ({
     rest: {
       repos: {
@@ -224,7 +225,7 @@ describe('main', () => {
     expect(octokit.rest.repos.createCommitStatus).toHaveBeenCalled();
   });
 
-  it('should not set failure commit status or create comment if the latest Visual Regression status is failure because tests failed to execute successfully', async () => {
+  it('should not set commit status or create comment if the latest Visual Regression status is failure because tests failed to execute successfully', async () => {
     (exec as jest.Mock).mockResolvedValue(0);
     (sync as unknown as jest.Mock).mockReturnValue([
       'path/to/screenshots/base.png',
@@ -259,3 +260,73 @@ describe('main', () => {
     expect(octokit.rest.issues.createComment).not.toHaveBeenCalled();
   });
 });
+
+describe('main with rerun', () => {
+  beforeEach(() => {
+    context.runNumber = 2;
+  });
+
+  it('should set successful commit status if a visual test failed to execute but this is a re-run', async () => {
+    (exec as jest.Mock).mockResolvedValue(0);
+    (sync as unknown as jest.Mock).mockReturnValue([
+      'path/to/screenshots/base.png'
+    ]);
+    (
+      octokit.rest.repos.listCommitStatusesForRef as unknown as jest.Mock
+    ).mockResolvedValue({
+      data: [
+        {
+          context: 'some context',
+          created_at: '2023-05-21T16:51:29Z',
+          state: 'success'
+        },
+        {
+          context: VISUAL_REGRESSION_CONTEXT,
+          created_at: '2023-05-21T16:51:29Z',
+          state: 'failure',
+          description: VISUAL_TESTS_FAILED_TO_EXECUTE
+        },
+        {
+          context: VISUAL_REGRESSION_CONTEXT,
+          created_at: '2023-05-21T15:51:29Z',
+          state: 'success'
+        }
+      ]
+    });
+    await run();
+    expect(octokit.rest.repos.createCommitStatus).toHaveBeenCalled();
+  });
+
+  it('should set failure commit status if a visual test failed to execute but this is a re-run', async () => {
+    (exec as jest.Mock).mockResolvedValue(0);
+    (sync as unknown as jest.Mock).mockReturnValue([
+      'path/to/screenshots/base.png',
+      'path/to/screenshots/diff.png',
+      'path/to/screenshots/new.png'
+    ]);
+    (
+      octokit.rest.repos.listCommitStatusesForRef as unknown as jest.Mock
+    ).mockResolvedValue({
+      data: [
+        {
+          context: 'some context',
+          created_at: '2023-05-21T16:51:29Z',
+          state: 'success'
+        },
+        {
+          context: VISUAL_REGRESSION_CONTEXT,
+          created_at: '2023-05-21T16:51:29Z',
+          state: 'failure',
+          description: VISUAL_TESTS_FAILED_TO_EXECUTE
+        },
+        {
+          context: VISUAL_REGRESSION_CONTEXT,
+          created_at: '2023-05-21T15:51:29Z',
+          state: 'success'
+        }
+      ]
+    });
+    await run();
+    expect(octokit.rest.repos.createCommitStatus).toHaveBeenCalled();
+  });
+});

app/backend/src/shouldAllowBaseImageUpdate.ts → app/backend/src/findReasonToPreventBaseImageUpdate.ts

@@ -5,11 +5,11 @@ import {
   VISUAL_TESTS_FAILED_TO_EXECUTE
 } from 'shared';
 
-export const shouldAllowBaseImageUpdate = async (
+export const findReasonToPreventBaseImageUpdate = async (
   owner: string,
   repo: string,
   sha: string
-): Promise<boolean> => {
+) => {
   const octokit = getOctokit(owner, repo);
 
   const { data } = await octokit.rest.repos.listCommitStatusesForRef({
@@ -21,7 +21,7 @@ export const shouldAllowBaseImageUpdate = async (
     ({ context }) => context === VISUAL_REGRESSION_CONTEXT
   )?.description;
   if (visualRegressionContextDescription === VISUAL_TESTS_FAILED_TO_EXECUTE)
-    return false;
+    return 'At least one visual test job failed to take a screenshot. All jobs must take a screenshot before reviewing and updating base images!';
   const nonVisualStatuses = data.filter(
     ({ context }) => context !== VISUAL_REGRESSION_CONTEXT
   );
@@ -33,5 +33,12 @@ export const shouldAllowBaseImageUpdate = async (
     ).reverse();
     return isEqual(status, contextsSortedByDescTime.find(Boolean));
   });
-  return mostRecentNonVisualStatuses.every(({ state }) => state === 'success');
+  const nonVisualChecksThatHaveNotPassed = mostRecentNonVisualStatuses
+    .filter(({ state }) => state !== 'success')
+    .map(({ context }) => context);
+  if (nonVisualChecksThatHaveNotPassed.length) {
+    return `All other PR checks must pass before updating base images! These checks have not passed on your PR: ${nonVisualChecksThatHaveNotPassed.join(
+      ', '
+    )}`;
+  }
 };

app/backend/src/updateBaseImagesInS3.ts

@@ -1,11 +1,6 @@
 import { S3Client } from './s3Client';
-import {
-  BASE_IMAGE_NAME,
-  BASE_IMAGES_DIRECTORY,
-  NEW_IMAGE_NAME,
-  UPDATE_BASE_IMAGES_ERROR_MESSAGE
-} from 'shared';
-import { shouldAllowBaseImageUpdate } from './shouldAllowBaseImageUpdate';
+import { BASE_IMAGE_NAME, BASE_IMAGES_DIRECTORY, NEW_IMAGE_NAME } from 'shared';
+import { findReasonToPreventBaseImageUpdate } from './findReasonToPreventBaseImageUpdate';
 import { TRPCError } from '@trpc/server';
 import { UpdateBaseImagesInput } from './schema';
 import { getKeysFromS3 } from './getKeysFromS3';
@@ -16,10 +11,15 @@ export const updateBaseImagesInS3 = async ({
   owner,
   repo
 }: UpdateBaseImagesInput) => {
-  if (!(await shouldAllowBaseImageUpdate(owner, repo, hash))) {
+  const reasonToPreventUpdate = await findReasonToPreventBaseImageUpdate(
+    owner,
+    repo,
+    hash
+  );
+  if (reasonToPreventUpdate) {
     throw new TRPCError({
       code: 'FORBIDDEN',
-      message: UPDATE_BASE_IMAGES_ERROR_MESSAGE
+      message: reasonToPreventUpdate
     });
   }
   const s3Paths = await getKeysFromS3(hash, bucket);