From d0e1a0ee627583f360a402dea658f04a9aa374fd Mon Sep 17 00:00:00 2001 From: jdickson Date: Fri, 13 Dec 2024 15:43:43 +0700 Subject: [PATCH 1/3] Add check before delete comment --- .vscode/settings.json | 2 +- src/Provider/GitLab/GitLab.spec.ts | 114 +++++++++++++++--------- src/Provider/GitLab/GitLabAdapter.ts | 77 +++++++++++----- src/Provider/GitLab/IGitLabMRService.ts | 1 - 4 files changed, 126 insertions(+), 68 deletions(-) diff --git a/.vscode/settings.json b/.vscode/settings.json index aab8069..a03ac94 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -1,6 +1,6 @@ { "editor.codeActionsOnSave": { - "source.fixAll.eslint": true + "source.fixAll.eslint": "explicit" }, "typescript.preferences.importModuleSpecifier": "relative" } diff --git a/src/Provider/GitLab/GitLab.spec.ts b/src/Provider/GitLab/GitLab.spec.ts index 8763a8c..95338d6 100644 --- a/src/Provider/GitLab/GitLab.spec.ts +++ b/src/Provider/GitLab/GitLab.spec.ts @@ -13,12 +13,24 @@ import { GitLabAdapter } from './GitLabAdapter'; import { AnalyzerBot } from '../../AnalyzerBot/AnalyzerBot'; const mockCurrentUserId = 123456; -const mockNoteIdToBeDeleted = 6544321; + +// Create helper function to generate consistent comment keys +const generateCommentKey = (file: string, line: number | undefined, text: string) => + `${file}:${line}:${text}`; const mockNotes = [ - { id: 1, author: { id: mockCurrentUserId }, system: true }, - { id: mockNoteIdToBeDeleted, author: { id: mockCurrentUserId }, system: false }, // only one to be deleted - { id: 3, author: { id: 99 }, system: false }, + { + id: 1, + author: { id: mockCurrentUserId }, + system: true, + body: 'system message' + }, + { + id: 2, + author: { id: mockCurrentUserId }, + system: false, + body: 'existing comment' + }, ] as MergeRequestNoteSchema[]; const mockVersionId = 3425234; @@ -47,7 +59,11 @@ function createGitLab(service: IGitLabMRService, configs: ConfigArgument) { } describe('VCS: GitLab', () => { - it('should returns true when there is no error', async () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + it('should return true when there is no error', async () => { const service = new MrServiceMock(); const gitLab = createGitLab(service, configs); @@ -60,7 +76,7 @@ describe('VCS: GitLab', () => { expect(result).toBe(true); }); - it('should returns false when there is some error', async () => { + it('should return false when there is some error', async () => { const service = new MrServiceMock(); const gitLab = createGitLab(service, configs); @@ -74,40 +90,32 @@ describe('VCS: GitLab', () => { expect(result).toBe(false); }); - it('should remove old self comments and reviews and post new ones', async () => { + it('should not create duplicate comments and should create new unique comments', async () => { const service = new MrServiceMock(); - const gitLab = createGitLab(service, { ...configs, removeOldComment: true }); - - await gitLab.report([ - touchFileError, - touchFileWarning, - untouchedError, - untouchedWarning, + + // Pre-populate with an existing comment for the error + const existingErrorText = `${touchFileError.source}:${touchFileError.line}::rotating_light: ${touchFileError.msg} \n`; + + service.listAllNotes.mockResolvedValue([ + ...mockNotes, + { + id: 4, + author: { id: mockCurrentUserId }, + system: false, + body: existingErrorText + } ]); - - expect(service.listAllNotes).toHaveBeenCalledTimes(1); - expect(service.getCurrentUserId).toHaveBeenCalledTimes(1); - - expect(service.deleteNote).toHaveBeenCalledTimes(1); - expect(service.deleteNote).toHaveBeenCalledWith(mockNoteIdToBeDeleted); - - expect(service.createMRDiscussion).toHaveBeenNthCalledWith( - 1, - mockVersion, - touchFileError.source, - touchFileError.line, - expect.any(String), - ); - - expect(service.createMRDiscussion).toHaveBeenNthCalledWith( - 2, + + const gitLab = createGitLab(service, configs); + await gitLab.report([touchFileError, touchFileWarning]); + + expect(service.createMRDiscussion).toHaveBeenCalledTimes(1); + expect(service.createMRDiscussion).toHaveBeenCalledWith( mockVersion, touchFileWarning.source, touchFileWarning.line, - expect.any(String), + expect.any(String) ); - - expect(service.createNote).toHaveBeenCalledTimes(1); }); it('should not comment if there is no relevant lint issue', async () => { @@ -120,40 +128,58 @@ describe('VCS: GitLab', () => { expect(service.createNote).not.toHaveBeenCalled(); }); + it('should not create comments that already exist', async () => { + const service = new MrServiceMock(); + const gitLab = createGitLab(service, configs); + + // Set up mock with existing summary comment + const existingSummaryComment = "## CodeCoach reports 1 issue\n:rotating_light: 0 error\n:warning: 1 warning"; + service.listAllNotes.mockResolvedValue([ + { + id: 1, + author: { id: mockCurrentUserId }, + system: false, + body: existingSummaryComment + } + ]); + + await gitLab.report([touchFileWarning]); + expect(service.createNote).not.toHaveBeenCalled(); + }); + describe('when failOnWarnings is true', () => { - it('should returns true when there is no error or warning', async () => { + const warningConfigs = { ...configs, failOnWarnings: true }; + + it('should return true when there is no error or warning', async () => { const service = new MrServiceMock(); - const gitLab = createGitLab(service, { ...configs, failOnWarnings: true }); + const gitLab = createGitLab(service, warningConfigs); const result = await gitLab.report([untouchedError, untouchedWarning]); - expect(result).toBe(true); }); - it('should returns false when there is some error', async () => { + it('should return false when there is some error', async () => { const service = new MrServiceMock(); - const gitLab = createGitLab(service, { ...configs, failOnWarnings: true }); + const gitLab = createGitLab(service, warningConfigs); const result = await gitLab.report([ touchFileError, untouchedError, untouchedWarning, ]); - expect(result).toBe(false); }); - it('should returns false when there is some warnings', async () => { + it('should return false when there is some warnings', async () => { const service = new MrServiceMock(); - const gitLab = createGitLab(service, { ...configs, failOnWarnings: true }); + const gitLab = createGitLab(service, warningConfigs); const result = await gitLab.report([ touchFileWarning, untouchedError, untouchedWarning, ]); - expect(result).toBe(false); }); }); -}); +}); \ No newline at end of file diff --git a/src/Provider/GitLab/GitLabAdapter.ts b/src/Provider/GitLab/GitLabAdapter.ts index 0121628..88c8fb6 100644 --- a/src/Provider/GitLab/GitLabAdapter.ts +++ b/src/Provider/GitLab/GitLabAdapter.ts @@ -2,15 +2,66 @@ import { VCSAdapter } from '../@interfaces/VCSAdapter'; import { Diff } from '../../Git/@types/PatchTypes'; import { Log } from '../../Logger'; import { IGitLabMRService } from './IGitLabMRService'; -import { MergeRequestDiffVersionsSchema } from '@gitbeaker/core'; +import { MergeRequestDiffVersionsSchema, MergeRequestNoteSchema } from '@gitbeaker/core'; import { IAnalyzerBot } from '../../AnalyzerBot/@interfaces/IAnalyzerBot'; export class GitLabAdapter implements VCSAdapter { private latestMrVersion: MergeRequestDiffVersionsSchema; + private existingComments: Set = new Set(); + constructor(private readonly mrService: IGitLabMRService) {} async init(): Promise { - this.latestMrVersion = await this.mrService.getLatestVersion(); + const [latestVersion, userId, notes] = await Promise.all([ + this.mrService.getLatestVersion(), + this.mrService.getCurrentUserId(), + this.mrService.listAllNotes(), + ]); + + this.latestMrVersion = latestVersion; + + // Store existing bot comments + notes + .filter((note: { author: { id: any; }; system: any; }) => note.author.id === userId && !note.system) + .forEach((note: { body: string; }) => this.existingComments.add(note.body)); + + Log.debug(`Found ${this.existingComments.size} existing CodeCoach comments`); + } + + private generateCommentKey(file: string, line: number | undefined, text: string): string { + return `${file}:${line}:${text}`; + } + + async createComment(comment: string): Promise { + if (!this.existingComments.has(comment)) { + await this.mrService.createNote(comment); + this.existingComments.add(comment); + Log.debug('Created new comment'); + } else { + Log.debug('Skipped creating duplicate comment'); + } + } + + async createReviewComment( + text: string, + file: string, + line: number, + nLines?: number + ): Promise { + const commentKey = this.generateCommentKey(file, line, text); + + if (!this.existingComments.has(commentKey)) { + await this.mrService.createMRDiscussion( + this.latestMrVersion, + file, + line, + text + ); + this.existingComments.add(commentKey); + Log.debug('Created new review comment'); + } else { + Log.debug('Skipped creating duplicate review comment'); + } } async wrapUp(analyzer: IAnalyzerBot): Promise { @@ -28,26 +79,8 @@ export class GitLabAdapter implements VCSAdapter { diff(): Promise { return this.mrService.diff(); } - createComment(comment: string): Promise { - return this.mrService.createNote(comment); - } - - createReviewComment(text: string, file: string, line: number): Promise { - return this.mrService.createMRDiscussion(this.latestMrVersion, file, line, text); - } async removeExistingComments(): Promise { - const [userId, notes] = await Promise.all([ - this.mrService.getCurrentUserId(), - this.mrService.listAllNotes(), - ]); - Log.debug('Get existing CodeCoach comments completed'); - - const deleteNotes = notes - .filter((note) => note.author.id === userId && !note.system) - .map((note) => this.mrService.deleteNote(note.id)); - - await Promise.all([...deleteNotes]); - Log.debug('Delete CodeCoach comments completed'); + Log.debug('Skipping comment removal as we now handle duplicates on creation'); } -} +} \ No newline at end of file diff --git a/src/Provider/GitLab/IGitLabMRService.ts b/src/Provider/GitLab/IGitLabMRService.ts index 12ad833..5824f58 100644 --- a/src/Provider/GitLab/IGitLabMRService.ts +++ b/src/Provider/GitLab/IGitLabMRService.ts @@ -7,7 +7,6 @@ export interface IGitLabMRService { deleteNote(noteId: number): Promise; getCurrentUserId(): Promise; diff(): Promise; - getLatestVersion(): Promise; createMRDiscussion( latestVersion: MergeRequestDiffVersionsSchema, From 23f56443868c83f29ce5ee5dca4f506cef86f2b1 Mon Sep 17 00:00:00 2001 From: jdickson Date: Fri, 13 Dec 2024 15:46:18 +0700 Subject: [PATCH 2/3] revert lint change --- .vscode/settings.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.vscode/settings.json b/.vscode/settings.json index a03ac94..aab8069 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -1,6 +1,6 @@ { "editor.codeActionsOnSave": { - "source.fixAll.eslint": "explicit" + "source.fixAll.eslint": true }, "typescript.preferences.importModuleSpecifier": "relative" } From e95b14487b13a947399e64638d5b0093f89b33be Mon Sep 17 00:00:00 2001 From: jdickson Date: Fri, 13 Dec 2024 15:51:05 +0700 Subject: [PATCH 3/3] fix lint --- src/Provider/GitLab/GitLab.spec.ts | 43 ++++++++++++++-------------- src/Provider/GitLab/GitLabAdapter.ts | 38 ++++++++++++------------ 2 files changed, 42 insertions(+), 39 deletions(-) diff --git a/src/Provider/GitLab/GitLab.spec.ts b/src/Provider/GitLab/GitLab.spec.ts index 95338d6..72e524d 100644 --- a/src/Provider/GitLab/GitLab.spec.ts +++ b/src/Provider/GitLab/GitLab.spec.ts @@ -15,21 +15,21 @@ import { AnalyzerBot } from '../../AnalyzerBot/AnalyzerBot'; const mockCurrentUserId = 123456; // Create helper function to generate consistent comment keys -const generateCommentKey = (file: string, line: number | undefined, text: string) => +const generateCommentKey = (file: string, line: number | undefined, text: string) => `${file}:${line}:${text}`; const mockNotes = [ - { - id: 1, - author: { id: mockCurrentUserId }, + { + id: 1, + author: { id: mockCurrentUserId }, system: true, - body: 'system message' + body: 'system message', }, - { - id: 2, - author: { id: mockCurrentUserId }, + { + id: 2, + author: { id: mockCurrentUserId }, system: false, - body: 'existing comment' + body: 'existing comment', }, ] as MergeRequestNoteSchema[]; @@ -92,29 +92,29 @@ describe('VCS: GitLab', () => { it('should not create duplicate comments and should create new unique comments', async () => { const service = new MrServiceMock(); - + // Pre-populate with an existing comment for the error const existingErrorText = `${touchFileError.source}:${touchFileError.line}::rotating_light: ${touchFileError.msg} \n`; - + service.listAllNotes.mockResolvedValue([ ...mockNotes, { id: 4, author: { id: mockCurrentUserId }, system: false, - body: existingErrorText - } + body: existingErrorText, + }, ]); - + const gitLab = createGitLab(service, configs); await gitLab.report([touchFileError, touchFileWarning]); - + expect(service.createMRDiscussion).toHaveBeenCalledTimes(1); expect(service.createMRDiscussion).toHaveBeenCalledWith( mockVersion, touchFileWarning.source, touchFileWarning.line, - expect.any(String) + expect.any(String), ); }); @@ -131,16 +131,17 @@ describe('VCS: GitLab', () => { it('should not create comments that already exist', async () => { const service = new MrServiceMock(); const gitLab = createGitLab(service, configs); - + // Set up mock with existing summary comment - const existingSummaryComment = "## CodeCoach reports 1 issue\n:rotating_light: 0 error\n:warning: 1 warning"; + const existingSummaryComment = + '## CodeCoach reports 1 issue\n:rotating_light: 0 error\n:warning: 1 warning'; service.listAllNotes.mockResolvedValue([ { id: 1, author: { id: mockCurrentUserId }, system: false, - body: existingSummaryComment - } + body: existingSummaryComment, + }, ]); await gitLab.report([touchFileWarning]); @@ -182,4 +183,4 @@ describe('VCS: GitLab', () => { expect(result).toBe(false); }); }); -}); \ No newline at end of file +}); diff --git a/src/Provider/GitLab/GitLabAdapter.ts b/src/Provider/GitLab/GitLabAdapter.ts index 88c8fb6..1aed012 100644 --- a/src/Provider/GitLab/GitLabAdapter.ts +++ b/src/Provider/GitLab/GitLabAdapter.ts @@ -8,7 +8,7 @@ import { IAnalyzerBot } from '../../AnalyzerBot/@interfaces/IAnalyzerBot'; export class GitLabAdapter implements VCSAdapter { private latestMrVersion: MergeRequestDiffVersionsSchema; private existingComments: Set = new Set(); - + constructor(private readonly mrService: IGitLabMRService) {} async init(): Promise { @@ -17,18 +17,25 @@ export class GitLabAdapter implements VCSAdapter { this.mrService.getCurrentUserId(), this.mrService.listAllNotes(), ]); - + this.latestMrVersion = latestVersion; - + // Store existing bot comments notes - .filter((note: { author: { id: any; }; system: any; }) => note.author.id === userId && !note.system) - .forEach((note: { body: string; }) => this.existingComments.add(note.body)); - + .filter( + (note: { author: { id: any }; system: any }) => + note.author.id === userId && !note.system, + ) + .forEach((note: { body: string }) => this.existingComments.add(note.body)); + Log.debug(`Found ${this.existingComments.size} existing CodeCoach comments`); } - private generateCommentKey(file: string, line: number | undefined, text: string): string { + private generateCommentKey( + file: string, + line: number | undefined, + text: string, + ): string { return `${file}:${line}:${text}`; } @@ -43,20 +50,15 @@ export class GitLabAdapter implements VCSAdapter { } async createReviewComment( - text: string, - file: string, + text: string, + file: string, line: number, - nLines?: number + nLines?: number, ): Promise { const commentKey = this.generateCommentKey(file, line, text); - + if (!this.existingComments.has(commentKey)) { - await this.mrService.createMRDiscussion( - this.latestMrVersion, - file, - line, - text - ); + await this.mrService.createMRDiscussion(this.latestMrVersion, file, line, text); this.existingComments.add(commentKey); Log.debug('Created new review comment'); } else { @@ -83,4 +85,4 @@ export class GitLabAdapter implements VCSAdapter { async removeExistingComments(): Promise { Log.debug('Skipping comment removal as we now handle duplicates on creation'); } -} \ No newline at end of file +}