From c4679a9bfe82f863a5ade0585d08cb928012daad Mon Sep 17 00:00:00 2001 From: Stephen Carter Date: Thu, 22 Aug 2024 16:41:02 -0400 Subject: [PATCH] CHANGE: @W-16569797@: Improve table view by displaying relative paths whenever possible --- messages/results-viewer.md | 6 +- package.json | 3 +- src/lib/viewers/ResultsViewer.ts | 60 ++++++++++--- test/lib/viewers/ResultsViewer.test.ts | 112 ++++++++++++------------- 4 files changed, 107 insertions(+), 74 deletions(-) diff --git a/messages/results-viewer.md b/messages/results-viewer.md index e8569a8f8..97629b94e 100644 --- a/messages/results-viewer.md +++ b/messages/results-viewer.md @@ -2,7 +2,7 @@ Found 0 violations. -# summary.found-results +# summary.detail.found-results Found %d violation(s) across %d file(s): @@ -22,6 +22,10 @@ Found %d violation(s): %d %s severity +# summary.table.found-results + +Found %d violation(s) across %d file(s) relative to '%s': + # summary.table.num-column # diff --git a/package.json b/package.json index dcf64d1a0..56775d8a5 100644 --- a/package.json +++ b/package.json @@ -124,6 +124,7 @@ "postpack": "rm -f oclif.manifest.json oclif.lock npm-shrinkwrap.json", "lint": "eslint ./src --max-warnings 0", "version": "oclif readme && git add README.md", - "test": "jest --coverage" + "test": "jest --coverage", + "showcoverage": "open ./coverage/lcov-report/index.html" } } diff --git a/src/lib/viewers/ResultsViewer.ts b/src/lib/viewers/ResultsViewer.ts index 73ef35194..50536d49d 100644 --- a/src/lib/viewers/ResultsViewer.ts +++ b/src/lib/viewers/ResultsViewer.ts @@ -1,8 +1,9 @@ import {Ux} from '@salesforce/sf-plugins-core'; -import {RunResults, SeverityLevel, Violation} from '@salesforce/code-analyzer-core'; +import {CodeLocation, RunResults, SeverityLevel, Violation} from '@salesforce/code-analyzer-core'; import {Display} from '../Display'; import {toStyledHeaderAndBody, toStyledHeader} from '../utils/StylingUtil'; import {BundleName, getMessage} from '../messages'; +import path from "node:path"; export interface ResultsViewer { view(results: RunResults): void; @@ -19,18 +20,11 @@ abstract class AbstractResultsViewer implements ResultsViewer { if (results.getViolationCount() === 0) { this.display.displayLog(getMessage(BundleName.ResultsViewer, 'summary.found-no-results')); } else { - this.displaySummary(results.getViolations()); this._view(results); } } - private displaySummary(violations: Violation[]): void { - const violationCount = violations.length; - const fileCount = this.countUniqueFiles(violations); - this.display.displayLog(getMessage(BundleName.ResultsViewer, 'summary.found-results', [violationCount, fileCount])); - } - - private countUniqueFiles(violations: Violation[]): number { + protected countUniqueFiles(violations: Violation[]): number { const fileSet: Set = new Set(); violations.forEach(v => { const primaryLocation = v.getCodeLocations()[v.getPrimaryLocationIndex()]; @@ -49,6 +43,8 @@ export class ResultsDetailViewer extends AbstractResultsViewer { 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); this.display.displayLog('\n'); this.displayBreakdown(results); @@ -125,18 +121,26 @@ const TABLE_COLUMNS: Ux.Table.Columns = { export class ResultsTableViewer extends AbstractResultsViewer { protected _view(results: RunResults) { - const resultRows: ResultRow[] = sortViolations(results.getViolations()) - .map((v, idx) => { + const violations: Violation[] = sortViolations(results.getViolations()); + const parentFolder: string = findLongestCommonParentFolderOf(violations.map(v => + getPrimaryLocation(v).getFile()).filter(f => f !== undefined)); + + const resultRows: ResultRow[] = violations.map((v, idx) => { const severity = v.getRule().getSeverityLevel(); - const primaryLocation = v.getCodeLocations()[v.getPrimaryLocationIndex()]; + const primaryLocation = getPrimaryLocation(v); + const relativeFile: string | null = primaryLocation.getFile() ? + path.relative(parentFolder, primaryLocation.getFile()!) : null; return { num: idx + 1, - location: `${primaryLocation.getFile()}:${primaryLocation.getStartLine()}:${primaryLocation.getStartColumn()}`, + location: `${relativeFile}:${primaryLocation.getStartLine()}:${primaryLocation.getStartColumn()}`, rule: `${v.getRule().getEngineName()}:${v.getRule().getName()}`, severity: `${severity.valueOf()} (${SeverityLevel[severity]})`, message: v.getMessage() } }); + + this.display.displayLog(getMessage(BundleName.ResultsViewer, 'summary.table.found-results', [ + violations.length, this.countUniqueFiles(violations), parentFolder])); this.display.displayTable(resultRows, TABLE_COLUMNS); } } @@ -171,3 +175,33 @@ function sortViolations(violations: Violation[]): Violation[] { return v1StartColumn - v2StartColumn; }); } + +// TODO: We should update core module to have this function directly on the Violation object and then remove this helper +function getPrimaryLocation(violation: Violation): CodeLocation { + return violation.getCodeLocations()[violation.getPrimaryLocationIndex()]; +} + +/** + * Returns the longest comment parent folder of the file paths provided + * Note that this function assumes that all the paths are indeed files and not folders + */ +export function findLongestCommonParentFolderOf(filePaths: string[]): string { + const roots: string[] = filePaths.map(filePath => path.parse(filePath).root); + const commonRoot: string = (new Set(roots)).size === 1 ? roots[0] : ''; + if (!commonRoot) { + return ''; + } + const commonFolders: string[] = []; + let depth: number = 0; + const explodedPaths: string[][] = filePaths.map(file => + path.dirname(file).slice(commonRoot.length).split(path.sep).filter(v => v.length > 0)); + while(explodedPaths.every(folders => folders.length > depth)) { + const currFolder: string = explodedPaths[0][depth]; + if (explodedPaths.some(folders => folders[depth] !== currFolder)) { + break; + } + commonFolders.push(currFolder); + depth++; + } + return path.join(commonRoot, ... commonFolders); +} diff --git a/test/lib/viewers/ResultsViewer.test.ts b/test/lib/viewers/ResultsViewer.test.ts index a00d9c383..62f04d324 100644 --- a/test/lib/viewers/ResultsViewer.test.ts +++ b/test/lib/viewers/ResultsViewer.test.ts @@ -1,20 +1,26 @@ import path from 'node:path'; import * as fs from 'node:fs/promises'; import ansis from 'ansis'; -import {CodeAnalyzer, CodeAnalyzerConfig, SeverityLevel} from '@salesforce/code-analyzer-core'; -import {Engine, RuleDescription, Violation} from '@salesforce/code-analyzer-engine-api'; - -import {ResultsDetailViewer, ResultsTableViewer} from '../../../src/lib/viewers/ResultsViewer'; +import {CodeAnalyzer, CodeAnalyzerConfig} from '@salesforce/code-analyzer-core'; +import {RuleDescription, Violation} from '@salesforce/code-analyzer-engine-api'; + +import { + findLongestCommonParentFolderOf, + ResultsDetailViewer, + ResultsTableViewer +} from '../../../src/lib/viewers/ResultsViewer'; import {BundleName, getMessage} from '../../../src/lib/messages'; import {DisplayEvent, DisplayEventType, SpyDisplay} from '../../stubs/SpyDisplay'; import {FunctionalStubEnginePlugin1, StubEngine1} from '../../stubs/StubEnginePlugins'; +import {platform} from "node:os"; const PATH_TO_COMPARISON_FILES = path.resolve(__dirname, '..', '..', '..', 'test', 'fixtures', 'comparison-files', 'lib', 'viewers', 'ResultsViewer.test.ts'); -const PATH_TO_SOME_FILE = path.resolve(__dirname, '..', '..', '..', 'test', 'sample-code', 'someFile.cls'); -const PATH_TO_FILE_A = path.resolve(__dirname, '..', '..', '..', 'test', 'sample-code', 'fileA.cls'); -const PATH_TO_FILE_Z = path.resolve(__dirname, '..', '..', '..', 'test', 'sample-code', 'fileZ.cls'); +const PATH_TO_SAMPLE_CODE = path.resolve(__dirname, '..', '..', '..', 'test', 'sample-code'); +const PATH_TO_SOME_FILE = path.resolve(PATH_TO_SAMPLE_CODE, 'someFile.cls'); +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('ResultsViewer implementations', () => { @@ -133,14 +139,6 @@ describe('ResultsViewer implementations', () => { }); describe('ResultsTableViewer', () => { - type TableRow = { - num: number; - location: string; - rule: string; - message: string; - severity: string; - }; - let viewer: ResultsTableViewer; beforeEach(() => { @@ -186,19 +184,10 @@ describe('ResultsViewer implementations', () => { // ==== ASSERTIONS ==== const displayEvents = spyDisplay.getDisplayEvents(); expect(displayEvents).toHaveLength(2); - const expectedTableRows = violations.map((v, idx) => { - return createTableRowExpectation(rule1, v, engine1, idx); - }); - expect(displayEvents).toEqual([{ - type: DisplayEventType.LOG, - data: getMessage(BundleName.ResultsViewer, 'summary.found-results', [10, 1]) - }, { - type: DisplayEventType.TABLE, - data: JSON.stringify({ - columns: ['#', 'Severity', 'Rule', 'Location', 'Message'], - rows: expectedTableRows - }) - }]); + expect(displayEvents[0].type).toEqual(DisplayEventType.LOG); + expect(displayEvents[0].data).toEqual(getMessage(BundleName.ResultsViewer, 'summary.table.found-results', [10, 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"},{"num":5,"location":"someFile.cls:1:1","rule":"stubEngine1:stub1RuleA","severity":"4 (Low)","message":"This is a message"},{"num":6,"location":"someFile.cls:1:1","rule":"stubEngine1:stub1RuleA","severity":"4 (Low)","message":"This is a message"},{"num":7,"location":"someFile.cls:1:1","rule":"stubEngine1:stub1RuleA","severity":"4 (Low)","message":"This is a message"},{"num":8,"location":"someFile.cls:1:1","rule":"stubEngine1:stub1RuleA","severity":"4 (Low)","message":"This is a message"},{"num":9,"location":"someFile.cls:1:1","rule":"stubEngine1:stub1RuleA","severity":"4 (Low)","message":"This is a message"},{"num":10,"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 @@ -225,42 +214,47 @@ describe('ResultsViewer implementations', () => { viewer.view(results); // ==== ASSERTIONS ==== - const expectedRows: TableRow[] = [ - // Violation 4 has the highest severity, so it goes first. - createTableRowExpectation(rule2, violations[3], engine1, 0), - // 1, 2, and 3 are tied for severity and 1 and 2 are tied for highest file, - // but 2 has the earliest location, so it goes next. - createTableRowExpectation(rule1, violations[1], engine1, 1), - // Violation 1 is later in the same high-alphabetical file, so it's next. - createTableRowExpectation(rule1, violations[0], engine1, 2), - // Violation 3 is last. - createTableRowExpectation(rule1, violations[2], engine1, 3), - ] const displayEvents = spyDisplay.getDisplayEvents(); expect(displayEvents).toHaveLength(2); - expect(displayEvents).toEqual([{ - type: DisplayEventType.LOG, - data: getMessage(BundleName.ResultsViewer, 'summary.found-results', [4, 2]) - }, { - type: DisplayEventType.TABLE, - data: JSON.stringify({ - columns: ['#', 'Severity', 'Rule', 'Location', 'Message'], - rows: expectedRows - }) - }]) + 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"}]}`); }); + }); +}); + +describe('Tests for the findLongestCommonParentFolderOf helper function', () => { + it('When a single file is given, then its direct parent is returned', () => { + expect(findLongestCommonParentFolderOf([path.resolve(__dirname,'ResultsViewer.test.ts')])).toEqual(__dirname); + }); - function createTableRowExpectation(rule: RuleDescription, violation: Violation, engine: Engine, index: number): TableRow { - const primaryLocation = violation.codeLocations[violation.primaryLocationIndex]; - return { - num: index + 1, - location: `${primaryLocation.file}:${primaryLocation.startLine}:${primaryLocation.startColumn}`, - rule: `${engine.getName()}:${rule.name}`, - severity: `${rule.severityLevel} (${SeverityLevel[rule.severityLevel]})`, - message: violation.message - }; - } + it('When paths share common parent folders, then longest common folder is returned', () => { + const path1 = path.resolve(__dirname,'..','actions','RunAction.test.ts'); + const path2 = path.resolve(__dirname,'ResultsViewer.test.ts'); + const path3 = path.resolve(__dirname,'..','actions','RulesAction.test.ts'); + expect(findLongestCommonParentFolderOf([path1, path2, path3])).toEqual(path.resolve(__dirname,'..')); }); + + if(platform() === 'win32') { // The following tests only run on Windows machines + it('When paths do not share common root (which can happen on Windows machines), then empty string is returned', () => { + const path1 = 'C:\\Windows\\someFile.txt'; + const path2 = 'D:\\anotherFile.txt'; + expect(findLongestCommonParentFolderOf([path1, path2])).toEqual(''); + }); + + it('When windows paths share common root only, then common root is returned', () => { + const path1 = 'C:\\Windows\\someFile.txt'; + const path2 = 'C:\\Users\\anotherFile.txt'; + expect(findLongestCommonParentFolderOf([path1, path2])).toEqual('C:\\'); + }); + } else { // The following test only runs on Unix-based machines + it('When unix paths share common root only, then common root is returned', () => { + const path1 = '/temp/someFile.txt'; + const path2 = '/Users/anotherFile.txt'; + expect(findLongestCommonParentFolderOf([path1, path2])).toEqual('/'); + }); + } }); function createViolation(ruleName: string, file: string, startLine: number, startColumn: number): Violation {