Skip to content

Commit 23909bc

Browse files
icmpechoicmpecho
authored
Support Regexp in rule suppression (#151)
* 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 * Support regexp in rule suppression * Fix misleading regexp * Make suppressRules non-nullable
1 parent 0894d4a commit 23909bc

File tree

7 files changed

+73
-30
lines changed

7 files changed

+73
-30
lines changed

‎README.md

+19-19
Original file line numberDiff line numberDiff line change
@@ -81,25 +81,25 @@ Will do the same thing.
8181

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

104104

105105
##### `--buildLogFile` or `-f`

‎src/AnalyzerBot/AnalyzerBot.ts

+1-4
Original file line numberDiff line numberDiff line change
@@ -19,10 +19,7 @@ export class AnalyzerBot implements IAnalyzerBot {
1919
this.touchedFileLog = logs
2020
.filter(onlySeverity(LogSeverity.error, LogSeverity.warning))
2121
.filter(onlyIn(touchedDiff));
22-
this.comments = groupComments(
23-
this.touchedFileLog,
24-
new Set(this.config.suppressRules),
25-
);
22+
this.comments = groupComments(this.touchedFileLog, this.config.suppressRules);
2623
this.nError = this.comments.reduce((sum, comment) => sum + comment.errors, 0);
2724
this.nWarning = this.comments.reduce((sum, comment) => sum + comment.warnings, 0);
2825
}

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

+42-3
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ describe('groupComments', () => {
1212
const logs: LogType[] = [touchFileError, touchFileWarning];
1313

1414
it('returns comments based on lint logs', () => {
15-
const comments = groupComments(logs, new Set<string>());
15+
const comments = groupComments(logs, []);
1616
expect(comments).toEqual([
1717
{
1818
file: mockTouchFile,
@@ -44,7 +44,7 @@ describe('groupComments', () => {
4444
lineOffset: 33,
4545
},
4646
],
47-
new Set<string>(),
47+
[],
4848
);
4949

5050
expect(comments).toEqual([
@@ -79,7 +79,46 @@ describe('groupComments', () => {
7979
ruleId: 'UNIMPORTANT_RULE2',
8080
},
8181
],
82-
new Set(['UNIMPORTANT_RULE1', 'UNIMPORTANT_RULE2']),
82+
['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+
109+
it('support regexp in suppressRules', () => {
110+
const comments = groupComments(
111+
[
112+
...logs,
113+
{
114+
...touchFileError,
115+
msg: 'additional warning',
116+
severity: LogSeverity.warning,
117+
lineOffset: 33,
118+
ruleId: 'UNIMPORTANT_RULE/RULE2',
119+
},
120+
],
121+
['UNIMPORTANT_RULE/.*'],
83122
);
84123

85124
expect(comments).toEqual([

‎src/AnalyzerBot/utils/commentUtil.ts

+8-3
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import { LogSeverity, LogType } from '../../Parser';
22
import { Comment, CommentStructure } from '../@types/CommentTypes';
33
import { MessageUtil } from './message.util';
44

5-
export function groupComments(logs: LogType[], suppressRules: Set<string>): Comment[] {
5+
export function groupComments(logs: LogType[], suppressRules: Array<string>): Comment[] {
66
const commentMap = logs.reduce((state: CommentStructure, log) => {
77
const { source: file, line } = log;
88

@@ -61,12 +61,17 @@ function calculateSuppresses(currentComment: Comment, isSuppressed: boolean): nu
6161
return currentComment.suppresses + (isSuppressed ? 1 : 0);
6262
}
6363

64+
function shouldBeSuppressed(log: LogType, suppressRules: Array<string>): boolean {
65+
const suppressRegexps: Array<RegExp> = suppressRules.map((rule) => new RegExp(rule));
66+
return suppressRegexps.some((regexp) => regexp.test(log.ruleId));
67+
}
68+
6469
function updateComment(
6570
currentComment: Comment,
6671
log: LogType,
67-
suppressRules: Set<string>,
72+
suppressRules: Array<string>,
6873
): Comment {
69-
const isSuppressed = suppressRules.has(log.ruleId);
74+
const isSuppressed = shouldBeSuppressed(log, suppressRules);
7075
return {
7176
text: buildText(currentComment, log, isSuppressed),
7277
errors: calculateErrors(currentComment, log, isSuppressed),

‎src/Config/Config.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,7 @@ and <cwd> is build root directory (optional (Will use current context as cwd)).
108108
string: true,
109109
number: false,
110110
describe:
111-
'List of rules to suppress, bot will still make comment but mark it as suppressed and not failing the job',
111+
'List of rules to suppress (in RegExp format), bot will still make comment but mark it as suppressed and not failing the job',
112112
default: [],
113113
})
114114
.option('dryRun', {

‎src/Provider/GitHub/GitHub.spec.ts

+1
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@ class PrServiceMock implements IGitHubPRService {
4545
const configs = {
4646
removeOldComment: false,
4747
failOnWarnings: false,
48+
suppressRules: [] as Array<string>,
4849
} as ConfigArgument;
4950

5051
function createGitHub(service: IGitHubPRService, configs: ConfigArgument) {

‎src/Provider/GitLab/GitLab.spec.ts

+1
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ class MrServiceMock implements IGitLabMRService {
3939
const configs = {
4040
removeOldComment: false,
4141
failOnWarnings: false,
42+
suppressRules: [] as Array<string>,
4243
} as ConfigArgument;
4344

4445
function createGitLab(service: IGitLabMRService, configs: ConfigArgument) {

0 commit comments

Comments
 (0)