From 8ac944d2190e855e4a9086c8917b0094003e4c60 Mon Sep 17 00:00:00 2001 From: Jonathan Burke Date: Thu, 21 Apr 2022 10:21:53 +0200 Subject: [PATCH] Fix failing inline reviews Signed-off-by: Jonathan Burke --- source/runner/Executor.ts | 46 ++++++++++++---------- source/runner/_tests/_executor.test.ts | 54 +++++++++++++++++++------- 2 files changed, 65 insertions(+), 35 deletions(-) diff --git a/source/runner/Executor.ts b/source/runner/Executor.ts index 0a05d5424..385fe605f 100644 --- a/source/runner/Executor.ts +++ b/source/runner/Executor.ts @@ -194,7 +194,7 @@ export class Executor { output = `Danger: ${tick} passed review, received no feedback.` } - const allMessages = [...fails, ...warnings, ...messages, ...markdowns].map(m => m.message) + const allMessages = [...fails, ...warnings, ...messages, ...markdowns].map((m) => m.message) const oneMessage = allMessages.join("\n") const longMessage = oneMessage.split("\n").length > 30 @@ -205,16 +205,16 @@ export class Executor { } const table = [ - fails.length && { name: "Failures", messages: fails.map(f => f.message) }, - warnings.length && { name: "Warnings", messages: warnings.map(w => w.message) }, - messages.length && { name: "Messages", messages: messages.map(m => m.message) }, - markdowns.length && { name: "Markdowns", messages: markdowns.map(m => m.message) }, - ].filter(r => r !== 0) as { name: string; messages: string[] }[] + fails.length && { name: "Failures", messages: fails.map((f) => f.message) }, + warnings.length && { name: "Warnings", messages: warnings.map((w) => w.message) }, + messages.length && { name: "Messages", messages: messages.map((m) => m.message) }, + markdowns.length && { name: "Markdowns", messages: markdowns.map((m) => m.message) }, + ].filter((r) => r !== 0) as { name: string; messages: string[] }[] // Consider looking at getting the terminal width, and making it 60% // if over a particular size - table.forEach(row => { + table.forEach((row) => { this.log(`## ${chalk.bold(row.name)}`) this.log(row.messages.join(chalk.bold("\n-\n"))) }) @@ -361,7 +361,7 @@ export class Executor { */ sendInlineComments(results: DangerResults, git: GitDSL, previousComments: Comment[] | null): Promise { if (!this.platform.supportsInlineComments) { - return new Promise(resolve => resolve(results)) + return new Promise((resolve) => resolve(results)) } const inlineResults = resultsIntoInlineResults(results) @@ -379,11 +379,11 @@ export class Executor { // if there is - update it and remove comment from deleteComments array (comments prepared for deletion) // if there isn't - create a new comment // Leftovers in deleteComments array should all be deleted afterwards - let deleteComments = Array.isArray(previousComments) ? previousComments.filter(c => c.ownedByDanger) : [] + let deleteComments = Array.isArray(previousComments) ? previousComments.filter((c) => c.ownedByDanger) : [] let commentPromises: Promise[] = [] const inlineResultsForReview: DangerInlineResults[] = [] for (let inlineResult of sortedInlineResults) { - const index = deleteComments.findIndex(p => + const index = deleteComments.findIndex((p) => p.body.includes(fileLineToString(inlineResult.file, inlineResult.line)) ) let promise: Promise | undefined = undefined @@ -399,22 +399,28 @@ export class Executor { } } if (promise) { - commentPromises.push(promise.then(_r => emptyDangerResults).catch(_e => inlineResultsIntoResults(inlineResult))) + commentPromises.push( + promise.then((_r) => emptyDangerResults).catch((_e) => inlineResultsIntoResults(inlineResult)) + ) } } - deleteComments.forEach(comment => { + deleteComments.forEach((comment) => { let promise = this.deleteInlineComment(comment) - commentPromises.push(promise.then(_r => emptyDangerResults).catch(_e => emptyDangerResults)) + commentPromises.push(promise.then((_r) => emptyDangerResults).catch((_e) => emptyDangerResults)) }) return Promise.all([ - this.sendInlineReview(git, inlineResultsForReview).catch(_e => - inlineResultsForReview.forEach(inlineResult => inlineResultsIntoResults(inlineResult)) - ), + this.sendInlineReview(git, inlineResultsForReview) + .then((_r) => emptyDangerResults) + .catch((_e) => + inlineResultsForReview + .map((inlineResult) => inlineResultsIntoResults(inlineResult)) + .reduce(mergeResults, emptyResult) + ), ...commentPromises, - ]).then(dangerResults => { - return new Promise(resolve => { - resolve(dangerResults.slice(1).reduce((acc, r) => mergeResults(acc, r), emptyResult)) + ]).then((dangerResults) => { + return new Promise((resolve) => { + resolve(dangerResults.reduce((acc, r) => mergeResults(acc, r), emptyResult)) }) }) } @@ -425,7 +431,7 @@ export class Executor { } return await this.platform.createInlineReview( git, - inlineResultsForReview.map(result => ({ + inlineResultsForReview.map((result) => ({ comment: this.inlineCommentTemplate(result), path: result.file, line: result.line, diff --git a/source/runner/_tests/_executor.test.ts b/source/runner/_tests/_executor.test.ts index 7a1a41928..d765de6b7 100644 --- a/source/runner/_tests/_executor.test.ts +++ b/source/runner/_tests/_executor.test.ts @@ -37,7 +37,7 @@ class FakeProcces { const fakeCI = new FakeCI({}) const defaultDsl = (platform: any): Promise => { - return jsonDSLGenerator(platform, fakeCI, {} as any).then(jsonDSL => { + return jsonDSLGenerator(platform, fakeCI, {} as any).then((jsonDSL) => { jsonDSL.github = { pr: { number: 1, @@ -50,7 +50,7 @@ const defaultDsl = (platform: any): Promise => { } const mockPayloadForResults = (results: DangerResults): any => { - return resultsIntoInlineResults(results).map(inlineResult => { + return resultsIntoInlineResults(results).map((inlineResult) => { const comment = inlineTemplate( defaultConfig.dangerID, inlineResultsIntoResults(inlineResult), @@ -245,6 +245,24 @@ describe("setup", () => { await exec.handleResults(inlineMultipleWarnResults, dsl.git) expect(platform.createInlineReview).toBeCalled() expect(platform.createInlineComment).not.toBeCalled() + expect(platform.updateOrCreateComment).not.toBeCalled() + }) + + it("Creates multiple inline comments if review fails", async () => { + const platform = new FakePlatform() + const exec = new Executor(new FakeCI({}), platform, inlineRunner, defaultConfig, new FakeProcces()) + const dsl = await defaultDsl(platform) + platform.createInlineReview = jest.fn().mockImplementation(() => { + throw new Error("Should not be called") + }) + platform.createInlineComment = jest.fn() + platform.createComment = jest.fn() + platform.updateOrCreateComment = jest.fn() + + await exec.handleResults(inlineMultipleWarnResults, dsl.git) + expect(platform.createInlineReview).toBeCalled() + expect(platform.createInlineComment).not.toBeCalled() + expect(platform.updateOrCreateComment).toBeCalled() }) it("Invalid inline comment is ignored if ignoreOutOfDiffComments is true", async () => { @@ -270,7 +288,7 @@ describe("setup", () => { const inlineResults = resultsIntoInlineResults(previousResults)[0] const comment = inlineTemplate(defaultConfig.dangerID, previousResults, inlineResults.file, inlineResults.line) const previousComments = [{ id: 1234, body: comment, ownedByDanger: true }] - platform.getInlineComments = jest.fn().mockReturnValue(new Promise(r => r(previousComments))) + platform.getInlineComments = jest.fn().mockReturnValue(new Promise((r) => r(previousComments))) platform.updateInlineComment = jest.fn() platform.createInlineComment = jest.fn() @@ -286,7 +304,7 @@ describe("setup", () => { const previousResults = inlineMultipleWarnResults const newResults = inlineMultipleWarnResults2 const previousComments = mockPayloadForResults(previousResults) - platform.getInlineComments = jest.fn().mockReturnValue(new Promise(r => r(previousComments))) + platform.getInlineComments = jest.fn().mockReturnValue(new Promise((r) => r(previousComments))) platform.updateInlineComment = jest.fn() platform.createInlineComment = jest.fn() platform.deleteInlineComment = jest.fn() @@ -306,7 +324,7 @@ describe("setup", () => { const inlineResults = resultsIntoInlineResults(previousResults)[0] const comment = inlineTemplate(defaultConfig.dangerID, previousResults, inlineResults.file, inlineResults.line) const previousComments = [{ id: 1234, body: comment, ownedByDanger: true }] - platform.getInlineComments = jest.fn().mockReturnValue(new Promise(r => r(previousComments))) + platform.getInlineComments = jest.fn().mockReturnValue(new Promise((r) => r(previousComments))) platform.updateInlineComment = jest.fn() platform.createInlineComment = jest.fn() @@ -324,7 +342,7 @@ describe("setup", () => { const inlineResults = resultsIntoInlineResults(previousResults)[0] const comment = inlineTemplate(defaultConfig.dangerID, previousResults, inlineResults.file, inlineResults.line) const previousComments = [{ id: 1234, body: comment, ownedByDanger: true }] - platform.getInlineComments = jest.fn().mockReturnValue(new Promise(r => r(previousComments))) + platform.getInlineComments = jest.fn().mockReturnValue(new Promise((r) => r(previousComments))) platform.updateInlineComment = jest.fn() platform.createInlineComment = jest.fn() @@ -339,14 +357,17 @@ describe("setup", () => { const dsl = await defaultDsl(platform) const previousResults = { fails: [], - warnings: [{ message: "1", file: "1.swift", line: 1 }, { message: "2", file: "2.swift", line: 2 }], + warnings: [ + { message: "1", file: "1.swift", line: 1 }, + { message: "2", file: "2.swift", line: 2 }, + ], messages: [], markdowns: [], } const previousComments = mockPayloadForResults(previousResults) const newResults = emptyResults - platform.getInlineComments = jest.fn().mockReturnValue(new Promise(r => r(previousComments))) + platform.getInlineComments = jest.fn().mockReturnValue(new Promise((r) => r(previousComments))) platform.updateInlineComment = jest.fn() platform.createInlineComment = jest.fn() platform.deleteInlineComment = jest.fn() @@ -369,10 +390,7 @@ describe("setup", () => { const dsl = await defaultDsl(platform) const previousComments = mockPayloadForResults(inlineMultipleWarnResults) - platform.getInlineComments = jest - .fn() - .mockResolvedValueOnce(previousComments) - .mockResolvedValueOnce([]) + platform.getInlineComments = jest.fn().mockResolvedValueOnce(previousComments).mockResolvedValueOnce([]) platform.updateInlineComment = jest.fn() platform.createInlineComment = jest.fn() platform.deleteInlineComment = jest.fn() @@ -387,19 +405,25 @@ describe("setup", () => { const dsl = await defaultDsl(platform) const previousResults = { fails: [], - warnings: [{ message: "1", file: "1.swift", line: 1 }, { message: "2", file: "2.swift", line: 2 }], + warnings: [ + { message: "1", file: "1.swift", line: 1 }, + { message: "2", file: "2.swift", line: 2 }, + ], messages: [], markdowns: [], } const newResults = { fails: [], - warnings: [{ message: "1", file: "1.swift", line: 2 }, { message: "2", file: "2.swift", line: 3 }], + warnings: [ + { message: "1", file: "1.swift", line: 2 }, + { message: "2", file: "2.swift", line: 3 }, + ], messages: [], markdowns: [], } const previousComments = mockPayloadForResults(previousResults) - platform.getInlineComments = jest.fn().mockReturnValue(new Promise(r => r(previousComments))) + platform.getInlineComments = jest.fn().mockReturnValue(new Promise((r) => r(previousComments))) platform.updateInlineComment = jest.fn() platform.createInlineComment = jest.fn() platform.deleteInlineComment = jest.fn()