From a5ccea652bc89a0b7772589b4f5a5abbae2a85c9 Mon Sep 17 00:00:00 2001 From: Stephen Carter Date: Wed, 3 Jan 2024 11:31:34 -0500 Subject: [PATCH 01/10] @W-14689342@: (Part 1) Move RunResultsProcessor to output directory --- src/lib/InputProcessor.ts | 2 +- src/lib/actions/AbstractRunAction.ts | 2 +- src/lib/{util => output}/RunResultsProcessor.ts | 0 test/lib/{util => output}/RunOutputProcessor.test.ts | 2 +- 4 files changed, 3 insertions(+), 3 deletions(-) rename src/lib/{util => output}/RunResultsProcessor.ts (100%) rename test/lib/{util => output}/RunOutputProcessor.test.ts (99%) diff --git a/src/lib/InputProcessor.ts b/src/lib/InputProcessor.ts index 47cadaebf..1ae620e53 100644 --- a/src/lib/InputProcessor.ts +++ b/src/lib/InputProcessor.ts @@ -3,7 +3,7 @@ import normalize = require('normalize-path'); import path = require('path'); import untildify = require("untildify"); import {RunOptions} from "./RuleManager"; -import {RunOutputOptions} from "./util/RunResultsProcessor"; +import {RunOutputOptions} from "./output/RunResultsProcessor"; import {inferFormatFromOutfile, OutputFormat} from "./output/OutputFormat"; /** diff --git a/src/lib/actions/AbstractRunAction.ts b/src/lib/actions/AbstractRunAction.ts index ba36a08fe..8654a63ce 100644 --- a/src/lib/actions/AbstractRunAction.ts +++ b/src/lib/actions/AbstractRunAction.ts @@ -10,7 +10,7 @@ import {Display} from "../Display"; import {RuleFilter} from "../RuleFilter"; import {RuleFilterFactory} from "../RuleFilterFactory"; import {Controller} from "../../Controller"; -import {RunOutputOptions, RunResultsProcessor} from "../util/RunResultsProcessor"; +import {RunOutputOptions, RunResultsProcessor} from "../output/RunResultsProcessor"; import {InputProcessor} from "../InputProcessor"; import {EngineOptionsFactory} from "../EngineOptionsFactory"; import {INTERNAL_ERROR_CODE} from "../../Constants"; diff --git a/src/lib/util/RunResultsProcessor.ts b/src/lib/output/RunResultsProcessor.ts similarity index 100% rename from src/lib/util/RunResultsProcessor.ts rename to src/lib/output/RunResultsProcessor.ts diff --git a/test/lib/util/RunOutputProcessor.test.ts b/test/lib/output/RunOutputProcessor.test.ts similarity index 99% rename from test/lib/util/RunOutputProcessor.test.ts rename to test/lib/output/RunOutputProcessor.test.ts index 4785c918c..317503a6d 100644 --- a/test/lib/util/RunOutputProcessor.test.ts +++ b/test/lib/output/RunOutputProcessor.test.ts @@ -1,6 +1,6 @@ import {expect} from 'chai'; -import {RunOutputOptions, RunResultsProcessor} from '../../../src/lib/util/RunResultsProcessor'; +import {RunOutputOptions, RunResultsProcessor} from '../../../src/lib/output/RunResultsProcessor'; import {EngineExecutionSummary, FormattedOutput, RuleResult} from '../../../src/types'; import {AnyJson} from '@salesforce/ts-types'; import Sinon = require('sinon'); From 4be7b97ec9d53fbb015ad9888fbbfd88eabf5cca Mon Sep 17 00:00:00 2001 From: Stephen Carter Date: Wed, 3 Jan 2024 11:47:36 -0500 Subject: [PATCH 02/10] @W-14689342@: (Part 2) Convert RunResultsProcessor to return void --- src/lib/actions/AbstractRunAction.ts | 6 ++- src/lib/output/RunResultsProcessor.ts | 16 ++++--- test/lib/output/RunOutputProcessor.test.ts | 50 +++++++++++++--------- 3 files changed, 45 insertions(+), 27 deletions(-) diff --git a/src/lib/actions/AbstractRunAction.ts b/src/lib/actions/AbstractRunAction.ts index 8654a63ce..f9024ab35 100644 --- a/src/lib/actions/AbstractRunAction.ts +++ b/src/lib/actions/AbstractRunAction.ts @@ -76,7 +76,9 @@ export abstract class AbstractRunAction implements Action { const targetPaths: string[] = this.inputProcessor.resolveTargetPaths(inputs); const runOptions: RunOptions = this.inputProcessor.createRunOptions(inputs, this.isDfa()); const engineOptions: EngineOptions = this.engineOptionsFactory.createEngineOptions(inputs); + const outputOptions: RunOutputOptions = this.inputProcessor.createRunOutputOptions(inputs); + const runResultsProcessor: RunResultsProcessor = new RunResultsProcessor(this.display, outputOptions, inputs["verbose-violations"] as boolean); // TODO: Inject RuleManager as a dependency to improve testability by removing coupling to runtime implementation const ruleManager: RuleManager = await Controller.createRuleManager(); @@ -84,8 +86,8 @@ export abstract class AbstractRunAction implements Action { try { const results: Results = await ruleManager.runRulesMatchingCriteria(filters, targetPaths, runOptions, engineOptions); this.logger.trace(`Processing output with format ${outputOptions.format}`); - return new RunResultsProcessor(this.display, outputOptions, inputs["verbose-violations"] as boolean) - .processResults(results); + await runResultsProcessor.processResults(results); + return runResultsProcessor.getJsonReturnValue(); } catch (e) { // Rethrow any errors as SF errors. diff --git a/src/lib/output/RunResultsProcessor.ts b/src/lib/output/RunResultsProcessor.ts index ffd415e4d..37b56f0c5 100644 --- a/src/lib/output/RunResultsProcessor.ts +++ b/src/lib/output/RunResultsProcessor.ts @@ -18,6 +18,7 @@ export class RunResultsProcessor { private readonly display: Display; private readonly opts: RunOutputOptions; private readonly verboseViolations: boolean; + private jsonReturnValue: AnyJson = []; public constructor(display: Display, opts: RunOutputOptions, verboseViolations: boolean) { this.display = display; @@ -25,7 +26,11 @@ export class RunResultsProcessor { this.verboseViolations = verboseViolations; } - public async processResults(results: Results): Promise { + public getJsonReturnValue(): AnyJson { + return this.jsonReturnValue; + } + + public async processResults(results: Results): Promise { const minSev: number = results.getMinSev(); const summaryMap: Map = results.getSummaryMap(); const formattedOutput = await results.toFormattedOutput(this.opts.format, this.verboseViolations); @@ -42,7 +47,8 @@ export class RunResultsProcessor { // ...log it to the console... this.display.displayInfo(msg); // ...and return it for use with the --json flag. - return msg; + this.jsonReturnValue = msg; + return; } // If we have violations (or an outfile but no violations), we'll build an array of @@ -68,15 +74,15 @@ export class RunResultsProcessor { // Finally, we need to return something for use by the --json flag. if (this.opts.outfile) { // If we used an outfile, we should just return the summary message, since that says where the file is. - return msg; + this.jsonReturnValue = msg; } else if (typeof formattedOutput === 'string') { // If the specified output format was JSON, then the results are a huge stringified JSON that we should parse // before returning. Otherwise, we should just return the result string. - return this.opts.format === OutputFormat.JSON ? JSON.parse(formattedOutput) as AnyJson : formattedOutput; + this.jsonReturnValue = this.opts.format === OutputFormat.JSON ? JSON.parse(formattedOutput) as AnyJson : formattedOutput; } else { // If the results are a JSON, return the `rows` property, since that's all of the data that would be displayed // in the table. - return formattedOutput.rows; + this.jsonReturnValue = formattedOutput.rows; } } diff --git a/test/lib/output/RunOutputProcessor.test.ts b/test/lib/output/RunOutputProcessor.test.ts index 317503a6d..447cb9309 100644 --- a/test/lib/output/RunOutputProcessor.test.ts +++ b/test/lib/output/RunOutputProcessor.test.ts @@ -153,7 +153,7 @@ describe('RunOutputProcessor', () => { const opts: RunOutputOptions = { format: OutputFormat.TABLE }; - const rop = new RunResultsProcessor(display, opts, false); + const rrp = new RunResultsProcessor(display, opts, false); const summaryMap: Map = new Map(); summaryMap.set('pmd', {fileCount: 0, violationCount: 0}); summaryMap.set('eslint', {fileCount: 0, violationCount: 0}); @@ -161,7 +161,8 @@ describe('RunOutputProcessor', () => { .withMinSev(0).withSummaryMap(summaryMap).withFormattedOutput(''); // THIS IS THE PART BEING TESTED. - const output: AnyJson = await rop.processResults(fakeResults); + await rrp.processResults(fakeResults); + const output: AnyJson = rrp.getJsonReturnValue(); // We expect that the message logged to the console and the message returned should both be the default const expectedMsg = getMessage(BundleName.RunOutputProcessor, 'output.noViolationsDetected', ['pmd, eslint']); @@ -177,10 +178,11 @@ describe('RunOutputProcessor', () => { const opts: RunOutputOptions = { format: OutputFormat.TABLE }; - const rop = new RunResultsProcessor(display, opts, false); + const rrp = new RunResultsProcessor(display, opts, false); // THIS IS THE PART BEING TESTED. - const output: AnyJson = await rop.processResults(fakeTableResults); + await rrp.processResults(fakeTableResults); + const output: AnyJson = rrp.getJsonReturnValue(); const expectedTableSummary = `${getMessage(BundleName.RunOutputProcessor, 'output.engineSummaryTemplate', ['pmd', 1, 1])} ${getMessage(BundleName.RunOutputProcessor, 'output.engineSummaryTemplate', ['eslint-typescript', 2, 1])} @@ -199,11 +201,12 @@ ${getMessage(BundleName.RunOutputProcessor, 'output.writtenToConsole')}`; format: OutputFormat.TABLE, severityForError: 1 }; - const rop = new RunResultsProcessor(display, opts, false); + const rrp = new RunResultsProcessor(display, opts, false); // THIS IS THE PART BEING TESTED. try { - const output: AnyJson = await rop.processResults(fakeTableResults); + await rrp.processResults(fakeTableResults); + const output: AnyJson = rrp.getJsonReturnValue(); expect(true).to.equal(false, `Unexpectedly returned ${output} instead of throwing error`); } catch (e) { expect(display.getOutputText()).to.satisfy(msg => msg.startsWith("[Table]")); @@ -230,10 +233,11 @@ ${getMessage(BundleName.RunOutputProcessor, 'output.writtenToConsole')}`; format: OutputFormat.CSV }; - const rop = new RunResultsProcessor(display, opts, false); + const rrp = new RunResultsProcessor(display, opts, false); // THIS IS THE PART BEING TESTED. - const output: AnyJson = await rop.processResults(fakeCsvResults); + await rrp.processResults(fakeCsvResults); + const output: AnyJson = rrp.getJsonReturnValue(); expect(display.getOutputText()).to.equal("[Info]: " + FAKE_CSV_OUTPUT); expect(output).to.equal(FAKE_CSV_OUTPUT, 'CSV should be returned as a string'); }); @@ -244,11 +248,12 @@ ${getMessage(BundleName.RunOutputProcessor, 'output.writtenToConsole')}`; severityForError: 2 }; - const rop = new RunResultsProcessor(display, opts, false); + const rrp = new RunResultsProcessor(display, opts, false); // THIS IS THE PART BEING TESTED. try { - const output: AnyJson = await rop.processResults(fakeCsvResults); + await rrp.processResults(fakeCsvResults); + const output: AnyJson = rrp.getJsonReturnValue(); expect(true).to.equal(false, `Unexpectedly returned ${output} instead of throwing error`); } catch (e) { expect(display.getOutputText()).to.equal("[Info]: " + FAKE_CSV_OUTPUT); @@ -270,10 +275,11 @@ ${getMessage(BundleName.RunOutputProcessor, 'output.writtenToConsole')}`; format: OutputFormat.JSON }; - const rop = new RunResultsProcessor(display, opts, false); + const rrp = new RunResultsProcessor(display, opts, false); // THIS IS THE PART BEING TESTED - const output: AnyJson = await rop.processResults(fakeJsonResults); + await rrp.processResults(fakeJsonResults); + const output: AnyJson = rrp.getJsonReturnValue(); expect(display.getOutputText()).to.equal("[Info]: " + FAKE_JSON_OUTPUT); expect(output).to.deep.equal(JSON.parse(FAKE_JSON_OUTPUT), 'JSON should be returned as a parsed object'); @@ -285,11 +291,12 @@ ${getMessage(BundleName.RunOutputProcessor, 'output.writtenToConsole')}`; severityForError: 1 }; - const rop = new RunResultsProcessor(display, opts, false); + const rrp = new RunResultsProcessor(display, opts, false); // THIS IS THE PART BEING TESTED try { - const output: AnyJson = await rop.processResults(fakeJsonResults); + await rrp.processResults(fakeJsonResults); + const output: AnyJson = rrp.getJsonReturnValue(); expect(true).to.equal(false, `Unexpectedly returned ${output} instead of throwing error`); } catch (e) { expect(display.getOutputText()).to.equal("[Info]: " + FAKE_JSON_OUTPUT); @@ -306,7 +313,7 @@ ${getMessage(BundleName.RunOutputProcessor, 'output.writtenToConsole')}`; format: OutputFormat.CSV, outfile: fakeFilePath }; - const rop = new RunResultsProcessor(display, opts, false); + const rrp = new RunResultsProcessor(display, opts, false); const summaryMap: Map = new Map(); summaryMap.set('pmd', {fileCount: 0, violationCount: 0}); summaryMap.set('eslint', {fileCount: 0, violationCount: 0}); @@ -314,7 +321,8 @@ ${getMessage(BundleName.RunOutputProcessor, 'output.writtenToConsole')}`; .withMinSev(0).withSummaryMap(summaryMap).withFormattedOutput('"Problem","Severity","File","Line","Column","Rule","Description","URL","Category","Engine"'); // THIS IS THE PART BEING TESTED. - const output: AnyJson = await rop.processResults(fakeResults); + await rrp.processResults(fakeResults); + const output: AnyJson = rrp.getJsonReturnValue(); // We expect the empty CSV output followed by the default engine summary and written-to-file messages are logged to the console const expectedMsg = `${getMessage(BundleName.RunOutputProcessor, 'output.engineSummaryTemplate', ['pmd', 0, 0])} @@ -336,10 +344,11 @@ ${getMessage(BundleName.RunOutputProcessor, 'output.writtenToOutFile', [fakeFile outfile: fakeFilePath }; - const rop = new RunResultsProcessor(display, opts, false); + const rrp = new RunResultsProcessor(display, opts, false); // THIS IS THE PART BEING TESTED. - const output: AnyJson = await rop.processResults(fakeCsvResults); + await rrp.processResults(fakeCsvResults); + const output: AnyJson = rrp.getJsonReturnValue(); const expectedCsvSummary = `${getMessage(BundleName.RunOutputProcessor, 'output.engineSummaryTemplate', ['pmd', 1, 1])} ${getMessage(BundleName.RunOutputProcessor, 'output.engineSummaryTemplate', ['eslint-typescript', 2, 1])} @@ -357,11 +366,12 @@ ${getMessage(BundleName.RunOutputProcessor, 'output.writtenToOutFile', [fakeFile outfile: fakeFilePath }; - const rop = new RunResultsProcessor(display, opts, false); + const rrp = new RunResultsProcessor(display, opts, false); // THIS IS THE PART BEING TESTED. try { - const output: AnyJson = await rop.processResults(fakeCsvResults); + await rrp.processResults(fakeCsvResults); + const output: AnyJson = rrp.getJsonReturnValue(); expect(true).to.equal(false, `Unexpectedly returned ${output} instead of throwing error`); } catch (e) { expect(display.getOutputText()).to.equal(""); From d46f6e1de955a180df4cb34895d674ff832cf8d7 Mon Sep 17 00:00:00 2001 From: Stephen Carter Date: Wed, 3 Jan 2024 11:59:36 -0500 Subject: [PATCH 03/10] @W-14689342@: (Part 3) Introduce ResultsProcessor interface with composite --- src/lib/actions/AbstractRunAction.ts | 7 ++- src/lib/output/ResultsProcessor.ts | 19 +++++++ src/lib/output/RunResultsProcessor.ts | 5 +- .../output/CompositeResultsProcessor.test.ts | 31 +++++++++++ test/lib/output/FakeResults.ts | 52 +++++++++++++++++++ test/lib/output/RunOutputProcessor.test.ts | 52 +------------------ 6 files changed, 114 insertions(+), 52 deletions(-) create mode 100644 src/lib/output/ResultsProcessor.ts create mode 100644 test/lib/output/CompositeResultsProcessor.test.ts create mode 100644 test/lib/output/FakeResults.ts diff --git a/src/lib/actions/AbstractRunAction.ts b/src/lib/actions/AbstractRunAction.ts index f9024ab35..afbf8080a 100644 --- a/src/lib/actions/AbstractRunAction.ts +++ b/src/lib/actions/AbstractRunAction.ts @@ -16,6 +16,7 @@ import {EngineOptionsFactory} from "../EngineOptionsFactory"; import {INTERNAL_ERROR_CODE} from "../../Constants"; import {Results} from "../output/Results"; import {inferFormatFromOutfile, OutputFormat} from "../output/OutputFormat"; +import {CompositeResultsProcessor, ResultsProcessor} from "../output/ResultsProcessor"; /** * Abstract Action to share a common implementation behind the "run" and "run dfa" commands @@ -80,13 +81,17 @@ export abstract class AbstractRunAction implements Action { const outputOptions: RunOutputOptions = this.inputProcessor.createRunOutputOptions(inputs); const runResultsProcessor: RunResultsProcessor = new RunResultsProcessor(this.display, outputOptions, inputs["verbose-violations"] as boolean); + const compositeResultsProcessor: ResultsProcessor = new CompositeResultsProcessor([ + runResultsProcessor + ]); + // TODO: Inject RuleManager as a dependency to improve testability by removing coupling to runtime implementation const ruleManager: RuleManager = await Controller.createRuleManager(); try { const results: Results = await ruleManager.runRulesMatchingCriteria(filters, targetPaths, runOptions, engineOptions); this.logger.trace(`Processing output with format ${outputOptions.format}`); - await runResultsProcessor.processResults(results); + await compositeResultsProcessor.processResults(results); return runResultsProcessor.getJsonReturnValue(); } catch (e) { diff --git a/src/lib/output/ResultsProcessor.ts b/src/lib/output/ResultsProcessor.ts new file mode 100644 index 000000000..7013eac97 --- /dev/null +++ b/src/lib/output/ResultsProcessor.ts @@ -0,0 +1,19 @@ +import {Results} from "./Results"; + +export interface ResultsProcessor { + processResults(results: Results): Promise; +} + +export class CompositeResultsProcessor implements ResultsProcessor { + private readonly delegates: ResultsProcessor[]; + + public constructor(delegateResultsProcessors: ResultsProcessor[]) { + this.delegates = delegateResultsProcessors; + } + + async processResults(results: Results): Promise { + for (const delegate of this.delegates) { + await delegate.processResults(results); + } + } +} diff --git a/src/lib/output/RunResultsProcessor.ts b/src/lib/output/RunResultsProcessor.ts index 37b56f0c5..3f8951a4f 100644 --- a/src/lib/output/RunResultsProcessor.ts +++ b/src/lib/output/RunResultsProcessor.ts @@ -7,6 +7,7 @@ import {Display} from "../Display"; import {INTERNAL_ERROR_CODE} from "../../Constants"; import {OutputFormat} from "../output/OutputFormat"; import {Results} from "../output/Results"; +import {ResultsProcessor} from "./ResultsProcessor"; export type RunOutputOptions = { format: OutputFormat; @@ -14,7 +15,9 @@ export type RunOutputOptions = { outfile?: string; } -export class RunResultsProcessor { +// TODO: We should consider separating this into multiple ResultProcessor classes: +// --> 1 for processing the json return value, 1 for creating the users outfile, and 1 for creating console output +export class RunResultsProcessor implements ResultsProcessor { private readonly display: Display; private readonly opts: RunOutputOptions; private readonly verboseViolations: boolean; diff --git a/test/lib/output/CompositeResultsProcessor.test.ts b/test/lib/output/CompositeResultsProcessor.test.ts new file mode 100644 index 000000000..2b7e3890d --- /dev/null +++ b/test/lib/output/CompositeResultsProcessor.test.ts @@ -0,0 +1,31 @@ +import {CompositeResultsProcessor, ResultsProcessor} from "../../../src/lib/output/ResultsProcessor"; +import {FakeResults} from "./FakeResults"; +import {Results} from "../../../lib/lib/output/Results"; +import {expect} from "chai"; + +describe('CompositeResultsProcessor Tests', () => { + it('Empty composite should not blow up', async () => { + const compositeResultsProcessor: CompositeResultsProcessor = new CompositeResultsProcessor([]); + await compositeResultsProcessor.processResults(new FakeResults()); + }); + + it('Each delegate is called correctly', async () => { + const results: Results = new FakeResults(); + const resultsProcessors: FakeResultsProcessor[] = [ + new FakeResultsProcessor(), new FakeResultsProcessor(), new FakeResultsProcessor()]; + const compositeResultsProcessor: CompositeResultsProcessor = new CompositeResultsProcessor(resultsProcessors); + await compositeResultsProcessor.processResults(results); + for (let i = 0; i < resultsProcessors.length; i++) { + expect(resultsProcessors[i].lastResultsProcessed).to.equal(results); + } + }); +}); + +class FakeResultsProcessor implements ResultsProcessor { + lastResultsProcessed: Results = null; + + processResults(results: Results): Promise { + this.lastResultsProcessed = results; + return Promise.resolve(); + } +} diff --git a/test/lib/output/FakeResults.ts b/test/lib/output/FakeResults.ts new file mode 100644 index 000000000..b54ee7b98 --- /dev/null +++ b/test/lib/output/FakeResults.ts @@ -0,0 +1,52 @@ +import {Results} from "../../../src/lib/output/Results"; +import {EngineExecutionSummary, FormattedOutput, RuleResult} from "../../../src/types"; +import {OutputFormat} from "../../../src/lib/output/OutputFormat"; + +export class FakeResults implements Results { + private minSev: number = 0; + private summaryMap: Map; + private formattedOutput: FormattedOutput; + + withMinSev(minSev: number): FakeResults { + this.minSev = minSev; + return this; + } + + withSummaryMap(summaryMap: Map): FakeResults { + this.summaryMap = summaryMap; + return this; + } + + withFormattedOutput(formattedOutput: FormattedOutput): FakeResults { + this.formattedOutput = formattedOutput; + return this; + } + + getExecutedEngines(): Set { + throw new Error("Not implemented"); + } + + getMinSev(): number { + return this.minSev; + } + + getRuleResults(): RuleResult[] { + throw new Error("Not implemented"); + } + + getSummaryMap(): Map { + return this.summaryMap; + } + + isEmpty(): boolean { + throw new Error("Not implemented"); + } + + violationsAreDfa(): boolean { + throw new Error("Not implemented"); + } + + toFormattedOutput(_format: OutputFormat, _verboseViolations: boolean): Promise { + return Promise.resolve(this.formattedOutput); + } +} diff --git a/test/lib/output/RunOutputProcessor.test.ts b/test/lib/output/RunOutputProcessor.test.ts index 447cb9309..26312d905 100644 --- a/test/lib/output/RunOutputProcessor.test.ts +++ b/test/lib/output/RunOutputProcessor.test.ts @@ -1,7 +1,7 @@ import {expect} from 'chai'; import {RunOutputOptions, RunResultsProcessor} from '../../../src/lib/output/RunResultsProcessor'; -import {EngineExecutionSummary, FormattedOutput, RuleResult} from '../../../src/types'; +import {EngineExecutionSummary} from '../../../src/types'; import {AnyJson} from '@salesforce/ts-types'; import Sinon = require('sinon'); import fs = require('fs'); @@ -10,6 +10,7 @@ import {FakeDisplay} from "../FakeDisplay"; import {PATHLESS_COLUMNS} from "../../../src/lib/output/TableOutputFormatter"; import {OutputFormat} from "../../../src/lib/output/OutputFormat"; import {Results} from "../../../src/lib/output/Results"; +import {FakeResults} from "./FakeResults"; const FAKE_SUMMARY_MAP: Map = new Map(); FAKE_SUMMARY_MAP.set('pmd', {fileCount: 1, violationCount: 1}); @@ -78,55 +79,6 @@ const FAKE_JSON_OUTPUT = `[{ }] }]`; -class FakeResults implements Results { - private minSev: number = 0; - private summaryMap: Map; - private formattedOutput: FormattedOutput; - - withMinSev(minSev: number): FakeResults { - this.minSev = minSev; - return this; - } - - withSummaryMap(summaryMap: Map): FakeResults { - this.summaryMap = summaryMap; - return this; - } - - withFormattedOutput(formattedOutput: FormattedOutput): FakeResults { - this.formattedOutput = formattedOutput; - return this; - } - - getExecutedEngines(): Set { - throw new Error("Not implemented"); - } - - getMinSev(): number { - return this.minSev; - } - - getRuleResults(): RuleResult[] { - throw new Error("Not implemented"); - } - - getSummaryMap(): Map { - return this.summaryMap; - } - - isEmpty(): boolean { - throw new Error("Not implemented"); - } - - violationsAreDfa(): boolean { - throw new Error("Not implemented"); - } - - toFormattedOutput(_format: OutputFormat, _verboseViolations: boolean): Promise { - return Promise.resolve(this.formattedOutput); - } -} - describe('RunOutputProcessor', () => { let fakeFiles: {path; data}[] = []; let display: FakeDisplay; From 6aa205f9c11875a7261202c92c5732379400c4eb Mon Sep 17 00:00:00 2001 From: Stephen Carter Date: Wed, 3 Jan 2024 16:28:50 -0500 Subject: [PATCH 04/10] @W-14689342@: (Part 4) Introduce OutfileResultsProcessor and internal environment varaible --- src/Constants.ts | 3 +- src/lib/actions/AbstractRunAction.ts | 16 ++-- src/lib/output/OutfileResultsProcessor.ts | 21 ++++++ src/lib/output/OutputFormat.ts | 3 + src/lib/output/OutputUtils.ts | 13 ++++ src/lib/output/RunResultsProcessor.ts | 15 ++-- test/commands/scanner/run.test.ts | 75 +++++++++++++------ .../output/CompositeResultsProcessor.test.ts | 2 +- test/lib/output/FakeResults.ts | 16 +++- .../output/OutfileResultsProcessor.test.ts | 68 +++++++++++++++++ 10 files changed, 188 insertions(+), 44 deletions(-) create mode 100644 src/lib/output/OutfileResultsProcessor.ts create mode 100644 src/lib/output/OutputUtils.ts create mode 100644 test/lib/output/OutfileResultsProcessor.test.ts diff --git a/src/Constants.ts b/src/Constants.ts index 1ca5d1f33..b80f85d3f 100644 --- a/src/Constants.ts +++ b/src/Constants.ts @@ -13,7 +13,8 @@ export const PMD_CATALOG_FILE = 'PmdCatalog.json'; // TODO: We should flesh this one-off solution out into one that handles all the various env vars we use. // E.g., the ones defined in `EnvironmentVariable.ts` and `dfa.ts`. export const ENV_VAR_NAMES = { - SCANNER_PATH_OVERRIDE: 'SCANNER_PATH_OVERRIDE' + SCANNER_PATH_OVERRIDE: 'SCANNER_PATH_OVERRIDE', + SCANNER_INTERNAL_OUTFILE: 'SCANNER_INTERNAL_OUTFILE' }; export const INTERNAL_ERROR_CODE = 1; diff --git a/src/lib/actions/AbstractRunAction.ts b/src/lib/actions/AbstractRunAction.ts index afbf8080a..30715db4a 100644 --- a/src/lib/actions/AbstractRunAction.ts +++ b/src/lib/actions/AbstractRunAction.ts @@ -13,10 +13,11 @@ import {Controller} from "../../Controller"; import {RunOutputOptions, RunResultsProcessor} from "../output/RunResultsProcessor"; import {InputProcessor} from "../InputProcessor"; import {EngineOptionsFactory} from "../EngineOptionsFactory"; -import {INTERNAL_ERROR_CODE} from "../../Constants"; +import {ENV_VAR_NAMES, INTERNAL_ERROR_CODE} from "../../Constants"; import {Results} from "../output/Results"; import {inferFormatFromOutfile, OutputFormat} from "../output/OutputFormat"; import {CompositeResultsProcessor, ResultsProcessor} from "../output/ResultsProcessor"; +import {OutfileResultsProcessor} from "../output/OutfileResultsProcessor"; /** * Abstract Action to share a common implementation behind the "run" and "run dfa" commands @@ -79,11 +80,16 @@ export abstract class AbstractRunAction implements Action { const engineOptions: EngineOptions = this.engineOptionsFactory.createEngineOptions(inputs); const outputOptions: RunOutputOptions = this.inputProcessor.createRunOutputOptions(inputs); - const runResultsProcessor: RunResultsProcessor = new RunResultsProcessor(this.display, outputOptions, inputs["verbose-violations"] as boolean); + const verboseViolations: boolean = inputs["verbose-violations"] as boolean; - const compositeResultsProcessor: ResultsProcessor = new CompositeResultsProcessor([ - runResultsProcessor - ]); + const runResultsProcessor: RunResultsProcessor = new RunResultsProcessor(this.display, outputOptions, verboseViolations); + const resultsProcessors: ResultsProcessor[] = [runResultsProcessor]; + const internalOutfile: string = process.env[ENV_VAR_NAMES.SCANNER_INTERNAL_OUTFILE]; + if (internalOutfile && internalOutfile.length > 0) { + const internalOutputFormat: OutputFormat = inferFormatFromOutfile(internalOutfile); + resultsProcessors.push(new OutfileResultsProcessor(internalOutputFormat, internalOutfile, verboseViolations)); + } + const compositeResultsProcessor: ResultsProcessor = new CompositeResultsProcessor(resultsProcessors); // TODO: Inject RuleManager as a dependency to improve testability by removing coupling to runtime implementation const ruleManager: RuleManager = await Controller.createRuleManager(); diff --git a/src/lib/output/OutfileResultsProcessor.ts b/src/lib/output/OutfileResultsProcessor.ts new file mode 100644 index 000000000..a84ac933e --- /dev/null +++ b/src/lib/output/OutfileResultsProcessor.ts @@ -0,0 +1,21 @@ +import {ResultsProcessor} from "./ResultsProcessor"; +import {Results} from "./Results"; +import {OutputFormat} from "./OutputFormat"; +import {writeToFile} from "./OutputUtils"; +import {FormattedOutput} from "../../types"; + +export class OutfileResultsProcessor implements ResultsProcessor { + private readonly outputFormat: OutputFormat; + private readonly outfile: string; + private readonly verboseViolations: boolean; + public constructor(format:OutputFormat, outfile: string, verboseViolations: boolean) { + this.outputFormat = format; + this.outfile = outfile; + this.verboseViolations = verboseViolations; + } + + public async processResults(results: Results): Promise { + const fileContents: FormattedOutput = await results.toFormattedOutput(this.outputFormat, this.verboseViolations); + writeToFile(this.outfile, fileContents as string); + } +} diff --git a/src/lib/output/OutputFormat.ts b/src/lib/output/OutputFormat.ts index 9205fc875..5c8a2402a 100644 --- a/src/lib/output/OutputFormat.ts +++ b/src/lib/output/OutputFormat.ts @@ -13,6 +13,9 @@ export enum OutputFormat { } export function inferFormatFromOutfile(outfile: string): OutputFormat { + // TODO: Since the this is being used now for both the --outfile and the internal outfile, we should move where + // validation takes place for --outfile and make these messages more generic. + const lastPeriod: number = outfile.lastIndexOf('.'); if (lastPeriod < 1 || lastPeriod + 1 === outfile.length) { throw new SfError(getMessage(BundleName.CommonRun, 'validations.outfileMustBeValid'), null, null, INTERNAL_ERROR_CODE); diff --git a/src/lib/output/OutputUtils.ts b/src/lib/output/OutputUtils.ts new file mode 100644 index 000000000..907f42bad --- /dev/null +++ b/src/lib/output/OutputUtils.ts @@ -0,0 +1,13 @@ +import {SfError} from "@salesforce/core"; +import {INTERNAL_ERROR_CODE} from "../../Constants"; +import fs = require('fs'); + +export function writeToFile(file: string, fileContents: string): void { + try { + fs.writeFileSync(file, fileContents); + } catch (e) { + // Rethrow any errors as SfError. + const message: string = e instanceof Error ? e.message : e as string; + throw new SfError(message, null, null, INTERNAL_ERROR_CODE); + } +} diff --git a/src/lib/output/RunResultsProcessor.ts b/src/lib/output/RunResultsProcessor.ts index 3f8951a4f..9d853877f 100644 --- a/src/lib/output/RunResultsProcessor.ts +++ b/src/lib/output/RunResultsProcessor.ts @@ -1,6 +1,5 @@ import {AnyJson} from '@salesforce/ts-types'; import {SfError} from '@salesforce/core'; -import fs = require('fs'); import {FormattedOutput, EngineExecutionSummary} from '../../types'; import {BundleName, getMessage} from "../../MessageCatalog"; import {Display} from "../Display"; @@ -8,6 +7,7 @@ import {INTERNAL_ERROR_CODE} from "../../Constants"; import {OutputFormat} from "../output/OutputFormat"; import {Results} from "../output/Results"; import {ResultsProcessor} from "./ResultsProcessor"; +import {writeToFile} from "./OutputUtils"; export type RunOutputOptions = { format: OutputFormat; @@ -126,15 +126,10 @@ export class RunResultsProcessor implements ResultsProcessor { } private writeToOutfile(results: string | {columns; rows}): string { - try { - // At this point, we can cast `results` to a string, since it being an object would indicate that the format - // is `table`, and we already have validations preventing tables from being written to files. - fs.writeFileSync(this.opts.outfile, results as string); - } catch (e) { - // Rethrow any errors as SfdxErrors. - const message: string = e instanceof Error ? e.message : e as string; - throw new SfError(message, null, null, INTERNAL_ERROR_CODE); - } + // At this point, we can cast `results` to a string, since it being an object would indicate that the format + // is `table`, and we already have validations preventing tables from being written to files. + writeToFile(this.opts.outfile, results as string); + // Return a message indicating the action we took. return getMessage(BundleName.RunOutputProcessor, 'output.writtenToOutFile', [this.opts.outfile]); } diff --git a/test/commands/scanner/run.test.ts b/test/commands/scanner/run.test.ts index 0431cfa59..84db09e62 100644 --- a/test/commands/scanner/run.test.ts +++ b/test/commands/scanner/run.test.ts @@ -6,6 +6,8 @@ import path = require('path'); import process = require('process'); import tildify = require('tildify'); import {BundleName, getMessage} from "../../../src/MessageCatalog"; +import * as os from "os"; +import {ENV_VAR_NAMES} from "../../../src/Constants"; const pathToApexFolder = path.join('test', 'code-fixtures', 'apex'); const pathToSomeTestClass = path.join('test', 'code-fixtures', 'apex', 'SomeTestClass.cls'); @@ -16,18 +18,6 @@ const pathToYetAnotherTestClass = path.join('test', 'code-fixtures', 'apex', 'Ye describe('scanner run', function () { describe('E2E', () => { describe('Output Type: XML', () => { - function validateXmlOutput(xml: string): void { - // We'll split the output by the tag, so we can get individual violations. - const violations = xml.split(' tag, so we can get individual violations. @@ -300,17 +290,6 @@ describe('scanner run', function () { }); describe('Output Type: JSON', () => { - function validateJsonOutput(json: string): void { - const output = JSON.parse(json); - // Only PMD rules should have run. - expect(output.length).to.equal(1, 'Should only be violations from one engine'); - expect(output[0].engine).to.equal('pmd', 'Engine should be PMD'); - - expect(output[0].violations.length).to.equal(2, 'Should be 2 violations'); - expect(output[0].violations[0].line).to.equal(11, 'Violation #1 should occur on the expected line'); - expect(output[0].violations[1].line).to.equal(19, 'Violation #2 should occur on the expected line'); - } - function validateNoViolationsJsonOutput(json: string): void { const output = JSON.parse(json); // There should be no violations. @@ -621,4 +600,54 @@ describe('scanner run', function () { }); }); + + describe('We can create internal outfile with SCANNER_INTERNAL_OUTFILE environment variable', () => { + let tmpDir: string = null; + let internalOutfile: string = null; + beforeEach(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'InternalOutfileTest-')); + internalOutfile = path.join(tmpDir, "internalOutfile.json"); + process.env[ENV_VAR_NAMES.SCANNER_INTERNAL_OUTFILE] = internalOutfile; + }); + afterEach(() => { + fs.rmSync(tmpDir, {recursive: true, force: true}); + delete process.env[ENV_VAR_NAMES.SCANNER_INTERNAL_OUTFILE]; + }); + it('Can write a user file and an internal file with different formats at same time', () => { + const userOutfile = path.join(tmpDir, "userOutfile.xml"); + runCommand(`scanner run --target ${pathToSomeTestClass} --ruleset ApexUnit --outfile ${userOutfile}`); + + expect(fs.existsSync(userOutfile)).to.equal(true, 'The command should have created the expected user output file'); + const userFileContents = fs.readFileSync(userOutfile).toString(); + validateXmlOutput(userFileContents); + + expect(fs.existsSync(userOutfile)).to.equal(true, 'The command should have created the expected internal output file'); + const internalFileContents = fs.readFileSync(internalOutfile).toString(); + validateJsonOutput(internalFileContents); + }); + }); }); + +function validateXmlOutput(xml: string): void { + // We'll split the output by the tag, so we can get individual violations. + const violations = xml.split(' { diff --git a/test/lib/output/FakeResults.ts b/test/lib/output/FakeResults.ts index b54ee7b98..b4c11901d 100644 --- a/test/lib/output/FakeResults.ts +++ b/test/lib/output/FakeResults.ts @@ -5,7 +5,7 @@ import {OutputFormat} from "../../../src/lib/output/OutputFormat"; export class FakeResults implements Results { private minSev: number = 0; private summaryMap: Map; - private formattedOutput: FormattedOutput; + private formattedOutputMap: Map = new Map(); withMinSev(minSev: number): FakeResults { this.minSev = minSev; @@ -18,7 +18,12 @@ export class FakeResults implements Results { } withFormattedOutput(formattedOutput: FormattedOutput): FakeResults { - this.formattedOutput = formattedOutput; + this.formattedOutputMap.set("default", formattedOutput); + return this; + } + + withFormattedOutputForFormat(format: OutputFormat, formattedOutput: FormattedOutput): FakeResults { + this.formattedOutputMap.set(format as string, formattedOutput); return this; } @@ -46,7 +51,10 @@ export class FakeResults implements Results { throw new Error("Not implemented"); } - toFormattedOutput(_format: OutputFormat, _verboseViolations: boolean): Promise { - return Promise.resolve(this.formattedOutput); + toFormattedOutput(format: OutputFormat, _verboseViolations: boolean): Promise { + if(this.formattedOutputMap.has(format as string)) { + return Promise.resolve(this.formattedOutputMap.get(format as string)); + } + return Promise.resolve(this.formattedOutputMap.get("default")); } } diff --git a/test/lib/output/OutfileResultsProcessor.test.ts b/test/lib/output/OutfileResultsProcessor.test.ts new file mode 100644 index 000000000..524abcacf --- /dev/null +++ b/test/lib/output/OutfileResultsProcessor.test.ts @@ -0,0 +1,68 @@ +import {FakeResults} from "./FakeResults"; +import {Results} from "../../../src/lib/output/Results"; +import {ResultsProcessor} from "../../../src/lib/output/ResultsProcessor"; +import {OutfileResultsProcessor} from "../../../src/lib/output/OutfileResultsProcessor"; +import * as os from "os"; +import * as path from "path"; +import {expect} from "chai"; +import {OutputFormat} from "../../../src/lib/output/OutputFormat"; +import fs = require('fs'); + +describe('OutfileResultsProcessor Tests', () => { + let tmpDir: string = null; + const results: Results = new FakeResults() + .withFormattedOutputForFormat(OutputFormat.CSV, "dummy csv contents") + .withFormattedOutputForFormat(OutputFormat.HTML, "dummy html contents") + .withFormattedOutputForFormat(OutputFormat.JSON, "dummy json contents") + .withFormattedOutputForFormat(OutputFormat.JUNIT, "dummy junit contents") + .withFormattedOutputForFormat(OutputFormat.SARIF, "dummy sarif contents") + .withFormattedOutputForFormat(OutputFormat.XML, "dummy xml contents"); + beforeEach(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'OutfileResultsProcessorTest-')); + }); + afterEach(() => { + fs.rmSync(tmpDir, {recursive: true, force: true}); + }); + + it('csv file', async () => { + const outfile: string = path.join(tmpDir, "out_file.csv"); + const resultsProcessor: ResultsProcessor = new OutfileResultsProcessor(OutputFormat.CSV, outfile, false); + await resultsProcessor.processResults(results); + expect(fs.readFileSync(outfile).toString()).to.equal('dummy csv contents'); + }); + + it('html file', async () => { + const outfile: string = path.join(tmpDir, "out_file.HTML"); + const resultsProcessor: ResultsProcessor = new OutfileResultsProcessor(OutputFormat.HTML, outfile, false); + await resultsProcessor.processResults(results); + expect(fs.readFileSync(outfile).toString()).to.equal('dummy html contents'); + }); + + it('json file', async () => { + const outfile: string = path.join(tmpDir, "out_file.json"); + const resultsProcessor: ResultsProcessor = new OutfileResultsProcessor(OutputFormat.JSON, outfile, false); + await resultsProcessor.processResults(results); + expect(fs.readFileSync(outfile).toString()).to.equal('dummy json contents'); + }); + + it('junit xml file', async () => { + const outfile: string = path.join(tmpDir, "out_file.xml"); + const resultsProcessor: ResultsProcessor = new OutfileResultsProcessor(OutputFormat.JUNIT, outfile, false); + await resultsProcessor.processResults(results); + expect(fs.readFileSync(outfile).toString()).to.equal('dummy junit contents'); + }); + + it('sarif file', async () => { + const outfile: string = path.join(tmpDir, "out_file.sarif"); + const resultsProcessor: ResultsProcessor = new OutfileResultsProcessor(OutputFormat.SARIF, outfile, false); + await resultsProcessor.processResults(results); + expect(fs.readFileSync(outfile).toString()).to.equal('dummy sarif contents'); + }); + + it('xml file', async () => { + const outfile: string = path.join(tmpDir, "out_file.xMl"); + const resultsProcessor: ResultsProcessor = new OutfileResultsProcessor(OutputFormat.XML, outfile, false); + await resultsProcessor.processResults(results); + expect(fs.readFileSync(outfile).toString()).to.equal('dummy xml contents'); + }); +}); From 7a4cce386b400c838fadc21cdb055df0369fe644 Mon Sep 17 00:00:00 2001 From: Stephen Carter Date: Wed, 3 Jan 2024 16:54:46 -0500 Subject: [PATCH 05/10] @W-14689342@: (Part 5) Fix internal validation message --- messages/run-common.md | 8 ++++++ src/lib/actions/AbstractRunAction.ts | 4 +-- src/lib/output/OutputFormat.ts | 19 ++++++++++---- test/commands/scanner/run.test.ts | 39 ++++++++++++++++++++++------ 4 files changed, 55 insertions(+), 15 deletions(-) diff --git a/messages/run-common.md b/messages/run-common.md index 3fb219b72..807d9221b 100644 --- a/messages/run-common.md +++ b/messages/run-common.md @@ -46,6 +46,14 @@ throw an error when a violation threshold is reached, the --normalize-severity i Throws an error when violations are found with equal or greater severity than the provided value. Values are 1 (high), 2 (moderate), and 3 (low). Exit code is the most severe violation. Using this flag also invokes the --normalize-severity flag. +# internal.outfileMustBeValid + +The %s environment variable must be a well-formed filepath. + +# internal.outfileMustBeSupportedType + +The %s environment variable must be of a supported type: .csv; .xml; .json; .html; .sarif. + # validations.cannotWriteTableToFile Format 'table' can't be written to a file. Specify a different format. diff --git a/src/lib/actions/AbstractRunAction.ts b/src/lib/actions/AbstractRunAction.ts index 30715db4a..7008a5540 100644 --- a/src/lib/actions/AbstractRunAction.ts +++ b/src/lib/actions/AbstractRunAction.ts @@ -15,7 +15,7 @@ import {InputProcessor} from "../InputProcessor"; import {EngineOptionsFactory} from "../EngineOptionsFactory"; import {ENV_VAR_NAMES, INTERNAL_ERROR_CODE} from "../../Constants"; import {Results} from "../output/Results"; -import {inferFormatFromOutfile, OutputFormat} from "../output/OutputFormat"; +import {inferFormatFromInternalOutfile, inferFormatFromOutfile, OutputFormat} from "../output/OutputFormat"; import {CompositeResultsProcessor, ResultsProcessor} from "../output/ResultsProcessor"; import {OutfileResultsProcessor} from "../output/OutfileResultsProcessor"; @@ -86,7 +86,7 @@ export abstract class AbstractRunAction implements Action { const resultsProcessors: ResultsProcessor[] = [runResultsProcessor]; const internalOutfile: string = process.env[ENV_VAR_NAMES.SCANNER_INTERNAL_OUTFILE]; if (internalOutfile && internalOutfile.length > 0) { - const internalOutputFormat: OutputFormat = inferFormatFromOutfile(internalOutfile); + const internalOutputFormat: OutputFormat = inferFormatFromInternalOutfile(internalOutfile); resultsProcessors.push(new OutfileResultsProcessor(internalOutputFormat, internalOutfile, verboseViolations)); } const compositeResultsProcessor: ResultsProcessor = new CompositeResultsProcessor(resultsProcessors); diff --git a/src/lib/output/OutputFormat.ts b/src/lib/output/OutputFormat.ts index 5c8a2402a..502fa6075 100644 --- a/src/lib/output/OutputFormat.ts +++ b/src/lib/output/OutputFormat.ts @@ -1,6 +1,6 @@ import {SfError} from "@salesforce/core"; import {BundleName, getMessage} from "../../MessageCatalog"; -import {INTERNAL_ERROR_CODE} from "../../Constants"; +import {ENV_VAR_NAMES, INTERNAL_ERROR_CODE} from "../../Constants"; export enum OutputFormat { CSV = 'csv', @@ -13,12 +13,21 @@ export enum OutputFormat { } export function inferFormatFromOutfile(outfile: string): OutputFormat { - // TODO: Since the this is being used now for both the --outfile and the internal outfile, we should move where - // validation takes place for --outfile and make these messages more generic. + return determineOutputFormat(outfile, + getMessage(BundleName.CommonRun, 'validations.outfileMustBeValid'), + getMessage(BundleName.CommonRun, 'validations.outfileMustBeSupportedType')) +} + +export function inferFormatFromInternalOutfile(outfile: string): OutputFormat { + return determineOutputFormat(outfile, + getMessage(BundleName.CommonRun, 'internal.outfileMustBeValid', [ENV_VAR_NAMES.SCANNER_INTERNAL_OUTFILE]), + getMessage(BundleName.CommonRun, 'internal.outfileMustBeSupportedType', [ENV_VAR_NAMES.SCANNER_INTERNAL_OUTFILE])); +} +function determineOutputFormat(outfile: string, invalidFileMsg: string, invalidExtensionMsg): OutputFormat { const lastPeriod: number = outfile.lastIndexOf('.'); if (lastPeriod < 1 || lastPeriod + 1 === outfile.length) { - throw new SfError(getMessage(BundleName.CommonRun, 'validations.outfileMustBeValid'), null, null, INTERNAL_ERROR_CODE); + throw new SfError(invalidFileMsg, null, null, INTERNAL_ERROR_CODE); } const fileExtension: string = outfile.slice(lastPeriod + 1).toLowerCase(); switch (fileExtension) { @@ -29,6 +38,6 @@ export function inferFormatFromOutfile(outfile: string): OutputFormat { case OutputFormat.XML: return fileExtension; default: - throw new SfError(getMessage(BundleName.CommonRun, 'validations.outfileMustBeSupportedType'), null, null, INTERNAL_ERROR_CODE); + throw new SfError(invalidExtensionMsg, null, null, INTERNAL_ERROR_CODE); } } diff --git a/test/commands/scanner/run.test.ts b/test/commands/scanner/run.test.ts index 84db09e62..fa080381d 100644 --- a/test/commands/scanner/run.test.ts +++ b/test/commands/scanner/run.test.ts @@ -1,13 +1,13 @@ import {expect} from 'chai'; // @ts-ignore import {runCommand} from '../../TestUtils'; +import {BundleName, getMessage} from "../../../src/MessageCatalog"; +import * as os from "os"; +import {ENV_VAR_NAMES} from "../../../src/Constants"; import fs = require('fs'); import path = require('path'); import process = require('process'); import tildify = require('tildify'); -import {BundleName, getMessage} from "../../../src/MessageCatalog"; -import * as os from "os"; -import {ENV_VAR_NAMES} from "../../../src/Constants"; const pathToApexFolder = path.join('test', 'code-fixtures', 'apex'); const pathToSomeTestClass = path.join('test', 'code-fixtures', 'apex', 'SomeTestClass.cls'); @@ -601,19 +601,19 @@ describe('scanner run', function () { }); }); - describe('We can create internal outfile with SCANNER_INTERNAL_OUTFILE environment variable', () => { + describe('Create internal outfile with SCANNER_INTERNAL_OUTFILE environment variable', () => { let tmpDir: string = null; - let internalOutfile: string = null; beforeEach(() => { tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'InternalOutfileTest-')); - internalOutfile = path.join(tmpDir, "internalOutfile.json"); - process.env[ENV_VAR_NAMES.SCANNER_INTERNAL_OUTFILE] = internalOutfile; }); afterEach(() => { fs.rmSync(tmpDir, {recursive: true, force: true}); delete process.env[ENV_VAR_NAMES.SCANNER_INTERNAL_OUTFILE]; }); + it('Can write a user file and an internal file with different formats at same time', () => { + const internalOutfile = path.join(tmpDir, "internalOutfile.json"); + process.env[ENV_VAR_NAMES.SCANNER_INTERNAL_OUTFILE] = internalOutfile; const userOutfile = path.join(tmpDir, "userOutfile.xml"); runCommand(`scanner run --target ${pathToSomeTestClass} --ruleset ApexUnit --outfile ${userOutfile}`); @@ -621,10 +621,33 @@ describe('scanner run', function () { const userFileContents = fs.readFileSync(userOutfile).toString(); validateXmlOutput(userFileContents); - expect(fs.existsSync(userOutfile)).to.equal(true, 'The command should have created the expected internal output file'); + expect(fs.existsSync(internalOutfile)).to.equal(true, 'The command should have created the expected internal output file'); + const internalFileContents = fs.readFileSync(internalOutfile).toString(); + validateJsonOutput(internalFileContents); + }); + + + it('Can write to internal file and write to console', () => { + const internalOutfile = path.join(tmpDir, "internalOutfile.json"); + process.env[ENV_VAR_NAMES.SCANNER_INTERNAL_OUTFILE] = internalOutfile; + const output = runCommand(`scanner run --target ${pathToSomeTestClass} --ruleset ApexUnit --format xml`); + + validateXmlOutput(output.shellOutput.stdout); + + expect(fs.existsSync(internalOutfile)).to.equal(true, 'The command should have created the expected internal output file'); const internalFileContents = fs.readFileSync(internalOutfile).toString(); validateJsonOutput(internalFileContents); }); + + it('Invalid internal file name gives appropriate error', () => { + const internalOutfile = path.join(tmpDir, "internalOutfile.notSupported"); + process.env[ENV_VAR_NAMES.SCANNER_INTERNAL_OUTFILE] = internalOutfile; + const userOutfile = path.join(tmpDir, "userOutfile.xml"); + const output = runCommand(`scanner run --target ${pathToSomeTestClass} --ruleset ApexUnit --outfile ${userOutfile}`); + + expect(output.shellOutput.stderr).contains( + getMessage(BundleName.CommonRun, 'internal.outfileMustBeSupportedType', [ENV_VAR_NAMES.SCANNER_INTERNAL_OUTFILE])); + }); }); }); From 845a8187894d8e5c5d968f2e4f3d5b5f6a998181 Mon Sep 17 00:00:00 2001 From: Stephen Carter Date: Wed, 3 Jan 2024 17:04:22 -0500 Subject: [PATCH 06/10] @W-14689342@: (Part 6) Add caching of results formatted output --- src/lib/output/OutputFormat.ts | 2 +- src/lib/output/Results.ts | 10 +++++++++- src/lib/output/RunResultsProcessor.ts | 4 ++-- 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/src/lib/output/OutputFormat.ts b/src/lib/output/OutputFormat.ts index 502fa6075..297c91869 100644 --- a/src/lib/output/OutputFormat.ts +++ b/src/lib/output/OutputFormat.ts @@ -24,7 +24,7 @@ export function inferFormatFromInternalOutfile(outfile: string): OutputFormat { getMessage(BundleName.CommonRun, 'internal.outfileMustBeSupportedType', [ENV_VAR_NAMES.SCANNER_INTERNAL_OUTFILE])); } -function determineOutputFormat(outfile: string, invalidFileMsg: string, invalidExtensionMsg): OutputFormat { +function determineOutputFormat(outfile: string, invalidFileMsg: string, invalidExtensionMsg: string): OutputFormat { const lastPeriod: number = outfile.lastIndexOf('.'); if (lastPeriod < 1 || lastPeriod + 1 === outfile.length) { throw new SfError(invalidFileMsg, null, null, INTERNAL_ERROR_CODE); diff --git a/src/lib/output/Results.ts b/src/lib/output/Results.ts index aa3f9157a..23448d907 100644 --- a/src/lib/output/Results.ts +++ b/src/lib/output/Results.ts @@ -33,6 +33,7 @@ export interface Results { export class RunResults implements Results { private readonly ruleResults: RuleResult[]; private readonly executedEngines: Set; + private readonly formattedResultsCache: Map = new Map; constructor(ruleResults: RuleResult[], executedEngines: Set, ) { this.ruleResults = ruleResults; @@ -100,6 +101,11 @@ export class RunResults implements Results { } public async toFormattedOutput(format: OutputFormat, verboseViolations: boolean): Promise { + const cacheKey: string = String(OutputFormat) + '_' + String(verboseViolations); + if (this.formattedResultsCache.has(cacheKey)) { + return Promise.resolve(this.formattedResultsCache.get(cacheKey)); + } + let outputFormatter: OutputFormatter; switch (format) { case OutputFormat.CSV: @@ -126,6 +132,8 @@ export class RunResults implements Results { default: throw new SfError('Unrecognized output format.'); } - return await outputFormatter.format(this); + const formattedOutput: FormattedOutput = await outputFormatter.format(this); + this.formattedResultsCache.set(cacheKey, formattedOutput); + return formattedOutput; } } diff --git a/src/lib/output/RunResultsProcessor.ts b/src/lib/output/RunResultsProcessor.ts index 9d853877f..47345be1a 100644 --- a/src/lib/output/RunResultsProcessor.ts +++ b/src/lib/output/RunResultsProcessor.ts @@ -4,8 +4,8 @@ import {FormattedOutput, EngineExecutionSummary} from '../../types'; import {BundleName, getMessage} from "../../MessageCatalog"; import {Display} from "../Display"; import {INTERNAL_ERROR_CODE} from "../../Constants"; -import {OutputFormat} from "../output/OutputFormat"; -import {Results} from "../output/Results"; +import {OutputFormat} from "./OutputFormat"; +import {Results} from "./Results"; import {ResultsProcessor} from "./ResultsProcessor"; import {writeToFile} from "./OutputUtils"; From 8c8e4bd5255d3987b192f9cdb4b7f834fb9848e8 Mon Sep 17 00:00:00 2001 From: Stephen Carter Date: Thu, 4 Jan 2024 08:53:39 -0500 Subject: [PATCH 07/10] @W-14689342@: (Part 7) Move json into a holder and verboseViolations into output options --- src/lib/InputProcessor.ts | 1 + src/lib/actions/AbstractRunAction.ts | 10 +-- src/lib/output/JsonReturnValueHolder.ts | 16 +++++ src/lib/output/RunResultsProcessor.ts | 24 ++++---- test/lib/output/RunOutputProcessor.test.ts | 71 ++++++++++++++-------- 5 files changed, 78 insertions(+), 44 deletions(-) create mode 100644 src/lib/output/JsonReturnValueHolder.ts diff --git a/src/lib/InputProcessor.ts b/src/lib/InputProcessor.ts index 1ae620e53..d7ed2f177 100644 --- a/src/lib/InputProcessor.ts +++ b/src/lib/InputProcessor.ts @@ -62,6 +62,7 @@ export class InputProcessorImpl implements InputProcessor { public createRunOutputOptions(inputs: Inputs): RunOutputOptions { return { format: outputFormatFromInputs(inputs), + verboseViolations: inputs["verbose-violations"] as boolean, severityForError: inputs['severity-threshold'] as number, outfile: inputs.outfile as string }; diff --git a/src/lib/actions/AbstractRunAction.ts b/src/lib/actions/AbstractRunAction.ts index 7008a5540..753a1f606 100644 --- a/src/lib/actions/AbstractRunAction.ts +++ b/src/lib/actions/AbstractRunAction.ts @@ -18,6 +18,7 @@ import {Results} from "../output/Results"; import {inferFormatFromInternalOutfile, inferFormatFromOutfile, OutputFormat} from "../output/OutputFormat"; import {CompositeResultsProcessor, ResultsProcessor} from "../output/ResultsProcessor"; import {OutfileResultsProcessor} from "../output/OutfileResultsProcessor"; +import {JsonReturnValueHolder} from "../output/JsonReturnValueHolder"; /** * Abstract Action to share a common implementation behind the "run" and "run dfa" commands @@ -78,16 +79,15 @@ export abstract class AbstractRunAction implements Action { const targetPaths: string[] = this.inputProcessor.resolveTargetPaths(inputs); const runOptions: RunOptions = this.inputProcessor.createRunOptions(inputs, this.isDfa()); const engineOptions: EngineOptions = this.engineOptionsFactory.createEngineOptions(inputs); - const outputOptions: RunOutputOptions = this.inputProcessor.createRunOutputOptions(inputs); - const verboseViolations: boolean = inputs["verbose-violations"] as boolean; + const jsonReturnValueHolder: JsonReturnValueHolder = new JsonReturnValueHolder(); - const runResultsProcessor: RunResultsProcessor = new RunResultsProcessor(this.display, outputOptions, verboseViolations); + const runResultsProcessor: RunResultsProcessor = new RunResultsProcessor(this.display, outputOptions, jsonReturnValueHolder); const resultsProcessors: ResultsProcessor[] = [runResultsProcessor]; const internalOutfile: string = process.env[ENV_VAR_NAMES.SCANNER_INTERNAL_OUTFILE]; if (internalOutfile && internalOutfile.length > 0) { const internalOutputFormat: OutputFormat = inferFormatFromInternalOutfile(internalOutfile); - resultsProcessors.push(new OutfileResultsProcessor(internalOutputFormat, internalOutfile, verboseViolations)); + resultsProcessors.push(new OutfileResultsProcessor(internalOutputFormat, internalOutfile, outputOptions.verboseViolations)); } const compositeResultsProcessor: ResultsProcessor = new CompositeResultsProcessor(resultsProcessors); @@ -98,7 +98,7 @@ export abstract class AbstractRunAction implements Action { const results: Results = await ruleManager.runRulesMatchingCriteria(filters, targetPaths, runOptions, engineOptions); this.logger.trace(`Processing output with format ${outputOptions.format}`); await compositeResultsProcessor.processResults(results); - return runResultsProcessor.getJsonReturnValue(); + return jsonReturnValueHolder.get(); } catch (e) { // Rethrow any errors as SF errors. diff --git a/src/lib/output/JsonReturnValueHolder.ts b/src/lib/output/JsonReturnValueHolder.ts new file mode 100644 index 000000000..a05d41bbf --- /dev/null +++ b/src/lib/output/JsonReturnValueHolder.ts @@ -0,0 +1,16 @@ +import {AnyJson} from "@salesforce/ts-types"; + +/** + * Container to hold the json return value for the --json flag used by some of the cli commands + */ +export class JsonReturnValueHolder { + private jsonReturnValue: AnyJson; + + public set(jsonReturnValue: AnyJson): void { + this.jsonReturnValue = jsonReturnValue; + } + + public get(): AnyJson { + return this.jsonReturnValue; + } +} diff --git a/src/lib/output/RunResultsProcessor.ts b/src/lib/output/RunResultsProcessor.ts index 47345be1a..742ba8243 100644 --- a/src/lib/output/RunResultsProcessor.ts +++ b/src/lib/output/RunResultsProcessor.ts @@ -8,9 +8,11 @@ import {OutputFormat} from "./OutputFormat"; import {Results} from "./Results"; import {ResultsProcessor} from "./ResultsProcessor"; import {writeToFile} from "./OutputUtils"; +import {JsonReturnValueHolder} from "./JsonReturnValueHolder"; export type RunOutputOptions = { format: OutputFormat; + verboseViolations: boolean; severityForError?: number; outfile?: string; } @@ -20,23 +22,18 @@ export type RunOutputOptions = { export class RunResultsProcessor implements ResultsProcessor { private readonly display: Display; private readonly opts: RunOutputOptions; - private readonly verboseViolations: boolean; - private jsonReturnValue: AnyJson = []; + private readonly jsonReturnValueHolder: JsonReturnValueHolder; - public constructor(display: Display, opts: RunOutputOptions, verboseViolations: boolean) { + public constructor(display: Display, opts: RunOutputOptions, jsonReturnValueHolder: JsonReturnValueHolder) { this.display = display; this.opts = opts; - this.verboseViolations = verboseViolations; - } - - public getJsonReturnValue(): AnyJson { - return this.jsonReturnValue; + this.jsonReturnValueHolder = jsonReturnValueHolder; } public async processResults(results: Results): Promise { const minSev: number = results.getMinSev(); const summaryMap: Map = results.getSummaryMap(); - const formattedOutput = await results.toFormattedOutput(this.opts.format, this.verboseViolations); + const formattedOutput = await results.toFormattedOutput(this.opts.format, this.opts.verboseViolations); const hasViolations = [...summaryMap.values()].some(summary => summary.violationCount !== 0); @@ -50,7 +47,7 @@ export class RunResultsProcessor implements ResultsProcessor { // ...log it to the console... this.display.displayInfo(msg); // ...and return it for use with the --json flag. - this.jsonReturnValue = msg; + this.jsonReturnValueHolder.set(msg); return; } @@ -77,15 +74,16 @@ export class RunResultsProcessor implements ResultsProcessor { // Finally, we need to return something for use by the --json flag. if (this.opts.outfile) { // If we used an outfile, we should just return the summary message, since that says where the file is. - this.jsonReturnValue = msg; + this.jsonReturnValueHolder.set(msg); } else if (typeof formattedOutput === 'string') { // If the specified output format was JSON, then the results are a huge stringified JSON that we should parse // before returning. Otherwise, we should just return the result string. - this.jsonReturnValue = this.opts.format === OutputFormat.JSON ? JSON.parse(formattedOutput) as AnyJson : formattedOutput; + this.jsonReturnValueHolder.set( + this.opts.format === OutputFormat.JSON ? JSON.parse(formattedOutput) as AnyJson : formattedOutput); } else { // If the results are a JSON, return the `rows` property, since that's all of the data that would be displayed // in the table. - this.jsonReturnValue = formattedOutput.rows; + this.jsonReturnValueHolder.set(formattedOutput.rows); } } diff --git a/test/lib/output/RunOutputProcessor.test.ts b/test/lib/output/RunOutputProcessor.test.ts index 26312d905..ab445895f 100644 --- a/test/lib/output/RunOutputProcessor.test.ts +++ b/test/lib/output/RunOutputProcessor.test.ts @@ -11,6 +11,7 @@ import {PATHLESS_COLUMNS} from "../../../src/lib/output/TableOutputFormatter"; import {OutputFormat} from "../../../src/lib/output/OutputFormat"; import {Results} from "../../../src/lib/output/Results"; import {FakeResults} from "./FakeResults"; +import {JsonReturnValueHolder} from "../../../src/lib/output/JsonReturnValueHolder"; const FAKE_SUMMARY_MAP: Map = new Map(); FAKE_SUMMARY_MAP.set('pmd', {fileCount: 1, violationCount: 1}); @@ -103,9 +104,11 @@ describe('RunOutputProcessor', () => { describe('Writing to console', () => { it('Empty results yield expected message', async () => { const opts: RunOutputOptions = { - format: OutputFormat.TABLE + format: OutputFormat.TABLE, + verboseViolations: false }; - const rrp = new RunResultsProcessor(display, opts, false); + const jsonHolder: JsonReturnValueHolder = new JsonReturnValueHolder(); + const rrp = new RunResultsProcessor(display, opts, jsonHolder); const summaryMap: Map = new Map(); summaryMap.set('pmd', {fileCount: 0, violationCount: 0}); summaryMap.set('eslint', {fileCount: 0, violationCount: 0}); @@ -114,7 +117,7 @@ describe('RunOutputProcessor', () => { // THIS IS THE PART BEING TESTED. await rrp.processResults(fakeResults); - const output: AnyJson = rrp.getJsonReturnValue(); + const output: AnyJson = jsonHolder.get(); // We expect that the message logged to the console and the message returned should both be the default const expectedMsg = getMessage(BundleName.RunOutputProcessor, 'output.noViolationsDetected', ['pmd, eslint']); @@ -128,13 +131,15 @@ describe('RunOutputProcessor', () => { it('Table-type output should be followed by summary', async () => { const opts: RunOutputOptions = { - format: OutputFormat.TABLE + format: OutputFormat.TABLE, + verboseViolations: false }; - const rrp = new RunResultsProcessor(display, opts, false); + const jsonHolder: JsonReturnValueHolder = new JsonReturnValueHolder(); + const rrp = new RunResultsProcessor(display, opts, jsonHolder); // THIS IS THE PART BEING TESTED. await rrp.processResults(fakeTableResults); - const output: AnyJson = rrp.getJsonReturnValue(); + const output: AnyJson = jsonHolder.get(); const expectedTableSummary = `${getMessage(BundleName.RunOutputProcessor, 'output.engineSummaryTemplate', ['pmd', 1, 1])} ${getMessage(BundleName.RunOutputProcessor, 'output.engineSummaryTemplate', ['eslint-typescript', 2, 1])} @@ -151,14 +156,16 @@ ${getMessage(BundleName.RunOutputProcessor, 'output.writtenToConsole')}`; it('Throws severity-based exception on request', async () => { const opts: RunOutputOptions = { format: OutputFormat.TABLE, + verboseViolations: false, severityForError: 1 }; - const rrp = new RunResultsProcessor(display, opts, false); + const jsonHolder: JsonReturnValueHolder = new JsonReturnValueHolder(); + const rrp = new RunResultsProcessor(display, opts, jsonHolder); // THIS IS THE PART BEING TESTED. try { await rrp.processResults(fakeTableResults); - const output: AnyJson = rrp.getJsonReturnValue(); + const output: AnyJson = jsonHolder.get(); expect(true).to.equal(false, `Unexpectedly returned ${output} instead of throwing error`); } catch (e) { expect(display.getOutputText()).to.satisfy(msg => msg.startsWith("[Table]")); @@ -182,14 +189,16 @@ ${getMessage(BundleName.RunOutputProcessor, 'output.writtenToConsole')}`; // need to change. it('CSV-type output should NOT be followed by summary', async () => { const opts: RunOutputOptions = { - format: OutputFormat.CSV + format: OutputFormat.CSV, + verboseViolations: false }; - const rrp = new RunResultsProcessor(display, opts, false); + const jsonHolder: JsonReturnValueHolder = new JsonReturnValueHolder(); + const rrp = new RunResultsProcessor(display, opts, jsonHolder); // THIS IS THE PART BEING TESTED. await rrp.processResults(fakeCsvResults); - const output: AnyJson = rrp.getJsonReturnValue(); + const output: AnyJson = jsonHolder.get(); expect(display.getOutputText()).to.equal("[Info]: " + FAKE_CSV_OUTPUT); expect(output).to.equal(FAKE_CSV_OUTPUT, 'CSV should be returned as a string'); }); @@ -197,15 +206,17 @@ ${getMessage(BundleName.RunOutputProcessor, 'output.writtenToConsole')}`; it('Throws severity-based exception on request', async () => { const opts: RunOutputOptions = { format: OutputFormat.CSV, + verboseViolations: false, severityForError: 2 }; - const rrp = new RunResultsProcessor(display, opts, false); + const jsonHolder: JsonReturnValueHolder = new JsonReturnValueHolder(); + const rrp = new RunResultsProcessor(display, opts, jsonHolder); // THIS IS THE PART BEING TESTED. try { await rrp.processResults(fakeCsvResults); - const output: AnyJson = rrp.getJsonReturnValue(); + const output: AnyJson = jsonHolder.get(); expect(true).to.equal(false, `Unexpectedly returned ${output} instead of throwing error`); } catch (e) { expect(display.getOutputText()).to.equal("[Info]: " + FAKE_CSV_OUTPUT); @@ -224,14 +235,16 @@ ${getMessage(BundleName.RunOutputProcessor, 'output.writtenToConsole')}`; // need to change. it('JSON-type output with no violations should output be an empty violation set', async () => { const opts: RunOutputOptions = { - format: OutputFormat.JSON + format: OutputFormat.JSON, + verboseViolations: false }; - const rrp = new RunResultsProcessor(display, opts, false); + const jsonHolder: JsonReturnValueHolder = new JsonReturnValueHolder(); + const rrp = new RunResultsProcessor(display, opts, jsonHolder); // THIS IS THE PART BEING TESTED await rrp.processResults(fakeJsonResults); - const output: AnyJson = rrp.getJsonReturnValue(); + const output: AnyJson = jsonHolder.get(); expect(display.getOutputText()).to.equal("[Info]: " + FAKE_JSON_OUTPUT); expect(output).to.deep.equal(JSON.parse(FAKE_JSON_OUTPUT), 'JSON should be returned as a parsed object'); @@ -240,15 +253,17 @@ ${getMessage(BundleName.RunOutputProcessor, 'output.writtenToConsole')}`; it('Throws severity-based exception on request', async () => { const opts: RunOutputOptions = { format: OutputFormat.JSON, + verboseViolations: false, severityForError: 1 }; - const rrp = new RunResultsProcessor(display, opts, false); + const jsonHolder: JsonReturnValueHolder = new JsonReturnValueHolder(); + const rrp = new RunResultsProcessor(display, opts, jsonHolder); // THIS IS THE PART BEING TESTED try { await rrp.processResults(fakeJsonResults); - const output: AnyJson = rrp.getJsonReturnValue(); + const output: AnyJson = jsonHolder.get(); expect(true).to.equal(false, `Unexpectedly returned ${output} instead of throwing error`); } catch (e) { expect(display.getOutputText()).to.equal("[Info]: " + FAKE_JSON_OUTPUT); @@ -263,9 +278,11 @@ ${getMessage(BundleName.RunOutputProcessor, 'output.writtenToConsole')}`; it('Empty results yield expected message', async () => { const opts: RunOutputOptions = { format: OutputFormat.CSV, + verboseViolations: false, outfile: fakeFilePath }; - const rrp = new RunResultsProcessor(display, opts, false); + const jsonHolder: JsonReturnValueHolder = new JsonReturnValueHolder(); + const rrp = new RunResultsProcessor(display, opts, jsonHolder); const summaryMap: Map = new Map(); summaryMap.set('pmd', {fileCount: 0, violationCount: 0}); summaryMap.set('eslint', {fileCount: 0, violationCount: 0}); @@ -274,7 +291,7 @@ ${getMessage(BundleName.RunOutputProcessor, 'output.writtenToConsole')}`; // THIS IS THE PART BEING TESTED. await rrp.processResults(fakeResults); - const output: AnyJson = rrp.getJsonReturnValue(); + const output: AnyJson = jsonHolder.get(); // We expect the empty CSV output followed by the default engine summary and written-to-file messages are logged to the console const expectedMsg = `${getMessage(BundleName.RunOutputProcessor, 'output.engineSummaryTemplate', ['pmd', 0, 0])} @@ -293,14 +310,15 @@ ${getMessage(BundleName.RunOutputProcessor, 'output.writtenToOutFile', [fakeFile it('Results are properly written to file', async () => { const opts: RunOutputOptions = { format: OutputFormat.CSV, + verboseViolations: false, outfile: fakeFilePath }; - - const rrp = new RunResultsProcessor(display, opts, false); + const jsonHolder: JsonReturnValueHolder = new JsonReturnValueHolder(); + const rrp = new RunResultsProcessor(display, opts, jsonHolder); // THIS IS THE PART BEING TESTED. await rrp.processResults(fakeCsvResults); - const output: AnyJson = rrp.getJsonReturnValue(); + const output: AnyJson = jsonHolder.get(); const expectedCsvSummary = `${getMessage(BundleName.RunOutputProcessor, 'output.engineSummaryTemplate', ['pmd', 1, 1])} ${getMessage(BundleName.RunOutputProcessor, 'output.engineSummaryTemplate', ['eslint-typescript', 2, 1])} @@ -314,16 +332,17 @@ ${getMessage(BundleName.RunOutputProcessor, 'output.writtenToOutFile', [fakeFile it('Throws severity-based exception on request', async () => { const opts: RunOutputOptions = { format: OutputFormat.CSV, + verboseViolations: false, severityForError: 1, outfile: fakeFilePath }; - - const rrp = new RunResultsProcessor(display, opts, false); + const jsonHolder: JsonReturnValueHolder = new JsonReturnValueHolder(); + const rrp = new RunResultsProcessor(display, opts, jsonHolder); // THIS IS THE PART BEING TESTED. try { await rrp.processResults(fakeCsvResults); - const output: AnyJson = rrp.getJsonReturnValue(); + const output: AnyJson = jsonHolder.get(); expect(true).to.equal(false, `Unexpectedly returned ${output} instead of throwing error`); } catch (e) { expect(display.getOutputText()).to.equal(""); From 5d920d1425869992a896b9460c3934fe0bb39973 Mon Sep 17 00:00:00 2001 From: Stephen Carter Date: Thu, 4 Jan 2024 09:10:04 -0500 Subject: [PATCH 08/10] @W-14689342@: (Part 8) Introduce ResultsProcessorFactory and inject it as dependency to run actions --- src/commands/scanner/run.ts | 5 +++- src/commands/scanner/run/dfa.ts | 5 +++- src/lib/actions/AbstractRunAction.ts | 29 ++++++++---------- src/lib/actions/RunAction.ts | 6 ++-- src/lib/actions/RunDfaAction.ts | 6 ++-- src/lib/output/Results.ts | 2 +- src/lib/output/ResultsProcessorFactory.ts | 28 +++++++++++++++++ ...or.test.ts => RunResultsProcessor.test.ts} | 30 ++++++++++++------- 8 files changed, 77 insertions(+), 34 deletions(-) create mode 100644 src/lib/output/ResultsProcessorFactory.ts rename test/lib/output/{RunOutputProcessor.test.ts => RunResultsProcessor.test.ts} (92%) diff --git a/src/commands/scanner/run.ts b/src/commands/scanner/run.ts index 535f5d83e..967335f1c 100644 --- a/src/commands/scanner/run.ts +++ b/src/commands/scanner/run.ts @@ -9,6 +9,7 @@ import {Action} from "../../lib/ScannerCommand"; import {Display} from "../../lib/Display"; import {RunAction} from "../../lib/actions/RunAction"; import {RuleFilterFactory, RuleFilterFactoryImpl} from "../../lib/RuleFilterFactory"; +import {ResultsProcessorFactory, ResultsProcessorFactoryImpl} from "../../lib/output/ResultsProcessorFactory"; /** * Defines the "run" command for the "scanner" cli. @@ -91,6 +92,8 @@ export default class Run extends ScannerRunCommand { const inputProcessor: InputProcessor = new InputProcessorImpl(this.config.version); const ruleFilterFactory: RuleFilterFactory = new RuleFilterFactoryImpl(); const engineOptionsFactory: EngineOptionsFactory = new RunEngineOptionsFactory(inputProcessor); - return new RunAction(logger, display, inputProcessor, ruleFilterFactory, engineOptionsFactory); + const resultsProcessorFactory: ResultsProcessorFactory = new ResultsProcessorFactoryImpl(); + return new RunAction(logger, display, inputProcessor, ruleFilterFactory, engineOptionsFactory, + resultsProcessorFactory); } } diff --git a/src/commands/scanner/run/dfa.ts b/src/commands/scanner/run/dfa.ts index 80db54145..63a4572cc 100644 --- a/src/commands/scanner/run/dfa.ts +++ b/src/commands/scanner/run/dfa.ts @@ -8,6 +8,7 @@ import {Display} from "../../../lib/Display"; import {Action} from "../../../lib/ScannerCommand"; import {RunDfaAction} from "../../../lib/actions/RunDfaAction"; import {RuleFilterFactory, RuleFilterFactoryImpl} from "../../../lib/RuleFilterFactory"; +import {ResultsProcessorFactory, ResultsProcessorFactoryImpl} from "../../../lib/output/ResultsProcessorFactory"; /** * Defines the "run dfa" command for the "scanner" cli. @@ -79,6 +80,8 @@ export default class Dfa extends ScannerRunCommand { const inputProcessor: InputProcessor = new InputProcessorImpl(this.config.version); const ruleFilterFactory: RuleFilterFactory = new RuleFilterFactoryImpl(); const engineOptionsFactory: EngineOptionsFactory = new RunDfaEngineOptionsFactory(inputProcessor); - return new RunDfaAction(logger, display, inputProcessor, ruleFilterFactory, engineOptionsFactory); + const resultsProcessorFactory: ResultsProcessorFactory = new ResultsProcessorFactoryImpl(); + return new RunDfaAction(logger, display, inputProcessor, ruleFilterFactory, engineOptionsFactory, + resultsProcessorFactory); } } diff --git a/src/lib/actions/AbstractRunAction.ts b/src/lib/actions/AbstractRunAction.ts index 753a1f606..eff771a29 100644 --- a/src/lib/actions/AbstractRunAction.ts +++ b/src/lib/actions/AbstractRunAction.ts @@ -10,14 +10,14 @@ import {Display} from "../Display"; import {RuleFilter} from "../RuleFilter"; import {RuleFilterFactory} from "../RuleFilterFactory"; import {Controller} from "../../Controller"; -import {RunOutputOptions, RunResultsProcessor} from "../output/RunResultsProcessor"; +import {RunOutputOptions} from "../output/RunResultsProcessor"; import {InputProcessor} from "../InputProcessor"; import {EngineOptionsFactory} from "../EngineOptionsFactory"; -import {ENV_VAR_NAMES, INTERNAL_ERROR_CODE} from "../../Constants"; +import {INTERNAL_ERROR_CODE} from "../../Constants"; import {Results} from "../output/Results"; -import {inferFormatFromInternalOutfile, inferFormatFromOutfile, OutputFormat} from "../output/OutputFormat"; -import {CompositeResultsProcessor, ResultsProcessor} from "../output/ResultsProcessor"; -import {OutfileResultsProcessor} from "../output/OutfileResultsProcessor"; +import {inferFormatFromOutfile, OutputFormat} from "../output/OutputFormat"; +import {ResultsProcessor} from "../output/ResultsProcessor"; +import {ResultsProcessorFactory} from "../output/ResultsProcessorFactory"; import {JsonReturnValueHolder} from "../output/JsonReturnValueHolder"; /** @@ -29,14 +29,17 @@ export abstract class AbstractRunAction implements Action { private readonly inputProcessor: InputProcessor; private readonly ruleFilterFactory: RuleFilterFactory; private readonly engineOptionsFactory: EngineOptionsFactory; + private readonly resultsProcessorFactory: ResultsProcessorFactory; protected constructor(logger: Logger, display: Display, inputProcessor: InputProcessor, - ruleFilterFactory: RuleFilterFactory, engineOptionsFactory: EngineOptionsFactory) { + ruleFilterFactory: RuleFilterFactory, engineOptionsFactory: EngineOptionsFactory, + resultsProcessorFactory: ResultsProcessorFactory) { this.logger = logger; this.display = display; this.inputProcessor = inputProcessor; this.ruleFilterFactory = ruleFilterFactory; this.engineOptionsFactory = engineOptionsFactory; + this.resultsProcessorFactory = resultsProcessorFactory; } protected abstract isDfa(): boolean; @@ -80,16 +83,10 @@ export abstract class AbstractRunAction implements Action { const runOptions: RunOptions = this.inputProcessor.createRunOptions(inputs, this.isDfa()); const engineOptions: EngineOptions = this.engineOptionsFactory.createEngineOptions(inputs); const outputOptions: RunOutputOptions = this.inputProcessor.createRunOutputOptions(inputs); - const jsonReturnValueHolder: JsonReturnValueHolder = new JsonReturnValueHolder(); - const runResultsProcessor: RunResultsProcessor = new RunResultsProcessor(this.display, outputOptions, jsonReturnValueHolder); - const resultsProcessors: ResultsProcessor[] = [runResultsProcessor]; - const internalOutfile: string = process.env[ENV_VAR_NAMES.SCANNER_INTERNAL_OUTFILE]; - if (internalOutfile && internalOutfile.length > 0) { - const internalOutputFormat: OutputFormat = inferFormatFromInternalOutfile(internalOutfile); - resultsProcessors.push(new OutfileResultsProcessor(internalOutputFormat, internalOutfile, outputOptions.verboseViolations)); - } - const compositeResultsProcessor: ResultsProcessor = new CompositeResultsProcessor(resultsProcessors); + const jsonReturnValueHolder: JsonReturnValueHolder = new JsonReturnValueHolder(); + const resultsProcessor: ResultsProcessor = this.resultsProcessorFactory.createResultsProcessor( + this.display, outputOptions, jsonReturnValueHolder); // TODO: Inject RuleManager as a dependency to improve testability by removing coupling to runtime implementation const ruleManager: RuleManager = await Controller.createRuleManager(); @@ -97,7 +94,7 @@ export abstract class AbstractRunAction implements Action { try { const results: Results = await ruleManager.runRulesMatchingCriteria(filters, targetPaths, runOptions, engineOptions); this.logger.trace(`Processing output with format ${outputOptions.format}`); - await compositeResultsProcessor.processResults(results); + await resultsProcessor.processResults(results); return jsonReturnValueHolder.get(); } catch (e) { diff --git a/src/lib/actions/RunAction.ts b/src/lib/actions/RunAction.ts index 82e2a9de1..e1c2cd566 100644 --- a/src/lib/actions/RunAction.ts +++ b/src/lib/actions/RunAction.ts @@ -6,14 +6,16 @@ import {Inputs} from "../../types"; import {Logger, SfError} from "@salesforce/core"; import {BundleName, getMessage} from "../../MessageCatalog"; import {RuleFilterFactory} from "../RuleFilterFactory"; +import {ResultsProcessorFactory} from "../output/ResultsProcessorFactory"; /** * The Action behind the "run" command */ export class RunAction extends AbstractRunAction { public constructor(logger: Logger, display: Display, inputProcessor: InputProcessor, - ruleFilterFactory: RuleFilterFactory, engineOptionsFactory: EngineOptionsFactory) { - super(logger, display, inputProcessor, ruleFilterFactory, engineOptionsFactory); + ruleFilterFactory: RuleFilterFactory, engineOptionsFactory: EngineOptionsFactory, + resultsProcessorFactory: ResultsProcessorFactory) { + super(logger, display, inputProcessor, ruleFilterFactory, engineOptionsFactory, resultsProcessorFactory); } public override async validateInputs(inputs: Inputs): Promise { diff --git a/src/lib/actions/RunDfaAction.ts b/src/lib/actions/RunDfaAction.ts index da9679572..af216fa63 100644 --- a/src/lib/actions/RunDfaAction.ts +++ b/src/lib/actions/RunDfaAction.ts @@ -8,14 +8,16 @@ import {FileHandler} from "../util/FileHandler"; import {Logger, SfError} from "@salesforce/core"; import {BundleName, getMessage} from "../../MessageCatalog"; import * as globby from "globby"; +import {ResultsProcessorFactory} from "../output/ResultsProcessorFactory"; /** * The Action behind the "run dfa" command */ export class RunDfaAction extends AbstractRunAction { public constructor(logger: Logger, display: Display, inputProcessor: InputProcessor, - ruleFilterFactory: RuleFilterFactory, engineOptionsFactory: EngineOptionsFactory) { - super(logger, display, inputProcessor, ruleFilterFactory, engineOptionsFactory); + ruleFilterFactory: RuleFilterFactory, engineOptionsFactory: EngineOptionsFactory, + resultsProcessorFactory: ResultsProcessorFactory) { + super(logger, display, inputProcessor, ruleFilterFactory, engineOptionsFactory, resultsProcessorFactory); } public override async validateInputs(inputs: Inputs): Promise { diff --git a/src/lib/output/Results.ts b/src/lib/output/Results.ts index 23448d907..3200bc944 100644 --- a/src/lib/output/Results.ts +++ b/src/lib/output/Results.ts @@ -101,7 +101,7 @@ export class RunResults implements Results { } public async toFormattedOutput(format: OutputFormat, verboseViolations: boolean): Promise { - const cacheKey: string = String(OutputFormat) + '_' + String(verboseViolations); + const cacheKey: string = String(format) + '_' + String(verboseViolations); if (this.formattedResultsCache.has(cacheKey)) { return Promise.resolve(this.formattedResultsCache.get(cacheKey)); } diff --git a/src/lib/output/ResultsProcessorFactory.ts b/src/lib/output/ResultsProcessorFactory.ts new file mode 100644 index 000000000..8ddcc916c --- /dev/null +++ b/src/lib/output/ResultsProcessorFactory.ts @@ -0,0 +1,28 @@ +import {Display} from "../Display"; +import {JsonReturnValueHolder} from "./JsonReturnValueHolder"; +import {RunOutputOptions, RunResultsProcessor} from "./RunResultsProcessor"; +import {CompositeResultsProcessor, ResultsProcessor} from "./ResultsProcessor"; +import {ENV_VAR_NAMES} from "../../Constants"; +import {inferFormatFromInternalOutfile, OutputFormat} from "./OutputFormat"; +import {OutfileResultsProcessor} from "./OutfileResultsProcessor"; + +export interface ResultsProcessorFactory { + createResultsProcessor(display: Display, runOutputOptions: RunOutputOptions, + jsonReturnValueHolder: JsonReturnValueHolder): ResultsProcessor +} + +export class ResultsProcessorFactoryImpl implements ResultsProcessorFactory { + public createResultsProcessor(display: Display, runOutputOptions: RunOutputOptions, + jsonReturnValueHolder: JsonReturnValueHolder): ResultsProcessor { + + const resultsProcessors: ResultsProcessor[] = [new RunResultsProcessor(display, runOutputOptions, jsonReturnValueHolder)]; + + const internalOutfile: string = process.env[ENV_VAR_NAMES.SCANNER_INTERNAL_OUTFILE]; + if (internalOutfile && internalOutfile.length > 0) { + const internalOutputFormat: OutputFormat = inferFormatFromInternalOutfile(internalOutfile); + resultsProcessors.push(new OutfileResultsProcessor(internalOutputFormat, internalOutfile, runOutputOptions.verboseViolations)); + } + + return new CompositeResultsProcessor(resultsProcessors); + } +} diff --git a/test/lib/output/RunOutputProcessor.test.ts b/test/lib/output/RunResultsProcessor.test.ts similarity index 92% rename from test/lib/output/RunOutputProcessor.test.ts rename to test/lib/output/RunResultsProcessor.test.ts index ab445895f..f16da1bf9 100644 --- a/test/lib/output/RunOutputProcessor.test.ts +++ b/test/lib/output/RunResultsProcessor.test.ts @@ -1,6 +1,6 @@ import {expect} from 'chai'; -import {RunOutputOptions, RunResultsProcessor} from '../../../src/lib/output/RunResultsProcessor'; +import {RunOutputOptions} from '../../../src/lib/output/RunResultsProcessor'; import {EngineExecutionSummary} from '../../../src/types'; import {AnyJson} from '@salesforce/ts-types'; import Sinon = require('sinon'); @@ -12,6 +12,9 @@ import {OutputFormat} from "../../../src/lib/output/OutputFormat"; import {Results} from "../../../src/lib/output/Results"; import {FakeResults} from "./FakeResults"; import {JsonReturnValueHolder} from "../../../src/lib/output/JsonReturnValueHolder"; +import {ResultsProcessor} from "../../../src/lib/output/ResultsProcessor"; +import {ResultsProcessorFactoryImpl} from "../../../src/lib/output/ResultsProcessorFactory"; +import {Display} from "../../../src/lib/Display"; const FAKE_SUMMARY_MAP: Map = new Map(); FAKE_SUMMARY_MAP.set('pmd', {fileCount: 1, violationCount: 1}); @@ -108,7 +111,7 @@ describe('RunOutputProcessor', () => { verboseViolations: false }; const jsonHolder: JsonReturnValueHolder = new JsonReturnValueHolder(); - const rrp = new RunResultsProcessor(display, opts, jsonHolder); + const rrp = createResultsProcessor(display, opts, jsonHolder); const summaryMap: Map = new Map(); summaryMap.set('pmd', {fileCount: 0, violationCount: 0}); summaryMap.set('eslint', {fileCount: 0, violationCount: 0}); @@ -135,7 +138,7 @@ describe('RunOutputProcessor', () => { verboseViolations: false }; const jsonHolder: JsonReturnValueHolder = new JsonReturnValueHolder(); - const rrp = new RunResultsProcessor(display, opts, jsonHolder); + const rrp = createResultsProcessor(display, opts, jsonHolder); // THIS IS THE PART BEING TESTED. await rrp.processResults(fakeTableResults); @@ -160,7 +163,7 @@ ${getMessage(BundleName.RunOutputProcessor, 'output.writtenToConsole')}`; severityForError: 1 }; const jsonHolder: JsonReturnValueHolder = new JsonReturnValueHolder(); - const rrp = new RunResultsProcessor(display, opts, jsonHolder); + const rrp = createResultsProcessor(display, opts, jsonHolder); // THIS IS THE PART BEING TESTED. try { @@ -194,7 +197,7 @@ ${getMessage(BundleName.RunOutputProcessor, 'output.writtenToConsole')}`; }; const jsonHolder: JsonReturnValueHolder = new JsonReturnValueHolder(); - const rrp = new RunResultsProcessor(display, opts, jsonHolder); + const rrp = createResultsProcessor(display, opts, jsonHolder); // THIS IS THE PART BEING TESTED. await rrp.processResults(fakeCsvResults); @@ -211,7 +214,7 @@ ${getMessage(BundleName.RunOutputProcessor, 'output.writtenToConsole')}`; }; const jsonHolder: JsonReturnValueHolder = new JsonReturnValueHolder(); - const rrp = new RunResultsProcessor(display, opts, jsonHolder); + const rrp = createResultsProcessor(display, opts, jsonHolder); // THIS IS THE PART BEING TESTED. try { @@ -240,7 +243,7 @@ ${getMessage(BundleName.RunOutputProcessor, 'output.writtenToConsole')}`; }; const jsonHolder: JsonReturnValueHolder = new JsonReturnValueHolder(); - const rrp = new RunResultsProcessor(display, opts, jsonHolder); + const rrp = createResultsProcessor(display, opts, jsonHolder); // THIS IS THE PART BEING TESTED await rrp.processResults(fakeJsonResults); @@ -258,7 +261,7 @@ ${getMessage(BundleName.RunOutputProcessor, 'output.writtenToConsole')}`; }; const jsonHolder: JsonReturnValueHolder = new JsonReturnValueHolder(); - const rrp = new RunResultsProcessor(display, opts, jsonHolder); + const rrp = createResultsProcessor(display, opts, jsonHolder); // THIS IS THE PART BEING TESTED try { @@ -282,7 +285,7 @@ ${getMessage(BundleName.RunOutputProcessor, 'output.writtenToConsole')}`; outfile: fakeFilePath }; const jsonHolder: JsonReturnValueHolder = new JsonReturnValueHolder(); - const rrp = new RunResultsProcessor(display, opts, jsonHolder); + const rrp = createResultsProcessor(display, opts, jsonHolder); const summaryMap: Map = new Map(); summaryMap.set('pmd', {fileCount: 0, violationCount: 0}); summaryMap.set('eslint', {fileCount: 0, violationCount: 0}); @@ -314,7 +317,7 @@ ${getMessage(BundleName.RunOutputProcessor, 'output.writtenToOutFile', [fakeFile outfile: fakeFilePath }; const jsonHolder: JsonReturnValueHolder = new JsonReturnValueHolder(); - const rrp = new RunResultsProcessor(display, opts, jsonHolder); + const rrp = createResultsProcessor(display, opts, jsonHolder); // THIS IS THE PART BEING TESTED. await rrp.processResults(fakeCsvResults); @@ -337,7 +340,7 @@ ${getMessage(BundleName.RunOutputProcessor, 'output.writtenToOutFile', [fakeFile outfile: fakeFilePath }; const jsonHolder: JsonReturnValueHolder = new JsonReturnValueHolder(); - const rrp = new RunResultsProcessor(display, opts, jsonHolder); + const rrp = createResultsProcessor(display, opts, jsonHolder); // THIS IS THE PART BEING TESTED. try { @@ -359,3 +362,8 @@ ${getMessage(BundleName.RunOutputProcessor, 'output.writtenToOutFile', [fakeFile }); }); }); + +function createResultsProcessor(display: Display, runOutputOptions: RunOutputOptions, + jsonReturnValueHolder: JsonReturnValueHolder): ResultsProcessor { + return (new ResultsProcessorFactoryImpl()).createResultsProcessor(display, runOutputOptions, jsonReturnValueHolder); +} From 1bd1c02be3e760618845e30d74b8860a63d00384 Mon Sep 17 00:00:00 2001 From: Stephen Carter Date: Thu, 4 Jan 2024 09:58:14 -0500 Subject: [PATCH 09/10] @W-14689342@: (Part 9) Final polish --- src/lib/output/OutfileResultsProcessor.ts | 3 +++ src/lib/output/ResultsProcessor.ts | 6 ++++++ src/lib/output/ResultsProcessorFactory.ts | 15 +++++++++++---- src/lib/output/RunResultsProcessor.ts | 14 +++++++++++--- 4 files changed, 31 insertions(+), 7 deletions(-) diff --git a/src/lib/output/OutfileResultsProcessor.ts b/src/lib/output/OutfileResultsProcessor.ts index a84ac933e..515b4f889 100644 --- a/src/lib/output/OutfileResultsProcessor.ts +++ b/src/lib/output/OutfileResultsProcessor.ts @@ -4,6 +4,9 @@ import {OutputFormat} from "./OutputFormat"; import {writeToFile} from "./OutputUtils"; import {FormattedOutput} from "../../types"; +/** + * Processes results to produce an output file + */ export class OutfileResultsProcessor implements ResultsProcessor { private readonly outputFormat: OutputFormat; private readonly outfile: string; diff --git a/src/lib/output/ResultsProcessor.ts b/src/lib/output/ResultsProcessor.ts index 7013eac97..8ecdfb2e8 100644 --- a/src/lib/output/ResultsProcessor.ts +++ b/src/lib/output/ResultsProcessor.ts @@ -1,9 +1,15 @@ import {Results} from "./Results"; +/** + * Interface to process run results + */ export interface ResultsProcessor { processResults(results: Results): Promise; } +/** + * A composite results processor + */ export class CompositeResultsProcessor implements ResultsProcessor { private readonly delegates: ResultsProcessor[]; diff --git a/src/lib/output/ResultsProcessorFactory.ts b/src/lib/output/ResultsProcessorFactory.ts index 8ddcc916c..40c72fe1d 100644 --- a/src/lib/output/ResultsProcessorFactory.ts +++ b/src/lib/output/ResultsProcessorFactory.ts @@ -6,23 +6,30 @@ import {ENV_VAR_NAMES} from "../../Constants"; import {inferFormatFromInternalOutfile, OutputFormat} from "./OutputFormat"; import {OutfileResultsProcessor} from "./OutfileResultsProcessor"; +/** + * Interface for creating a ResultsProcessor + */ export interface ResultsProcessorFactory { createResultsProcessor(display: Display, runOutputOptions: RunOutputOptions, jsonReturnValueHolder: JsonReturnValueHolder): ResultsProcessor } +/** + * Runtime implementation of the ResultsProcessorFactory interface + */ export class ResultsProcessorFactoryImpl implements ResultsProcessorFactory { public createResultsProcessor(display: Display, runOutputOptions: RunOutputOptions, jsonReturnValueHolder: JsonReturnValueHolder): ResultsProcessor { - const resultsProcessors: ResultsProcessor[] = [new RunResultsProcessor(display, runOutputOptions, jsonReturnValueHolder)]; + this.addProcessorForInternalOutfileIfNeeded(resultsProcessors, runOutputOptions.verboseViolations); + return new CompositeResultsProcessor(resultsProcessors); + } + private addProcessorForInternalOutfileIfNeeded(resultsProcessors: ResultsProcessor[], verboseViolations: boolean): void { const internalOutfile: string = process.env[ENV_VAR_NAMES.SCANNER_INTERNAL_OUTFILE]; if (internalOutfile && internalOutfile.length > 0) { const internalOutputFormat: OutputFormat = inferFormatFromInternalOutfile(internalOutfile); - resultsProcessors.push(new OutfileResultsProcessor(internalOutputFormat, internalOutfile, runOutputOptions.verboseViolations)); + resultsProcessors.push(new OutfileResultsProcessor(internalOutputFormat, internalOutfile, verboseViolations)); } - - return new CompositeResultsProcessor(resultsProcessors); } } diff --git a/src/lib/output/RunResultsProcessor.ts b/src/lib/output/RunResultsProcessor.ts index 742ba8243..a938e86e8 100644 --- a/src/lib/output/RunResultsProcessor.ts +++ b/src/lib/output/RunResultsProcessor.ts @@ -17,12 +17,20 @@ export type RunOutputOptions = { outfile?: string; } -// TODO: We should consider separating this into multiple ResultProcessor classes: -// --> 1 for processing the json return value, 1 for creating the users outfile, and 1 for creating console output +/** + * The primary ResultsProcessor used in production + * + * Note: We should consider separating this into multiple ResultProcessor classes to separate the responsibilities: + * --> creating the json return value + * --> creating the users outfile (where we can reuse the OutfileResultsProcessor) + * --> creating console output + * --> checking and throwing an exception if over severity threshold + * Right now all of these are entangled with one another below and a design change would be needed to do this. + */ export class RunResultsProcessor implements ResultsProcessor { private readonly display: Display; private readonly opts: RunOutputOptions; - private readonly jsonReturnValueHolder: JsonReturnValueHolder; + private jsonReturnValueHolder: JsonReturnValueHolder; public constructor(display: Display, opts: RunOutputOptions, jsonReturnValueHolder: JsonReturnValueHolder) { this.display = display; From 12502fdd22472642faadd12f319ed3c6d340a26c Mon Sep 17 00:00:00 2001 From: Stephen Carter Date: Thu, 4 Jan 2024 13:52:38 -0500 Subject: [PATCH 10/10] @W-14689342@: (Part 10) implement review feedback --- src/lib/output/OutfileResultsProcessor.ts | 4 +-- src/lib/output/OutputUtils.ts | 13 ---------- src/lib/output/RunResultsProcessor.ts | 4 +-- src/lib/util/FileHandler.ts | 31 ++++++++++++++++------- 4 files changed, 26 insertions(+), 26 deletions(-) delete mode 100644 src/lib/output/OutputUtils.ts diff --git a/src/lib/output/OutfileResultsProcessor.ts b/src/lib/output/OutfileResultsProcessor.ts index 515b4f889..0da855785 100644 --- a/src/lib/output/OutfileResultsProcessor.ts +++ b/src/lib/output/OutfileResultsProcessor.ts @@ -1,8 +1,8 @@ import {ResultsProcessor} from "./ResultsProcessor"; import {Results} from "./Results"; import {OutputFormat} from "./OutputFormat"; -import {writeToFile} from "./OutputUtils"; import {FormattedOutput} from "../../types"; +import {FileHandler} from "../util/FileHandler"; /** * Processes results to produce an output file @@ -19,6 +19,6 @@ export class OutfileResultsProcessor implements ResultsProcessor { public async processResults(results: Results): Promise { const fileContents: FormattedOutput = await results.toFormattedOutput(this.outputFormat, this.verboseViolations); - writeToFile(this.outfile, fileContents as string); + (new FileHandler()).writeFileSync(this.outfile, fileContents as string); } } diff --git a/src/lib/output/OutputUtils.ts b/src/lib/output/OutputUtils.ts deleted file mode 100644 index 907f42bad..000000000 --- a/src/lib/output/OutputUtils.ts +++ /dev/null @@ -1,13 +0,0 @@ -import {SfError} from "@salesforce/core"; -import {INTERNAL_ERROR_CODE} from "../../Constants"; -import fs = require('fs'); - -export function writeToFile(file: string, fileContents: string): void { - try { - fs.writeFileSync(file, fileContents); - } catch (e) { - // Rethrow any errors as SfError. - const message: string = e instanceof Error ? e.message : e as string; - throw new SfError(message, null, null, INTERNAL_ERROR_CODE); - } -} diff --git a/src/lib/output/RunResultsProcessor.ts b/src/lib/output/RunResultsProcessor.ts index a938e86e8..58267b02c 100644 --- a/src/lib/output/RunResultsProcessor.ts +++ b/src/lib/output/RunResultsProcessor.ts @@ -7,8 +7,8 @@ import {INTERNAL_ERROR_CODE} from "../../Constants"; import {OutputFormat} from "./OutputFormat"; import {Results} from "./Results"; import {ResultsProcessor} from "./ResultsProcessor"; -import {writeToFile} from "./OutputUtils"; import {JsonReturnValueHolder} from "./JsonReturnValueHolder"; +import {FileHandler} from "../util/FileHandler"; export type RunOutputOptions = { format: OutputFormat; @@ -134,7 +134,7 @@ export class RunResultsProcessor implements ResultsProcessor { private writeToOutfile(results: string | {columns; rows}): string { // At this point, we can cast `results` to a string, since it being an object would indicate that the format // is `table`, and we already have validations preventing tables from being written to files. - writeToFile(this.opts.outfile, results as string); + (new FileHandler()).writeFileSync(this.opts.outfile, results as string); // Return a message indicating the action we took. return getMessage(BundleName.RunOutputProcessor, 'output.writtenToOutFile', [this.opts.outfile]); diff --git a/src/lib/util/FileHandler.ts b/src/lib/util/FileHandler.ts index e0adaa8b7..498e7cfb3 100644 --- a/src/lib/util/FileHandler.ts +++ b/src/lib/util/FileHandler.ts @@ -1,5 +1,8 @@ -import {Stats, promises as fs, constants as fsConstants} from 'fs'; +import {Stats, promises as fsp, constants as fsConstants} from 'fs'; +import fs = require('fs'); import tmp = require('tmp'); +import {SfError} from "@salesforce/core"; +import {INTERNAL_ERROR_CODE} from "../../Constants"; type DuplicationFn = (src: string, target: string) => Promise; @@ -10,7 +13,7 @@ type DuplicationFn = (src: string, target: string) => Promise; export class FileHandler { async exists(filename: string): Promise { try { - await fs.access(filename, fsConstants.F_OK); + await fsp.access(filename, fsConstants.F_OK); return true; } catch (e) { return false; @@ -18,7 +21,7 @@ export class FileHandler { } stats(filename: string): Promise { - return fs.stat(filename); + return fsp.stat(filename); } async isDir(filename: string): Promise { @@ -30,24 +33,34 @@ export class FileHandler { } readDir(filename: string): Promise { - return fs.readdir(filename); + return fsp.readdir(filename); } readFileAsBuffer(filename: string): Promise { - return fs.readFile(filename); + return fsp.readFile(filename); } readFile(filename: string): Promise { - return fs.readFile(filename, 'utf-8'); + return fsp.readFile(filename, 'utf-8'); } async mkdirIfNotExists(dir: string): Promise { - await fs.mkdir(dir, {recursive: true}); + await fsp.mkdir(dir, {recursive: true}); return; } writeFile(filename: string, fileContent: string): Promise { - return fs.writeFile(filename, fileContent); + return fsp.writeFile(filename, fileContent); + } + + writeFileSync(filename: string, fileContent: string): void { + try { + fs.writeFileSync(filename, fileContent); + } catch (e) { + // Rethrow any errors as SfError. + const message: string = e instanceof Error ? e.message : e as string; + throw new SfError(message, null, null, INTERNAL_ERROR_CODE); + } } // Create a temp file that will automatically be cleaned up when the process exits. @@ -86,7 +99,7 @@ export class FileHandler { // at the moment. So we'll go with this semi-naive implementation, aware that it performs SLIGHTLY worse than // an optimal one, and prepared to address it if there's somehow a problem. // These are the file duplication functions available to us, in order of preference. - const dupFns: DuplicationFn[] = [fs.symlink, fs.link, fs.copyFile]; + const dupFns: DuplicationFn[] = [fsp.symlink, fsp.link, fsp.copyFile]; const errMsgs: string[] = []; // Iterate over the potential duplication methods....