Skip to content

Commit

Permalink
Fix failing inline reviews
Browse files Browse the repository at this point in the history 
Signed-off-by: Jonathan Burke <jonathan.burke@codecentric.de>
  • Loading branch information
@Rouby
Rouby committed Apr 21, 2022
1 parent dcc6b5f commit aca34d6
Show file tree
Hide file tree
Showing 2 changed files with 65 additions and 35 deletions.
46 changes: 26 additions & 20 deletions source/runner/Executor.ts
View file
Expand Up @@ -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

Expand All @@ -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")))
})
Expand Down Expand Up @@ -361,7 +361,7 @@ export class Executor {
*/
sendInlineComments(results: DangerResults, git: GitDSL, previousComments: Comment[] | null): Promise<DangerResults> {
if (!this.platform.supportsInlineComments) {
return new Promise(resolve => resolve(results))
return new Promise((resolve) => resolve(results))
}

const inlineResults = resultsIntoInlineResults(results)
Expand All @@ -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<any>[] = []
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<any> | undefined = undefined
Expand All @@ -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<DangerResults>(resolve => {
resolve(dangerResults.slice(1).reduce((acc, r) => mergeResults(acc, r), emptyResult))
]).then((dangerResults) => {
return new Promise<DangerResults>((resolve) => {
resolve(dangerResults.reduce((acc, r) => mergeResults(acc, r), emptyResult))
})
})
}
Expand All @@ -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,
Expand Down
54 changes: 39 additions & 15 deletions source/runner/_tests/_executor.test.ts
View file
Expand Up @@ -37,7 +37,7 @@ class FakeProcces {
const fakeCI = new FakeCI({})

const defaultDsl = (platform: any): Promise<DangerDSLType> => {
return jsonDSLGenerator(platform, fakeCI, {} as any).then(jsonDSL => {
return jsonDSLGenerator(platform, fakeCI, {} as any).then((jsonDSL) => {
jsonDSL.github = {
pr: {
number: 1,
Expand All @@ -50,7 +50,7 @@ const defaultDsl = (platform: any): Promise<DangerDSLType> => {
}

const mockPayloadForResults = (results: DangerResults): any => {
return resultsIntoInlineResults(results).map(inlineResult => {
return resultsIntoInlineResults(results).map((inlineResult) => {
const comment = inlineTemplate(
defaultConfig.dangerID,
inlineResultsIntoResults(inlineResult),
Expand Down Expand Up @@ -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 () => {
Expand All @@ -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()

Expand All @@ -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()
Expand All @@ -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()

Expand All @@ -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()

Expand All @@ -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()
Expand All @@ -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()
Expand All @@ -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()
Expand Down

0 comments on commit aca34d6

Please sign in to comment.