Skip to content

Commit b9f8764

Browse files
stephen-carter-at-sfstephen-carter-at-sf
authored
Merge pull request #1678 from forcedotcom/sc/polishDetailView
CHANGE: @W-17272495@: Polish detail view output
2 parents 0b225e1 + c294715 commit b9f8764

File tree

6 files changed

+112
-82
lines changed

6 files changed

+112
-82
lines changed

‎messages/results-viewer.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22

33
%d. %s
44

5-
# summary.table.results-relative-to
5+
# summary.shared.results-relative-to
66

77
Violation file paths relative to '%s'.
88

‎src/lib/viewers/ResultsViewer.ts‎

Lines changed: 47 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -43,17 +43,18 @@ abstract class AbstractResultsDisplayer implements ResultsViewer {
4343
export class ResultsDetailDisplayer extends AbstractResultsDisplayer {
4444
protected _view(results: RunResults): void {
4545
const violations = sortViolations(results.getViolations());
46-
47-
this.displayDetails(violations);
46+
const runDir: string = results.getRunDirectory();
47+
this.display.displayLog(getMessage(BundleName.ResultsViewer, 'summary.shared.results-relative-to', [runDir]) + "\n");
48+
this.displayDetails(violations, runDir);
4849
}
4950

50-
private displayDetails(violations: Violation[]): void {
51+
private displayDetails(violations: Violation[], runDir: string): void {
5152
const styledViolations: string[] = violations
52-
.map((violation, idx) => this.styleViolation(violation, idx));
53+
.map((violation, idx) => this.styleViolation(violation, idx, runDir));
5354
this.display.displayLog(styledViolations.join('\n\n'));
5455
}
5556

56-
private styleViolation(violation: Violation, idx: number): string {
57+
private styleViolation(violation: Violation, idx: number, runDir: string): string {
5758
const rule = violation.getRule();
5859
const sev = rule.getSeverityLevel();
5960

@@ -62,41 +63,52 @@ export class ResultsDetailDisplayer extends AbstractResultsDisplayer {
6263
'summary.detail.violation-header',
6364
[idx + 1, rule.getName()]
6465
);
65-
if (violation.getCodeLocations().length > 1) {
66-
const body = {
67-
severity: `${sev.valueOf()} (${SeverityLevel[sev]})`,
68-
engine: rule.getEngineName(),
69-
message: violation.getMessage(),
70-
locations: stringifyLocations(violation.getCodeLocations(), violation.getPrimaryLocationIndex()),
71-
resources: violation.getResourceUrls().join(',')
72-
};
73-
const keys = ['severity', 'engine', 'message', 'locations', 'resources'];
74-
return toStyledHeaderAndBody(header, body, keys);
75-
} else {
76-
const body = {
77-
severity: `${sev.valueOf()} (${SeverityLevel[sev]})`,
78-
engine: rule.getEngineName(),
79-
message: violation.getMessage(),
80-
location: stringifyLocations(violation.getCodeLocations())[0],
81-
resources: violation.getResourceUrls().join(',')
82-
};
83-
const keys = ['severity', 'engine', 'message', 'location', 'resources'];
84-
return toStyledHeaderAndBody(header, body, keys);
66+
const body = {
67+
severity: `${sev.valueOf()} (${SeverityLevel[sev]})`,
68+
engine: rule.getEngineName(),
69+
message: violation.getMessage()
70+
}
71+
const keys: string[] = ['severity', 'engine', 'message'];
72+
if (violation.getCodeLocations().length == 1) {
73+
body['location'] = stringifyLocation(violation.getCodeLocations()[0], false, runDir);
74+
keys.push('location');
75+
} else if (violation.getCodeLocations().length > 1) {
76+
body['locations'] = stringifyLocations(violation.getCodeLocations(), violation.getPrimaryLocationIndex(), runDir);
77+
keys.push('locations');
8578
}
79+
if (violation.getResourceUrls().length == 1) {
80+
body['resource'] = violation.getResourceUrls()[0];
81+
keys.push('resource');
82+
} else if (violation.getResourceUrls().length > 1) {
83+
body['resources'] = violation.getResourceUrls();
84+
keys.push('resources');
85+
}
86+
return toStyledHeaderAndBody(header, body, keys);
8687
}
8788
}
8889

89-
function stringifyLocations(codeLocations: CodeLocation[], primaryIndex?: number): string[] {
90-
const locationStrings: string[] = [];
90+
function stringifyLocations(codeLocations: CodeLocation[], primaryIndex: number, runDir: string): string[] {
91+
return codeLocations.map((loc, idx) =>
92+
stringifyLocation(loc, codeLocations.length > 1 && primaryIndex === idx, runDir));
93+
}
9194

92-
codeLocations.forEach((loc, idx) => {
93-
const commentPortion: string = loc.getComment() ? ` ${loc.getComment()}` : '';
94-
const locationString: string = `${loc.getFile()}:${loc.getStartLine()}:${loc.getStartColumn()}${commentPortion}`;
95-
const mainPortion: string = primaryIndex != null && primaryIndex === idx ? '(main) ' : '';
96-
locationStrings.push(`${mainPortion}${locationString}`);
97-
});
95+
function stringifyLocation(loc: CodeLocation, displayMain: boolean, runDir: string): string {
96+
const mainPortion: string = displayMain ? '(main) ' : '';
97+
let filePortion: string | undefined = loc.getFile();
98+
if (filePortion && filePortion.startsWith(runDir)) {
99+
filePortion = filePortion.slice(runDir.length);
100+
}
101+
let rangePortion: string = '';
102+
if (loc.getStartLine()) {
103+
rangePortion += ` (${loc.getStartLine()}:${loc.getStartColumn() || 1}`;
104+
if (loc.getEndLine()) {
105+
rangePortion += `-${loc.getEndLine()}:${loc.getEndColumn() || 1}`;
106+
}
107+
rangePortion += ')';
108+
}
109+
const commentPortion: string = loc.getComment() ? ` "${loc.getComment()}"` : '';
98110

99-
return locationStrings;
111+
return `${mainPortion}${filePortion}${rangePortion}${commentPortion}`;
100112
}
101113

102114
type ResultRow = {
@@ -145,7 +157,7 @@ export class ResultsTableDisplayer extends AbstractResultsDisplayer {
145157
}
146158
});
147159

148-
this.display.displayLog(getMessage(BundleName.ResultsViewer, 'summary.table.results-relative-to', [parentFolder]));
160+
this.display.displayLog(getMessage(BundleName.ResultsViewer, 'summary.shared.results-relative-to', [parentFolder]));
149161
this.display.displayTable(resultRows, TABLE_COLUMNS);
150162
}
151163
}

‎test/fixtures/comparison-files/lib/viewers/ResultsViewer.test.ts/four-identical-violations-details.txt‎

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,28 +1,38 @@
11

2+
Violation file paths relative to '{{RUNDIR}}'.
3+
24
=== 1. stub1RuleA
35
severity: 4 (Low)
46
engine: stubEngine1
57
message: This is a message
6-
location: __PATH_TO_SOME_FILE__:1:1
7-
resources: https://example.com/stub1RuleA
8+
location: test{{PATHSEP}}sample-code{{PATHSEP}}someFile.cls (1:1)
9+
resources:
10+
https://example.com/stub1RuleA
11+
https://violation_specific.url
812

913
=== 2. stub1RuleA
1014
severity: 4 (Low)
1115
engine: stubEngine1
1216
message: This is a message
13-
location: __PATH_TO_SOME_FILE__:1:1
14-
resources: https://example.com/stub1RuleA
17+
location: test{{PATHSEP}}sample-code{{PATHSEP}}someFile.cls (1:1)
18+
resources:
19+
https://example.com/stub1RuleA
20+
https://violation_specific.url
1521

1622
=== 3. stub1RuleA
1723
severity: 4 (Low)
1824
engine: stubEngine1
1925
message: This is a message
20-
location: __PATH_TO_SOME_FILE__:1:1
21-
resources: https://example.com/stub1RuleA
26+
location: test{{PATHSEP}}sample-code{{PATHSEP}}someFile.cls (1:1)
27+
resources:
28+
https://example.com/stub1RuleA
29+
https://violation_specific.url
2230

2331
=== 4. stub1RuleA
2432
severity: 4 (Low)
2533
engine: stubEngine1
2634
message: This is a message
27-
location: __PATH_TO_SOME_FILE__:1:1
28-
resources: https://example.com/stub1RuleA
35+
location: test{{PATHSEP}}sample-code{{PATHSEP}}someFile.cls (1:1)
36+
resources:
37+
https://example.com/stub1RuleA
38+
https://violation_specific.url

‎test/fixtures/comparison-files/lib/viewers/ResultsViewer.test.ts/four-unique-violations-details.txt‎

Lines changed: 22 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,28 +1,30 @@
11

2+
Violation file paths relative to '{{RUNDIR}}'.
3+
24
=== 1. stub1RuleB
3-
severity: 2 (High)
4-
engine: stubEngine1
5-
message: This is a message
6-
location: __PATH_TO_FILE_Z__:20:1
7-
resources: https://example.com/stub1RuleB
5+
severity: 2 (High)
6+
engine: stubEngine1
7+
message: This is a message
8+
location: test{{PATHSEP}}sample-code{{PATHSEP}}fileZ.cls (20:1)
9+
resource: https://example.com/stub1RuleB
810

911
=== 2. stub1RuleA
10-
severity: 4 (Low)
11-
engine: stubEngine1
12-
message: This is a message
13-
location: __PATH_TO_FILE_A__:1:1
14-
resources: https://example.com/stub1RuleA
12+
severity: 4 (Low)
13+
engine: stubEngine1
14+
message: This is a message
15+
location: test{{PATHSEP}}sample-code{{PATHSEP}}fileA.cls (1:1)
16+
resource: https://example.com/stub1RuleA
1517

1618
=== 3. stub1RuleA
17-
severity: 4 (Low)
18-
engine: stubEngine1
19-
message: This is a message
20-
location: __PATH_TO_FILE_A__:20:1
21-
resources: https://example.com/stub1RuleA
19+
severity: 4 (Low)
20+
engine: stubEngine1
21+
message: This is a message
22+
location: test{{PATHSEP}}sample-code{{PATHSEP}}fileA.cls (20:1)
23+
resource: https://example.com/stub1RuleA
2224

2325
=== 4. stub1RuleA
24-
severity: 4 (Low)
25-
engine: stubEngine1
26-
message: This is a message
27-
location: __PATH_TO_FILE_Z__:1:1
28-
resources: https://example.com/stub1RuleA
26+
severity: 4 (Low)
27+
engine: stubEngine1
28+
message: This is a message
29+
location: test{{PATHSEP}}sample-code{{PATHSEP}}fileZ.cls (1:1)
30+
resource: https://example.com/stub1RuleA

‎test/fixtures/comparison-files/lib/viewers/ResultsViewer.test.ts/one-multilocation-violation-details.txt‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,12 @@
11

2+
Violation file paths relative to '{{RUNDIR}}'.
3+
24
=== 1. stub1RuleA
35
severity: 4 (Low)
46
engine: stubEngine1
57
message: This is a message
68
locations:
7-
__PATH_TO_FILE_A__:20:1
8-
(main) __PATH_TO_FILE_Z__:2:1 This is a comment at Location 2
9-
__PATH_TO_FILE_A__:1:1 This is a comment at Location 3
10-
resources: https://example.com/stub1RuleA
9+
test{{PATHSEP}}sample-code{{PATHSEP}}fileA.cls (20:1)
10+
(main) test{{PATHSEP}}sample-code{{PATHSEP}}fileZ.cls (2:1) "This is a comment at Location 2"
11+
test{{PATHSEP}}sample-code{{PATHSEP}}fileA.cls (1:1-3:1) "This is a comment at Location 3"
12+
resource: https://example.com/stub1RuleA

‎test/lib/viewers/ResultsViewer.test.ts‎

Lines changed: 18 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ describe('ResultsViewer implementations', () => {
6969
// ==== TEST SETUP ====
7070
// This test doesn't care about sorting, so just assign our engine several copies of the same violation.
7171
const violations: Violation[] = repeatViolation(
72-
createViolation(rule1.name, PATH_TO_SOME_FILE, 1, 1),
72+
createViolation(rule1.name, PATH_TO_SOME_FILE, 1, 1, ['https://violation_specific.url']),
7373
4);
7474
engine1.resultsToReturn = {violations};
7575
const workspace = await codeAnalyzerCore.createWorkspace([PATH_TO_SOME_FILE]);
@@ -90,8 +90,9 @@ describe('ResultsViewer implementations', () => {
9090
// Rip off all of ansis's styling, so we're just comparing plain text.
9191
const actualEventText = ansis.strip(actualDisplayEvents.map(e => e.data).join('\n'));
9292
const expectedViolationDetails = (await readComparisonFile('four-identical-violations-details.txt'))
93-
.replace(/__PATH_TO_SOME_FILE__/g, PATH_TO_SOME_FILE);
94-
expect(actualEventText).toContain(expectedViolationDetails);
93+
.replaceAll("{{PATHSEP}}", path.sep)
94+
.replace("{{RUNDIR}}", results.getRunDirectory());
95+
expect(actualEventText).toEqual(expectedViolationDetails);
9596
});
9697

9798
// The reasoning behind this sorting order is so that the Detail view can function as a "show me the N most
@@ -128,9 +129,9 @@ describe('ResultsViewer implementations', () => {
128129
// Rip off all of ansis's styling, so we're just comparing plain text.
129130
const actualEventText = ansis.strip(actualDisplayEvents.map(e => e.data).join('\n'));
130131
const expectedViolationDetails = (await readComparisonFile('four-unique-violations-details.txt'))
131-
.replace(/__PATH_TO_FILE_A__/g, PATH_TO_FILE_A)
132-
.replace(/__PATH_TO_FILE_Z__/g, PATH_TO_FILE_Z);
133-
expect(actualEventText).toContain(expectedViolationDetails);
132+
.replaceAll("{{PATHSEP}}", path.sep)
133+
.replace("{{RUNDIR}}", results.getRunDirectory());
134+
expect(actualEventText).toEqual(expectedViolationDetails);
134135
});
135136

136137
it('Multi-location violations are correctly displayed', async () => {
@@ -146,11 +147,13 @@ describe('ResultsViewer implementations', () => {
146147
file: PATH_TO_FILE_Z,
147148
startLine: 2,
148149
startColumn: 1,
149-
comment: 'This is a comment at Location 2'
150+
endColumn: 7,
151+
comment: 'This is a comment at Location 2',
150152
}, {
151153
file: PATH_TO_FILE_A,
152154
startLine: 1,
153155
startColumn: 1,
156+
endLine: 3,
154157
comment: 'This is a comment at Location 3'
155158
});
156159
// Declare the second location to be the primary.
@@ -175,9 +178,9 @@ describe('ResultsViewer implementations', () => {
175178
// Rip off all of ansis's styling, so we're just comparing plain text.
176179
const actualEventText = ansis.strip(actualDisplayEvents.map(e => e.data).join('\n'));
177180
const expectedViolationDetails = (await readComparisonFile('one-multilocation-violation-details.txt'))
178-
.replace(/__PATH_TO_FILE_A__/g, PATH_TO_FILE_A)
179-
.replace(/__PATH_TO_FILE_Z__/g, PATH_TO_FILE_Z);
180-
expect(actualEventText).toContain(expectedViolationDetails);
181+
.replaceAll("{{PATHSEP}}", path.sep)
182+
.replace("{{RUNDIR}}", results.getRunDirectory());
183+
expect(actualEventText).toEqual(expectedViolationDetails);
181184
})
182185
});
183186

@@ -227,7 +230,7 @@ describe('ResultsViewer implementations', () => {
227230
expect(displayEvents[0].type).toEqual(DisplayEventType.LOG);
228231
expect(displayEvents[0].data).toEqual('');
229232
expect(displayEvents[1].type).toEqual(DisplayEventType.LOG);
230-
expect(displayEvents[1].data).toEqual(getMessage(BundleName.ResultsViewer, 'summary.table.results-relative-to', [PATH_TO_SAMPLE_CODE]));
233+
expect(displayEvents[1].data).toEqual(getMessage(BundleName.ResultsViewer, 'summary.shared.results-relative-to', [PATH_TO_SAMPLE_CODE]));
231234
expect(displayEvents[2].type).toEqual(DisplayEventType.TABLE);
232235
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"}]}`);
233236
});
@@ -261,7 +264,7 @@ describe('ResultsViewer implementations', () => {
261264
expect(displayEvents[0].type).toEqual(DisplayEventType.LOG);
262265
expect(displayEvents[0].data).toEqual('');
263266
expect(displayEvents[1].type).toEqual(DisplayEventType.LOG);
264-
expect(displayEvents[1].data).toEqual(getMessage(BundleName.ResultsViewer, 'summary.table.results-relative-to', [PATH_TO_SAMPLE_CODE]));
267+
expect(displayEvents[1].data).toEqual(getMessage(BundleName.ResultsViewer, 'summary.shared.results-relative-to', [PATH_TO_SAMPLE_CODE]));
265268
expect(displayEvents[2].type).toEqual(DisplayEventType.TABLE);
266269
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"}]}`);
267270
});
@@ -301,7 +304,7 @@ describe('Tests for the findLongestCommonParentFolderOf helper function', () =>
301304
}
302305
});
303306

304-
function createViolation(ruleName: string, file: string, startLine: number, startColumn: number): Violation {
307+
function createViolation(ruleName: string, file: string, startLine: number, startColumn: number, resourceUrls: string[] = []): Violation {
305308
return {
306309
ruleName,
307310
message: 'This is a message',
@@ -310,7 +313,8 @@ function createViolation(ruleName: string, file: string, startLine: number, star
310313
startLine,
311314
startColumn
312315
}],
313-
primaryLocationIndex: 0
316+
primaryLocationIndex: 0,
317+
resourceUrls: resourceUrls
314318
};
315319
}
316320

0 commit comments

Comments
 (0)