fix(action): fail job correctly if visualTestsIsolated is false (#728)

Co-authored-by: danadajian <danadajian@users.noreply.github.com>

Author: danadajian

action/dist/main.js

@@ -163870,23 +163870,22 @@ var run = async () => {
   }, 0);
   const newFileCount = newFilePaths.length;
   const visualTestsIsolated = getBooleanInput("visual-tests-isolated");
-  const visualTestsFailedForReasonOtherThanDiff = numVisualTestFailures > diffFileCount;
-  if (visualTestsFailedForReasonOtherThanDiff) {
-    if (visualTestsIsolated) {
-      setFailed(
-        "Visual tests failed to execute successfully. Perhaps one failed to take a screenshot?"
-      );
-      if (!commitHash) return;
-      return octokit.rest.repos.createCommitStatus({
-        sha: commitHash,
-        context: VISUAL_REGRESSION_CONTEXT,
-        state: "failure",
-        description: VISUAL_TESTS_FAILED_TO_EXECUTE,
-        ...context2.repo
-      });
-    } else {
-      warning("The job failed, but this might not be due to visual tests.");
-    }
+  if (visualTestsIsolated && numVisualTestFailures > diffFileCount) {
+    setFailed(
+      "Visual tests failed to execute successfully. Perhaps one failed to take a screenshot?"
+    );
+    if (!commitHash) return;
+    return octokit.rest.repos.createCommitStatus({
+      sha: commitHash,
+      context: VISUAL_REGRESSION_CONTEXT,
+      state: "failure",
+      description: VISUAL_TESTS_FAILED_TO_EXECUTE,
+      ...context2.repo
+    });
+  }
+  if (!visualTestsIsolated && numVisualTestFailures > 0) {
+    setFailed("The job failed, but this might not be due to visual tests.");
+    return;
   }
   const latestVisualRegressionStatus = commitHash ? await getLatestVisualRegressionStatus(commitHash) : null;
   const isRetry = context2.runAttempt > 1;

action/dist/main.js.map

@@ -86,24 +86,23 @@ export const run = async () => {
 
   const visualTestsIsolated = getBooleanInput('visual-tests-isolated');
 
-  const visualTestsFailedForReasonOtherThanDiff =
-    numVisualTestFailures > diffFileCount;
-  if (visualTestsFailedForReasonOtherThanDiff) {
-    if (visualTestsIsolated) {
-      setFailed(
-        'Visual tests failed to execute successfully. Perhaps one failed to take a screenshot?'
-      );
-      if (!commitHash) return;
-      return octokit.rest.repos.createCommitStatus({
-        sha: commitHash,
-        context: VISUAL_REGRESSION_CONTEXT,
-        state: 'failure',
-        description: VISUAL_TESTS_FAILED_TO_EXECUTE,
-        ...context.repo
-      });
-    } else {
-      warning('The job failed, but this might not be due to visual tests.');
-    }
+  if (visualTestsIsolated && numVisualTestFailures > diffFileCount) {
+    setFailed(
+      'Visual tests failed to execute successfully. Perhaps one failed to take a screenshot?'
+    );
+    if (!commitHash) return;
+    return octokit.rest.repos.createCommitStatus({
+      sha: commitHash,
+      context: VISUAL_REGRESSION_CONTEXT,
+      state: 'failure',
+      description: VISUAL_TESTS_FAILED_TO_EXECUTE,
+      ...context.repo
+    });
+  }
+
+  if (!visualTestsIsolated && numVisualTestFailures > 0) {
+    setFailed('The job failed, but this might not be due to visual tests.');
+    return;
   }
 
   const latestVisualRegressionStatus = commitHash

action/src/run.ts

@@ -768,17 +768,37 @@ describe('main', () => {
     );
   });
 
-  it('should call warning instead of setFailed when visual tests fail for non-diff reason and visual-tests-isolated is false', async () => {
+  it('should call setFailed without setting commit status when visual tests fail for non-diff reason and visual-tests-isolated is false', async () => {
     execMock.mockResolvedValue(1);
     getBooleanInputMock.mockImplementation(name =>
       name === 'visual-tests-isolated' ? false : undefined
     );
     globMock.mockResolvedValue([]);
     await runAction();
-    expect(setFailedMock).not.toHaveBeenCalled();
-    expect(warningMock).toHaveBeenCalledWith(
+    expect(setFailedMock).toHaveBeenCalledWith(
       'The job failed, but this might not be due to visual tests.'
     );
+    expect(createCommitStatusMock).not.toHaveBeenCalled();
+  });
+
+  it('should call setFailed without setting commit status when visual-tests-isolated is false and 1 failure with multiple diffs', async () => {
+    execMock.mockResolvedValue(1);
+    getBooleanInputMock.mockImplementation(name =>
+      name === 'visual-tests-isolated' ? false : undefined
+    );
+    globMock.mockResolvedValue([
+      'path/to/screenshots/diff1.png',
+      'path/to/screenshots/new1.png',
+      'path/to/screenshots/diff2.png',
+      'path/to/screenshots/new2.png',
+      'path/to/screenshots/diff3.png',
+      'path/to/screenshots/new3.png'
+    ]);
+    await runAction();
+    expect(setFailedMock).toHaveBeenCalledWith(
+      'The job failed, but this might not be due to visual tests.'
+    );
+    expect(createCommitStatusMock).not.toHaveBeenCalled();
   });
 });