Skip to content

Commit 7de43b7

Browse files
icmpechoicmpecho
authored
Add suppressRules config (#147)
* Create common VCS engine * Add suppressPattern config, pass config object to gitlab and github directly * Create VCSEngine config interface to avoid accessing unrelated configs from VCSEngine * Move bot logic into AnalyzerBot * Move files around and clean up complex methods * Remove unused imports * Change inheritance into composition * Remove dedicate GitHub and GitLab engine and use VCSEngine directly with different adapter * Remove unrelated changes from refactoring to make it less confusing * Add test for AnalyzerBot * Make analyzer bot injectable to simplify testing * Create testcases for VCSEngine * Add suppressRules option * Add unit test for commentUtil * Make groupComments function easier to understand * Support rule suppression in commentUtil * Fix errors because of additional field * Pass through suppressRules config from AnalyzerBot * Update readme
1 parent 1196b69 commit 7de43b7

File tree

13 files changed

+237
-42
lines changed

13 files changed

+237
-42
lines changed

‎README.md‎

Lines changed: 19 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -80,24 +80,25 @@ Will do the same thing.
8080

8181
> Warning: If both config file and options are supplied, options will override config in file.
8282
83-
| Option | Required | Value | Description |
84-
|---------------------------|---------------------------|------------------------------|-------------------------------------------------------|
85-
| `vcs` / `-g` | when `dryRun` is `false` | `github` or `gitlab` | |
86-
| | | | |
87-
| `githubRepoUrl` | when `vcs` is `github` | | Repository's HTTPS URL |
88-
| `githubPr` | when `vcs` is `github` | | Pull request ID |
89-
| `githubToken` | when `vcs` is `github` | | Personal Access Token |
90-
| | | | |
91-
| `gitlabHost` | when `vcs` is `gitlab` | `https://gitlab.company.com` | GitLab Server (Could also be `https://gitlab.com`) |
92-
| `gitlabProjectId` | when `vcs` is `gitlab` | | Project ID |
93-
| `gitlabMrIid` | when `vcs` is `gitlab` | | MergeRequest IID (not to be confused with ID) |
94-
| `gitlabToken` | when `vcs` is `gitlab` | | Access Token |
95-
| | | | |
96-
| `buildLogFile` / `-f` | yes, repeatable | | Read below |
97-
| `output` / `-o` | no | | CodeCoach parsed output for debugging |
98-
| `removeOldComment` / `-r` | no | `true` or `false` | Remove CodeCoach's old comments before adding new one |
99-
| `failOnWarnings` | no | `true` or `false` | Fail the job when warnings are found |
100-
| `dryRun` | no | `true` or `false` | Running CodeCoach without reporting to VCS |
83+
| Option | Required | Value | Description |
84+
|---------------------------|---------------------------|------------------------------|----------------------------------------------------------------------------------------|
85+
| `vcs` / `-g` | when `dryRun` is `false` | `github` or `gitlab` | |
86+
| | | | |
87+
| `githubRepoUrl` | when `vcs` is `github` | | Repository's HTTPS URL |
88+
| `githubPr` | when `vcs` is `github` | | Pull request ID |
89+
| `githubToken` | when `vcs` is `github` | | Personal Access Token |
90+
| | | | |
91+
| `gitlabHost` | when `vcs` is `gitlab` | `https://gitlab.company.com` | GitLab Server (Could also be `https://gitlab.com`) |
92+
| `gitlabProjectId` | when `vcs` is `gitlab` | | Project ID |
93+
| `gitlabMrIid` | when `vcs` is `gitlab` | | MergeRequest IID (not to be confused with ID) |
94+
| `gitlabToken` | when `vcs` is `gitlab` | | Access Token |
95+
| | | | |
96+
| `buildLogFile` / `-f` | yes, repeatable | | Read below |
97+
| `output` / `-o` | no | | CodeCoach parsed output for debugging |
98+
| `removeOldComment` / `-r` | no | `true` or `false` | Remove CodeCoach's old comments before adding new one |
99+
| `failOnWarnings` | no | `true` or `false` | Fail the job when warnings are found |
100+
| `dryRun` | no | `true` or `false` | Running CodeCoach without reporting to VCS |
101+
| `suppressRules` | no | `rule-id-1` `rule-id-2` | Rule IDs that CodeCoach will still comment but no longer treated as errors or warnings |
101102

102103

103104
##### `--buildLogFile` or `-f`

‎sample/config/gitlab.json‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,5 +6,6 @@
66
"gitlabToken": "mockGitLabToken",
77
"buildLogFile": "dotnetbuild;./sample/dotnetbuild/build.content;/repo/src",
88
"output": "./tmp/out.json",
9-
"removeOldComment": true
9+
"removeOldComment": true,
10+
"suppressRules": ["RULE1", "RULE2"]
1011
}

‎src/AnalyzerBot/@interfaces/AnalyzerBotConfig.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
11
export interface AnalyzerBotConfig {
22
failOnWarnings: boolean;
3+
suppressRules: string[];
34
}

‎src/AnalyzerBot/@types/CommentTypes.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ export interface Comment {
44
text: string;
55
errors: number;
66
warnings: number;
7+
suppresses: number;
78
}
89

910
export interface CommentStructure {

‎src/AnalyzerBot/AnalyzerBot.spec.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import { MessageUtil } from './utils/message.util';
1414

1515
const config: AnalyzerBotConfig = {
1616
failOnWarnings: false,
17+
suppressRules: [],
1718
};
1819
describe('AnalyzerBot', () => {
1920
const logs = [touchFileError, touchFileWarning, untouchedError, untouchedWarning];
@@ -41,6 +42,7 @@ describe('AnalyzerBot', () => {
4142
) + ' \n',
4243
errors: 1,
4344
warnings: 0,
45+
suppresses: 0,
4446
},
4547
{
4648
file: mockTouchFile,
@@ -52,6 +54,7 @@ describe('AnalyzerBot', () => {
5254
) + ' \n',
5355
errors: 0,
5456
warnings: 1,
57+
suppresses: 0,
5558
},
5659
]);
5760
});

‎src/AnalyzerBot/AnalyzerBot.ts‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,11 +15,14 @@ export class AnalyzerBot implements IAnalyzerBot {
1515

1616
constructor(private readonly config: AnalyzerBotConfig) {}
1717

18-
analyze(logs: LogType[], touchedDiff: Diff[]) {
18+
analyze(logs: LogType[], touchedDiff: Diff[]): void {
1919
this.touchedFileLog = logs
2020
.filter(onlySeverity(LogSeverity.error, LogSeverity.warning))
2121
.filter(onlyIn(touchedDiff));
22-
this.comments = groupComments(this.touchedFileLog);
22+
this.comments = groupComments(
23+
this.touchedFileLog,
24+
new Set(this.config.suppressRules),
25+
);
2326
this.nError = this.comments.reduce((sum, comment) => sum + comment.errors, 0);
2427
this.nWarning = this.comments.reduce((sum, comment) => sum + comment.warnings, 0);
2528
}

‎src/AnalyzerBot/utils/commentUtil.spec.ts‎

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,108 @@
1+
import { LogSeverity, LogType } from '../../Parser';
2+
import {
3+
file1TouchLine,
4+
file2TouchLine,
5+
mockTouchFile,
6+
touchFileError,
7+
touchFileWarning,
8+
} from '../../Provider/mockData';
9+
import { groupComments } from './commentUtil';
10+
11+
describe('groupComments', () => {
12+
const logs: LogType[] = [touchFileError, touchFileWarning];
13+
14+
it('returns comments based on lint logs', () => {
15+
const comments = groupComments(logs, new Set<string>());
16+
expect(comments).toEqual([
17+
{
18+
file: mockTouchFile,
19+
line: file1TouchLine,
20+
errors: 1,
21+
warnings: 0,
22+
suppresses: 0,
23+
text: ':rotating_light: msg1' + ' \n',
24+
},
25+
{
26+
file: mockTouchFile,
27+
line: file2TouchLine,
28+
errors: 0,
29+
warnings: 1,
30+
suppresses: 0,
31+
text: ':warning: msg3' + ' \n',
32+
},
33+
]);
34+
});
35+
36+
it('group multiple logs on the same line to the same comment', () => {
37+
const comments = groupComments(
38+
[
39+
...logs,
40+
{
41+
...touchFileError,
42+
msg: 'additional warning',
43+
severity: LogSeverity.warning,
44+
lineOffset: 33,
45+
},
46+
],
47+
new Set<string>(),
48+
);
49+
50+
expect(comments).toEqual([
51+
{
52+
file: mockTouchFile,
53+
line: file1TouchLine,
54+
errors: 1,
55+
warnings: 1,
56+
suppresses: 0,
57+
text: ':rotating_light: msg1' + ' \n' + ':warning: additional warning' + ' \n',
58+
},
59+
{
60+
file: mockTouchFile,
61+
line: file2TouchLine,
62+
errors: 0,
63+
warnings: 1,
64+
suppresses: 0,
65+
text: ':warning: msg3' + ' \n',
66+
},
67+
]);
68+
});
69+
70+
it('suppress errors and warnings according to provided suppressRules', () => {
71+
const comments = groupComments(
72+
[
73+
...logs,
74+
{
75+
...touchFileError,
76+
msg: 'additional warning',
77+
severity: LogSeverity.warning,
78+
lineOffset: 33,
79+
ruleId: 'UNIMPORTANT_RULE2',
80+
},
81+
],
82+
new Set(['UNIMPORTANT_RULE1', 'UNIMPORTANT_RULE2']),
83+
);
84+
85+
expect(comments).toEqual([
86+
{
87+
file: mockTouchFile,
88+
line: file1TouchLine,
89+
errors: 1,
90+
warnings: 0,
91+
suppresses: 1,
92+
text:
93+
':rotating_light: msg1' +
94+
' \n' +
95+
':warning: (SUPPRESSED) additional warning' +
96+
' \n',
97+
},
98+
{
99+
file: mockTouchFile,
100+
line: file2TouchLine,
101+
errors: 0,
102+
warnings: 1,
103+
suppresses: 0,
104+
text: ':warning: msg3' + ' \n',
105+
},
106+
]);
107+
});
108+
});

‎src/AnalyzerBot/utils/commentUtil.ts‎

Lines changed: 79 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,32 +1,91 @@
11
import { LogSeverity, LogType } from '../../Parser';
2-
import { Comment, CommentFileStructure, CommentStructure } from '../@types/CommentTypes';
2+
import { Comment, CommentStructure } from '../@types/CommentTypes';
33
import { MessageUtil } from './message.util';
44

5-
export function groupComments(logs: LogType[]): Comment[] {
6-
const commentMap = logs.reduce((map: CommentStructure, log) => {
7-
const { source: file, line, severity, msg } = log;
8-
const text = MessageUtil.createMessageWithEmoji(msg, severity);
5+
export function groupComments(logs: LogType[], suppressRules: Set<string>): Comment[] {
6+
const commentMap = logs.reduce((state: CommentStructure, log) => {
7+
const { source: file, line } = log;
98

10-
if (!line) return map;
9+
if (!line) return state;
1110

12-
const currentWarnings = map?.[file]?.[line]?.warnings ?? 0;
13-
const currentErrors = map?.[file]?.[line]?.errors ?? 0;
14-
const currentText = map?.[file]?.[line]?.text ?? '';
11+
const currentComment = getOrInitComment(state, file, line);
12+
const updatedComment = updateComment(currentComment, log, suppressRules);
13+
return updateCommentStructure(state, updatedComment);
14+
}, {});
15+
16+
return Object.values(commentMap).flatMap((file) => Object.values(file));
17+
}
1518

16-
const nextObject: Comment = {
17-
text: `${currentText}${text} \n`,
18-
errors: currentErrors + (severity === LogSeverity.error ? 1 : 0),
19-
warnings: currentWarnings + (severity === LogSeverity.warning ? 1 : 0),
19+
function getOrInitComment(map: CommentStructure, file: string, line: number): Comment {
20+
return (
21+
map?.[file]?.[line] ?? {
22+
text: '',
23+
errors: 0,
24+
warnings: 0,
25+
suppresses: 0,
2026
file,
2127
line,
22-
};
28+
}
29+
);
30+
}
2331

24-
const fileCommentUpdater: CommentFileStructure = { [line]: nextObject };
25-
const updatedFileComment = Object.assign({}, map?.[file], fileCommentUpdater);
32+
function buildText(currentComment: Comment, log: LogType, isSuppressed: boolean): string {
33+
const { severity, msg } = log;
34+
const { text: currentText } = currentComment;
35+
const msgWithSuppression = isSuppressed ? `(SUPPRESSED) ${msg}` : msg;
36+
const text = MessageUtil.createMessageWithEmoji(msgWithSuppression, severity);
37+
return `${currentText}${text} \n`;
38+
}
2639

27-
const mapUpdater: CommentStructure = { [file]: updatedFileComment };
28-
return Object.assign({}, map, mapUpdater);
29-
}, {});
40+
function calculateErrors(
41+
currentComment: Comment,
42+
log: LogType,
43+
isSuppressed: boolean,
44+
): number {
45+
if (isSuppressed) return currentComment.errors;
46+
const { severity } = log;
47+
return currentComment.errors + (severity === LogSeverity.error ? 1 : 0);
48+
}
3049

31-
return Object.values(commentMap).flatMap((file) => Object.values(file));
50+
function calculateWarnings(
51+
currentComment: Comment,
52+
log: LogType,
53+
isSuppressed: boolean,
54+
): number {
55+
if (isSuppressed) return currentComment.warnings;
56+
const { severity } = log;
57+
return currentComment.warnings + (severity === LogSeverity.warning ? 1 : 0);
58+
}
59+
60+
function calculateSuppresses(currentComment: Comment, isSuppressed: boolean): number {
61+
return currentComment.suppresses + (isSuppressed ? 1 : 0);
62+
}
63+
64+
function updateComment(
65+
currentComment: Comment,
66+
log: LogType,
67+
suppressRules: Set<string>,
68+
): Comment {
69+
const isSuppressed = suppressRules.has(log.ruleId);
70+
return {
71+
text: buildText(currentComment, log, isSuppressed),
72+
errors: calculateErrors(currentComment, log, isSuppressed),
73+
warnings: calculateWarnings(currentComment, log, isSuppressed),
74+
suppresses: calculateSuppresses(currentComment, isSuppressed),
75+
file: currentComment.file,
76+
line: currentComment.line,
77+
};
78+
}
79+
80+
function updateCommentStructure(
81+
currentStructure: CommentStructure,
82+
updatedComment: Comment,
83+
): CommentStructure {
84+
return {
85+
...currentStructure,
86+
[updatedComment.file]: {
87+
...currentStructure?.[updatedComment.file],
88+
[updatedComment.line]: updatedComment,
89+
},
90+
};
3291
}

‎src/Config/@types/configArgument.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,5 +13,6 @@ export type ConfigArgument = {
1313
output: string; // =logFilePath
1414
removeOldComment: boolean;
1515
failOnWarnings: boolean;
16+
suppressRules: string[];
1617
dryRun: boolean;
1718
};

‎src/Config/Config.spec.ts‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,9 @@ const GITLAB_ENV_ARGS = [
4040
`-f=${mockBuildLogFile}`,
4141
`-o=${mockOutput}`,
4242
'--failOnWarnings',
43+
'--suppressRules',
44+
'RULE1',
45+
'RULE2',
4346
];
4447

4548
const GITLAB_FILE_ARGS = ['node', 'app.ts', '--config=sample/config/gitlab.json'];
@@ -75,6 +78,7 @@ describe('Config parsing Test', () => {
7578
expect(config.githubToken).toBe(mockGitHubToken);
7679
expect(config.removeOldComment).toBe(true);
7780
expect(config.failOnWarnings).toBe(false);
81+
expect(config.suppressRules).toEqual([]);
7882

7983
validateBuildLog(config.buildLogFile);
8084
});
@@ -88,6 +92,7 @@ describe('Config parsing Test', () => {
8892
expect(config.githubToken).toBe(mockGitHubToken);
8993
expect(config.removeOldComment).toBe(false);
9094
expect(config.failOnWarnings).toBe(false);
95+
expect(config.suppressRules).toEqual([]);
9196

9297
validateBuildLog(config.buildLogFile);
9398
});
@@ -102,6 +107,7 @@ describe('Config parsing Test', () => {
102107
expect(config.gitlabToken).toBe(mockGitLabToken);
103108
expect(config.removeOldComment).toBe(false);
104109
expect(config.failOnWarnings).toBe(true);
110+
expect(config.suppressRules).toEqual(['RULE1', 'RULE2']);
105111

106112
validateBuildLog(config.buildLogFile);
107113
});
@@ -116,6 +122,7 @@ describe('Config parsing Test', () => {
116122
expect(config.gitlabToken).toBe(mockGitLabToken);
117123
expect(config.removeOldComment).toBe(true);
118124
expect(config.failOnWarnings).toBe(false);
125+
expect(config.suppressRules).toEqual(['RULE1', 'RULE2']);
119126

120127
validateBuildLog(config.buildLogFile);
121128
});

0 commit comments

Comments
 (0)