feat(ado-improvements-1): Refactor reporter errors (#1046)

microsoft · Feb 11, 2022 · 11c16e4 · 11c16e4
1 parent f371612
commit 11c16e4
Show file tree

Hide file tree

Showing 17 changed files with 239 additions and 102 deletions.
diff --git a/packages/ado-extension/src/ado-extension.ts b/packages/ado-extension/src/ado-extension.ts
@@ -4,13 +4,14 @@
 import 'reflect-metadata';
 import './module-name-mapper';
 
+import { ExitCode } from '@accessibility-insights-action/shared';
 import { Logger } from '@accessibility-insights-action/shared';
 import { hookStderr } from '@accessibility-insights-action/shared';
 import { hookStdout } from '@accessibility-insights-action/shared';
 import { Scanner } from '@accessibility-insights-action/shared';
 import { setupIocContainer } from './ioc/setup-ioc-container';
 
-export function runScan() {
+export function runScan(): void {
     (async () => {
         hookStderr();
         hookStdout();
@@ -20,9 +21,9 @@ export function runScan() {
         await logger.setup();
 
         const scanner = container.get(Scanner);
-        await scanner.scan();
+        process.exit((await scanner.scan()) ? ExitCode.ScanCompletedNoUserActionIsNeeded : ExitCode.ScanCompletedUserActionIsNeeded);
     })().catch((error) => {
-        console.log('Exception thrown in extension: ', error);
-        process.exit(1);
+        console.log('##[error][Exception] Exception thrown in extension: ', error);
+        process.exit(ExitCode.ScanFailedToComplete);
     });
 }
diff --git a/packages/ado-extension/src/install-runtime-dependencies.ts b/packages/ado-extension/src/install-runtime-dependencies.ts
@@ -3,7 +3,7 @@
 
 import { execSync } from 'child_process';
 
-export function installRuntimeDependencies() {
+export function installRuntimeDependencies(): void {
     console.log('##[group]Installing runtime dependencies');
     execSync('yarn install --prod --ignore-engines --frozen-lockfile', {
         stdio: 'inherit',

diff --git a/packages/ado-extension/src/ioc/setup-ioc-container.ts b/packages/ado-extension/src/ioc/setup-ioc-container.ts
@@ -21,7 +21,6 @@ export function setupIocContainer(container = new inversify.Container({ autoBind
     container
         .bind(iocTypes.ProgressReporters)
         .toDynamicValue((context) => {
-            // Note: Keep the WorkflowEnforcer creator last in the array!
             return [context.container.get(AdoConsoleCommentCreator), context.container.get(WorkflowEnforcer)];
         })
         .inSingletonScope();

diff --git a/packages/ado-extension/src/progress-reporter/console/ado-console-comment-creator.spec.ts b/packages/ado-extension/src/progress-reporter/console/ado-console-comment-creator.spec.ts
@@ -23,7 +23,7 @@ describe(AdoConsoleCommentCreator, () => {
 
     describe('constructor', () => {
         it('should initialize', () => {
-            adoConsoleCommentCreator = buildAdoConsoleCommentCreatorWithMocks();
+            buildAdoConsoleCommentCreatorWithMocks();
 
             verifyAllMocks();
         });
@@ -64,16 +64,31 @@ describe(AdoConsoleCommentCreator, () => {
 
     describe('failRun', () => {
         it('does nothing interesting', async () => {
-            const message = 'message';
-            adoConsoleCommentCreator = buildAdoConsoleCommentCreatorWithMocks();
+            const adoConsoleCommentCreator = buildAdoConsoleCommentCreatorWithMocks();
 
-            await adoConsoleCommentCreator.failRun(message);
+            await adoConsoleCommentCreator.failRun();
 
             verifyAllMocks();
         });
     });
 
-    const buildAdoConsoleCommentCreatorWithMocks = () =>
+    describe('didScanSucceed', () => {
+        it('returns true by default', async () => {
+            const adoConsoleCommentCreator = buildAdoConsoleCommentCreatorWithMocks();
+
+            await expect(adoConsoleCommentCreator.didScanSucceed()).resolves.toBe(true);
+        });
+
+        it('returns true after failRun() is called', async () => {
+            const adoConsoleCommentCreator = buildAdoConsoleCommentCreatorWithMocks();
+
+            await adoConsoleCommentCreator.failRun();
+
+            await expect(adoConsoleCommentCreator.didScanSucceed()).resolves.toBe(true);
+        });
+    });
+
+    const buildAdoConsoleCommentCreatorWithMocks = (): AdoConsoleCommentCreator =>
         new AdoConsoleCommentCreator(adoTaskConfigMock.object, reportMarkdownConvertorMock.object, loggerMock.object);
 
     const verifyAllMocks = () => {

diff --git a/packages/ado-extension/src/progress-reporter/console/ado-console-comment-creator.ts b/packages/ado-extension/src/progress-reporter/console/ado-console-comment-creator.ts
@@ -39,8 +39,8 @@ export class AdoConsoleCommentCreator extends ProgressReporter {
     }
 
     // eslint-disable-next-line @typescript-eslint/no-unused-vars
-    public async failRun(message: string): Promise<void> {
-        // This gets handled in the WorkflowEnforcer
+    public async failRun(): Promise<void> {
+        // We don't do anything for failed runs
     }
 
     private getBaselineInfo(baselineEvaluation?: BaselineEvaluation): BaselineInfo {

diff --git a/packages/ado-extension/src/progress-reporter/enforcement/workflow-enforcer.spec.ts b/packages/ado-extension/src/progress-reporter/enforcement/workflow-enforcer.spec.ts
@@ -8,13 +8,16 @@ import { CombinedReportParameters } from 'accessibility-insights-report';
 
 import { BaselineEvaluation, BaselineFileContent } from 'accessibility-insights-scan';
 import { WorkflowEnforcer } from './workflow-enforcer';
+import { Logger } from '@accessibility-insights-action/shared';
 
 describe(WorkflowEnforcer, () => {
     let adoTaskConfigMock: IMock<ADOTaskConfig>;
+    let loggerMock: IMock<Logger>;
     let workflowEnforcer: WorkflowEnforcer;
 
     beforeEach(() => {
         adoTaskConfigMock = Mock.ofType<ADOTaskConfig>(undefined, MockBehavior.Strict);
+        loggerMock = Mock.ofType<Logger>(undefined, MockBehavior.Strict);
     });
 
     describe('constructor', () => {
@@ -26,7 +29,7 @@ describe(WorkflowEnforcer, () => {
     });
 
     describe('completeRun', () => {
-        it('throws correct error if accessibility error occurred', async () => {
+        it('logs correct error if accessibility error occurred', async () => {
             const reportStub = {
                 results: {
                     urlResults: {
@@ -37,30 +40,28 @@ describe(WorkflowEnforcer, () => {
             const baselineEvaluationStub = {} as BaselineEvaluation;
 
             setupFailOnAccessibilityError(true);
+            setupLoggerWithErrorMessage('An accessibility error was found and you specified the "failOnAccessibilityError" flag.');
 
-            workflowEnforcer = buildWorkflowEnforcerWithMocks();
+            const workflowEnforcer = buildWorkflowEnforcerWithMocks();
 
-            await expect(workflowEnforcer.completeRun(reportStub, baselineEvaluationStub)).rejects.toThrowError(
-                'Failed Accessibility Error',
-            );
+            await workflowEnforcer.completeRun(reportStub, baselineEvaluationStub);
 
             verifyAllMocks();
         });
 
-        it('throws correct error if baseline needs to be updated', async () => {
+        it('logs correct error if baseline needs to be updated', async () => {
             const reportStub = {} as CombinedReportParameters;
             const baselineEvaluationStub = {
                 suggestedBaselineUpdate: {} as BaselineFileContent,
             } as BaselineEvaluation;
 
             setupFailOnAccessibilityError(false);
             setupBaselineFileParameterExists();
+            setupLoggerWithErrorMessage('The baseline file does not match scan results.');
 
-            workflowEnforcer = buildWorkflowEnforcerWithMocks();
+            const workflowEnforcer = buildWorkflowEnforcerWithMocks();
 
-            await expect(workflowEnforcer.completeRun(reportStub, baselineEvaluationStub)).rejects.toThrowError(
-                'Failed: The baseline file does not match scan results. If this is a PR, check the PR comments.',
-            );
+            await workflowEnforcer.completeRun(reportStub, baselineEvaluationStub);
 
             verifyAllMocks();
         });
@@ -72,7 +73,7 @@ describe(WorkflowEnforcer, () => {
             setupFailOnAccessibilityError(false);
             setupBaselineFileParameterExists();
 
-            workflowEnforcer = buildWorkflowEnforcerWithMocks();
+            const workflowEnforcer = buildWorkflowEnforcerWithMocks();
 
             await workflowEnforcer.completeRun(reportStub, baselineEvaluationStub);
 
@@ -92,16 +93,20 @@ describe(WorkflowEnforcer, () => {
         });
     });
 
-    describe('failRun', () => {
-        it('reject promise with matching error', async () => {
-            workflowEnforcer = buildWorkflowEnforcerWithMocks();
+    describe('didScanSucceed', () => {
+        it('returns true by default', async () => {
+            const workflowEnforcer = buildWorkflowEnforcerWithMocks();
+            await expect(workflowEnforcer.didScanSucceed()).resolves.toBe(true);
+        });
 
-            await expect(workflowEnforcer.failRun('message')).rejects.toThrowError('message');
-            verifyAllMocks();
+        it('returns false after failRun() is called', async () => {
+            const workflowEnforcer = buildWorkflowEnforcerWithMocks();
+            await workflowEnforcer.failRun();
+            await expect(workflowEnforcer.didScanSucceed()).resolves.toBe(false);
         });
     });
 
-    const buildWorkflowEnforcerWithMocks = () => new WorkflowEnforcer(adoTaskConfigMock.object);
+    const buildWorkflowEnforcerWithMocks = () => new WorkflowEnforcer(adoTaskConfigMock.object, loggerMock.object);
 
     const verifyAllMocks = () => {
         adoTaskConfigMock.verifyAll();
@@ -120,4 +125,8 @@ describe(WorkflowEnforcer, () => {
             .returns(() => 'baseline-file')
             .verifiable(Times.atLeastOnce());
     };
+
+    const setupLoggerWithErrorMessage = (message: string) => {
+        loggerMock.setup((o) => o.logError(message)).verifiable(Times.once());
+    };
 });
diff --git a/packages/ado-extension/src/progress-reporter/enforcement/workflow-enforcer.ts b/packages/ado-extension/src/progress-reporter/enforcement/workflow-enforcer.ts
@@ -1,16 +1,17 @@
 // Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT License.
 
-import { AdoIocTypes } from '../../ioc/ado-ioc-types';
 import { ADOTaskConfig } from '../../task-config/ado-task-config';
 import { inject, injectable } from 'inversify';
-import { ProgressReporter } from '@accessibility-insights-action/shared';
+import { Logger, ProgressReporter } from '@accessibility-insights-action/shared';
 import { CombinedReportParameters } from 'accessibility-insights-report';
 import { BaselineEvaluation } from 'accessibility-insights-scan';
 
 @injectable()
 export class WorkflowEnforcer extends ProgressReporter {
-    constructor(@inject(ADOTaskConfig) private readonly adoTaskConfig: ADOTaskConfig) {
+    private scanSucceeded = true;
+
+    constructor(@inject(ADOTaskConfig) private readonly adoTaskConfig: ADOTaskConfig, @inject(Logger) private readonly logger: Logger) {
         super();
     }
 
@@ -20,24 +21,35 @@ export class WorkflowEnforcer extends ProgressReporter {
 
     // eslint-disable-next-line @typescript-eslint/require-await
     public async completeRun(combinedReportResult: CombinedReportParameters, baselineEvaluation?: BaselineEvaluation): Promise<void> {
-        this.failIfAccessibilityErrorExists(combinedReportResult);
-        this.failIfBaselineNeedsUpdating(baselineEvaluation);
+        if (!(await this.failIfAccessibilityErrorExists(combinedReportResult))) {
+            await this.failIfBaselineNeedsUpdating(baselineEvaluation);
+        }
     }
 
     // eslint-disable-next-line @typescript-eslint/require-await
-    public async failRun(message: string): Promise<void> {
-        throw new Error(message);
+    public async failRun(): Promise<void> {
+        this.scanSucceeded = false;
+    }
+
+    public didScanSucceed(): Promise<boolean> {
+        return Promise.resolve(this.scanSucceeded);
     }
 
-    private failIfAccessibilityErrorExists(combinedReportResult: CombinedReportParameters): void {
+    private async failIfAccessibilityErrorExists(combinedReportResult: CombinedReportParameters): Promise<boolean> {
         if (this.adoTaskConfig.getFailOnAccessibilityError() && combinedReportResult.results.urlResults.failedUrls > 0) {
-            throw new Error('Failed Accessibility Error');
+            this.logger.logError('An accessibility error was found and you specified the "failOnAccessibilityError" flag.');
+            await this.failRun();
+            return true;
         }
+        return false;
     }
 
-    private failIfBaselineNeedsUpdating(baselineEvaluation?: BaselineEvaluation): void {
+    private async failIfBaselineNeedsUpdating(baselineEvaluation?: BaselineEvaluation): Promise<boolean> {
         if (baselineEvaluation && this.adoTaskConfig.getBaselineFile() && baselineEvaluation.suggestedBaselineUpdate) {
-            throw new Error('Failed: The baseline file does not match scan results. If this is a PR, check the PR comments.');
+            this.logger.logError('The baseline file does not match scan results.');
+            await this.failRun();
+            return true;
         }
+        return false;
     }
 }
diff --git a/packages/gh-action/src/console/console-comment-creator.spec.ts b/packages/gh-action/src/console/console-comment-creator.spec.ts
@@ -32,10 +32,14 @@ describe(ConsoleCommentCreator, () => {
         });
     });
 
-    describe('failRun', () => {
-        it('throws error', async () => {
-            const errorMessage = 'some error message';
-            await expect(testSubject.failRun(errorMessage)).rejects.toBe(errorMessage);
+    describe('didScanSucceed', () => {
+        it('returns true by default', async () => {
+            await expect(testSubject.didScanSucceed()).resolves.toBe(true);
+        });
+
+        it('returns false after failRun() is called', async () => {
+            await testSubject.failRun();
+            await expect(testSubject.didScanSucceed()).resolves.toBe(false);
         });
     });
 

diff --git a/packages/gh-action/src/console/console-comment-creator.ts b/packages/gh-action/src/console/console-comment-creator.ts
@@ -7,6 +7,8 @@ import { CombinedReportParameters } from 'accessibility-insights-report';
 
 @injectable()
 export class ConsoleCommentCreator extends ProgressReporter {
+    private scanSucceeded = true;
+
     constructor(
         @inject(ReportMarkdownConvertor) private readonly reportMarkdownConvertor: ReportMarkdownConvertor,
         @inject(Logger) private readonly logger: Logger,
@@ -27,7 +29,11 @@ export class ConsoleCommentCreator extends ProgressReporter {
     }
 
     // eslint-disable-next-line @typescript-eslint/require-await
-    public async failRun(message: string): Promise<void> {
-        throw message;
+    public async failRun(): Promise<void> {
+        this.scanSucceeded = false;
+    }
+
+    public didScanSucceed(): Promise<boolean> {
+        return Promise.resolve(this.scanSucceeded);
     }
 }
diff --git a/packages/gh-action/src/index.ts b/packages/gh-action/src/index.ts
@@ -3,6 +3,7 @@
 import 'reflect-metadata';
 import './module-name-mapper';
 
+import { ExitCode } from '@accessibility-insights-action/shared';
 import { Logger } from '@accessibility-insights-action/shared';
 import { hookStderr } from '@accessibility-insights-action/shared';
 import { hookStdout } from '@accessibility-insights-action/shared';
@@ -18,8 +19,8 @@ import { setupIocContainer } from './ioc/setup-ioc-container';
     await logger.setup();
 
     const scanner = container.get(Scanner);
-    await scanner.scan();
+    process.exit((await scanner.scan()) ? ExitCode.ScanCompletedNoUserActionIsNeeded : ExitCode.ScanCompletedUserActionIsNeeded);
 })().catch((error) => {
-    console.log('Exception thrown in action: ', error);
-    process.exit(1);
+    console.log('::error::[Exception] Exception thrown in extension: ', error);
+    process.exit(ExitCode.ScanFailedToComplete);
 });
diff --git a/packages/shared/src/exit-code.ts b/packages/shared/src/exit-code.ts
@@ -0,0 +1,8 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+
+export const ExitCode = {
+    ScanCompletedNoUserActionIsNeeded: 0,
+    ScanCompletedUserActionIsNeeded: 1,
+    ScanFailedToComplete: 2,
+};
diff --git a/packages/shared/src/index.ts b/packages/shared/src/index.ts
@@ -15,3 +15,4 @@ export { BaselineInfo } from './baseline-info';
 export { ArtifactsInfoProvider } from './artifacts-info-provider';
 export { hookStdout } from './output-hooks/hook-stdout';
 export { hookStderr } from './output-hooks/hook-stderr';
+export { ExitCode } from './exit-code';