Skip to content

Commit

Permalink
extend github api to send complete 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 committed Nov 19, 2021
1 parent ca792af commit fb630ec
Show file tree
Hide file tree
Showing 7 changed files with 112 additions and 6 deletions.
5 changes: 5 additions & 0 deletions source/platforms/FakePlatform.ts
Original file line number Diff line number Diff line change
Expand Up @@ -85,4 +85,9 @@ export class FakePlatform implements Platform {
}

getFileContents = (path: string) => new Promise<string>(res => res(readFileSync(path, "utf8")))

createInlineReview?: (
git: GitDSL,
comments: { comment: string; path: string; line: number }[]
) => Promise<any> = undefined
}
25 changes: 25 additions & 0 deletions source/platforms/github/GitHubAPI.ts
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,31 @@ export class GitHubAPI {
}
}

postInlinePRReview = async (commitId: string, comments: { comment: string; path: string; position: number }[]) => {
const repo = this.repoMetadata.repoSlug
const prID = this.repoMetadata.pullRequestID
const res = await this.post(
`repos/${repo}/pulls/${prID}/reviews`,
{},
{
body: "",
event: "COMMENT",
commit_id: commitId,
comments: comments.map(({ comment, path, position }) => ({
body: comment,
path,
position,
})),
},
false
)
if (res.ok) {
return res.json()
} else {
throw await res.json()
}
}

updateInlinePRComment = async (comment: string, commentId: string) => {
const repo = this.repoMetadata.repoSlug
const res = await this.patch(
Expand Down
17 changes: 17 additions & 0 deletions source/platforms/github/_tests/_github_api.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,23 @@ describe("API testing", () => {
await expect(api.postInlinePRComment("", "", "", 0)).rejects.toEqual(expectedJSON)
})

it("postInlinePRReview success", async () => {
api.fetch = fetchJSON
const expectedJSON = {
api: "https://api.github.com/repos/artsy/emission/pulls/1/reviews",
headers: {
Authorization: "token ABCDE",
"Content-Type": "application/json",
},
method: "POST",
body: '{"body":"","event":"COMMENT","commit_id":"","comments":[{"body":"","path":"","position":0}]}',
}
expect.assertions(1)
await expect(api.postInlinePRReview("", [{ comment: "", path: "", position: 0 }])).resolves.toMatchObject(
expectedJSON
)
})

it("updateStatus('pending') success", async () => {
api.fetch = jest.fn().mockReturnValue({ ok: true })
api.getPullRequestInfo = await requestWithFixturedJSON("github_pr.json")
Expand Down
14 changes: 14 additions & 0 deletions source/platforms/github/comms/issueCommenter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,20 @@ export const GitHubIssueCommenter = (api: GitHubAPI) => {
})
},

createInlineReview: (git: GitDSL, comments: { comment: string; path: string; line: number }[]) => {
let commitId = git.commits[git.commits.length - 1].sha
d("Finishing review. Commit: " + commitId)
return Promise.all(
comments.map(comment =>
findPositionForInlineComment(git, comment.line, comment.path).then(position => ({
comment: comment.comment,
path: comment.path,
position,
}))
)
).then(comments => api.postInlinePRReview(commitId, comments))
},

/**
* Updates an inline comment if possible. If platform can't update an inline comment,
* it returns a promise rejection. (e.g. platform doesn't support inline comments or line was out of diff).
Expand Down
2 changes: 2 additions & 0 deletions source/platforms/platform.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,8 @@ export interface PlatformCommunicator {
updateInlineComment: (comment: string, commentId: string) => Promise<any>
/** Delete an inline comment */
deleteInlineComment: (commentId: string) => Promise<boolean>
/** Create a review with inline comments */
createInlineReview?: (git: GitDSL, comments: { comment: string; path: string; line: number }[]) => Promise<any>
/** Delete the main Danger comment */
deleteMainComment: (dangerID: string) => Promise<boolean>
/** Replace the main Danger comment, returning the URL to the issue */
Expand Down
36 changes: 31 additions & 5 deletions source/runner/Executor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -381,32 +381,58 @@ export class Executor {
// Leftovers in deleteComments array should all be deleted afterwards
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 =>
p.body.includes(fileLineToString(inlineResult.file, inlineResult.line))
)
let promise: Promise<any>
let promise: Promise<any> | undefined = undefined
if (index != -1) {
let previousComment = deleteComments[index]
deleteComments.splice(index, 1)
promise = this.updateInlineComment(inlineResult, previousComment)
} else {
promise = this.sendInlineComment(git, inlineResult)
if (typeof this.platform.createInlineReview === "function") {
inlineResultsForReview.push(inlineResult)
} else {
promise = this.sendInlineComment(git, inlineResult)
}
}
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 => {
let promise = this.deleteInlineComment(comment)
commentPromises.push(promise.then(_r => emptyDangerResults).catch(_e => emptyDangerResults))
})

return Promise.all(commentPromises).then(dangerResults => {
return Promise.all([
this.sendInlineReview(git, inlineResultsForReview).catch(_e =>
inlineResultsForReview.forEach(inlineResult => inlineResultsIntoResults(inlineResult))
),
...commentPromises,
]).then(dangerResults => {
return new Promise<DangerResults>(resolve => {
resolve(dangerResults.reduce((acc, r) => mergeResults(acc, r), emptyResult))
resolve(dangerResults.slice(1).reduce((acc, r) => mergeResults(acc, r), emptyResult))
})
})
}

async sendInlineReview(git: GitDSL, inlineResultsForReview: DangerInlineResults[]): Promise<any> {
if (inlineResultsForReview.length === 0 || typeof this.platform.createInlineReview !== "function") {
return emptyDangerResults
}
return await this.platform.createInlineReview(
git,
inlineResultsForReview.map(result => ({
comment: this.inlineCommentTemplate(result),
path: result.file,
line: result.line,
}))
)
}

async sendInlineComment(git: GitDSL, inlineResults: DangerInlineResults): Promise<any> {
const comment = this.inlineCommentTemplate(inlineResults)
return await this.platform.createInlineComment(git, comment, inlineResults.file, inlineResults.line)
Expand Down
19 changes: 18 additions & 1 deletion source/runner/_tests/_executor.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,20 @@ describe("setup", () => {
expect(platform.createInlineComment).toBeCalled()
})

it("Creates multiple inline comments as a review", 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()
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()
})

it("Invalid inline comment is ignored if ignoreOutOfDiffComments is true", async () => {
const platform = new FakePlatform()
const config = Object.assign({}, defaultConfig, { ignoreOutOfDiffComments: true })
Expand Down Expand Up @@ -355,7 +369,10 @@ 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 Down

0 comments on commit fb630ec

Please sign in to comment.