Skip to content

Commit 1196b69

Browse files
icmpechoicmpecho
authored
Refactor AnalyzerBot into DI pattern and add unit tests (#145)
* 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
1 parent d6c619a commit 1196b69

File tree

11 files changed

+318
-24
lines changed

11 files changed

+318
-24
lines changed

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

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
import { LogType } from '../../Parser';
2+
import { Comment } from '../@types/CommentTypes';
3+
import { Diff } from '../../Git/@types/PatchTypes';
4+
5+
export interface IAnalyzerBot {
6+
touchedFileLog: LogType[];
7+
comments: Comment[];
8+
nError: number;
9+
nWarning: number;
10+
11+
analyze(logs: LogType[], touchedDiff: Diff[]): void;
12+
13+
shouldGenerateOverview(): boolean;
14+
15+
getOverviewMessage(): string;
16+
17+
getCommitDescription(): string;
18+
19+
isSuccess(): boolean;
20+
}

‎src/AnalyzerBot/AnalyzerBot.spec.ts‎

Lines changed: 146 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,146 @@
1+
import { AnalyzerBot } from './AnalyzerBot';
2+
import { AnalyzerBotConfig } from './@interfaces/AnalyzerBotConfig';
3+
import {
4+
file1TouchLine,
5+
file2TouchLine,
6+
mockTouchDiff,
7+
mockTouchFile,
8+
touchFileError,
9+
touchFileWarning,
10+
untouchedError,
11+
untouchedWarning,
12+
} from '../Provider/mockData';
13+
import { MessageUtil } from './utils/message.util';
14+
15+
const config: AnalyzerBotConfig = {
16+
failOnWarnings: false,
17+
};
18+
describe('AnalyzerBot', () => {
19+
const logs = [touchFileError, touchFileWarning, untouchedError, untouchedWarning];
20+
const diff = [mockTouchDiff];
21+
const analyzer = new AnalyzerBot(config);
22+
23+
describe('.touchedFileLog', () => {
24+
it('should return only logs that are in touchedDiff', () => {
25+
analyzer.analyze(logs, diff);
26+
expect(analyzer.touchedFileLog).toEqual([touchFileError, touchFileWarning]);
27+
});
28+
});
29+
30+
describe('.comments', () => {
31+
it('should returns comments for each touched file', () => {
32+
analyzer.analyze(logs, diff);
33+
expect(analyzer.comments).toEqual([
34+
{
35+
file: mockTouchFile,
36+
line: file1TouchLine,
37+
text:
38+
MessageUtil.createMessageWithEmoji(
39+
touchFileError.msg,
40+
touchFileError.severity,
41+
) + ' \n',
42+
errors: 1,
43+
warnings: 0,
44+
},
45+
{
46+
file: mockTouchFile,
47+
line: file2TouchLine,
48+
text:
49+
MessageUtil.createMessageWithEmoji(
50+
touchFileWarning.msg,
51+
touchFileWarning.severity,
52+
) + ' \n',
53+
errors: 0,
54+
warnings: 1,
55+
},
56+
]);
57+
});
58+
59+
it('should be empty when there is no relevant lint errors', () => {
60+
analyzer.analyze([untouchedError, untouchedWarning], diff);
61+
expect(analyzer.comments).toEqual([]);
62+
});
63+
});
64+
65+
describe('.nError', () => {
66+
it('should return the number of related lint errors', () => {
67+
analyzer.analyze(logs, diff);
68+
expect(analyzer.nError).toEqual(1);
69+
});
70+
});
71+
72+
describe('.nWarning', () => {
73+
it('should return the number of related lint warnings', () => {
74+
analyzer.analyze(logs, diff);
75+
expect(analyzer.nWarning).toEqual(1);
76+
});
77+
});
78+
79+
describe('.shouldGenerateOverview', () => {
80+
it('should return true when there is at least one lint error or warning', () => {
81+
analyzer.analyze(logs, diff);
82+
expect(analyzer.shouldGenerateOverview()).toEqual(true);
83+
});
84+
85+
it('should return false when there is no lint error or warning', () => {
86+
analyzer.analyze([untouchedError, untouchedWarning], diff);
87+
expect(analyzer.shouldGenerateOverview()).toEqual(false);
88+
});
89+
});
90+
91+
describe('.getOverviewMessage', () => {
92+
it('should return a proxy result from MessageUtil.generateOverviewMessage based on the number of errors and warnings', () => {
93+
analyzer.analyze(logs, diff);
94+
expect(analyzer.getOverviewMessage()).toEqual(
95+
MessageUtil.generateOverviewMessage(analyzer.nError, analyzer.nWarning),
96+
);
97+
});
98+
});
99+
100+
describe('.getCommitDescription', () => {
101+
it('should return a proxy result from MessageUtil.generateCommitDescription based on the number of errors', () => {
102+
analyzer.analyze(logs, diff);
103+
expect(analyzer.getCommitDescription()).toEqual(
104+
MessageUtil.generateCommitDescription(analyzer.nError),
105+
);
106+
});
107+
});
108+
109+
describe('.isSuccess', () => {
110+
describe('when failOnWarnings is false', () => {
111+
it('should return true when there is no lint error or warning', () => {
112+
analyzer.analyze([untouchedError, untouchedWarning], diff);
113+
expect(analyzer.isSuccess()).toEqual(true);
114+
});
115+
116+
it('should return true when there is only lint warnings', () => {
117+
analyzer.analyze([touchFileWarning, untouchedError, untouchedWarning], diff);
118+
expect(analyzer.isSuccess()).toEqual(true);
119+
});
120+
121+
it('should return false when there is at least one lint error', () => {
122+
analyzer.analyze(logs, diff);
123+
expect(analyzer.isSuccess()).toEqual(false);
124+
});
125+
});
126+
127+
describe('when failOnWarnings is true', () => {
128+
const analyzer = new AnalyzerBot({ ...config, failOnWarnings: true });
129+
130+
it('should return true when there is no lint error or warning', () => {
131+
analyzer.analyze([untouchedError, untouchedWarning], diff);
132+
expect(analyzer.isSuccess()).toEqual(true);
133+
});
134+
135+
it('should return false when there is a lint warning', () => {
136+
analyzer.analyze([touchFileWarning, untouchedError, untouchedWarning], diff);
137+
expect(analyzer.isSuccess()).toEqual(false);
138+
});
139+
140+
it('should return false when there is a lint error', () => {
141+
analyzer.analyze(logs, diff);
142+
expect(analyzer.isSuccess()).toEqual(false);
143+
});
144+
});
145+
});
146+
});

‎src/AnalyzerBot/AnalyzerBot.ts‎

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -5,18 +5,17 @@ import { groupComments } from './utils/commentUtil';
55
import { MessageUtil } from './utils/message.util';
66
import { AnalyzerBotConfig } from './@interfaces/AnalyzerBotConfig';
77
import { Comment } from './@types/CommentTypes';
8+
import { IAnalyzerBot } from './@interfaces/IAnalyzerBot';
89

9-
export class AnalyzerBot {
10-
readonly touchedFileLog: LogType[];
11-
readonly comments: Comment[];
12-
readonly nError: number;
13-
readonly nWarning: number;
10+
export class AnalyzerBot implements IAnalyzerBot {
11+
touchedFileLog: LogType[];
12+
comments: Comment[];
13+
nError: number;
14+
nWarning: number;
1415

15-
constructor(
16-
private readonly config: AnalyzerBotConfig,
17-
private readonly logs: LogType[],
18-
private readonly touchedDiff: Diff[],
19-
) {
16+
constructor(private readonly config: AnalyzerBotConfig) {}
17+
18+
analyze(logs: LogType[], touchedDiff: Diff[]) {
2019
this.touchedFileLog = logs
2120
.filter(onlySeverity(LogSeverity.error, LogSeverity.warning))
2221
.filter(onlyIn(touchedDiff));

‎src/Provider/@interfaces/VCSAdapter.ts‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,13 @@
11
import { Diff } from '../../Git/@types/PatchTypes';
2-
import { AnalyzerBot } from '../../AnalyzerBot/AnalyzerBot';
2+
import { IAnalyzerBot } from '../../AnalyzerBot/@interfaces/IAnalyzerBot';
33

44
export interface VCSAdapter {
55
init(): Promise<void>;
6-
wrapUp(analyzerBot: AnalyzerBot): Promise<boolean>;
6+
wrapUp(analyzerBot: IAnalyzerBot): Promise<boolean>;
77
getName(): string;
88
getLatestCommitSha(): string;
99
diff(): Promise<Diff[]>;
1010
createComment(comment: string): Promise<void>;
1111
createReviewComment(text: string, file: string, line: number): Promise<void>;
12-
1312
removeExistingComments(): Promise<void>;
1413
}

‎src/Provider/CommonVCS/VCSEngine.spec.ts‎

Lines changed: 126 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,126 @@
1+
import { VCSEngineConfig } from '../@interfaces/VCSEngineConfig';
2+
import { VCSEngine } from './VCSEngine';
3+
import { VCSAdapter } from '../@interfaces/VCSAdapter';
4+
import { IAnalyzerBot } from '../../AnalyzerBot/@interfaces/IAnalyzerBot';
5+
import { mockTouchDiff } from '../mockData';
6+
import { Comment } from '../../AnalyzerBot/@types/CommentTypes';
7+
8+
const config: VCSEngineConfig = {
9+
removeOldComment: false,
10+
failOnWarnings: false,
11+
};
12+
13+
function createMockAdapter(): VCSAdapter {
14+
return {
15+
getName: jest.fn(),
16+
init: jest.fn(),
17+
diff: jest.fn(),
18+
getLatestCommitSha: jest.fn(),
19+
createComment: jest.fn(),
20+
createReviewComment: jest.fn(),
21+
removeExistingComments: jest.fn(),
22+
wrapUp: jest.fn(),
23+
};
24+
}
25+
26+
function createMockAnalyzerBot(): IAnalyzerBot {
27+
return {
28+
touchedFileLog: [],
29+
getCommitDescription: jest.fn(),
30+
isSuccess: jest.fn(),
31+
analyze: jest.fn(),
32+
comments: [],
33+
shouldGenerateOverview: jest.fn(),
34+
getOverviewMessage: jest.fn(),
35+
nError: 0,
36+
nWarning: 0,
37+
};
38+
}
39+
40+
const comment1: Comment = {
41+
file: 'file1',
42+
line: 1,
43+
text: 'text1',
44+
errors: 1,
45+
warnings: 0,
46+
};
47+
48+
const comment2: Comment = {
49+
file: 'file2',
50+
line: 2,
51+
text: 'text2',
52+
errors: 0,
53+
warnings: 1,
54+
};
55+
56+
function setup(vcsConfig: VCSEngineConfig = config) {
57+
const analyzer = createMockAnalyzerBot();
58+
const adapter = createMockAdapter();
59+
const vcs = new VCSEngine(vcsConfig, analyzer, adapter);
60+
return { analyzer, adapter, vcs };
61+
}
62+
63+
describe('VCSEngine', () => {
64+
describe('report', () => {
65+
it('initialize the adapter', async () => {
66+
const { adapter, vcs } = setup();
67+
await vcs.report([]);
68+
expect(adapter.init).toBeCalledTimes(1);
69+
});
70+
71+
it('setup the analyzer bot using diff from adapter', async () => {
72+
const { adapter, analyzer, vcs } = setup();
73+
adapter.diff = jest.fn().mockResolvedValue([mockTouchDiff]);
74+
await vcs.report([]);
75+
expect(analyzer.analyze).toBeCalledWith([], [mockTouchDiff]);
76+
});
77+
78+
describe('when removeOldComment is true', () => {
79+
it('remove existing comments', async () => {
80+
const { adapter, vcs } = setup({ ...config, removeOldComment: true });
81+
await vcs.report([]);
82+
expect(adapter.removeExistingComments).toBeCalledTimes(1);
83+
});
84+
});
85+
86+
describe('when removeOldComment is false', () => {
87+
it('does not remove existing comments', async () => {
88+
const { adapter, vcs } = setup({ ...config, removeOldComment: false });
89+
await vcs.report([]);
90+
expect(adapter.removeExistingComments).not.toBeCalled();
91+
});
92+
});
93+
94+
it('create review comments for each comments from analyzer bot', async () => {
95+
const { adapter, analyzer, vcs } = setup();
96+
analyzer.comments = [comment1, comment2];
97+
await vcs.report([]);
98+
expect(adapter.createReviewComment).toBeCalledTimes(2);
99+
});
100+
101+
describe('when analyzerBot.shouldGenerateOverview is true', () => {
102+
it('create summary comment using result from anayzerBot.getOverviewMessage', async () => {
103+
const { adapter, analyzer, vcs } = setup();
104+
analyzer.shouldGenerateOverview = jest.fn().mockReturnValue(true);
105+
analyzer.getOverviewMessage = jest.fn().mockReturnValue('overview');
106+
await vcs.report([]);
107+
expect(adapter.createComment).toBeCalledWith('overview');
108+
});
109+
});
110+
111+
describe('when analyzerBot.shouldGenerateOverview is false', () => {
112+
it('does not create summary comment', async () => {
113+
const { adapter, analyzer, vcs } = setup();
114+
analyzer.shouldGenerateOverview = jest.fn().mockReturnValue(false);
115+
await vcs.report([]);
116+
expect(adapter.createComment).not.toBeCalled();
117+
});
118+
});
119+
120+
it('wrap up the adapter, passing in analyzer bot', async () => {
121+
const { adapter, analyzer, vcs } = setup();
122+
await vcs.report([]);
123+
expect(adapter.wrapUp).toBeCalledWith(analyzer);
124+
});
125+
});
126+
});

‎src/Provider/CommonVCS/VCSEngine.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,13 +3,13 @@ import { LogType } from '../../Parser';
33
import { Log } from '../../Logger';
44
import { Comment } from '../../AnalyzerBot/@types/CommentTypes';
55
import { VCSEngineConfig } from '../@interfaces/VCSEngineConfig';
6-
import { AnalyzerBot } from '../../AnalyzerBot/AnalyzerBot';
76
import { VCSAdapter } from '../@interfaces/VCSAdapter';
7+
import { IAnalyzerBot } from '../../AnalyzerBot/@interfaces/IAnalyzerBot';
88

99
export class VCSEngine implements VCS {
10-
protected analyzerBot: AnalyzerBot;
1110
constructor(
1211
private readonly config: VCSEngineConfig,
12+
private readonly analyzerBot: IAnalyzerBot,
1313
private readonly adapter: VCSAdapter,
1414
) {}
1515

@@ -37,7 +37,7 @@ export class VCSEngine implements VCS {
3737

3838
private async setup(logs: LogType[]) {
3939
const touchedDiff = await this.adapter.diff();
40-
this.analyzerBot = new AnalyzerBot(this.config, logs, touchedDiff);
40+
this.analyzerBot.analyze(logs, touchedDiff);
4141

4242
Log.debug(`VCS Setup`, {
4343
sha: this.adapter.getLatestCommitSha(),

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import {
1414
import { ConfigArgument } from '../../Config';
1515
import { VCSEngine } from '../CommonVCS/VCSEngine';
1616
import { GitHubAdapter } from './GitHubAdapter';
17+
import { AnalyzerBot } from '../../AnalyzerBot/AnalyzerBot';
1718

1819
const mockedCommentId = 45346;
1920
const mockedReviewId = 324145;
@@ -47,7 +48,7 @@ const configs = {
4748
} as ConfigArgument;
4849

4950
function createGitHub(service: IGitHubPRService, configs: ConfigArgument) {
50-
return new VCSEngine(configs, new GitHubAdapter(service));
51+
return new VCSEngine(configs, new AnalyzerBot(configs), new GitHubAdapter(service));
5152
}
5253

5354
describe('VCS: GitHub', () => {

0 commit comments

Comments
 (0)