Skip to content

Commit

Permalink
Merge pull request #1598 from forcedotcom/sc/ImproveTableView
Browse files Browse the repository at this point in the history 
CHANGE: @W-16569797@: Improve table view by displaying relative paths whenever possible
  • Loading branch information
@stephen-carter-at-sf
stephen-carter-at-sf authored Aug 23, 2024
2 parents 112ea73 + c4679a9 commit 94c1842
Show file tree
Hide file tree
Showing 4 changed files with 107 additions and 74 deletions.
6 changes: 5 additions & 1 deletion messages/results-viewer.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

Found 0 violations.

# summary.found-results
# summary.detail.found-results

Found %d violation(s) across %d file(s):

Expand All @@ -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

#
Expand Down
3 changes: 2 additions & 1 deletion package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
}
}
60 changes: 47 additions & 13 deletions src/lib/viewers/ResultsViewer.ts
View file
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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<string> = new Set();
violations.forEach(v => {
const primaryLocation = v.getCodeLocations()[v.getPrimaryLocationIndex()];
Expand All @@ -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);
Expand Down Expand Up @@ -125,18 +121,26 @@ const TABLE_COLUMNS: Ux.Table.Columns<ResultRow> = {

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);
}
}
Expand Down Expand Up @@ -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);
}
112 changes: 53 additions & 59 deletions test/lib/viewers/ResultsViewer.test.ts
View file
Original file line number Diff line number Diff line change
@@ -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', () => {

Expand Down Expand Up @@ -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(() => {
Expand Down Expand Up @@ -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
Expand All @@ -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 {
Expand Down

0 comments on commit 94c1842

Please sign in to comment.