Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix Danger doesn't add a comment if commented line is not part of diff #1272

Merged
merged 1 commit into from Apr 21, 2022
+65 −35
Merged

Fix Danger doesn't add a comment if commented line is not part of diff #1272

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
46 changes: 26 additions & 20 deletions source/runner/Executor.ts
View file
Open in desktop
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)
),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great fix! I had a suspicion the root issue was around here, but the 3 times I traced it, I couldn’t figure it out — indeed it looked like everything was being caught & handled..

...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
Open in desktop
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