From 6f2a29378a6a23040618a425163795e4e13d8995 Mon Sep 17 00:00:00 2001 From: Joshua Feingold Date: Fri, 8 Nov 2024 11:18:43 -0600 Subject: [PATCH] CHANGE @W-17053004@ Polished run command output --- messages/action-summary-viewer.md | 16 ++ messages/progress-event-listener.md | 2 +- messages/results-viewer.md | 8 +- messages/run-summary-viewer.md | 27 --- src/commands/code-analyzer/config.ts | 2 +- src/commands/code-analyzer/run.ts | 5 +- src/lib/actions/RunAction.ts | 12 +- src/lib/listeners/ProgressEventListener.ts | 2 +- src/lib/messages.ts | 2 +- src/lib/viewers/ActionSummaryViewer.ts | 60 +++++- src/lib/viewers/ResultsViewer.ts | 20 +- src/lib/viewers/RunSummaryViewer.ts | 62 ------- src/lib/writers/ResultsWriter.ts | 13 +- test/commands/code-analyzer/run.test.ts | 10 + .../no-violations.txt.goldfile | 6 + .../some-outfiles.txt.goldfile | 11 ++ .../some-violations.txt.goldfile | 8 + .../four-identical-violations-details.txt | 2 +- .../four-unique-violations-details.txt | 2 +- .../four-unique-violations-summary.txt | 3 - test/lib/actions/RunAction.test.ts | 174 +++++++++++++++++- .../listeners/ProgressEventListener.test.ts | 2 +- test/lib/viewers/ResultsViewer.test.ts | 20 +- test/lib/viewers/RunSummaryViewer.test.ts | 150 --------------- test/stubs/SpyResultsWriter.ts | 8 +- 25 files changed, 330 insertions(+), 297 deletions(-) delete mode 100644 messages/run-summary-viewer.md delete mode 100644 src/lib/viewers/RunSummaryViewer.ts create mode 100644 test/fixtures/comparison-files/lib/actions/RunAction.test.ts/action-summaries/no-violations.txt.goldfile create mode 100644 test/fixtures/comparison-files/lib/actions/RunAction.test.ts/action-summaries/some-outfiles.txt.goldfile create mode 100644 test/fixtures/comparison-files/lib/actions/RunAction.test.ts/action-summaries/some-violations.txt.goldfile delete mode 100644 test/fixtures/comparison-files/lib/viewers/RunSummaryViewer.test.ts/four-unique-violations-summary.txt delete mode 100644 test/lib/viewers/RunSummaryViewer.test.ts diff --git a/messages/action-summary-viewer.md b/messages/action-summary-viewer.md index bf7fa37ce..ad5f1be66 100644 --- a/messages/action-summary-viewer.md +++ b/messages/action-summary-viewer.md @@ -21,3 +21,19 @@ Found %d rule(s) from %d engine(s): # rules-action.rules-item %d %s rule(s) found. + +# run-action.found-no-violations + +Found 0 violations. + +# run-action.violations-total + +Found %d violation(s) across %d file(s): + +# run-action.violations-item + +%d %s severity violation(s) found. + +# run-action.outfiles-total + +Results written to: diff --git a/messages/progress-event-listener.md b/messages/progress-event-listener.md index b98fa90d0..fee84255f 100644 --- a/messages/progress-event-listener.md +++ b/messages/progress-event-listener.md @@ -11,7 +11,7 @@ done. Executing rules # execution-spinner.progress-summary -%d of %d engines still executing after %ds. +%d of %d engines finished after %ds. # execution-spinner.engine-status - %s at %d% completion. diff --git a/messages/results-viewer.md b/messages/results-viewer.md index 4d85513f3..8ed4253fe 100644 --- a/messages/results-viewer.md +++ b/messages/results-viewer.md @@ -1,11 +1,11 @@ -# summary.detail.found-results - -Found %d violation(s) across %d file(s): - # summary.detail.violation-header %d. %s +# summary.table.results-relative-to + +Violation file paths relative to '%s'. + # summary.table.found-results Found %d violation(s) across %d file(s) relative to '%s': diff --git a/messages/run-summary-viewer.md b/messages/run-summary-viewer.md deleted file mode 100644 index ab712ae84..000000000 --- a/messages/run-summary-viewer.md +++ /dev/null @@ -1,27 +0,0 @@ -# summary.header - -Summary - -# summary.found-no-violations - -Found 0 violations. - -# summary.violations-total - -Found %d violation(s): - -# summary.violations-item - -%d %s severity violation(s) found. - -# summary.no-outfiles - -No results files were specified. - -# summary.outfiles-total - -Results written to: - -# summary.log-file-location - -Additional log information written to: diff --git a/src/commands/code-analyzer/config.ts b/src/commands/code-analyzer/config.ts index 04459fdeb..951ebdf7d 100644 --- a/src/commands/code-analyzer/config.ts +++ b/src/commands/code-analyzer/config.ts @@ -62,7 +62,7 @@ export default class ConfigCommand extends SfCommand implements Displayabl protected createDependencies(outputFile?: string): ConfigDependencies { const uxDisplay: UxDisplay = new UxDisplay(this, this.spinner); - const modelGeneratorFunction = (relevantEngines: Set, userContext: ConfigContext, defaultContext: ConfigContext) => { + const modelGeneratorFunction = /* istanbul ignore next - Model tested separately */ (relevantEngines: Set, userContext: ConfigContext, defaultContext: ConfigContext) => { return AnnotatedConfigModel.fromSelection(relevantEngines, userContext, defaultContext); }; const dependencies: ConfigDependencies = { diff --git a/src/commands/code-analyzer/run.ts b/src/commands/code-analyzer/run.ts index b2cd008ca..8584447a8 100644 --- a/src/commands/code-analyzer/run.ts +++ b/src/commands/code-analyzer/run.ts @@ -6,7 +6,7 @@ import {CodeAnalyzerConfigFactoryImpl} from '../../lib/factories/CodeAnalyzerCon import {EnginePluginsFactoryImpl} from '../../lib/factories/EnginePluginsFactory'; import {CompositeResultsWriter} from '../../lib/writers/ResultsWriter'; import {ResultsDetailDisplayer, ResultsTableDisplayer} from '../../lib/viewers/ResultsViewer'; -import {RunSummaryDisplayer} from '../../lib/viewers/RunSummaryViewer'; +import {RunActionSummaryViewer} from '../../lib/viewers/ActionSummaryViewer'; import {BundleName, getMessage, getMessages} from '../../lib/messages'; import {LogEventDisplayer} from '../../lib/listeners/LogEventListener'; import {EngineRunProgressSpinner, RuleSelectionProgressSpinner} from '../../lib/listeners/ProgressEventListener'; @@ -60,6 +60,7 @@ export default class RunCommand extends SfCommand implements Displayable { description: getMessage(BundleName.RunCommand, 'flags.view.description'), char: 'v', default: View.TABLE, + exclusive: ['output-file'], options: Object.values(View) }), 'output-file': Flags.string({ @@ -108,7 +109,7 @@ export default class RunCommand extends SfCommand implements Displayable { resultsViewer: view === View.TABLE ? new ResultsTableDisplayer(uxDisplay) : new ResultsDetailDisplayer(uxDisplay), - runSummaryViewer: new RunSummaryDisplayer(uxDisplay) + actionSummaryViewer: new RunActionSummaryViewer(uxDisplay) }; } } diff --git a/src/lib/actions/RunAction.ts b/src/lib/actions/RunAction.ts index 43780761c..5f1d04582 100644 --- a/src/lib/actions/RunAction.ts +++ b/src/lib/actions/RunAction.ts @@ -13,7 +13,7 @@ import {EnginePluginsFactory} from '../factories/EnginePluginsFactory'; import {createPathStarts} from '../utils/PathStartUtil'; import {createWorkspace} from '../utils/WorkspaceUtil'; import {ResultsViewer} from '../viewers/ResultsViewer'; -import {RunSummaryViewer} from '../viewers/RunSummaryViewer'; +import {RunActionSummaryViewer} from '../viewers/ActionSummaryViewer'; import {ResultsWriter} from '../writers/ResultsWriter'; import {LogFileWriter} from '../writers/LogWriter'; import {LogEventListener, LogEventLogger} from '../listeners/LogEventListener'; @@ -27,7 +27,7 @@ export type RunDependencies = { progressListeners: ProgressEventListener[]; writer: ResultsWriter; resultsViewer: ResultsViewer; - runSummaryViewer: RunSummaryViewer; + actionSummaryViewer: RunActionSummaryViewer; } export type RunInput = { @@ -78,9 +78,11 @@ export class RunAction { // or file handlers that must be gracefully ended. this.dependencies.progressListeners.forEach(listener => listener.stopListening()); this.dependencies.logEventListeners.forEach(listener => listener.stopListening()); - this.dependencies.writer.write(results); - this.dependencies.resultsViewer.view(results); - this.dependencies.runSummaryViewer.view(results, logWriter.getLogDestination(), input['output-file']); + const resultsWritten: boolean = this.dependencies.writer.write(results); + if (!resultsWritten) { + this.dependencies.resultsViewer.view(results); + } + this.dependencies.actionSummaryViewer.view(results, logWriter.getLogDestination(), input['output-file']); const thresholdValue = input['severity-threshold']; if (thresholdValue) { diff --git a/src/lib/listeners/ProgressEventListener.ts b/src/lib/listeners/ProgressEventListener.ts index 06dce3656..55a5a7f7a 100644 --- a/src/lib/listeners/ProgressEventListener.ts +++ b/src/lib/listeners/ProgressEventListener.ts @@ -207,7 +207,7 @@ export class EngineRunProgressSpinner extends ProgressSpinner implements Progres engineLines.push(getMessage(BundleName.ProgressEventListener, 'execution-spinner.engine-status', [name, progress])); } return [ - getMessage(BundleName.ProgressEventListener, 'execution-spinner.progress-summary', [unfinishedEngines, totalEngines, secondsRunning]), + getMessage(BundleName.ProgressEventListener, 'execution-spinner.progress-summary', [totalEngines - unfinishedEngines, totalEngines, secondsRunning]), ...engineLines ].join('\n'); } diff --git a/src/lib/messages.ts b/src/lib/messages.ts index 218091880..a73b71b1a 100644 --- a/src/lib/messages.ts +++ b/src/lib/messages.ts @@ -50,6 +50,6 @@ export function getMessage(bundle: BundleName, messageKey: string, tokens?: Toke } export function getMessages(bundle: BundleName, messageKey: string, tokens?: Tokens): string[] { - INSTANCE = INSTANCE || new MessageCatalog(); + INSTANCE = INSTANCE || /* istanbul ignore next */ new MessageCatalog(); return INSTANCE.getMessages(bundle, messageKey, tokens); } diff --git a/src/lib/viewers/ActionSummaryViewer.ts b/src/lib/viewers/ActionSummaryViewer.ts index fb0515400..7325533e5 100644 --- a/src/lib/viewers/ActionSummaryViewer.ts +++ b/src/lib/viewers/ActionSummaryViewer.ts @@ -1,5 +1,5 @@ import {Display} from '../Display'; -import {RuleSelection} from '@salesforce/code-analyzer-core'; +import {RuleSelection, RunResults, SeverityLevel, Violation} from '@salesforce/code-analyzer-core'; import {toStyledHeader, indent} from '../utils/StylingUtil'; import {BundleName, getMessage} from '../messages'; @@ -75,6 +75,64 @@ export class RulesActionSummaryViewer extends AbstractActionSummaryViewer { this.display.displayLog(indent(getMessage(BundleName.ActionSummaryViewer, 'rules-action.rules-item', [ruleCountForEngine, engineName]))); } } +} + +export class RunActionSummaryViewer extends AbstractActionSummaryViewer { + public constructor(display: Display) { + super(display); + } + + public view(results: RunResults, logFile: string, outfiles: string[]): void { + // Start with separator to cleanly break from anything that's already been logged. + this.displayLineSeparator(); + this.displaySummaryHeader(); + this.displayLineSeparator(); + + if (results.getViolationCount() === 0) { + this.display.displayLog(getMessage(BundleName.ActionSummaryViewer, 'run-action.found-no-violations')); + } else { + this.displayResultsSummary(results); + } + this.displayLineSeparator(); + if (outfiles.length > 0) { + this.displayOutfiles(outfiles); + this.displayLineSeparator(); + } + this.displayLogFileInfo(logFile); + } + + private displayResultsSummary(results: RunResults): void { + this.display.displayLog(getMessage(BundleName.ActionSummaryViewer, 'run-action.violations-total', [results.getViolationCount(), this.countUniqueFiles(results.getViolations())])); + for (const sev of Object.values(SeverityLevel)) { + // Some of the keys will be numbers, since the enum is numerical. Skip those. + if (typeof sev !== 'string') { + continue; + } + const sevCount = results.getViolationCountOfSeverity(SeverityLevel[sev] as SeverityLevel); + if (sevCount > 0) { + this.display.displayLog(indent(getMessage(BundleName.ActionSummaryViewer, 'run-action.violations-item', [sevCount, sev]))); + } + } + } + + private countUniqueFiles(violations: Violation[]): number { + const fileSet: Set = new Set(); + violations.forEach(v => { + const primaryLocation = v.getCodeLocations()[v.getPrimaryLocationIndex()]; + const file = primaryLocation.getFile(); + if (file) { + fileSet.add(file); + } + }); + return fileSet.size; + } + + private displayOutfiles(outfiles: string[]): void { + this.display.displayLog(getMessage(BundleName.ActionSummaryViewer, 'run-action.outfiles-total')); + for (const outfile of outfiles) { + this.display.displayLog(indent(outfile)); + } + } } diff --git a/src/lib/viewers/ResultsViewer.ts b/src/lib/viewers/ResultsViewer.ts index 34d795467..f744a0b6e 100644 --- a/src/lib/viewers/ResultsViewer.ts +++ b/src/lib/viewers/ResultsViewer.ts @@ -20,21 +20,14 @@ abstract class AbstractResultsDisplayer implements ResultsViewer { if (results.getViolationCount() === 0) { return; } else { + this.displayLineSeparator(); this._view(results); - this.display.displayLog('\n'); + this.displayLineSeparator(); } } - protected countUniqueFiles(violations: Violation[]): number { - const fileSet: Set = new Set(); - violations.forEach(v => { - const primaryLocation = v.getCodeLocations()[v.getPrimaryLocationIndex()]; - const file = primaryLocation.getFile(); - if (file) { - fileSet.add(file); - } - }); - return fileSet.size; + protected displayLineSeparator(): void { + this.display.displayLog(''); } protected abstract _view(results: RunResults): void; @@ -44,8 +37,6 @@ export class ResultsDetailDisplayer extends AbstractResultsDisplayer { protected _view(results: RunResults): void { const violations = sortViolations(results.getViolations()); - this.display.displayLog(getMessage(BundleName.ResultsViewer, 'summary.detail.found-results', [ - violations.length, this.countUniqueFiles(violations)])); this.displayDetails(violations); } @@ -123,8 +114,7 @@ export class ResultsTableDisplayer extends AbstractResultsDisplayer { } }); - this.display.displayLog(getMessage(BundleName.ResultsViewer, 'summary.table.found-results', [ - violations.length, this.countUniqueFiles(violations), parentFolder])); + this.display.displayLog(getMessage(BundleName.ResultsViewer, 'summary.table.results-relative-to', [parentFolder])); this.display.displayTable(resultRows, TABLE_COLUMNS); } } diff --git a/src/lib/viewers/RunSummaryViewer.ts b/src/lib/viewers/RunSummaryViewer.ts deleted file mode 100644 index 290c464fb..000000000 --- a/src/lib/viewers/RunSummaryViewer.ts +++ /dev/null @@ -1,62 +0,0 @@ -import {RunResults, SeverityLevel} from '@salesforce/code-analyzer-core'; -import {Display} from '../Display'; -import {indent, toStyledHeader} from '../utils/StylingUtil'; -import {BundleName, getMessage} from '../messages'; - -export interface RunSummaryViewer { - view(results: RunResults, logFile: string, outfiles: string[]): void; -} - -export class RunSummaryDisplayer implements RunSummaryViewer { - protected display: Display; - - public constructor(display: Display) { - this.display = display; - } - - public view(results: RunResults, logFile: string, outfiles: string[]): void { - this.display.displayLog(toStyledHeader(getMessage(BundleName.RunSummaryViewer, 'summary.header'))); - // Use empty line as a visual separator - this.display.displayLog(''); - - if (results.getViolationCount() === 0) { - this.display.displayLog(getMessage(BundleName.RunSummaryViewer, 'summary.found-no-violations')); - } else { - this.displayResultsSummary(results); - } - // Use empty line as a visual separator - this.display.displayLog(''); - - if (outfiles.length === 0) { - this.display.displayLog(getMessage(BundleName.RunSummaryViewer, 'summary.no-outfiles')); - } else { - this.displayOutfiles(outfiles); - } - // Use empty line as a visual separator - this.display.displayLog(''); - - this.display.displayLog(getMessage(BundleName.RunSummaryViewer, 'summary.log-file-location')); - this.display.displayLog(indent(logFile)); - } - - private displayResultsSummary(results: RunResults): void { - this.display.displayLog(getMessage(BundleName.RunSummaryViewer, 'summary.violations-total', [results.getViolationCount()])); - for (const sev of Object.values(SeverityLevel)) { - // Some of the keys will be numbers, since the enum is numerical. Skip those. - if (typeof sev !== 'string') { - continue; - } - const sevCount = results.getViolationCountOfSeverity(SeverityLevel[sev] as SeverityLevel); - if (sevCount > 0) { - this.display.displayLog(indent(getMessage(BundleName.RunSummaryViewer, 'summary.violations-item', [sevCount, sev]))); - } - } - } - - private displayOutfiles(outfiles: string[]): void { - this.display.displayLog(getMessage(BundleName.RunSummaryViewer, 'summary.outfiles-total')); - for (const outfile of outfiles) { - this.display.displayLog(indent(outfile)); - } - } -} diff --git a/src/lib/writers/ResultsWriter.ts b/src/lib/writers/ResultsWriter.ts index 4fcae8a7c..4869819cf 100644 --- a/src/lib/writers/ResultsWriter.ts +++ b/src/lib/writers/ResultsWriter.ts @@ -4,7 +4,7 @@ import {OutputFormat, RunResults} from '@salesforce/code-analyzer-core'; import {BundleName, getMessage} from '../messages'; export interface ResultsWriter { - write(results: RunResults): void; + write(results: RunResults): boolean; } export class CompositeResultsWriter implements ResultsWriter { @@ -14,8 +14,12 @@ export class CompositeResultsWriter implements ResultsWriter { this.writers = writers; } - public write(results: RunResults): void { - this.writers.forEach(w => w.write(results)); + public write(results: RunResults): boolean { + let written: boolean = false; + for (const writer of this.writers) { + written = writer.write(results) || /* istanbul ignore next */ written; + } + return written; } public static fromFiles(files: string[]): CompositeResultsWriter { @@ -49,7 +53,8 @@ export class ResultsFileWriter implements ResultsWriter { } } - public write(results: RunResults): void { + public write(results: RunResults): boolean { fs.writeFileSync(this.file, results.toFormattedOutput(this.format)); + return true; } } diff --git a/test/commands/code-analyzer/run.test.ts b/test/commands/code-analyzer/run.test.ts index 094ece083..f8d54a997 100644 --- a/test/commands/code-analyzer/run.test.ts +++ b/test/commands/code-analyzer/run.test.ts @@ -334,5 +334,15 @@ describe('`code-analyzer run` tests', () => { expect(receivedActionDependencies.resultsViewer.constructor.name).toEqual('ResultsDetailDisplayer'); }); }); + + describe('Flag interactions', () => { + it('--output-file and --view are mutually exclusive', async () => { + const outfileInputValue = './somefile.json'; + const viewInputValue = 'detail'; + const executionPromise = RunCommand.run(['--output-file', outfileInputValue, '--view', viewInputValue]); + await expect(executionPromise).rejects.toThrow('--output-file=./somefile.json cannot also be provided when using --view'); + expect(executeSpy).not.toHaveBeenCalled(); + }); + }); }); diff --git a/test/fixtures/comparison-files/lib/actions/RunAction.test.ts/action-summaries/no-violations.txt.goldfile b/test/fixtures/comparison-files/lib/actions/RunAction.test.ts/action-summaries/no-violations.txt.goldfile new file mode 100644 index 000000000..9b02dca8f --- /dev/null +++ b/test/fixtures/comparison-files/lib/actions/RunAction.test.ts/action-summaries/no-violations.txt.goldfile @@ -0,0 +1,6 @@ + +=== Summary + +Found 0 violations. + +Additional log information written to: \ No newline at end of file diff --git a/test/fixtures/comparison-files/lib/actions/RunAction.test.ts/action-summaries/some-outfiles.txt.goldfile b/test/fixtures/comparison-files/lib/actions/RunAction.test.ts/action-summaries/some-outfiles.txt.goldfile new file mode 100644 index 000000000..f0f45442b --- /dev/null +++ b/test/fixtures/comparison-files/lib/actions/RunAction.test.ts/action-summaries/some-outfiles.txt.goldfile @@ -0,0 +1,11 @@ + +=== Summary + +Found 1 violation(s) across 1 file(s): + 1 Low severity violation(s) found. + +Results written to: + {{PATH_TO_FILE1}} + {{PATH_TO_FILE2}} + +Additional log information written to: \ No newline at end of file diff --git a/test/fixtures/comparison-files/lib/actions/RunAction.test.ts/action-summaries/some-violations.txt.goldfile b/test/fixtures/comparison-files/lib/actions/RunAction.test.ts/action-summaries/some-violations.txt.goldfile new file mode 100644 index 000000000..7b04ba246 --- /dev/null +++ b/test/fixtures/comparison-files/lib/actions/RunAction.test.ts/action-summaries/some-violations.txt.goldfile @@ -0,0 +1,8 @@ + +=== Summary + +Found 2 violation(s) across 1 file(s): + 1 High severity violation(s) found. + 1 Low severity violation(s) found. + +Additional log information written to: \ No newline at end of file diff --git a/test/fixtures/comparison-files/lib/viewers/ResultsViewer.test.ts/four-identical-violations-details.txt b/test/fixtures/comparison-files/lib/viewers/ResultsViewer.test.ts/four-identical-violations-details.txt index b180c09b8..adb9b67b1 100644 --- a/test/fixtures/comparison-files/lib/viewers/ResultsViewer.test.ts/four-identical-violations-details.txt +++ b/test/fixtures/comparison-files/lib/viewers/ResultsViewer.test.ts/four-identical-violations-details.txt @@ -1,4 +1,4 @@ -Found 4 violation(s) across 1 file(s): + === 1. stub1RuleA severity: 4 (Low) engine: stubEngine1 diff --git a/test/fixtures/comparison-files/lib/viewers/ResultsViewer.test.ts/four-unique-violations-details.txt b/test/fixtures/comparison-files/lib/viewers/ResultsViewer.test.ts/four-unique-violations-details.txt index 35080fcf8..63f537b94 100644 --- a/test/fixtures/comparison-files/lib/viewers/ResultsViewer.test.ts/four-unique-violations-details.txt +++ b/test/fixtures/comparison-files/lib/viewers/ResultsViewer.test.ts/four-unique-violations-details.txt @@ -1,4 +1,4 @@ -Found 4 violation(s) across 2 file(s): + === 1. stub1RuleB severity: 2 (High) engine: stubEngine1 diff --git a/test/fixtures/comparison-files/lib/viewers/RunSummaryViewer.test.ts/four-unique-violations-summary.txt b/test/fixtures/comparison-files/lib/viewers/RunSummaryViewer.test.ts/four-unique-violations-summary.txt deleted file mode 100644 index fdfd3a17d..000000000 --- a/test/fixtures/comparison-files/lib/viewers/RunSummaryViewer.test.ts/four-unique-violations-summary.txt +++ /dev/null @@ -1,3 +0,0 @@ -Found 4 violation(s): - 1 High severity violation(s) found. - 3 Low severity violation(s) found. diff --git a/test/lib/actions/RunAction.test.ts b/test/lib/actions/RunAction.test.ts index 70a6cefa3..6b3daeb0e 100644 --- a/test/lib/actions/RunAction.test.ts +++ b/test/lib/actions/RunAction.test.ts @@ -1,26 +1,31 @@ import path from 'node:path'; +import * as fsp from 'node:fs/promises'; import {SfError} from '@salesforce/core'; +import ansis from 'ansis'; import {SeverityLevel} from '@salesforce/code-analyzer-core'; import {SpyResultsViewer} from '../../stubs/SpyResultsViewer'; -import {SpyRunSummaryViewer} from '../../stubs/SpyRunSummaryViewer'; import {SpyResultsWriter} from '../../stubs/SpyResultsWriter'; +import {SpyDisplay, DisplayEventType} from '../../stubs/SpyDisplay'; import {StubDefaultConfigFactory} from '../../stubs/StubCodeAnalyzerConfigFactories'; import {ConfigurableStubEnginePlugin1, StubEngine1, TargetDependentEngine1} from '../../stubs/StubEnginePlugins'; import {RunAction, RunInput, RunDependencies} from '../../../src/lib/actions/RunAction'; +import {RunActionSummaryViewer} from '../../../src/lib/viewers/ActionSummaryViewer'; import { StubEnginePluginsFactory_withPreconfiguredStubEngines, StubEnginePluginsFactory_withThrowingStubPlugin } from '../../stubs/StubEnginePluginsFactories'; const PATH_TO_FILE_A = path.resolve('test', 'sample-code', 'fileA.cls'); +const PATH_TO_GOLDFILES = path.join(__dirname, '..', '..', 'fixtures', 'comparison-files', 'lib', 'actions', 'RunAction.test.ts'); describe('RunAction tests', () => { + let spyDisplay: SpyDisplay; let engine1: StubEngine1; let stubEnginePlugin: ConfigurableStubEnginePlugin1; let pluginsFactory: StubEnginePluginsFactory_withPreconfiguredStubEngines; let writer: SpyResultsWriter; let resultsViewer: SpyResultsViewer; - let runSummaryViewer: SpyRunSummaryViewer; + let actionSummaryViewer: RunActionSummaryViewer; let dependencies: RunDependencies; let action: RunAction; @@ -33,9 +38,10 @@ describe('RunAction tests', () => { pluginsFactory.addPreconfiguredEnginePlugin(stubEnginePlugin); // Set up the writer and viewers. - writer = new SpyResultsWriter(); + spyDisplay = new SpyDisplay(); + writer = new SpyResultsWriter(false); resultsViewer = new SpyResultsViewer(); - runSummaryViewer = new SpyRunSummaryViewer(); + actionSummaryViewer = new RunActionSummaryViewer(spyDisplay); // Initialize our dependency object. dependencies = { @@ -45,7 +51,7 @@ describe('RunAction tests', () => { progressListeners: [], writer, resultsViewer, - runSummaryViewer + actionSummaryViewer }; // Create the action. action = RunAction.createAction(dependencies); @@ -94,8 +100,50 @@ describe('RunAction tests', () => { expect(writer.getCallHistory()[0].getViolations()[0].getMessage()).toEqual('Fake message'); expect(resultsViewer.getCallHistory()[0].getViolationCount()).toEqual(1); expect(resultsViewer.getCallHistory()[0].getViolations()[0].getMessage()).toEqual('Fake message'); - expect(runSummaryViewer.getCallHistory()[0].results.getViolationCount()).toEqual(1); - expect(runSummaryViewer.getCallHistory()[0].results.getViolations()[0].getMessage()).toEqual('Fake message'); + }); + + it('When outfile is supplied, viewer is unused', async () => { + // ==== SETUP ==== + const expectedRules = ['stub1RuleA', 'stub1RuleB', 'stub1RuleC', 'stub1RuleD', 'stub1RuleE'] + // Create the input. + const input: RunInput = { + // Use the selector provided by the test + 'rule-selector': ['all'], + // Use the current directory, for convenience. + 'workspace': ['.'], + // Outfile is non-null. + 'output-file': ['beep.json'] + }; + // Configure the engine to return a violation for the first expected rule. + engine1.resultsToReturn = { + violations: [{ + ruleName: 'stub1RuleA', + message: 'Fake message', + codeLocations: [{ + file: PATH_TO_FILE_A, + startLine: 5, + startColumn: 1 + }], + primaryLocationIndex: 0 + }] + }; + // Use a writer that simulates writing to a file. + const simulatedFileWriter: SpyResultsWriter = new SpyResultsWriter(true); + dependencies.writer = simulatedFileWriter + + // ==== TESTED BEHAVIOR ==== + await action.execute(input); + + // ==== ASSERTIONS ==== + // Verify that the expected rules were executed on the right files. + const actualExecutedRules = engine1.runRulesCallHistory[0].ruleNames; + expect(actualExecutedRules).toEqual(expectedRules); + const actualTargetFiles = engine1.runRulesCallHistory[0].runOptions.workspace.getFilesAndFolders(); + expect(actualTargetFiles).toEqual([path.resolve('.')]); + // Verify that the expected results were passed into the Writer. + expect(simulatedFileWriter.getCallHistory()[0].getViolationCount()).toEqual(1); + expect(simulatedFileWriter.getCallHistory()[0].getViolations()[0].getMessage()).toEqual('Fake message'); + expect(resultsViewer.getCallHistory()).toHaveLength(0); }); it('Engines with target-dependent rules run the right rules', async () => { @@ -217,7 +265,7 @@ describe('RunAction tests', () => { progressListeners: [], writer, resultsViewer, - runSummaryViewer + actionSummaryViewer }; // Instantiate our action, intentionally using a different instance than the one set up in // the before-each. @@ -234,6 +282,116 @@ describe('RunAction tests', () => { // ==== ASSERTIONS ==== await expect(executionPromise).rejects.toThrow('SomeErrorFromGetAvailableEngineNames'); }); + + describe('Summary generation', () => { + it.each([ + {quantifier: 'no', expectation: 'Summarizer reflects this fact', goldfile: 'no-violations.txt.goldfile', resultsToReturn: []}, + {quantifier: 'some', expectation: 'Summarizer breaks them down by severity', goldfile: 'some-violations.txt.goldfile', + resultsToReturn: [{ + ruleName: 'stub1RuleA', + message: 'Fake message', + codeLocations: [{ + file: PATH_TO_FILE_A, + startLine: 5, + startColumn: 1 + }], + primaryLocationIndex: 0 + }, { + ruleName: 'stub1RuleB', + message: 'Fake message', + codeLocations: [{ + file: PATH_TO_FILE_A, + startLine: 5, + startColumn: 1 + }], + primaryLocationIndex: 0 + }] + } + ])('When $quantifier violations are found, $expectation', async ({resultsToReturn, goldfile}) => { + // ==== SETUP ==== + const expectedRules = ['stub1RuleA', 'stub1RuleB', 'stub1RuleC', 'stub1RuleD', 'stub1RuleE'] + // Create the input. + const input: RunInput = { + // Use the selector provided by the test + 'rule-selector': ['all'], + // Use the current directory, for convenience. + 'workspace': ['.'], + // Outfile is just an empty list + 'output-file': [] + }; + // Configure the engine to return a violation for the first expected rule. + engine1.resultsToReturn = { + violations: resultsToReturn + }; + + // ==== TESTED BEHAVIOR ==== + await action.execute(input); + + // ==== ASSERTIONS ==== + // Verify that the expected rules were executed on the right files. + const actualExecutedRules = engine1.runRulesCallHistory[0].ruleNames; + expect(actualExecutedRules).toEqual(expectedRules); + const actualTargetFiles = engine1.runRulesCallHistory[0].runOptions.workspace.getFilesAndFolders(); + expect(actualTargetFiles).toEqual([path.resolve('.')]); + // Verify that the summary output matches the expectation. + const goldfileContents: string = await fsp.readFile(path.join(PATH_TO_GOLDFILES, 'action-summaries', goldfile), 'utf-8'); + const displayEvents = spyDisplay.getDisplayEvents(); + const displayedLogEvents = ansis.strip(displayEvents + .filter(e => e.type === DisplayEventType.LOG) + .map(e => e.data) + .join('\n')); + expect(displayedLogEvents).toContain(goldfileContents); + }); + + it('When Outfiles are provided, they are mentioned', async () => { + // ==== SETUP ==== + const expectedRules = ['stub1RuleA', 'stub1RuleB', 'stub1RuleC', 'stub1RuleD', 'stub1RuleE'] + const outfilePath1 = path.join('the', 'specifics', 'of', 'this', 'path', 'do', 'not', 'matter.csv'); + const outfilePath2 = path.join('neither', 'do', 'the', 'specifics', 'of', 'this', 'one.json'); + // Create the input. + const input: RunInput = { + // Use the selector provided by the test + 'rule-selector': ['all'], + // Use the current directory, for convenience. + 'workspace': ['.'], + // Outfiles are provided + 'output-file': [outfilePath1, outfilePath2] + }; + // Configure the engine to return a violation for the first expected rule. + engine1.resultsToReturn = { + violations: [{ + ruleName: 'stub1RuleA', + message: 'Fake message', + codeLocations: [{ + file: PATH_TO_FILE_A, + startLine: 5, + startColumn: 1 + }], + primaryLocationIndex: 0 + }] + }; + + // ==== TESTED BEHAVIOR ==== + await action.execute(input); + + // ==== ASSERTIONS ==== + // Verify that the expected rules were executed on the right files. + const actualExecutedRules = engine1.runRulesCallHistory[0].ruleNames; + expect(actualExecutedRules).toEqual(expectedRules); + const actualTargetFiles = engine1.runRulesCallHistory[0].runOptions.workspace.getFilesAndFolders(); + expect(actualTargetFiles).toEqual([path.resolve('.')]); + // Verify that the summary output matches the expectation. + const goldfileContents: string = (await fsp.readFile(path.join(PATH_TO_GOLDFILES, 'action-summaries', 'some-outfiles.txt.goldfile'), 'utf-8')) + .replace(`{{PATH_TO_FILE1}}`, outfilePath1) + .replace(`{{PATH_TO_FILE2}}`, outfilePath2); + const displayEvents = spyDisplay.getDisplayEvents(); + const displayedLogEvents = ansis.strip(displayEvents + .filter(e => e.type === DisplayEventType.LOG) + .map(e => e.data) + .join('\n')); + expect(displayedLogEvents).toContain(goldfileContents); + }); + }); }); // TODO: Whenever we decide to document the custom_engine_plugin_modules flag in our configuration file, then we'll want diff --git a/test/lib/listeners/ProgressEventListener.test.ts b/test/lib/listeners/ProgressEventListener.test.ts index f875cd087..e2b85be67 100644 --- a/test/lib/listeners/ProgressEventListener.test.ts +++ b/test/lib/listeners/ProgressEventListener.test.ts @@ -247,7 +247,7 @@ describe('ProgressEventListener implementations', () => { const startEvent = displayEvents[0]; expect(startEvent).toHaveProperty('type', DisplayEventType.SPINNER_START); // The start event should always start with at least 1 known engine, to prevent "0 of 0 engines" scenarios. - expect(startEvent.data).toContain("1 of 1 engines"); + expect(startEvent.data).toContain("0 of 1 engines"); const percentagesInOrder = getDedupedCompletionPercentages(displayEvents.slice(1, displayEvents.length - 1)); expect(percentagesInOrder).toEqual([0, 50, 100]); const endEvent = displayEvents[displayEvents.length - 1]; diff --git a/test/lib/viewers/ResultsViewer.test.ts b/test/lib/viewers/ResultsViewer.test.ts index cb4c74ce9..cf9666018 100644 --- a/test/lib/viewers/ResultsViewer.test.ts +++ b/test/lib/viewers/ResultsViewer.test.ts @@ -176,11 +176,13 @@ describe('ResultsViewer implementations', () => { // ==== ASSERTIONS ==== const displayEvents = spyDisplay.getDisplayEvents(); - expect(displayEvents).toHaveLength(3); + expect(displayEvents).toHaveLength(4); expect(displayEvents[0].type).toEqual(DisplayEventType.LOG); - expect(displayEvents[0].data).toEqual(getMessage(BundleName.ResultsViewer, 'summary.table.found-results', [4, 1, PATH_TO_SAMPLE_CODE])); - expect(displayEvents[1].type).toEqual(DisplayEventType.TABLE); - expect(displayEvents[1].data).toEqual(`{"columns":["#","Severity","Rule","Location","Message"],"rows":[{"num":1,"location":"someFile.cls:1:1","rule":"stubEngine1:stub1RuleA","severity":"4 (Low)","message":"This is a message"},{"num":2,"location":"someFile.cls:1:1","rule":"stubEngine1:stub1RuleA","severity":"4 (Low)","message":"This is a message"},{"num":3,"location":"someFile.cls:1:1","rule":"stubEngine1:stub1RuleA","severity":"4 (Low)","message":"This is a message"},{"num":4,"location":"someFile.cls:1:1","rule":"stubEngine1:stub1RuleA","severity":"4 (Low)","message":"This is a message"}]}`); + expect(displayEvents[0].data).toEqual(''); + expect(displayEvents[1].type).toEqual(DisplayEventType.LOG); + expect(displayEvents[1].data).toEqual(getMessage(BundleName.ResultsViewer, 'summary.table.results-relative-to', [PATH_TO_SAMPLE_CODE])); + expect(displayEvents[2].type).toEqual(DisplayEventType.TABLE); + expect(displayEvents[2].data).toEqual(`{"columns":["#","Severity","Rule","Location","Message"],"rows":[{"num":1,"location":"someFile.cls:1:1","rule":"stubEngine1:stub1RuleA","severity":"4 (Low)","message":"This is a message"},{"num":2,"location":"someFile.cls:1:1","rule":"stubEngine1:stub1RuleA","severity":"4 (Low)","message":"This is a message"},{"num":3,"location":"someFile.cls:1:1","rule":"stubEngine1:stub1RuleA","severity":"4 (Low)","message":"This is a message"},{"num":4,"location":"someFile.cls:1:1","rule":"stubEngine1:stub1RuleA","severity":"4 (Low)","message":"This is a message"}]}`); }); // The reasoning behind this sorting order is so that the Table view can function as a "show me all the violations @@ -208,11 +210,13 @@ describe('ResultsViewer implementations', () => { // ==== ASSERTIONS ==== const displayEvents = spyDisplay.getDisplayEvents(); - expect(displayEvents).toHaveLength(3); + expect(displayEvents).toHaveLength(4); expect(displayEvents[0].type).toEqual(DisplayEventType.LOG); - expect(displayEvents[0].data).toEqual(getMessage(BundleName.ResultsViewer, 'summary.table.found-results', [4, 2, PATH_TO_SAMPLE_CODE])); - expect(displayEvents[1].type).toEqual(DisplayEventType.TABLE); - expect(displayEvents[1].data).toEqual(`{"columns":["#","Severity","Rule","Location","Message"],"rows":[{"num":1,"location":"fileZ.cls:20:1","rule":"stubEngine1:stub1RuleB","severity":"2 (High)","message":"This is a message"},{"num":2,"location":"fileA.cls:1:1","rule":"stubEngine1:stub1RuleA","severity":"4 (Low)","message":"This is a message"},{"num":3,"location":"fileA.cls:20:1","rule":"stubEngine1:stub1RuleA","severity":"4 (Low)","message":"This is a message"},{"num":4,"location":"fileZ.cls:1:1","rule":"stubEngine1:stub1RuleA","severity":"4 (Low)","message":"This is a message"}]}`); + expect(displayEvents[0].data).toEqual(''); + expect(displayEvents[1].type).toEqual(DisplayEventType.LOG); + expect(displayEvents[1].data).toEqual(getMessage(BundleName.ResultsViewer, 'summary.table.results-relative-to', [PATH_TO_SAMPLE_CODE])); + expect(displayEvents[2].type).toEqual(DisplayEventType.TABLE); + expect(displayEvents[2].data).toEqual(`{"columns":["#","Severity","Rule","Location","Message"],"rows":[{"num":1,"location":"fileZ.cls:20:1","rule":"stubEngine1:stub1RuleB","severity":"2 (High)","message":"This is a message"},{"num":2,"location":"fileA.cls:1:1","rule":"stubEngine1:stub1RuleA","severity":"4 (Low)","message":"This is a message"},{"num":3,"location":"fileA.cls:20:1","rule":"stubEngine1:stub1RuleA","severity":"4 (Low)","message":"This is a message"},{"num":4,"location":"fileZ.cls:1:1","rule":"stubEngine1:stub1RuleA","severity":"4 (Low)","message":"This is a message"}]}`); }); }); }); diff --git a/test/lib/viewers/RunSummaryViewer.test.ts b/test/lib/viewers/RunSummaryViewer.test.ts deleted file mode 100644 index 7a06b8d4f..000000000 --- a/test/lib/viewers/RunSummaryViewer.test.ts +++ /dev/null @@ -1,150 +0,0 @@ -import fs from "node:fs/promises"; -import path from "node:path"; -import ansis from 'ansis'; -import {CodeAnalyzer, CodeAnalyzerConfig, RunResults} from '@salesforce/code-analyzer-core'; -import {Violation} from "@salesforce/code-analyzer-engine-api"; - -import {RunSummaryDisplayer} from '../../../src/lib/viewers/RunSummaryViewer'; - -import {DisplayEvent, DisplayEventType, SpyDisplay} from "../../stubs/SpyDisplay"; -import {FunctionalStubEnginePlugin1, StubEngine1} from '../../stubs/StubEnginePlugins'; - -const PATH_TO_COMPARISON_FILES = path.resolve(__dirname, '..', '..', '..', 'test', 'fixtures', 'comparison-files', 'lib', - 'viewers', 'RunSummaryViewer.test.ts'); -const PATH_TO_SAMPLE_CODE = path.resolve(__dirname, '..', '..', '..', 'test', 'sample-code'); -const PATH_TO_OUTFILE1 = path.join('the', 'specifics', 'of', 'this', 'path', 'do', 'not', 'matter.csv'); -const PATH_TO_OUTFILE2 = path.join('neither', 'do', 'the', 'specifics', 'of', 'this', 'one.json'); -const PATH_TO_FILE_A = path.resolve(PATH_TO_SAMPLE_CODE, 'fileA.cls'); -const PATH_TO_FILE_Z = path.resolve(PATH_TO_SAMPLE_CODE, 'fileZ.cls'); - -describe('RunSummaryViewer implementations', () => { - // We need SpyDisplays for the cases where inputs are empty and non-empty. - const emptyInputsSpyDisplay: SpyDisplay = new SpyDisplay(); - const nonEmptyInputsSpyDisplay: SpyDisplay = new SpyDisplay(); - - // We need a config, core, and plugin. - const config: CodeAnalyzerConfig = CodeAnalyzerConfig.withDefaults(); - const codeAnalyzerCore: CodeAnalyzer = new CodeAnalyzer(config); - const stubEnginePlugin: FunctionalStubEnginePlugin1 = new FunctionalStubEnginePlugin1(); - - let emptyResults: RunResults; - let nonEmptyResults: RunResults; - - // The tests are similar enough that we can do all of the setup in the `beforeAll()` functions. - beforeAll(async () => { - // Add the stub plugin to the core. - await codeAnalyzerCore.addEnginePlugin(stubEnginePlugin); - - // Run the core once without assigning any violations. - const workspace = await codeAnalyzerCore.createWorkspace(['package.json']); - const rules = await codeAnalyzerCore.selectRules(['all'], {workspace}); - emptyResults = await codeAnalyzerCore.run(rules, {workspace}); - - // Assign some violations and then run the core again. - const engine1 = stubEnginePlugin.getCreatedEngine(`stubEngine1`) as StubEngine1; - const rule1 = (await engine1.describeRules())[0]; - const rule2 = (await engine1.describeRules())[1]; - const violations: Violation[] = [ - // A low-severity violation late in a high-alphabetical file. - createViolation(rule1.name, PATH_TO_FILE_A, 20, 1), - // A low-severity violation early in the same high-alphabetical file. - createViolation(rule1.name, PATH_TO_FILE_A, 1, 1), - // A low-severity violation early in a low-alphabetical file. - createViolation(rule1.name, PATH_TO_FILE_Z, 1, 1), - // A high-severity violation later in the same low-alphabetical file. - createViolation(rule2.name, PATH_TO_FILE_Z, 20, 1) - ]; - engine1.resultsToReturn = {violations}; - - nonEmptyResults = await codeAnalyzerCore.run(rules, {workspace}); - }); - - describe('RunSummaryDisplayer', () => { - // Create Displayers for the empty-input and non-empty-input cases. - const emptyInputsDisplayer: RunSummaryDisplayer = new RunSummaryDisplayer(emptyInputsSpyDisplay); - const nonEmptyInputsDisplayer: RunSummaryDisplayer = new RunSummaryDisplayer(nonEmptyInputsSpyDisplay); - const fakeLogFile: string = path.join('path', 'to', 'fakelogfile.log'); - - let emptyInputsDisplayEvents: DisplayEvent[]; - let nonEmptyInputsDisplayEvents: DisplayEvent[]; - - beforeAll(() => { - emptyInputsDisplayer.view(emptyResults, fakeLogFile, []); - emptyInputsDisplayEvents = emptyInputsSpyDisplay.getDisplayEvents(); - nonEmptyInputsDisplayer.view(nonEmptyResults, fakeLogFile, [PATH_TO_OUTFILE1, PATH_TO_OUTFILE2]) - nonEmptyInputsDisplayEvents = nonEmptyInputsSpyDisplay.getDisplayEvents(); - }); - - describe('Formatting', () => { - it('Output has correct header', () => { - expect(ansis.strip(emptyInputsDisplayEvents[0].data)).toEqual('=== Summary'); - expect(ansis.strip(nonEmptyInputsDisplayEvents[0].data)).toEqual('=== Summary'); - }); - - it('Output has correct log level', () => { - for (const event of emptyInputsDisplayEvents) { - expect(event.type).toEqual(DisplayEventType.LOG); - } - for (const event of nonEmptyInputsDisplayEvents) { - expect(event.type).toEqual(DisplayEventType.LOG); - } - }); - }); - - describe('Results breakdown', () => { - it('When no violations exist, correctly outputs this fact', () => { - const contents = emptyInputsDisplayEvents.map(e => e.data).join('\n'); - expect(contents).toContain('Found 0 violations.\n'); - }); - - it('When violations exist, they are broken down by severity', async () => { - const contents = nonEmptyInputsDisplayEvents.map(e => e.data).join('\n'); - const expectedViolationSummary = await readComparisonFile('four-unique-violations-summary.txt'); - expect(contents).toContain(expectedViolationSummary); - }); - }); - - describe('Outfile breakdown', () => { - it('When no outfiles were provided, correctly outputs this fact', () => { - const contents = emptyInputsDisplayEvents.map(e => e.data).join('\n'); - expect(contents).toContain('No results files were specified.\n'); - }); - - it('When outfiles were provided, they are properly listed', () => { - const contents = nonEmptyInputsDisplayEvents.map(e => e.data).join('\n'); - const expectation = `Results written to:\n` + - ` ${PATH_TO_OUTFILE1}\n` + - ` ${PATH_TO_OUTFILE2}\n`; - expect(contents).toContain(expectation); - }); - }); - - describe('Logging breakdown', () => { - it('Logfile is correctly displayed', () => { - const expectation = `Additional log information written to:\n` + - ` ${fakeLogFile}`; - const emptyContents = emptyInputsDisplayEvents.map(e => e.data).join('\n'); - const nonEmptyContents = nonEmptyInputsDisplayEvents.map(e => e.data).join('\n'); - expect(emptyContents).toContain(expectation); - expect(nonEmptyContents).toContain(expectation); - }); - }); - }); -}); - -function createViolation(ruleName: string, file: string, startLine: number, startColumn: number): Violation { - return { - ruleName, - message: 'This is a message', - codeLocations: [{ - file, - startLine, - startColumn - }], - primaryLocationIndex: 0 - }; -} - -function readComparisonFile(fileName: string): Promise { - return fs.readFile(path.join(PATH_TO_COMPARISON_FILES, fileName), {encoding: 'utf-8'}); -} diff --git a/test/stubs/SpyResultsWriter.ts b/test/stubs/SpyResultsWriter.ts index 9dca6a73e..cf6d3dff1 100644 --- a/test/stubs/SpyResultsWriter.ts +++ b/test/stubs/SpyResultsWriter.ts @@ -2,10 +2,16 @@ import {RunResults} from '@salesforce/code-analyzer-core'; import {ResultsWriter} from '../../src/lib/writers/ResultsWriter'; export class SpyResultsWriter implements ResultsWriter { + private writeReturnValue: boolean; private callHistory: RunResults[] = []; - public write(results: RunResults): void { + public constructor(writeReturnValue: boolean) { + this.writeReturnValue = writeReturnValue; + } + + public write(results: RunResults): boolean { this.callHistory.push(results); + return this.writeReturnValue; } public getCallHistory(): RunResults[] {