improve error messages when preventing base image update

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);

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

@@ -1,4 +1,4 @@
-import { shouldAllowBaseImageUpdate } from '../src/shouldAllowBaseImageUpdate';
+import { findReasonToPreventBaseImageUpdate } from '../src/findReasonToPreventBaseImageUpdate';
 import { getOctokit } from '../src/getOctokit';
 import {
   VISUAL_REGRESSION_CONTEXT,
@@ -7,8 +7,8 @@ import {
 
 jest.mock('../src/getOctokit');
 
-describe('shouldAllowBaseImageUpdate', () => {
-  it('should return true when all non-visual pr checks pass', async () => {
+describe('findReasonToPreventBaseImageUpdate', () => {
+  it('should return undefined when all non-visual pr checks pass', async () => {
     (getOctokit as jest.Mock).mockImplementation(() => ({
       rest: {
         repos: {
@@ -34,15 +34,15 @@ describe('shouldAllowBaseImageUpdate', () => {
         }
       }
     }));
-    const result = await shouldAllowBaseImageUpdate(
+    const result = await findReasonToPreventBaseImageUpdate(
       'github-owner',
       'github-repo',
       'sha'
     );
-    expect(result).toBe(true);
+    expect(result).toBeUndefined();
   });
 
-  it('should return false when at least one visual test job failed', async () => {
+  it('should return a reason to prevent update when at least one non-visual check failed', async () => {
     (getOctokit as jest.Mock).mockImplementation(() => ({
       rest: {
         repos: {
@@ -59,41 +59,12 @@ describe('shouldAllowBaseImageUpdate', () => {
                 created_at: '2023-05-02T19:11:02Z'
               },
               {
-                context: 'visual tests',
-                state: 'failure',
-                created_at: '2023-05-02T19:11:02Z'
-              }
-            ]
-          })
-        }
-      }
-    }));
-    const result = await shouldAllowBaseImageUpdate(
-      'github-owner',
-      'github-repo',
-      'sha'
-    );
-    expect(result).toBe(false);
-  });
-
-  it('should return false when at least one non-visual check failed', async () => {
-    (getOctokit as jest.Mock).mockImplementation(() => ({
-      rest: {
-        repos: {
-          listCommitStatusesForRef: jest.fn().mockReturnValue({
-            data: [
-              {
-                context: 'unit tests',
-                state: 'success',
-                created_at: '2023-05-02T19:11:02Z'
-              },
-              {
-                context: VISUAL_REGRESSION_CONTEXT,
+                context: 'other tests',
                 state: 'failure',
                 created_at: '2023-05-02T19:11:02Z'
               },
               {
-                context: 'other tests',
+                context: 'even more tests',
                 state: 'failure',
                 created_at: '2023-05-02T19:11:02Z'
               }
@@ -102,15 +73,17 @@ describe('shouldAllowBaseImageUpdate', () => {
         }
       }
     }));
-    const result = await shouldAllowBaseImageUpdate(
+    const result = await findReasonToPreventBaseImageUpdate(
       'github-owner',
       'github-repo',
       'sha'
     );
-    expect(result).toBe(false);
+    expect(result).toBe(
+      'All other PR checks must pass before updating base images! These checks have not passed on your PR: other tests, even more tests'
+    );
   });
 
-  it('should return false when all non-visual pr checks pass but some are pending', async () => {
+  it('should return a reason to prevent update when all non-visual pr checks pass but some are pending', async () => {
     (getOctokit as jest.Mock).mockImplementation(() => ({
       rest: {
         repos: {
@@ -136,15 +109,17 @@ describe('shouldAllowBaseImageUpdate', () => {
         }
       }
     }));
-    const result = await shouldAllowBaseImageUpdate(
+    const result = await findReasonToPreventBaseImageUpdate(
       'github-owner',
       'github-repo',
       'sha'
     );
-    expect(result).toBe(false);
+    expect(result).toBe(
+      'All other PR checks must pass before updating base images! These checks have not passed on your PR: other tests'
+    );
   });
 
-  it('should return true when a non-visual check failed on an early run but passed on the latest run', async () => {
+  it('should return undefined when a non-visual check failed on an early run but passed on the latest run', async () => {
     (getOctokit as jest.Mock).mockImplementation(() => ({
       rest: {
         repos: {
@@ -170,15 +145,15 @@ describe('shouldAllowBaseImageUpdate', () => {
         }
       }
     }));
-    const result = await shouldAllowBaseImageUpdate(
+    const result = await findReasonToPreventBaseImageUpdate(
       'github-owner',
       'github-repo',
       'sha'
     );
-    expect(result).toBe(true);
+    expect(result).toBeUndefined();
   });
 
-  it('should return false when a non-visual check fails on multiple runs and never passed', async () => {
+  it('should return a reason to prevent update when a non-visual check fails on multiple runs and never passed', async () => {
     (getOctokit as jest.Mock).mockImplementation(() => ({
       rest: {
         repos: {
@@ -204,12 +179,14 @@ describe('shouldAllowBaseImageUpdate', () => {
         }
       }
     }));
-    const result = await shouldAllowBaseImageUpdate(
+    const result = await findReasonToPreventBaseImageUpdate(
       'github-owner',
       'github-repo',
       'sha'
     );
-    expect(result).toBe(false);
+    expect(result).toBe(
+      'All other PR checks must pass before updating base images! These checks have not passed on your PR: unit tests'
+    );
   });
 
   it('should return false when visual tests failed to execute successfully', async () => {
@@ -234,11 +211,13 @@ describe('shouldAllowBaseImageUpdate', () => {
         }
       }
     }));
-    const result = await shouldAllowBaseImageUpdate(
+    const result = await findReasonToPreventBaseImageUpdate(
       'github-owner',
       'github-repo',
       'sha'
     );
-    expect(result).toBe(false);
+    expect(result).toBe(
+      'At least one visual test job failed to take a screenshot. All jobs must take a screenshot before reviewing and updating base images!'
+    );
   });
 });

app/backend/test/updateBaseImagesInS3.test.ts

@@ -6,9 +6,9 @@ import {
   updateBaseImagesInS3
 } from '../src/updateBaseImagesInS3';
 import { BASE_IMAGES_DIRECTORY } from 'shared';
-import { shouldAllowBaseImageUpdate } from '../src/shouldAllowBaseImageUpdate';
+import { findReasonToPreventBaseImageUpdate } from '../src/findReasonToPreventBaseImageUpdate';
 
-jest.mock('../src/shouldAllowBaseImageUpdate');
+jest.mock('../src/findReasonToPreventBaseImageUpdate');
 jest.mock('../src/s3Client');
 
 describe('filterNewImages', () => {
@@ -77,7 +77,9 @@ describe('updateBaseImagesInS3', () => {
   });
 
   it('should throw error if other required checks have not yet passed', async () => {
-    (shouldAllowBaseImageUpdate as jest.Mock).mockResolvedValue(false);
+    (findReasonToPreventBaseImageUpdate as jest.Mock).mockResolvedValue(
+      'Some reason to prevent update'
+    );
 
     const expectedBucket = 'expected-bucket-name';
     await expect(

app/frontend/cypress/component/App.cy.tsx

@@ -1,6 +1,5 @@
 import * as React from 'react';
 import App from '../../App';
-import { UPDATE_BASE_IMAGES_ERROR_MESSAGE } from 'shared';
 import {
   firstPage,
   noNewImagesPage,
@@ -9,7 +8,10 @@ import {
   secondPage
 } from '../mocks/pages';
 import { CyHttpMessages } from 'cypress/types/net-stubbing';
-import { baseImageUpdateRejection } from '../mocks/base-image-update-rejection';
+import {
+  baseImageUpdateRejection,
+  MOCK_ERROR_MESSAGE
+} from '../mocks/base-image-update-rejection';
 import { mutationResponse } from '../mocks/mutation';
 import { MemoryRouter } from 'react-router-dom';
 
@@ -165,7 +167,7 @@ describe('App', () => {
         'not.exist'
       );
       cy.findByRole('heading', { name: /Error/ }).should('be.visible');
-      cy.findByText(UPDATE_BASE_IMAGES_ERROR_MESSAGE).should('be.visible');
+      cy.findByText(MOCK_ERROR_MESSAGE).should('be.visible');
     });
   });
 

app/frontend/cypress/mocks/base-image-update-rejection.ts

@@ -1,9 +1,10 @@
 import { TRPCErrorResponse } from '@trpc/server/rpc';
 
+export const MOCK_ERROR_MESSAGE = 'Some error message';
+
 export const baseImageUpdateRejection: TRPCErrorResponse = {
   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!',
+    message: MOCK_ERROR_MESSAGE,
     code: -32603,
     data: {
       code: 'FORBIDDEN',

app/frontend/cypress/support/component.ts

@@ -2,7 +2,7 @@ import './commands';
 
 import { mount } from 'cypress/react18';
 import '../../App.css';
-import { UPDATE_BASE_IMAGES_ERROR_MESSAGE } from 'shared';
+import { MOCK_ERROR_MESSAGE } from '../mocks/base-image-update-rejection';
 
 declare global {
   namespace Cypress {
@@ -15,7 +15,7 @@ declare global {
 Cypress.Commands.add('mount', mount);
 
 Cypress.on('uncaught:exception', err => {
-  if (err.message.includes(UPDATE_BASE_IMAGES_ERROR_MESSAGE)) {
+  if (err.message.includes(MOCK_ERROR_MESSAGE)) {
     return false;
   }
 });

shared/index.ts

@@ -3,7 +3,5 @@ export const BASE_IMAGES_DIRECTORY = 'base-images';
 export const BASE_IMAGE_NAME = 'base';
 export const DIFF_IMAGE_NAME = 'diff';
 export const NEW_IMAGE_NAME = 'new';
-export const 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!';
 export const VISUAL_TESTS_FAILED_TO_EXECUTE =
   'Visual tests failed to execute successfully.';