From 1531c8b4fa66a23ba064e70a57a44a742cbbefa3 Mon Sep 17 00:00:00 2001 From: TomKristie <65685417+TomKristie@users.noreply.github.com> Date: Mon, 17 Aug 2020 14:05:42 -0400 Subject: [PATCH 01/10] feat(patch text to hunk bounds): support regex for patch texts (#83) * fix(patch text to hunk bounds): support regex for patch texts * more comments and more tests --- .../github-patch-format-handler.ts | 113 ++++++++++++++++++ src/types/index.ts | 28 +++++ test/github-regex-patch.ts | 59 +++++++++ 3 files changed, 200 insertions(+) create mode 100644 src/github-handler/comment-handler/github-patch-format-handler.ts create mode 100644 test/github-regex-patch.ts diff --git a/src/github-handler/comment-handler/github-patch-format-handler.ts b/src/github-handler/comment-handler/github-patch-format-handler.ts new file mode 100644 index 00000000..a090e133 --- /dev/null +++ b/src/github-handler/comment-handler/github-patch-format-handler.ts @@ -0,0 +1,113 @@ +// Copyright 2020 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import {Range} from '../../types'; + +const REGEX_INDEX_OF_UPDATED_HUNK = 2; + +/** + * @@ -, +, @@ + * i.e. @@ -132,7 +132,7 @@ + */ +const REGEX_MULTILINE_RANGE = /@@ -([0-9]+,[0-9]+) \+([0-9]+,[0-9]+) @@/g; + +/** + * @@ - +, @@ + * i.e. a deletion @@ -1 +0,0 @@ + */ +const REGEX_ONELINE_TO_MULTILINE_RANGE = /@@ -([0-9]+) \+([0-9]+,[0-9]+) @@/g; +/** + * @@ - + @@ + * i.e. @@ -1 +1 @@ + */ +const REGEX_ONELINE_RANGE = /@@ -([0-9]+) \+([0-9]+) @@/g; +/** + * @@ -, + @@ + * i.e. file creation @@ -0,0 +1 @@ + */ +const REGEX_MULTILINE_TO_ONELINE_RANGE = /@@ -([0-9]+,[0-9]+) \+([0-9]+) @@/g; + +/** + * Parses the GitHub line-based patch text + * Example output of one output of one regex exec + * + * '@@ -0,0 +1,12 @@\n', // original text + * '0,0', // original hunk + * '1,12', // new hunk + * index: 0, + * input: '@@ -0,0 +1,12 @@\n+Hello world%0A', + * groups: undefined + * @param {string} patchText + * @returns patch ranges + */ +export function getGitHubPatchRanges(patchText: string): Range[] { + const ranges: Range[] = []; + // CASE I: multiline patch ranges + // includes non-first single-line patches + // i.e. @@ -3,4 +3,4 @@ + // which only edits line 3, but is still a multiline patch range + for ( + let patch = REGEX_MULTILINE_RANGE.exec(patchText); + patch !== null; + patch = REGEX_MULTILINE_RANGE.exec(patchText) + ) { + // stricly interested in the updated/current github file content + const patchData = patch[REGEX_INDEX_OF_UPDATED_HUNK].split(','); + // the line number ranges of the updated text + const start = parseInt(patchData[0]); + const offset = parseInt(patchData[1]); + const range: Range = {start, end: start + offset}; + ranges.push(range); + } + // CASE II: oneline text becomes multiline text + for ( + let patch = REGEX_ONELINE_TO_MULTILINE_RANGE.exec(patchText); + patch !== null; + patch = REGEX_ONELINE_TO_MULTILINE_RANGE.exec(patchText) + ) { + // stricly interested in the updated/current github file content + const patchData = patch[REGEX_INDEX_OF_UPDATED_HUNK].split(','); + // the line number ranges of the updated text + const start = parseInt(patchData[0]); + const offset = parseInt(patchData[1]); + const range: Range = {start, end: start + offset}; + ranges.push(range); + } + // CASE III: first line of text updated + for ( + let patch = REGEX_ONELINE_RANGE.exec(patchText); + patch !== null; + patch = REGEX_ONELINE_RANGE.exec(patchText) + ) { + // stricly interested in the updated/current github file content + // the line number ranges of the updated text + const start = parseInt(patch[REGEX_INDEX_OF_UPDATED_HUNK]); + const range: Range = {start, end: start + 1}; + ranges.push(range); + } + // CASE IV: Multiline range is reduced to one line + // 0,0 constitutes a multi-line range + for ( + let patch = REGEX_MULTILINE_TO_ONELINE_RANGE.exec(patchText); + patch !== null; + patch = REGEX_MULTILINE_TO_ONELINE_RANGE.exec(patchText) + ) { + // stricly interested in the updated/current github file content + // the line number ranges of the updated text + const start = parseInt(patch[REGEX_INDEX_OF_UPDATED_HUNK]); + const range: Range = {start, end: start + 1}; + ranges.push(range); + } + return ranges; +} diff --git a/src/types/index.ts b/src/types/index.ts index aa401dcf..b5fa570b 100644 --- a/src/types/index.ts +++ b/src/types/index.ts @@ -116,3 +116,31 @@ export interface CreatePullRequest { // Whether or not maintainers can modify the PR. maintainersCanModify: boolean; } + +/** + * The file content of the original content and the patched content + */ +export interface RawContent { + readonly old_content: string; + readonly new_content: string; +} + +/** + * A range object defined by lower boundary as 'start' and upper boundary as 'end' + */ +export interface Range { + readonly start: number; + readonly end: number; +} + +/** + * The range of a patch along with the raw file content + */ +export interface Patch extends Range { + readonly raw_content: string; +} + +export type FilePatches = Map; +export type RawChanges = Map; +export type PatchText = Map; +export type FileRanges = Map; diff --git a/test/github-regex-patch.ts b/test/github-regex-patch.ts new file mode 100644 index 00000000..30406df8 --- /dev/null +++ b/test/github-regex-patch.ts @@ -0,0 +1,59 @@ +// Copyright 2020 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import {expect} from 'chai'; +import {describe, it} from 'mocha'; +import {getGitHubPatchRanges} from '../src/github-handler/comment-handler/github-patch-format-handler'; + +describe('Getting patch range from GitHub patch text', async () => { + it('parses original text for multiline modifies', () => { + const multiline_patch = + '@@ -1,2 +1,5 @@\n Hello world\n-!\n+Goodbye World\n+gOodBYE world\n+\n+Goodbye World'; + const ranges = getGitHubPatchRanges(multiline_patch); + expect(ranges[0].start).equals(1); + expect(ranges[0].end).equals(6); + expect(ranges.length).equals(1); + }); + it('parses original text for single line modifies at the start of the file', () => { + const first_line_patch = '@@ -1 +1 @@\n-Hello foo\n+'; + const ranges = getGitHubPatchRanges(first_line_patch); + expect(ranges[0].start).equals(1); + expect(ranges[0].end).equals(2); + expect(ranges.length).equals(1); + }); + it('parses original text for single line deletes', () => { + const single_line_to_multiline_format = '@@ -1 +0,0 @@\n-hello world'; + const ranges = getGitHubPatchRanges(single_line_to_multiline_format); + expect(ranges[0].start).equals(0); + expect(ranges[0].end).equals(0); + expect(ranges.length).equals(1); + }); + it('parses original text for single line file creations', () => { + const first_line_patch = "@@ -0,0 +1 @@\n+console.log('Hello World!');"; + const ranges = getGitHubPatchRanges(first_line_patch); + expect(ranges[0].start).equals(1); + expect(ranges[0].end).equals(2); + expect(ranges.length).equals(1); + }); + it('parses patch text with multiple patches', () => { + const first_line_patch = + '@@ -356,6 +356,7 @@ Hello\n Hello\n Hello\n Hello\n+Bye\n Hello\n Hello\n Hello\n@@ -6576,8 +6577,7 @@ Hello\n Hello\n Hello\n Hello\n-Hello\n-Hello\n+Bye\n Hello\n Hello\n Hello'; + const ranges = getGitHubPatchRanges(first_line_patch); + expect(ranges[0].start).equals(356); + expect(ranges[0].end).equals(363); + expect(ranges[1].start).equals(6577); + expect(ranges[1].end).equals(6584); + expect(ranges.length).equals(2); + }); +}); From 29f513667686208828933804fca83c9bee288f18 Mon Sep 17 00:00:00 2001 From: TomKristie <65685417+TomKristie@users.noreply.github.com> Date: Tue, 18 Aug 2020 16:34:54 -0400 Subject: [PATCH 02/10] fix(framework-core): core-library get remote patch ranges (#84) --- .../github-patch-format-handler.ts | 17 +- .../remote-patch-ranges-handler.ts | 125 ++++++ src/types/index.ts | 7 + test/github-regex-patch.ts | 57 ++- test/remote-github-patch-text.ts | 422 ++++++++++++++++++ 5 files changed, 615 insertions(+), 13 deletions(-) create mode 100644 src/github-handler/comment-handler/remote-patch-ranges-handler.ts create mode 100644 test/remote-github-patch-text.ts diff --git a/src/github-handler/comment-handler/github-patch-format-handler.ts b/src/github-handler/comment-handler/github-patch-format-handler.ts index a090e133..73c39fc4 100644 --- a/src/github-handler/comment-handler/github-patch-format-handler.ts +++ b/src/github-handler/comment-handler/github-patch-format-handler.ts @@ -12,7 +12,8 @@ // See the License for the specific language governing permissions and // limitations under the License. -import {Range} from '../../types'; +import {PatchSyntaxError, Range} from '../../types'; +import {logger} from '../../logger'; const REGEX_INDEX_OF_UPDATED_HUNK = 2; @@ -39,8 +40,9 @@ const REGEX_ONELINE_RANGE = /@@ -([0-9]+) \+([0-9]+) @@/g; const REGEX_MULTILINE_TO_ONELINE_RANGE = /@@ -([0-9]+,[0-9]+) \+([0-9]+) @@/g; /** - * Parses the GitHub line-based patch text - * Example output of one output of one regex exec + * Parses the GitHub line-based patch text. + * Throws an error if the patch text is undefined, null, or not a patch text. + * Example output of one output of one regex exec: * * '@@ -0,0 +1,12 @@\n', // original text * '0,0', // original hunk @@ -52,6 +54,9 @@ const REGEX_MULTILINE_TO_ONELINE_RANGE = /@@ -([0-9]+,[0-9]+) \+([0-9]+) @@/g; * @returns patch ranges */ export function getGitHubPatchRanges(patchText: string): Range[] { + if (typeof patchText !== 'string') { + throw new TypeError('GitHub patch text must be a string'); + } const ranges: Range[] = []; // CASE I: multiline patch ranges // includes non-first single-line patches @@ -109,5 +114,11 @@ export function getGitHubPatchRanges(patchText: string): Range[] { const range: Range = {start, end: start + 1}; ranges.push(range); } + if (!ranges.length) { + logger.error( + `Unexpected input patch text provided. Expected "${patchText}" to be of format @@ -[,] +[,] @@` + ); + throw new PatchSyntaxError('Unexpected patch text format'); + } return ranges; } diff --git a/src/github-handler/comment-handler/remote-patch-ranges-handler.ts b/src/github-handler/comment-handler/remote-patch-ranges-handler.ts new file mode 100644 index 00000000..22644fb3 --- /dev/null +++ b/src/github-handler/comment-handler/remote-patch-ranges-handler.ts @@ -0,0 +1,125 @@ +// Copyright 2020 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import {Octokit} from '@octokit/rest'; +import {FileRanges, PatchText, Range, RepoDomain} from '../../types'; +import {getGitHubPatchRanges} from './github-patch-format-handler'; +import {logger} from '../../logger'; + +/** + * For a pull request, get each remote file's patch text asynchronously + * Also get the list of files whose patch data could not be returned + * @param {Octokit} octokit the authenticated octokit instance + * @param {RepoDomain} remote the remote repository domain information + * @param {number} pullNumber the pull request number + * @param {number} pageSize the number of results to return per page + * @returns {Promise>} the stringified patch data for each file and the list of files whose patch data could not be resolved + */ +export async function getCurrentPullRequestPatches( + octokit: Octokit, + remote: RepoDomain, + pullNumber: number, + pageSize: number +): Promise<{patches: PatchText; filesMissingPatch: string[]}> { + // TODO support pagination + const filesMissingPatch: string[] = []; + const files = ( + await octokit.pulls.listFiles({ + owner: remote.owner, + repo: remote.repo, + pull_number: pullNumber, + per_page: pageSize, + }) + ).data; + const patches: PatchText = new Map(); + if (files.length === 0) { + logger.error( + `0 file results have returned from list files query for Pull Request #${pullNumber}. Cannot make suggestions on an empty Pull Request` + ); + throw Error('Empty Pull Request'); + } + files.forEach(file => { + if (file.patch === undefined) { + // files whose patch is too large do not return the patch text by default + // TODO handle file patches that are too large + logger.warn( + `File ${file.filename} may have a patch that is too large to display patch object.` + ); + filesMissingPatch.push(file.filename); + } else { + patches.set(file.filename, file.patch); + } + }); + if (patches.size === 0) { + logger.warn( + '0 patches have been returned. This could be because the patch results were too large to return.' + ); + } + return {patches, filesMissingPatch}; +} + +/** + * Given the patch text (for a whole file) for each file, + * get each file's hunk's (part of a file's) range + * @param {PatchText} validPatches patch text from the remote github file + * @returns {FileRanges} the range of the remote patch + */ +export function patchTextToRanges(validPatches: PatchText): FileRanges { + const allValidLineRanges: FileRanges = new Map(); + validPatches.forEach((patch, filename) => { + // get each hunk range in the patch string + try { + const validLineRanges = getGitHubPatchRanges(patch); + allValidLineRanges.set(filename, validLineRanges); + } catch (err) { + logger.info( + `Failed to parse the patch of file ${filename}. Resuming parsing patches...` + ); + } + }); + return allValidLineRanges; +} + +/** + * For a pull request, get each remote file's current patch range to identify the scope of each patch as a Map, + * as well as a list of files that cannot have suggestions applied to it within the Pull Request. + * The list of files are a subset of the total out-of-scope files. + * @param {Octokit} octokit the authenticated octokit instance + * @param {RepoDomain} remote the remote repository domain information + * @param {number} pullNumber the pull request number + * @param {number} pageSize the number of files to return per pull request list files query + * @returns {Promise>} the scope of each file in the pull request and the list of files whose patch data could not be resolved + */ +export async function getPullRequestScope( + octokit: Octokit, + remote: RepoDomain, + pullNumber: number, + pageSize: number +): Promise<{validFileLines: FileRanges; invalidFiles: string[]}> { + try { + const {patches, filesMissingPatch} = await getCurrentPullRequestPatches( + octokit, + remote, + pullNumber, + pageSize + ); + const validFileLines = patchTextToRanges(patches); + return {validFileLines, invalidFiles: filesMissingPatch}; + } catch (err) { + logger.error( + 'Could not convert the remote pull request file patch text to ranges' + ); + throw err; + } +} diff --git a/src/types/index.ts b/src/types/index.ts index b5fa570b..fb2028a0 100644 --- a/src/types/index.ts +++ b/src/types/index.ts @@ -117,6 +117,13 @@ export interface CreatePullRequest { maintainersCanModify: boolean; } +export class PatchSyntaxError extends Error { + constructor(message: string) { + super(message); + this.name = 'PatchSyntaxError'; + } +} + /** * The file content of the original content and the patched content */ diff --git a/test/github-regex-patch.ts b/test/github-regex-patch.ts index 30406df8..6e8b7ead 100644 --- a/test/github-regex-patch.ts +++ b/test/github-regex-patch.ts @@ -15,45 +15,82 @@ import {expect} from 'chai'; import {describe, it} from 'mocha'; import {getGitHubPatchRanges} from '../src/github-handler/comment-handler/github-patch-format-handler'; +import {PatchSyntaxError} from '../src/types'; describe('Getting patch range from GitHub patch text', async () => { it('parses original text for multiline modifies', () => { - const multiline_patch = + const multilinePatch = '@@ -1,2 +1,5 @@\n Hello world\n-!\n+Goodbye World\n+gOodBYE world\n+\n+Goodbye World'; - const ranges = getGitHubPatchRanges(multiline_patch); + const ranges = getGitHubPatchRanges(multilinePatch); expect(ranges[0].start).equals(1); expect(ranges[0].end).equals(6); expect(ranges.length).equals(1); }); it('parses original text for single line modifies at the start of the file', () => { - const first_line_patch = '@@ -1 +1 @@\n-Hello foo\n+'; - const ranges = getGitHubPatchRanges(first_line_patch); + const firstLinePatch = '@@ -1 +1 @@\n-Hello foo\n+'; + const ranges = getGitHubPatchRanges(firstLinePatch); expect(ranges[0].start).equals(1); expect(ranges[0].end).equals(2); expect(ranges.length).equals(1); }); it('parses original text for single line deletes', () => { - const single_line_to_multiline_format = '@@ -1 +0,0 @@\n-hello world'; - const ranges = getGitHubPatchRanges(single_line_to_multiline_format); + const singleLineToMultilineFormat = '@@ -1 +0,0 @@\n-hello world'; + const ranges = getGitHubPatchRanges(singleLineToMultilineFormat); expect(ranges[0].start).equals(0); expect(ranges[0].end).equals(0); expect(ranges.length).equals(1); }); it('parses original text for single line file creations', () => { - const first_line_patch = "@@ -0,0 +1 @@\n+console.log('Hello World!');"; - const ranges = getGitHubPatchRanges(first_line_patch); + const firstLinePatch = "@@ -0,0 +1 @@\n+console.log('Hello World!');"; + const ranges = getGitHubPatchRanges(firstLinePatch); expect(ranges[0].start).equals(1); expect(ranges[0].end).equals(2); expect(ranges.length).equals(1); }); it('parses patch text with multiple patches', () => { - const first_line_patch = + const multiplePatch = '@@ -356,6 +356,7 @@ Hello\n Hello\n Hello\n Hello\n+Bye\n Hello\n Hello\n Hello\n@@ -6576,8 +6577,7 @@ Hello\n Hello\n Hello\n Hello\n-Hello\n-Hello\n+Bye\n Hello\n Hello\n Hello'; - const ranges = getGitHubPatchRanges(first_line_patch); + const ranges = getGitHubPatchRanges(multiplePatch); expect(ranges[0].start).equals(356); expect(ranges[0].end).equals(363); expect(ranges[1].start).equals(6577); expect(ranges[1].end).equals(6584); expect(ranges.length).equals(2); }); + it('throws an error when the patch text is null', () => { + const patch = (null as unknown) as string; + try { + getGitHubPatchRanges(patch); + expect.fail('Should have filed because an invalid input was given'); + } catch (err) { + expect(err instanceof TypeError).equals(true); + } + }); + it('throws an error when the patch text is undefined', () => { + const patch = (undefined as unknown) as string; + try { + getGitHubPatchRanges(patch); + expect.fail('Should have filed because an invalid input was given'); + } catch (err) { + expect(err instanceof TypeError).equals(true); + } + }); + it('throws an error when the patch text does not contain hunk ranges and contains other text', () => { + const patch = '@ 1 1 @ invalid patch because it needs a + and - sign'; + try { + getGitHubPatchRanges(patch); + expect.fail('Should have filed because an invalid input was given'); + } catch (err) { + expect(err instanceof PatchSyntaxError).equals(true); + } + }); + it('throws an error when the patch text does not contain hunk ranges because it is an empty string', () => { + const patch = ''; + try { + getGitHubPatchRanges(patch); + expect.fail('Should have filed because an invalid input was given'); + } catch (err) { + expect(err instanceof PatchSyntaxError).equals(true); + } + }); }); diff --git a/test/remote-github-patch-text.ts b/test/remote-github-patch-text.ts new file mode 100644 index 00000000..af4561f8 --- /dev/null +++ b/test/remote-github-patch-text.ts @@ -0,0 +1,422 @@ +// Copyright 2020 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import {expect} from 'chai'; +import {describe, it, before, afterEach} from 'mocha'; +import {setup} from './util'; +import * as sinon from 'sinon'; +import { + patchTextToRanges, + getCurrentPullRequestPatches, + getPullRequestScope, +} from '../src/github-handler/comment-handler/remote-patch-ranges-handler'; +import {Octokit} from '@octokit/rest'; +import {logger} from '../src/logger'; + +before(() => { + setup(); +}); + +describe('getCurrentPullRequestPatches', () => { + const sandbox = sinon.createSandbox(); + afterEach(() => { + sandbox.restore(); + }); + const upstream = {owner: 'upstream-owner', repo: 'upstream-repo'}; + const pullNumber = 10; + const pageSize = 80; + const octokit = new Octokit({}); + + it('Calls Octokit with the correct values', async () => { + // setup + const listFilesOfPRResult = { + headers: {}, + status: 200, + url: 'http://fake-url.com', + data: [ + { + sha: 'a1d470fa4d7b04450715e3e02d240a34517cd988', + filename: 'Readme.md', + status: 'modified', + additions: 4, + deletions: 1, + changes: 5, + blob_url: + 'https://github.com/TomKristie/HelloWorld/blob/eb53f3871f56e8dd6321e44621fe6ac2da1bc120/Readme.md', + raw_url: + 'https://github.com/TomKristie/HelloWorld/raw/eb53f3871f56e8dd6321e44621fe6ac2da1bc120/Readme.md', + contents_url: + 'https://api.github.com/repos/TomKristie/HelloWorld/contents/Readme.md?ref=eb53f3871f56e8dd6321e44621fe6ac2da1bc120', + patch: + '@@ -1,2 +1,5 @@\n Hello world\n-!\n+Goodbye World\n+gOodBYE world\n+\n+Goodbye World', + }, + ], + }; + const stub = sandbox + .stub(octokit.pulls, 'listFiles') + .resolves(listFilesOfPRResult); + + // tests + await getCurrentPullRequestPatches(octokit, upstream, pullNumber, pageSize); + sandbox.assert.calledOnceWithExactly(stub, { + owner: upstream.owner, + repo: upstream.repo, + pull_number: pullNumber, + per_page: pageSize, + }); + }); + it('Returns all the valid patches', async () => { + // setup + const listFilesOfPRResult = { + headers: {}, + status: 200, + url: 'http://fake-url.com', + data: [ + { + sha: 'a1d470fa4d7b04450715e3e02d240a34517cd988', + filename: 'Readme.md', + status: 'modified', + additions: 4, + deletions: 1, + changes: 5, + blob_url: + 'https://github.com/TomKristie/HelloWorld/blob/eb53f3871f56e8dd6321e44621fe6ac2da1bc120/Readme.md', + raw_url: + 'https://github.com/TomKristie/HelloWorld/raw/eb53f3871f56e8dd6321e44621fe6ac2da1bc120/Readme.md', + contents_url: + 'https://api.github.com/repos/TomKristie/HelloWorld/contents/Readme.md?ref=eb53f3871f56e8dd6321e44621fe6ac2da1bc120', + patch: + '@@ -1,2 +1,5 @@\n Hello world\n-!\n+Goodbye World\n+gOodBYE world\n+\n+Goodbye World', + }, + { + sha: '8b137891791fe96927ad78e64b0aad7bded08bdc', + filename: 'foo/foo.txt', + status: 'modified', + additions: 1, + deletions: 1, + changes: 2, + blob_url: + 'https://github.com/TomKristie/HelloWorld/blob/eb53f3871f56e8dd6321e44621fe6ac2da1bc120/foo/foo.txt', + raw_url: + 'https://github.com/TomKristie/HelloWorld/raw/eb53f3871f56e8dd6321e44621fe6ac2da1bc120/foo/foo.txt', + contents_url: + 'https://api.github.com/repos/TomKristie/HelloWorld/contents/foo/foo.txt?ref=eb53f3871f56e8dd6321e44621fe6ac2da1bc120', + patch: '@@ -1 +1 @@\n-Hello foo\n+', + }, + { + sha: '3b18e512dba79e4c8300dd08aeb37f8e728b8dad', + filename: 'helloworld.txt', + status: 'removed', + additions: 0, + deletions: 1, + changes: 1, + blob_url: + 'https://github.com/TomKristie/HelloWorld/blob/f5da827a725a701302da7db2da16b1678f52fdcc/helloworld.txt', + raw_url: + 'https://github.com/TomKristie/HelloWorld/raw/f5da827a725a701302da7db2da16b1678f52fdcc/helloworld.txt', + contents_url: + 'https://api.github.com/repos/TomKristie/HelloWorld/contents/helloworld.txt?ref=f5da827a725a701302da7db2da16b1678f52fdcc', + patch: '@@ -1 +0,0 @@\n-hello world', + }, + ], + }; + sandbox.stub(octokit.pulls, 'listFiles').resolves(listFilesOfPRResult); + + // tests + const {patches, filesMissingPatch} = await getCurrentPullRequestPatches( + octokit, + upstream, + pullNumber, + pageSize + ); + expect(patches.size).equals(3); + expect(patches.get(listFilesOfPRResult.data[0].filename)).equals( + '@@ -1,2 +1,5 @@\n Hello world\n-!\n+Goodbye World\n+gOodBYE world\n+\n+Goodbye World' + ); + expect(patches.get(listFilesOfPRResult.data[1].filename)).equals( + '@@ -1 +1 @@\n-Hello foo\n+' + ); + expect(patches.get(listFilesOfPRResult.data[2].filename)).equals( + '@@ -1 +0,0 @@\n-hello world' + ); + expect(filesMissingPatch.length).equals(0); + }); + it('Passes the error message up from octokit when octokit fails', async () => { + // setup + const errorMsg = 'Error message'; + sandbox.stub(octokit.pulls, 'listFiles').rejects(Error(errorMsg)); + try { + await getCurrentPullRequestPatches( + octokit, + upstream, + pullNumber, + pageSize + ); + expect.fail( + 'The getCurrentPulLRequestPatches function should have failed because Octokit failed.' + ); + } catch (err) { + expect(err.message).to.equal(errorMsg); + } + }); + it('Throws when there is no list file data returned from octokit', async () => { + // setup + const listFilesOfPRResult = { + headers: {}, + status: 200, + url: 'http://fake-url.com', + data: [], + }; + sandbox.stub(octokit.pulls, 'listFiles').resolves(listFilesOfPRResult); + try { + await getCurrentPullRequestPatches( + octokit, + upstream, + pullNumber, + pageSize + ); + expect.fail( + 'The getCurrentPulLRequestPatches function should have failed because Octokit failed.' + ); + } catch (err) { + expect(err.message).to.equal('Empty Pull Request'); + } + }); + it('Does not error when there is list file data but no patch data', async () => { + // setup + const listFilesOfPRResult = { + headers: {}, + status: 200, + url: 'http://fake-url.com', + data: [ + { + sha: 'a1d470fa4d7b04450715e3e02d240a34517cd988', + filename: 'Readme.md', + status: 'modified', + additions: 4, + deletions: 1, + changes: 5, + blob_url: + 'https://github.com/TomKristie/HelloWorld/blob/eb53f3871f56e8dd6321e44621fe6ac2da1bc120/Readme.md', + raw_url: + 'https://github.com/TomKristie/HelloWorld/raw/eb53f3871f56e8dd6321e44621fe6ac2da1bc120/Readme.md', + contents_url: + 'https://api.github.com/repos/TomKristie/HelloWorld/contents/Readme.md?ref=eb53f3871f56e8dd6321e44621fe6ac2da1bc120', + }, + ], + }; + + /* eslint-disable @typescript-eslint/no-explicit-any */ + // these are real results from calling listFiles API and are a valid GitHub return type, but octokit type definition says otherwise + // cannot force another type cast since the GitHub API return types are not importable + // unknown type cast not allowed + const stub = sandbox + .stub(logger, 'warn') + .resolves(listFilesOfPRResult as any); + sandbox + .stub(octokit.pulls, 'listFiles') + .resolves(listFilesOfPRResult as any); + + // tests + const {filesMissingPatch} = await getCurrentPullRequestPatches( + octokit, + upstream, + pullNumber, + pageSize + ); + sandbox.assert.called(stub); + expect(filesMissingPatch.length).equals(1); + expect(filesMissingPatch[0]).equals('Readme.md'); + }); +}); + +describe('patchTextToRanges', () => { + it('Returns an empty range file record when there is no patch text', () => { + const patchText = new Map(); + const ranges = patchTextToRanges(patchText); + expect(ranges.size).equals(0); + }); + it('Does not throw an error when the patch text is an empty string and does not add the patch to the valid range map', () => { + const patchText = new Map(); + patchText.set('invalid-patch.txt', ''); + const ranges = patchTextToRanges(patchText); + expect(ranges.get('invalid-patch.txt')).equals(undefined); + }); + it('Does not throw an error when the patch text is a string that does not contain patch data add the patch to the valid range map', () => { + const patchText = new Map(); + patchText.set('invalid-patch.txt', '@@ this is some invalid patch data @@'); + const ranges = patchTextToRanges(patchText); + expect(ranges.get('invalid-patch.txt')).equals(undefined); + }); + it('Calculates ranges with an inclusive lower bound and an exclusive upper bound', () => { + const multilinePatch = + '@@ -1,2 +1,5 @@\n Hello world\n-!\n+Goodbye World\n+gOodBYE world\n+\n+Goodbye World'; + const multilineFileName = 'multi-line-patch.txt'; + const firstLinePatch = "@@ -0,0 +1 @@\n+console.log('Hello World!');"; + const multiToSingleLineFileName = 'multi-to-single-line-patch.txt'; + const patchText = new Map(); + patchText.set(multilineFileName, multilinePatch); + patchText.set(multiToSingleLineFileName, firstLinePatch); + const ranges = patchTextToRanges(patchText); + expect(ranges.get(multilineFileName)).not.equals(null); + expect(ranges.get(multilineFileName)?.length).equals(1); + expect(ranges.get(multilineFileName)![0].start).equals(1); + expect(ranges.get(multilineFileName)![0].end).not.equals(5); + expect(ranges.get(multilineFileName)![0].end).equals(6); + expect(ranges.get(multiToSingleLineFileName)).not.equals(null); + expect(ranges.get(multiToSingleLineFileName)?.length).equals(1); + expect(ranges.get(multiToSingleLineFileName)![0].start).equals(1); + expect(ranges.get(multiToSingleLineFileName)![0].end).not.equals(1); + expect(ranges.get(multiToSingleLineFileName)![0].end).equals(2); + }); + it('Returns a single range file record when there is a single multiline patch hunk', () => { + const multilinePatch = + '@@ -1,2 +1,5 @@\n Hello world\n-!\n+Goodbye World\n+gOodBYE world\n+\n+Goodbye World'; + const multilineFileName = 'multi-line-patch.txt'; + const patchText = new Map(); + patchText.set(multilineFileName, multilinePatch); + const ranges = patchTextToRanges(patchText); + expect(ranges.size).equals(1); + expect(ranges.get(multilineFileName)).not.equals(null); + expect(ranges.get(multilineFileName)?.length).equals(1); + expect(ranges.get(multilineFileName)![0].start).equals(1); + expect(ranges.get(multilineFileName)![0].end).equals(6); + }); + it('Returns a single range file record when there is a single 1 line patch hunk', () => { + const firstLinePatch = '@@ -1 +1 @@\n-Hello foo\n+'; + const singleLineFileName = 'single-line-patch.txt'; + const patchText = new Map(); + patchText.set(singleLineFileName, firstLinePatch); + const ranges = patchTextToRanges(patchText); + expect(ranges.size).equals(1); + expect(ranges.get(singleLineFileName)).not.equals(null); + expect(ranges.get(singleLineFileName)?.length).equals(1); + expect(ranges.get(singleLineFileName)![0].start).equals(1); + expect(ranges.get(singleLineFileName)![0].end).equals(2); + }); + it('Returns a single range file record when there is a single 1 line to multiline line patch hunk', () => { + const singleToMultilineFormat = '@@ -1 +0,0 @@\n-hello world'; + const singleToMultilineFileName = 'single-line-to-multiline-patch.txt'; + const patchText = new Map(); + patchText.set(singleToMultilineFileName, singleToMultilineFormat); + const ranges = patchTextToRanges(patchText); + expect(ranges.size).equals(1); + expect(ranges.get(singleToMultilineFileName)).not.equals(null); + expect(ranges.get(singleToMultilineFileName)?.length).equals(1); + expect(ranges.get(singleToMultilineFileName)![0].start).equals(0); + expect(ranges.get(singleToMultilineFileName)![0].end).equals(0); + }); + it('Returns a single range file record when there is a single multiline to 1 line patch hunk', () => { + const firstLinePatch = "@@ -0,0 +1 @@\n+console.log('Hello World!');"; + const multiToSingleLineFileName = 'multi-to-single-line-patch.txt'; + const patchText = new Map(); + patchText.set(multiToSingleLineFileName, firstLinePatch); + const ranges = patchTextToRanges(patchText); + expect(ranges.size).equals(1); + expect(ranges.get(multiToSingleLineFileName)).not.equals(null); + expect(ranges.get(multiToSingleLineFileName)?.length).equals(1); + expect(ranges.get(multiToSingleLineFileName)![0].start).equals(1); + expect(ranges.get(multiToSingleLineFileName)![0].end).equals(2); + }); + it('Returns a single range file record when there is a single multiline to 1 line patch hunk', () => { + const multiplePatches = + '@@ -356,6 +356,7 @@ Hello\n Hello\n Hello\n Hello\n+Bye\n Hello\n Hello\n Hello\n@@ -6576,8 +6577,7 @@ Hello\n Hello\n Hello\n Hello\n-Hello\n-Hello\n+Bye\n Hello\n Hello\n Hello'; + const multiplePatchesFileName = 'multiple-patches.txt'; + const patchText = new Map(); + patchText.set(multiplePatchesFileName, multiplePatches); + const ranges = patchTextToRanges(patchText); + expect(ranges.size).equals(1); + expect(ranges.get(multiplePatchesFileName)).not.equals(null); + expect(ranges.get(multiplePatchesFileName)?.length).equals(2); + expect(ranges.get(multiplePatchesFileName)![0].start).equals(356); + expect(ranges.get(multiplePatchesFileName)![0].end).equals(363); + expect(ranges.get(multiplePatchesFileName)![1].start).equals(6577); + expect(ranges.get(multiplePatchesFileName)![1].end).equals(6584); + }); +}); + +describe('getPullRequestScope', () => { + const upstream = {owner: 'upstream-owner', repo: 'upstream-repo'}; + const pullNumber = 10; + const pageSize = 80; + const octokit = new Octokit({}); + const sandbox = sinon.createSandbox(); + afterEach(() => { + sandbox.restore(); + }); + + it('Returns the correct values when octokit and patch text parsing function execute properly', async () => { + // setup + const listFilesOfPRResult = { + headers: {}, + status: 200, + url: 'http://fake-url.com', + data: [ + { + sha: 'a1d470fa4d7b04450715e3e02d240a34517cd988', + filename: 'Readme.md', + status: 'modified', + additions: 4, + deletions: 1, + changes: 5, + blob_url: + 'https://github.com/TomKristie/HelloWorld/blob/eb53f3871f56e8dd6321e44621fe6ac2da1bc120/Readme.md', + raw_url: + 'https://github.com/TomKristie/HelloWorld/raw/eb53f3871f56e8dd6321e44621fe6ac2da1bc120/Readme.md', + contents_url: + 'https://api.github.com/repos/TomKristie/HelloWorld/contents/Readme.md?ref=eb53f3871f56e8dd6321e44621fe6ac2da1bc120', + patch: + '@@ -1,2 +1,5 @@\n Hello world\n-!\n+Goodbye World\n+gOodBYE world\n+\n+Goodbye World', + }, + ], + }; + const stub = sandbox + .stub(octokit.pulls, 'listFiles') + .resolves(listFilesOfPRResult); + + // tests + const { + validFileLines, + invalidFiles: filesMissingPatch, + } = await getPullRequestScope(octokit, upstream, pullNumber, pageSize); + sandbox.assert.calledOnceWithExactly(stub, { + owner: upstream.owner, + repo: upstream.repo, + pull_number: pullNumber, + per_page: pageSize, + }); + expect(validFileLines.size).equals(1); + expect(validFileLines.get('Readme.md')).not.equals(null); + expect(validFileLines.get('Readme.md')?.length).equals(1); + expect(validFileLines.get('Readme.md')![0].start).equals(1); + expect(validFileLines.get('Readme.md')![0].end).equals(6); + expect(filesMissingPatch.length).equals(0); + }); + + it('Passes up the error when a sub-method fails', async () => { + // setup + const errorMsg = 'Test error for list files'; + sandbox.stub(octokit.pulls, 'listFiles').rejects(new Error(errorMsg)); + + // tests + try { + await getPullRequestScope(octokit, upstream, pullNumber, pageSize); + expect.fail( + 'The getPullRequestScope function should have failed because Octokit failed.' + ); + } catch (err) { + expect(err.message).equals(errorMsg); + } + }); +}); From bce5ef530fe1671dd5cf3d268845c80f870a7594 Mon Sep 17 00:00:00 2001 From: TomKristie <65685417+TomKristie@users.noreply.github.com> Date: Thu, 20 Aug 2020 17:52:23 -0400 Subject: [PATCH 03/10] fix(framework-core): given files old content and new content, compute the valid hunks (#86) * fix(framework-core): parse raw changes to ranges --- package.json | 2 + src/github-handler/comment-handler/index.ts | 43 +++ .../remote-patch-ranges-handler.ts | 20 +- .../suggestion-patch-handler/index.ts | 73 +++++ .../scope-suggestion-hunks-handler.ts | 163 ++++++++++ .../suggestion-hunk-handler.ts | 69 ++++ src/types/index.ts | 18 +- test/scope-suggestion-hunks.ts | 294 ++++++++++++++++++ test/suggestion-hunk.ts | 123 ++++++++ test/suggestion-patch.ts | 150 +++++++++ 10 files changed, 938 insertions(+), 17 deletions(-) create mode 100644 src/github-handler/comment-handler/index.ts create mode 100644 src/github-handler/comment-handler/suggestion-patch-handler/index.ts create mode 100644 src/github-handler/comment-handler/suggestion-patch-handler/scope-suggestion-hunks-handler.ts create mode 100644 src/github-handler/comment-handler/suggestion-patch-handler/suggestion-hunk-handler.ts create mode 100644 test/scope-suggestion-hunks.ts create mode 100644 test/suggestion-hunk.ts create mode 100644 test/suggestion-patch.ts diff --git a/package.json b/package.json index b55fc687..2fe9310f 100644 --- a/package.json +++ b/package.json @@ -44,6 +44,7 @@ "@octokit/rest": "^18.0.1", "@types/yargs": "^15.0.5", "async-retry": "^1.3.1", + "diff": "^4.0.2", "glob": "^7.1.6", "pino": "^6.3.2", "yargs": "^15.4.1" @@ -53,6 +54,7 @@ "@microsoft/api-extractor": "^7.8.10", "@types/async-retry": "^1.4.2", "@types/chai": "^4.2.11", + "@types/diff": "^4.0.2", "@types/mocha": "^8.0.0", "@types/node": "^14.0.20", "@types/pino": "^6.3.0", diff --git a/src/github-handler/comment-handler/index.ts b/src/github-handler/comment-handler/index.ts new file mode 100644 index 00000000..d634f593 --- /dev/null +++ b/src/github-handler/comment-handler/index.ts @@ -0,0 +1,43 @@ +// Copyright 2020 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import {RawContent, RepoDomain} from '../../types'; +import {getPullRequestScope} from './remote-patch-ranges-handler'; +import {Octokit} from '@octokit/rest'; +import {getSuggestionPatches} from './suggestion-patch-handler'; + +/** + * Comment on a Pull Request + * @param {Octokit} octokit authenticated octokit isntance + * @param {RepoDomain} remote the Pull Request repository + * @param {number} pullNumber the Pull Request number + * @param {number} pageSize the number of files to comment on // TODO pagination + * @param {Map} rawChanges the old and new contents of the files to suggest + */ +export async function comment( + octokit: Octokit, + remote: RepoDomain, + pullNumber: number, + pageSize: number, + rawChanges: Map +): Promise { + const {invalidFiles, validFileLines} = await getPullRequestScope( + octokit, + remote, + pullNumber, + pageSize + ); + getSuggestionPatches(rawChanges, invalidFiles, validFileLines); + // TODO get the raw file content from the new range of the hunk and make a PR with that +} diff --git a/src/github-handler/comment-handler/remote-patch-ranges-handler.ts b/src/github-handler/comment-handler/remote-patch-ranges-handler.ts index 22644fb3..e5d32b03 100644 --- a/src/github-handler/comment-handler/remote-patch-ranges-handler.ts +++ b/src/github-handler/comment-handler/remote-patch-ranges-handler.ts @@ -13,7 +13,7 @@ // limitations under the License. import {Octokit} from '@octokit/rest'; -import {FileRanges, PatchText, Range, RepoDomain} from '../../types'; +import {Range, RepoDomain} from '../../types'; import {getGitHubPatchRanges} from './github-patch-format-handler'; import {logger} from '../../logger'; @@ -31,7 +31,7 @@ export async function getCurrentPullRequestPatches( remote: RepoDomain, pullNumber: number, pageSize: number -): Promise<{patches: PatchText; filesMissingPatch: string[]}> { +): Promise<{patches: Map; filesMissingPatch: string[]}> { // TODO support pagination const filesMissingPatch: string[] = []; const files = ( @@ -42,7 +42,7 @@ export async function getCurrentPullRequestPatches( per_page: pageSize, }) ).data; - const patches: PatchText = new Map(); + const patches: Map = new Map(); if (files.length === 0) { logger.error( `0 file results have returned from list files query for Pull Request #${pullNumber}. Cannot make suggestions on an empty Pull Request` @@ -72,11 +72,13 @@ export async function getCurrentPullRequestPatches( /** * Given the patch text (for a whole file) for each file, * get each file's hunk's (part of a file's) range - * @param {PatchText} validPatches patch text from the remote github file - * @returns {FileRanges} the range of the remote patch + * @param {Map} validPatches patch text from the remote github file + * @returns {Map} the range of the remote patch */ -export function patchTextToRanges(validPatches: PatchText): FileRanges { - const allValidLineRanges: FileRanges = new Map(); +export function patchTextToRanges( + validPatches: Map +): Map { + const allValidLineRanges: Map = new Map(); validPatches.forEach((patch, filename) => { // get each hunk range in the patch string try { @@ -99,14 +101,14 @@ export function patchTextToRanges(validPatches: PatchText): FileRanges { * @param {RepoDomain} remote the remote repository domain information * @param {number} pullNumber the pull request number * @param {number} pageSize the number of files to return per pull request list files query - * @returns {Promise>} the scope of each file in the pull request and the list of files whose patch data could not be resolved + * @returns {Promise, string[]>>} the scope of each file in the pull request and the list of files whose patch data could not be resolved */ export async function getPullRequestScope( octokit: Octokit, remote: RepoDomain, pullNumber: number, pageSize: number -): Promise<{validFileLines: FileRanges; invalidFiles: string[]}> { +): Promise<{validFileLines: Map; invalidFiles: string[]}> { try { const {patches, filesMissingPatch} = await getCurrentPullRequestPatches( octokit, diff --git a/src/github-handler/comment-handler/suggestion-patch-handler/index.ts b/src/github-handler/comment-handler/suggestion-patch-handler/index.ts new file mode 100644 index 00000000..caed4849 --- /dev/null +++ b/src/github-handler/comment-handler/suggestion-patch-handler/index.ts @@ -0,0 +1,73 @@ +// Copyright 2020 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import {getRawSuggestionHunks} from './suggestion-hunk-handler'; +import { + getInScopeHunks, + getInScopeByFileName, + getOutOfScopeByFileName, + mergeOutOfScopeSuggestions, +} from './scope-suggestion-hunks-handler'; +import {RawContent, Hunk, Range, Patch} from '../../../types'; + +/** + * Get the in scope (of the corresponding pull request's) hunks and files + * @param {Map} rawChanges the raw old content and new content of a file + * @param {string[]} invalidFiles list of invalid files + * @param {Map} validFileLines a map of each file's in scope lines for a Pull Request + */ +export function getValidSuggestionHunks( + rawChanges: Map, + invalidFiles: string[], + validFileLines: Map +): { + inScopeSuggestions: Map; + outOfScopeSuggestions: Map; +} { + const totalfileHunks = getRawSuggestionHunks(rawChanges); + const outofScopeByFilename = getOutOfScopeByFileName( + invalidFiles, + totalfileHunks + ); + const inScopeByFileName = getInScopeByFileName(invalidFiles, totalfileHunks); + + const scopifiedHunks = getInScopeHunks(validFileLines, inScopeByFileName); + const outOfScopeSuggestions = mergeOutOfScopeSuggestions( + outofScopeByFilename, + scopifiedHunks.outOfScopeFilesHunks + ); + return { + inScopeSuggestions: scopifiedHunks.inScopeFilesHunks, + outOfScopeSuggestions, + }; +} + +/** + * Get the range of the old version of every file and the corresponding new text for that range + * whose old and new contents differ, under the constraints that the file + * is in scope for Pull Request, as well as its lines. + * @param {Map} rawChanges the raw old content and new content of a file + * @param {string[]} invalidFiles list of invalid files + * @param {Map} validFileLines a map of each file's in scope lines for a Pull Request + */ +export function getSuggestionPatches( + rawChanges: Map, + invalidFiles: string[], + validFileLines: Map +): Map { + const filePatches: Map = new Map(); + getValidSuggestionHunks(rawChanges, invalidFiles, validFileLines); + // TODO get patches from getValidSuggestionHunks output + return filePatches; +} diff --git a/src/github-handler/comment-handler/suggestion-patch-handler/scope-suggestion-hunks-handler.ts b/src/github-handler/comment-handler/suggestion-patch-handler/scope-suggestion-hunks-handler.ts new file mode 100644 index 00000000..6431822a --- /dev/null +++ b/src/github-handler/comment-handler/suggestion-patch-handler/scope-suggestion-hunks-handler.ts @@ -0,0 +1,163 @@ +// Copyright 2020 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import {Hunk, Range} from '../../../types'; + +/** + * Given a value and a list of ranges, find the index of the range domain + * that the value lies within. + * Return -1 otherwise. + * We expect the ranges to be ascending, and disjoint. + * @param {Range[]} ranges a list of range objects with a start value and end value + * @param {number} value the value to search for in the range list + * @param {number} from the lower bound of the search indices. (optional) + */ +export function findRange(ranges: Range[], value: number, from = 0): number { + function binarySearch(lo: number, hi: number): number { + const mid = Math.floor((hi + lo) / 2); + if (lo > hi) { + return -1; + } else if (ranges[mid].start > value) { + return binarySearch(lo, mid - 1); + } else if (ranges[mid].end < value) { + return binarySearch(mid + 1, hi); + } else { + return mid; + } + } + return binarySearch(from, ranges.length - 1); +} + +/** + * Get a subset of the hunks who exactly fit within a valid hunk scope. + * Also get the converse of the set of hunks that are invalid. + * @param {Range[]} scope the list of valid ranges that a hunk can fit between + * @param {Hunk[]} suggestedHunks the hunks for the file to suggest + */ +function getValidSuggestionHunks( + scope: Range[], + suggestedHunks: Hunk[] +): {inScopeHunks: Hunk[]; outOfScopeHunks: Hunk[]} { + const inScopeHunks: Hunk[] = []; + const outOfScopeHunks: Hunk[] = []; + let lastIndex = 0; + suggestedHunks.forEach(suggestedHunk => { + const startIndex = findRange(scope, suggestedHunk.oldStart, lastIndex); + const endIndex = findRange(scope, suggestedHunk.oldEnd, lastIndex); + lastIndex = endIndex; + // if the suggested range is not a subset of a single valid range, then skip + if (startIndex < 0 || endIndex < 0 || endIndex !== startIndex) { + outOfScopeHunks.push(suggestedHunk); + } else { + inScopeHunks.push(suggestedHunk); + } + }); + return {inScopeHunks, outOfScopeHunks}; +} + +/** + * Return all the files hunks whose scope lies inside a valid scope/ranges, and + * return files that have a non-empty hunk scope + * also return invalid hunks + * @param {Map} changeScope a map of each file's valid scope/ranges + * @param {Map} suggestions the hunks for each file to suggest + */ +export function getInScopeHunks( + changeScope: Map, + suggestions: Map +): { + inScopeFilesHunks: Map; + outOfScopeFilesHunks: Map; +} { + const inScopeFilesHunks: Map = new Map(); + const outOfScopeFilesHunks: Map = new Map(); + suggestions.forEach((suggestedHunks: Hunk[], fileName: string) => { + const scope = changeScope.get(fileName); + if (!scope) { + outOfScopeFilesHunks.set(fileName, suggestedHunks); + } else { + const {inScopeHunks, outOfScopeHunks} = getValidSuggestionHunks( + scope, + suggestedHunks + ); + if (inScopeHunks.length) { + inScopeFilesHunks.set(fileName, inScopeHunks); + } + if (outOfScopeHunks.length) { + outOfScopeFilesHunks.set(fileName, outOfScopeHunks); + } + } + }); + return {inScopeFilesHunks, outOfScopeFilesHunks}; +} + +/** + * Return the suggestions whose filename is not in the list of invalid files to suggest + * @param {string[]} invalidFiles a list of file names + * @param {Map} suggestions the hunks for each file to suggest + */ +export function getInScopeByFileName( + invalidFiles: string[], + suggestions: Map +): Map { + // copy original suggestions to our valid suggestions map + const inScopeFiles: Map = new Map(suggestions); + // filter invalid suggestions + invalidFiles.forEach(file => { + if (inScopeFiles.has(file)) { + inScopeFiles.delete(file); + } + }); + return inScopeFiles; +} + +/** + * Get the suggestions who are out of scope because their file name has + * been recorded in the invalid file list. The suggestions should be the original + * unfiltered suggestions, otherwise the method will return a subset of the possible + * out of scope file names (and their respective hunks) + * @param {string} invalidFiles + * @param {Map} suggestions the unfiltered suggestions + */ +export function getOutOfScopeByFileName( + invalidFiles: string[], + suggestions: Map +) { + const invalidFileSuggestions: Map = new Map(); + invalidFiles.forEach(file => { + if (suggestions.has(file)) { + invalidFileSuggestions.set(file, suggestions.get(file) as Hunk[]); + } + }); + return invalidFileSuggestions; +} + +/** + * Get all the out of scope suggestions + * @param {Map} outOfScopeFileNameSuggestions + * @param {Map} outOfScopeHunkSuggestions + */ +export function mergeOutOfScopeSuggestions( + outOfScopeFileNameSuggestions: Map, + outOfScopeHunkSuggestions: Map +): Map { + const invalidSuggestions: Map = new Map( + outOfScopeHunkSuggestions + ); + outOfScopeFileNameSuggestions.forEach((hunks: Hunk[], fileName: string) => { + // invalid file name suggestions should be a superset of invalid hunk suggestions + invalidSuggestions.set(fileName, hunks); + }); + return invalidSuggestions; +} diff --git a/src/github-handler/comment-handler/suggestion-patch-handler/suggestion-hunk-handler.ts b/src/github-handler/comment-handler/suggestion-patch-handler/suggestion-hunk-handler.ts new file mode 100644 index 00000000..bd5967c7 --- /dev/null +++ b/src/github-handler/comment-handler/suggestion-patch-handler/suggestion-hunk-handler.ts @@ -0,0 +1,69 @@ +// Copyright 2020 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import {Hunk, RawContent} from '../../../types'; +import {logger} from '../../../logger'; +import {structuredPatch, Hunk as RawHunk} from 'diff'; + +/** + * Given the old content and new content of a file + * compute the diff and extract the necessary hunk data. + * The hunk list is a list of disjoint ascending intervals. + * @param rawContent + * @param fileName + * @returns the hunks that are generated in ascending sorted order + */ +export function generateHunks( + rawContent: RawContent, + fileName: string +): Hunk[] { + const rawHunks: RawHunk[] = structuredPatch( + fileName, // old file name + fileName, // new file name + rawContent.oldContent, + rawContent.newContent + ).hunks; + const hunks: Hunk[] = rawHunks.map(rawHunk => ({ + oldStart: rawHunk.oldStart, + oldEnd: rawHunk.oldStart + rawHunk.oldLines, + newStart: rawHunk.newStart, + newEnd: rawHunk.newStart + rawHunk.newLines, + })); + return hunks; +} + +/** + * Given a map where the key is the file name and the value is the + * old content and new content of the file + * compute the hunk for each file whose old and new contents differ. + * Do not compute the hunk if the old content is the same as the new content. + * The hunk list is sorted and each interval is disjoint. + * @param {Map} rawChanges a map of the original file contents and the new file contents + * @returns the hunks for each file whose old and new contents differ + */ +export function getRawSuggestionHunks( + rawChanges: Map +): Map { + const fileHunks: Map = new Map(); + rawChanges.forEach((rawContent: RawContent, fileName: string) => { + // if identical don't calculate the hunk and continue in the loop + if (rawContent.oldContent === rawContent.newContent) { + return; + } + const hunks = generateHunks(rawContent, fileName); + fileHunks.set(fileName, hunks); + }); + logger.info('Parsed ranges of old and new patch'); + return fileHunks; +} diff --git a/src/types/index.ts b/src/types/index.ts index fb2028a0..9b489fe4 100644 --- a/src/types/index.ts +++ b/src/types/index.ts @@ -128,8 +128,8 @@ export class PatchSyntaxError extends Error { * The file content of the original content and the patched content */ export interface RawContent { - readonly old_content: string; - readonly new_content: string; + readonly oldContent: string; + readonly newContent: string; } /** @@ -140,14 +140,16 @@ export interface Range { readonly end: number; } +export interface Hunk { + readonly oldStart: number; + readonly oldEnd: number; + readonly newStart: number; + readonly newEnd: number; +} + /** * The range of a patch along with the raw file content */ export interface Patch extends Range { - readonly raw_content: string; + readonly newContent: string; } - -export type FilePatches = Map; -export type RawChanges = Map; -export type PatchText = Map; -export type FileRanges = Map; diff --git a/test/scope-suggestion-hunks.ts b/test/scope-suggestion-hunks.ts new file mode 100644 index 00000000..effd830f --- /dev/null +++ b/test/scope-suggestion-hunks.ts @@ -0,0 +1,294 @@ +// Copyright 2020 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import {expect} from 'chai'; +import {describe, it, before, beforeEach} from 'mocha'; +import {setup} from './util'; +import { + findRange, + getInScopeHunks, + getInScopeByFileName, + getOutOfScopeByFileName, + mergeOutOfScopeSuggestions, +} from '../src/github-handler/comment-handler/suggestion-patch-handler/scope-suggestion-hunks-handler'; +import {Range, Hunk} from '../src/types'; + +before(() => { + setup(); +}); + +describe('findRange', () => { + const ranges: Range[] = [ + {start: 1, end: 2}, + {start: 5, end: 8}, + {start: 10, end: 12}, + {start: 14, end: 15}, + {start: 20, end: 25}, + {start: 100, end: 125}, + ]; + it('Returns the index of the interval that contains the numeric input', () => { + expect(findRange(ranges, 1)).equals(0); + expect(findRange(ranges, 2)).equals(0); + expect(findRange(ranges, 100)).equals(5); + expect(findRange(ranges, 125)).equals(5); + expect(findRange(ranges, 126)).equals(-1); + expect(findRange(ranges, 0)).equals(-1); + expect(findRange(ranges, 3)).equals(-1); + }); + it('Returns -1 if there is no interval domain in which the numeric input satisfies', () => { + expect(findRange(ranges, 126)).equals(-1); + expect(findRange(ranges, 0)).equals(-1); + expect(findRange(ranges, 3)).equals(-1); + }); +}); + +describe('getInScopeHunks', () => { + const scope: Map = new Map(); + const suggestions: Map = new Map(); + const fileName1 = 'foo.txt'; + const fileName2 = 'bar.txt'; + const fileName3 = 'baz.txt'; + scope.set(fileName1, [ + {start: 1, end: 20}, + {start: 50, end: 79}, + ]); + scope.set(fileName2, [{start: 15, end: 54}]); + scope.set(fileName3, [{start: 1, end: 2}]); + + beforeEach(() => { + suggestions.clear(); + }); + + it('Calculates file hunks that are in scope for the pull request', () => { + suggestions.set(fileName1, [ + {oldStart: 1, oldEnd: 20, newStart: 1, newEnd: 30}, + ]); + suggestions.set(fileName2, [ + {oldStart: 50, oldEnd: 54, newStart: 50, newEnd: 53}, + ]); + suggestions.set(fileName3, [ + {oldStart: 1, oldEnd: 2, newStart: 1, newEnd: 2}, + ]); + const {inScopeFilesHunks} = getInScopeHunks(scope, suggestions); + expect(inScopeFilesHunks.size).equals(3); + expect(inScopeFilesHunks.get(fileName1)!).deep.equals([ + {oldStart: 1, oldEnd: 20, newStart: 1, newEnd: 30}, + ]); + expect(inScopeFilesHunks.get(fileName2)!).deep.equals([ + {oldStart: 50, oldEnd: 54, newStart: 50, newEnd: 53}, + ]); + expect(inScopeFilesHunks.get(fileName3)!).deep.equals([ + {oldStart: 1, oldEnd: 2, newStart: 1, newEnd: 2}, + ]); + }); + + it('Calculates file hunks that are in scope for the pull request even if the suggestions are a subset of the total PR scope', () => { + suggestions.set(fileName1, [ + {oldStart: 1, oldEnd: 20, newStart: 1, newEnd: 30}, + ]); + const {inScopeFilesHunks} = getInScopeHunks(scope, suggestions); + expect(inScopeFilesHunks.size).equals(1); + expect(inScopeFilesHunks.get(fileName1)!).deep.equals([ + {oldStart: 1, oldEnd: 20, newStart: 1, newEnd: 30}, + ]); + }); + + it('Calculates file hunks that are out of scope for the pull request even if the suggestions are a subset of the total PR scope', () => { + suggestions.set(fileName1, [ + {oldStart: 1, oldEnd: 20, newStart: 1, newEnd: 30}, + ]); + const {outOfScopeFilesHunks} = getInScopeHunks(scope, suggestions); + expect(outOfScopeFilesHunks.size).equals(0); + }); + + it('Calculates files hunks that are out of scope for the pull request because the lines are out of scope', () => { + suggestions.set(fileName1, [ + {oldStart: 20, oldEnd: 49, newStart: 20, newEnd: 49}, + ]); + suggestions.set(fileName2, [ + {oldStart: 14, oldEnd: 55, newStart: 15, newEnd: 54}, + ]); + const {inScopeFilesHunks, outOfScopeFilesHunks} = getInScopeHunks( + scope, + suggestions + ); + expect(inScopeFilesHunks.size).equals(0); + expect(outOfScopeFilesHunks.size).equals(2); + expect(outOfScopeFilesHunks.get(fileName1)).deep.equals([ + {oldStart: 20, oldEnd: 49, newStart: 20, newEnd: 49}, + ]); + }); + + it('Calculates files hunks that are out of scope for the pull request because the file is not in scope', () => { + suggestions.set('non-existant-file.txt', [ + {oldStart: 1, oldEnd: 2, newStart: 1, newEnd: 2}, + ]); + const {inScopeFilesHunks, outOfScopeFilesHunks} = getInScopeHunks( + scope, + suggestions + ); + expect(inScopeFilesHunks.size).equals(0); + expect(outOfScopeFilesHunks.size).equals(1); + expect(outOfScopeFilesHunks.get('non-existant-file.txt')).deep.equals([ + {oldStart: 1, oldEnd: 2, newStart: 1, newEnd: 2}, + ]); + }); +}); + +describe('getInScopeByFileName', () => { + const fileName1 = 'foo.txt'; + const fileName2 = 'bar.txt'; + const fileName3 = 'baz.txt'; + const invalidFiles = [ + fileName3, + 'BAZ.txt', + 'baz-1.txt', + 'bbaz.txt', + 'foo/bar/baz.txt', + ]; + const suggestions: Map = new Map(); + + beforeEach(() => { + suggestions.clear(); + }); + + it('Calculates files that are valid', () => { + suggestions.set(fileName1, [ + {oldStart: 1, oldEnd: 20, newStart: 1, newEnd: 30}, + ]); + suggestions.set(fileName2, [ + {oldStart: 50, oldEnd: 54, newStart: 50, newEnd: 53}, + ]); + const inScopeFiles = getInScopeByFileName(invalidFiles, suggestions); + expect(inScopeFiles.size).equals(2); + expect(inScopeFiles.get(fileName1)).deep.equals([ + {oldStart: 1, oldEnd: 20, newStart: 1, newEnd: 30}, + ]); + expect(inScopeFiles.get(fileName2)).deep.equals([ + {oldStart: 50, oldEnd: 54, newStart: 50, newEnd: 53}, + ]); + }); + + it('Does not include files that do not appear in the invalid list', () => { + suggestions.set(fileName3, [ + {oldStart: 1, oldEnd: 20, newStart: 1, newEnd: 30}, + ]); + const inScopeFiles = getInScopeByFileName(invalidFiles, suggestions); + expect(inScopeFiles.size).equals(0); + }); +}); + +describe('getOutOfScopeByFileName', () => { + const fileName1 = 'foo.txt'; + const fileName2 = 'bar.txt'; + const fileName3 = 'baz.txt'; + const invalidFiles = [fileName1, fileName2, fileName3]; + const suggestions: Map = new Map(); + + beforeEach(() => { + suggestions.clear(); + }); + + it('Calculates files that are invalid and gets their hunks', () => { + suggestions.set(fileName1, [ + {oldStart: 1, oldEnd: 20, newStart: 1, newEnd: 30}, + ]); + suggestions.set(fileName2, [ + {oldStart: 50, oldEnd: 54, newStart: 50, newEnd: 53}, + ]); + suggestions.set('valid-file.txt', [ + {oldStart: 50, oldEnd: 54, newStart: 50, newEnd: 53}, + ]); + const outOfScopeFiles = getOutOfScopeByFileName(invalidFiles, suggestions); + expect(outOfScopeFiles.size).equals(2); + expect(outOfScopeFiles.get(fileName1)).deep.equals([ + {oldStart: 1, oldEnd: 20, newStart: 1, newEnd: 30}, + ]); + expect(outOfScopeFiles.get(fileName2)).deep.equals([ + {oldStart: 50, oldEnd: 54, newStart: 50, newEnd: 53}, + ]); + expect(outOfScopeFiles.has('valid-file.txt')).equals(false); + }); + + it('Does not include files if they are all valid', () => { + suggestions.set('valid-file-1.txt', [ + {oldStart: 50, oldEnd: 54, newStart: 50, newEnd: 53}, + ]); + suggestions.set('valid-file-2.txt', [ + {oldStart: 50, oldEnd: 54, newStart: 50, newEnd: 53}, + ]); + const outofScopeFiles = getOutOfScopeByFileName(invalidFiles, suggestions); + expect(outofScopeFiles.size).equals(0); + }); +}); + +describe('mergeOutOfScopeSuggestions', () => { + const fileName1 = 'foo.txt'; + const fileName2 = 'bar.txt'; + const fileName3 = 'baz.txt'; + const invalidFileNameSuggestions: Map = new Map(); + const invalidHunkSuggestions: Map = new Map(); + + beforeEach(() => { + invalidFileNameSuggestions.clear(); + invalidHunkSuggestions.clear(); + }); + + it('Gets all the files whose file name is out of scope and/or has out of scope hunks', () => { + invalidFileNameSuggestions.set(fileName1, [ + {oldStart: 1, oldEnd: 20, newStart: 1, newEnd: 30}, + ]); + invalidFileNameSuggestions.set(fileName2, [ + {oldStart: 50, oldEnd: 54, newStart: 50, newEnd: 53}, + ]); + invalidHunkSuggestions.set(fileName3, [ + {oldStart: 2, oldEnd: 54, newStart: 2, newEnd: 10}, + ]); + const outOfScope = mergeOutOfScopeSuggestions( + invalidFileNameSuggestions, + invalidHunkSuggestions + ); + expect(outOfScope.size).equals(3); + expect(outOfScope.get(fileName1)).deep.equals([ + {oldStart: 1, oldEnd: 20, newStart: 1, newEnd: 30}, + ]); + expect(outOfScope.get(fileName2)).deep.equals([ + {oldStart: 50, oldEnd: 54, newStart: 50, newEnd: 53}, + ]); + expect(outOfScope.get(fileName3)).deep.equals([ + {oldStart: 2, oldEnd: 54, newStart: 2, newEnd: 10}, + ]); + }); + + it('Takes the invalid files hunk data over the invalid hunk data because invalid file hunks are a superset', () => { + invalidFileNameSuggestions.set(fileName1, [ + {oldStart: 1, oldEnd: 20, newStart: 1, newEnd: 30}, + {oldStart: 100, oldEnd: 120, newStart: 100, newEnd: 130}, + {oldStart: 1000, oldEnd: 1200, newStart: 1000, newEnd: 1300}, + ]); + invalidHunkSuggestions.set(fileName1, [ + {oldStart: 1, oldEnd: 20, newStart: 1, newEnd: 30}, + ]); + const outOfScope = mergeOutOfScopeSuggestions( + invalidFileNameSuggestions, + invalidHunkSuggestions + ); + expect(outOfScope.size).equals(1); + expect(outOfScope.get(fileName1)).deep.equals([ + {oldStart: 1, oldEnd: 20, newStart: 1, newEnd: 30}, + {oldStart: 100, oldEnd: 120, newStart: 100, newEnd: 130}, + {oldStart: 1000, oldEnd: 1200, newStart: 1000, newEnd: 1300}, + ]); + }); +}); diff --git a/test/suggestion-hunk.ts b/test/suggestion-hunk.ts new file mode 100644 index 00000000..79cc57b5 --- /dev/null +++ b/test/suggestion-hunk.ts @@ -0,0 +1,123 @@ +// Copyright 2020 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import {expect} from 'chai'; +import {describe, it, before} from 'mocha'; +import {setup} from './util'; +import { + generateHunks, + getRawSuggestionHunks, +} from '../src/github-handler/comment-handler/suggestion-patch-handler/suggestion-hunk-handler'; +import {RawContent} from '../src/types'; + +before(() => { + setup(); +}); + +describe('generateHunks', () => { + const rawContent: RawContent = {oldContent: 'foo', newContent: 'FOO'}; + const fileName = 'README.md'; + it('Does not update the user raw change input', () => { + generateHunks(rawContent, fileName); + expect(rawContent.oldContent).equals('foo'); + expect(rawContent.newContent).equals('FOO'); + }); + + it('Generates the hunks that are produced by the diff library given one file', () => { + const hunk = generateHunks(rawContent, fileName); + expect(hunk.length).equals(1); + expect(hunk[0].oldStart).equals(1); + expect(hunk[0].oldEnd).equals(2); + expect(hunk[0].newStart).equals(1); + expect(hunk[0].newEnd).equals(2); + }); + + it('Generates the hunks that are produced by the diff library given one file which was empty but now has content', () => { + const addingContentToEmpty: RawContent = { + oldContent: '', + newContent: 'FOO', + }; + const hunk = generateHunks(addingContentToEmpty, fileName); + expect(hunk.length).equals(1); + expect(hunk[0].oldStart).equals(1); + expect(hunk[0].oldEnd).equals(1); + expect(hunk[0].newStart).equals(1); + expect(hunk[0].newEnd).equals(2); + }); +}); + +describe('getRawSuggestionHunks', () => { + const rawChange: Map = new Map(); + const rawContent1: RawContent = {oldContent: 'foo', newContent: 'FOO'}; + const rawContent2: RawContent = { + oldContent: + 'bar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar', + newContent: + 'foo\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nfoo', + }; + const fileName1 = 'README.md'; + const fileName2 = 'bars.txt'; + rawChange.set(fileName1, rawContent1); + rawChange.set(fileName2, rawContent2); + + it('Does not update the user raw change input', () => { + getRawSuggestionHunks(rawChange); + expect(rawContent1.oldContent).equals('foo'); + expect(rawContent1.newContent).equals('FOO'); + expect(rawChange.get(fileName1)!.oldContent).equals('foo'); + expect(rawChange.get(fileName1)!.newContent).equals('FOO'); + expect(rawContent2.oldContent).equals( + 'bar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar' + ); + expect(rawContent2.newContent).equals( + 'foo\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nfoo' + ); + expect(rawChange.get(fileName2)!.oldContent).equals( + 'bar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar' + ); + expect(rawChange.get(fileName2)!.newContent).equals( + 'foo\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nfoo' + ); + }); + + it('Generates the hunks that are produced by the diff library for all files that are updated', () => { + const fileHunks = getRawSuggestionHunks(rawChange); + expect(fileHunks.size).equals(2); + expect(fileHunks.get(fileName1)!.length).equals(1); + expect(fileHunks.get(fileName1)![0].oldStart).equals(1); + expect(fileHunks.get(fileName1)![0].oldEnd).equals(2); + expect(fileHunks.get(fileName1)![0].newStart).equals(1); + expect(fileHunks.get(fileName1)![0].newEnd).equals(2); + expect(fileHunks.get(fileName2)!.length).equals(2); + expect(fileHunks.get(fileName2)![0].oldStart).equals(1); + expect(fileHunks.get(fileName2)![0].oldEnd).equals(5); + expect(fileHunks.get(fileName2)![0].newStart).equals(1); + expect(fileHunks.get(fileName2)![0].newEnd).equals(6); + expect(fileHunks.get(fileName2)![1].oldStart).equals(12); + expect(fileHunks.get(fileName2)![1].oldEnd).equals(17); + expect(fileHunks.get(fileName2)![1].newStart).equals(13); + expect(fileHunks.get(fileName2)![1].newEnd).equals(19); + }); + + it('Does not generate hunks for changes that contain no updates', () => { + const sameRawChanges = new Map(); + sameRawChanges.set('unchanged-1.txt', {oldContent: '', newContent: ''}); + sameRawChanges.set('unchanged-2.txt', { + oldContent: 'same', + newContent: 'same', + }); + const fileHunks = getRawSuggestionHunks(sameRawChanges); + expect(fileHunks.size).equals(0); + }); +}); diff --git a/test/suggestion-patch.ts b/test/suggestion-patch.ts new file mode 100644 index 00000000..a92e023e --- /dev/null +++ b/test/suggestion-patch.ts @@ -0,0 +1,150 @@ +// Copyright 2020 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import {expect} from 'chai'; +import {describe, it, before, beforeEach} from 'mocha'; +import {setup} from './util'; +import {getValidSuggestionHunks} from '../src/github-handler/comment-handler/suggestion-patch-handler'; +import {Range, RawContent} from '../src/types'; + +before(() => { + setup(); +}); + +describe('getValidSuggestionHunks', () => { + const scope: Map = new Map(); + const fileName1 = 'foo.txt'; + const fileName2 = 'bar.txt'; + const invalidFile = 'baz.txt'; + const invalidFiles = [invalidFile]; + scope.set(fileName1, [{start: 1, end: 2}]); + scope.set(fileName2, [{start: 2, end: 11}]); + const rawChanges: Map = new Map(); + + beforeEach(() => { + rawChanges.clear(); + }); + + it('Finds file hunks that are in scope for the pull request', () => { + rawChanges.set(fileName2, { + oldContent: + 'bar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar', + newContent: + 'bar\nbar\nbar\nbar\nbar\nbar\nfoo\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar', + }); + const suggestions = getValidSuggestionHunks( + rawChanges, + invalidFiles, + scope + ); + expect(suggestions.inScopeSuggestions.size).equals(1); + }); + + it('Excludes file hunks that are not in scope for the pull request', () => { + rawChanges.set(fileName1, { + oldContent: + 'bar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nfoo', + newContent: + 'bar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nfoo', + }); + const suggestions = getValidSuggestionHunks( + rawChanges, + invalidFiles, + scope + ); + expect(suggestions.inScopeSuggestions.size).equals(0); + }); + + it('Excludes suggestion files that are not in scope because the file is not in scope', () => { + rawChanges.set('non-existant-file-that-is-not-invalid.txt', { + oldContent: 'foo', + newContent: 'bar', + }); + const suggestions = getValidSuggestionHunks( + rawChanges, + invalidFiles, + scope + ); + expect(suggestions.inScopeSuggestions.size).equals(0); + }); + + it('Excludes suggestion files that are not in scope because the file is invalid', () => { + rawChanges.set(invalidFile, {oldContent: 'foo', newContent: 'bar'}); + const suggestions = getValidSuggestionHunks( + rawChanges, + invalidFiles, + scope + ); + expect(suggestions.inScopeSuggestions.size).equals(0); + }); + + it('Does not include suggestion files that have no change', () => { + rawChanges.set(fileName1, {oldContent: 'foo', newContent: 'foo'}); + const suggestions = getValidSuggestionHunks( + rawChanges, + invalidFiles, + scope + ); + expect(suggestions.inScopeSuggestions.size).equals(0); + }); + + it('Calculates in scope and out of scope files that are mutually exclusive', () => { + // in scope hunk + rawChanges.set(fileName2, { + oldContent: + 'bar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar', + newContent: + 'bar\nbar\nbar\nbar\nbar\nbar\nfoo\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar', + }); + // out of scope hunks + rawChanges.set(fileName1, { + oldContent: + 'bar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nfoo', + newContent: + 'bar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar', + }); + // same before and after + rawChanges.set('same-before-and-after.text', { + oldContent: + 'bar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nfoo', + newContent: + 'bar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nfoo', + }); + // out of scope file name + rawChanges.set('non-existant-file-that-is-not-invalid.txt', { + oldContent: 'foo', + newContent: 'bar', + }); + // out of scope file name + rawChanges.set(invalidFile, { + oldContent: 'foo', + newContent: 'bar', + }); + const suggestions = getValidSuggestionHunks( + rawChanges, + invalidFiles, + scope + ); + expect(suggestions.inScopeSuggestions.size).equals(1); + expect(suggestions.inScopeSuggestions.has(fileName2)).equals(true); + expect(suggestions.outOfScopeSuggestions.size).equals(3); + expect( + suggestions.outOfScopeSuggestions.has( + 'non-existant-file-that-is-not-invalid.txt' + ) + ).equals(true); + expect(suggestions.outOfScopeSuggestions.has(invalidFile)).equals(true); + expect(suggestions.outOfScopeSuggestions.has(fileName1)).equals(true); + }); +}); From 13908f86a0d3e977fb89b8c68822f2b217540eb9 Mon Sep 17 00:00:00 2001 From: TomKristie <65685417+TomKristie@users.noreply.github.com> Date: Fri, 21 Aug 2020 14:17:45 -0400 Subject: [PATCH 04/10] refactor(framework-core): rename modules, functions, & re-org project structure (#89) --- .../github-patch-text-handler.ts} | 4 +- .../get-hunk-scope-handler/index.ts | 15 +++++++ .../remote-patch-ranges-handler.ts | 6 +-- src/github-handler/comment-handler/index.ts | 4 +- .../in-scope-hunks-handler.ts} | 39 +++++++++++++++-- .../index.ts | 42 +------------------ .../raw-hunk-handler.ts} | 0 src/github-handler/index.ts | 14 +++++++ test/github-regex-patch.ts | 2 +- test/remote-github-patch-text.ts | 2 +- test/scope-suggestion-hunks.ts | 2 +- test/suggestion-hunk.ts | 2 +- test/suggestion-patch.ts | 2 +- 13 files changed, 79 insertions(+), 55 deletions(-) rename src/github-handler/comment-handler/{github-patch-format-handler.ts => get-hunk-scope-handler/github-patch-text-handler.ts} (97%) create mode 100644 src/github-handler/comment-handler/get-hunk-scope-handler/index.ts rename src/github-handler/comment-handler/{ => get-hunk-scope-handler}/remote-patch-ranges-handler.ts (96%) rename src/github-handler/comment-handler/{suggestion-patch-handler/scope-suggestion-hunks-handler.ts => patch-handler/in-scope-hunks-handler.ts} (81%) rename src/github-handler/comment-handler/{suggestion-patch-handler => patch-handler}/index.ts (50%) rename src/github-handler/comment-handler/{suggestion-patch-handler/suggestion-hunk-handler.ts => patch-handler/raw-hunk-handler.ts} (100%) diff --git a/src/github-handler/comment-handler/github-patch-format-handler.ts b/src/github-handler/comment-handler/get-hunk-scope-handler/github-patch-text-handler.ts similarity index 97% rename from src/github-handler/comment-handler/github-patch-format-handler.ts rename to src/github-handler/comment-handler/get-hunk-scope-handler/github-patch-text-handler.ts index 73c39fc4..0cdd04f0 100644 --- a/src/github-handler/comment-handler/github-patch-format-handler.ts +++ b/src/github-handler/comment-handler/get-hunk-scope-handler/github-patch-text-handler.ts @@ -12,8 +12,8 @@ // See the License for the specific language governing permissions and // limitations under the License. -import {PatchSyntaxError, Range} from '../../types'; -import {logger} from '../../logger'; +import {PatchSyntaxError, Range} from '../../../types'; +import {logger} from '../../../logger'; const REGEX_INDEX_OF_UPDATED_HUNK = 2; diff --git a/src/github-handler/comment-handler/get-hunk-scope-handler/index.ts b/src/github-handler/comment-handler/get-hunk-scope-handler/index.ts new file mode 100644 index 00000000..ee7ed517 --- /dev/null +++ b/src/github-handler/comment-handler/get-hunk-scope-handler/index.ts @@ -0,0 +1,15 @@ +// Copyright 2020 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +export {getPullRequestScope} from './remote-patch-ranges-handler'; diff --git a/src/github-handler/comment-handler/remote-patch-ranges-handler.ts b/src/github-handler/comment-handler/get-hunk-scope-handler/remote-patch-ranges-handler.ts similarity index 96% rename from src/github-handler/comment-handler/remote-patch-ranges-handler.ts rename to src/github-handler/comment-handler/get-hunk-scope-handler/remote-patch-ranges-handler.ts index e5d32b03..c4914a2f 100644 --- a/src/github-handler/comment-handler/remote-patch-ranges-handler.ts +++ b/src/github-handler/comment-handler/get-hunk-scope-handler/remote-patch-ranges-handler.ts @@ -13,9 +13,9 @@ // limitations under the License. import {Octokit} from '@octokit/rest'; -import {Range, RepoDomain} from '../../types'; -import {getGitHubPatchRanges} from './github-patch-format-handler'; -import {logger} from '../../logger'; +import {Range, RepoDomain} from '../../../types'; +import {getGitHubPatchRanges} from './github-patch-text-handler'; +import {logger} from '../../../logger'; /** * For a pull request, get each remote file's patch text asynchronously diff --git a/src/github-handler/comment-handler/index.ts b/src/github-handler/comment-handler/index.ts index d634f593..5e99f78a 100644 --- a/src/github-handler/comment-handler/index.ts +++ b/src/github-handler/comment-handler/index.ts @@ -13,9 +13,9 @@ // limitations under the License. import {RawContent, RepoDomain} from '../../types'; -import {getPullRequestScope} from './remote-patch-ranges-handler'; +import {getPullRequestScope} from './get-hunk-scope-handler/remote-patch-ranges-handler'; import {Octokit} from '@octokit/rest'; -import {getSuggestionPatches} from './suggestion-patch-handler'; +import {getSuggestionPatches} from './patch-handler'; /** * Comment on a Pull Request diff --git a/src/github-handler/comment-handler/suggestion-patch-handler/scope-suggestion-hunks-handler.ts b/src/github-handler/comment-handler/patch-handler/in-scope-hunks-handler.ts similarity index 81% rename from src/github-handler/comment-handler/suggestion-patch-handler/scope-suggestion-hunks-handler.ts rename to src/github-handler/comment-handler/patch-handler/in-scope-hunks-handler.ts index 6431822a..1a21ecb1 100644 --- a/src/github-handler/comment-handler/suggestion-patch-handler/scope-suggestion-hunks-handler.ts +++ b/src/github-handler/comment-handler/patch-handler/in-scope-hunks-handler.ts @@ -12,7 +12,8 @@ // See the License for the specific language governing permissions and // limitations under the License. -import {Hunk, Range} from '../../../types'; +import {Hunk, Range, RawContent} from '../../../types'; +import {getRawSuggestionHunks} from './raw-hunk-handler'; /** * Given a value and a list of ranges, find the index of the range domain @@ -45,7 +46,7 @@ export function findRange(ranges: Range[], value: number, from = 0): number { * @param {Range[]} scope the list of valid ranges that a hunk can fit between * @param {Hunk[]} suggestedHunks the hunks for the file to suggest */ -function getValidSuggestionHunks( +function getScopeSuggestionHunks( scope: Range[], suggestedHunks: Hunk[] ): {inScopeHunks: Hunk[]; outOfScopeHunks: Hunk[]} { @@ -87,7 +88,7 @@ export function getInScopeHunks( if (!scope) { outOfScopeFilesHunks.set(fileName, suggestedHunks); } else { - const {inScopeHunks, outOfScopeHunks} = getValidSuggestionHunks( + const {inScopeHunks, outOfScopeHunks} = getScopeSuggestionHunks( scope, suggestedHunks ); @@ -161,3 +162,35 @@ export function mergeOutOfScopeSuggestions( }); return invalidSuggestions; } + +/** + * Get the in scope (of the corresponding pull request's) hunks and files + * @param {Map} rawChanges the raw old content and new content of a file + * @param {string[]} invalidFiles list of invalid files + * @param {Map} validFileLines a map of each file's in scope lines for a Pull Request + */ +export function getValidSuggestionHunks( + rawChanges: Map, + invalidFiles: string[], + validFileLines: Map +): { + inScopeSuggestions: Map; + outOfScopeSuggestions: Map; +} { + const totalfileHunks = getRawSuggestionHunks(rawChanges); + const outofScopeByFilename = getOutOfScopeByFileName( + invalidFiles, + totalfileHunks + ); + const inScopeByFileName = getInScopeByFileName(invalidFiles, totalfileHunks); + + const scopifiedHunks = getInScopeHunks(validFileLines, inScopeByFileName); + const outOfScopeSuggestions = mergeOutOfScopeSuggestions( + outofScopeByFilename, + scopifiedHunks.outOfScopeFilesHunks + ); + return { + inScopeSuggestions: scopifiedHunks.inScopeFilesHunks, + outOfScopeSuggestions, + }; +} diff --git a/src/github-handler/comment-handler/suggestion-patch-handler/index.ts b/src/github-handler/comment-handler/patch-handler/index.ts similarity index 50% rename from src/github-handler/comment-handler/suggestion-patch-handler/index.ts rename to src/github-handler/comment-handler/patch-handler/index.ts index caed4849..8fb99442 100644 --- a/src/github-handler/comment-handler/suggestion-patch-handler/index.ts +++ b/src/github-handler/comment-handler/patch-handler/index.ts @@ -12,46 +12,8 @@ // See the License for the specific language governing permissions and // limitations under the License. -import {getRawSuggestionHunks} from './suggestion-hunk-handler'; -import { - getInScopeHunks, - getInScopeByFileName, - getOutOfScopeByFileName, - mergeOutOfScopeSuggestions, -} from './scope-suggestion-hunks-handler'; -import {RawContent, Hunk, Range, Patch} from '../../../types'; - -/** - * Get the in scope (of the corresponding pull request's) hunks and files - * @param {Map} rawChanges the raw old content and new content of a file - * @param {string[]} invalidFiles list of invalid files - * @param {Map} validFileLines a map of each file's in scope lines for a Pull Request - */ -export function getValidSuggestionHunks( - rawChanges: Map, - invalidFiles: string[], - validFileLines: Map -): { - inScopeSuggestions: Map; - outOfScopeSuggestions: Map; -} { - const totalfileHunks = getRawSuggestionHunks(rawChanges); - const outofScopeByFilename = getOutOfScopeByFileName( - invalidFiles, - totalfileHunks - ); - const inScopeByFileName = getInScopeByFileName(invalidFiles, totalfileHunks); - - const scopifiedHunks = getInScopeHunks(validFileLines, inScopeByFileName); - const outOfScopeSuggestions = mergeOutOfScopeSuggestions( - outofScopeByFilename, - scopifiedHunks.outOfScopeFilesHunks - ); - return { - inScopeSuggestions: scopifiedHunks.inScopeFilesHunks, - outOfScopeSuggestions, - }; -} +import {getValidSuggestionHunks} from './in-scope-hunks-handler'; +import {RawContent, Range, Patch} from '../../../types'; /** * Get the range of the old version of every file and the corresponding new text for that range diff --git a/src/github-handler/comment-handler/suggestion-patch-handler/suggestion-hunk-handler.ts b/src/github-handler/comment-handler/patch-handler/raw-hunk-handler.ts similarity index 100% rename from src/github-handler/comment-handler/suggestion-patch-handler/suggestion-hunk-handler.ts rename to src/github-handler/comment-handler/patch-handler/raw-hunk-handler.ts diff --git a/src/github-handler/index.ts b/src/github-handler/index.ts index e962e9d2..2e8b0c5a 100644 --- a/src/github-handler/index.ts +++ b/src/github-handler/index.ts @@ -1,3 +1,17 @@ +// Copyright 2020 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + export * from './fork-handler'; export {branch} from './branch-handler'; export {commitAndPush} from './commit-and-push-handler'; diff --git a/test/github-regex-patch.ts b/test/github-regex-patch.ts index 6e8b7ead..2738f6e6 100644 --- a/test/github-regex-patch.ts +++ b/test/github-regex-patch.ts @@ -14,7 +14,7 @@ import {expect} from 'chai'; import {describe, it} from 'mocha'; -import {getGitHubPatchRanges} from '../src/github-handler/comment-handler/github-patch-format-handler'; +import {getGitHubPatchRanges} from '../src/github-handler/comment-handler/get-hunk-scope-handler/github-patch-text-handler'; import {PatchSyntaxError} from '../src/types'; describe('Getting patch range from GitHub patch text', async () => { diff --git a/test/remote-github-patch-text.ts b/test/remote-github-patch-text.ts index af4561f8..7831bf4d 100644 --- a/test/remote-github-patch-text.ts +++ b/test/remote-github-patch-text.ts @@ -20,7 +20,7 @@ import { patchTextToRanges, getCurrentPullRequestPatches, getPullRequestScope, -} from '../src/github-handler/comment-handler/remote-patch-ranges-handler'; +} from '../src/github-handler/comment-handler/get-hunk-scope-handler/remote-patch-ranges-handler'; import {Octokit} from '@octokit/rest'; import {logger} from '../src/logger'; diff --git a/test/scope-suggestion-hunks.ts b/test/scope-suggestion-hunks.ts index effd830f..b42dc221 100644 --- a/test/scope-suggestion-hunks.ts +++ b/test/scope-suggestion-hunks.ts @@ -21,7 +21,7 @@ import { getInScopeByFileName, getOutOfScopeByFileName, mergeOutOfScopeSuggestions, -} from '../src/github-handler/comment-handler/suggestion-patch-handler/scope-suggestion-hunks-handler'; +} from '../src/github-handler/comment-handler/patch-handler/in-scope-hunks-handler'; import {Range, Hunk} from '../src/types'; before(() => { diff --git a/test/suggestion-hunk.ts b/test/suggestion-hunk.ts index 79cc57b5..4d1eff3c 100644 --- a/test/suggestion-hunk.ts +++ b/test/suggestion-hunk.ts @@ -18,7 +18,7 @@ import {setup} from './util'; import { generateHunks, getRawSuggestionHunks, -} from '../src/github-handler/comment-handler/suggestion-patch-handler/suggestion-hunk-handler'; +} from '../src/github-handler/comment-handler/patch-handler/raw-hunk-handler'; import {RawContent} from '../src/types'; before(() => { diff --git a/test/suggestion-patch.ts b/test/suggestion-patch.ts index a92e023e..7b1c3c4d 100644 --- a/test/suggestion-patch.ts +++ b/test/suggestion-patch.ts @@ -15,7 +15,7 @@ import {expect} from 'chai'; import {describe, it, before, beforeEach} from 'mocha'; import {setup} from './util'; -import {getValidSuggestionHunks} from '../src/github-handler/comment-handler/suggestion-patch-handler'; +import {getValidSuggestionHunks} from '../src/github-handler/comment-handler/patch-handler/in-scope-hunks-handler'; import {Range, RawContent} from '../src/types'; before(() => { From 5cc566e04aa9c9570d0d2f7f7b26c10ae8fd4d0d Mon Sep 17 00:00:00 2001 From: TomKristie <65685417+TomKristie@users.noreply.github.com> Date: Fri, 21 Aug 2020 17:22:09 -0400 Subject: [PATCH 05/10] fix(framework-core): hunk to patch object (#91) --- .../patch-handler/hunk-to-patch-handler.ts | 56 ++++ .../comment-handler/patch-handler/index.ts | 24 +- test/hunk-to-patch.ts | 240 ++++++++++++++++++ 3 files changed, 314 insertions(+), 6 deletions(-) create mode 100644 src/github-handler/comment-handler/patch-handler/hunk-to-patch-handler.ts create mode 100644 test/hunk-to-patch.ts diff --git a/src/github-handler/comment-handler/patch-handler/hunk-to-patch-handler.ts b/src/github-handler/comment-handler/patch-handler/hunk-to-patch-handler.ts new file mode 100644 index 00000000..6e8b0ded --- /dev/null +++ b/src/github-handler/comment-handler/patch-handler/hunk-to-patch-handler.ts @@ -0,0 +1,56 @@ +// Copyright 2020 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import {Hunk, Patch, RawContent} from '../../../types'; + +/** + * From the each file's hunk, generate each file's hunk's old version range + * and the new content (from the user's) to update that range with. + * @param filesHunks a list of hunks for each file where the lines are at least 1 + * @param rawChanges + * @returns patches to upload to octokit + */ +export function generatePatches( + filesHunks: Map, + rawChanges: Map +): Map { + const filesPatches: Map = new Map(); + filesHunks.forEach((hunks, fileName) => { + const patches: Patch[] = []; + // we expect all hunk lists to not be empty + hunks.forEach(hunk => { + // returns a copy of the hashmap value, then creates a new list with new substrings + const lines = rawChanges.get(fileName)!.newContent.split('\n'); + // creates a new shallow-copied subarray + // we assume the hunk new start and new end to be within the domain of the lines length + if ( + hunk.newStart < 1 || + hunk.newEnd < 1 || + hunk.oldStart < 1 || + hunk.oldEnd < 1 + ) { + throw new RangeError('The file line value should be at least 1'); + } + const newContent = lines.slice(hunk.newStart - 1, hunk.newEnd - 1); + const patch: Patch = { + newContent: newContent.join('\n'), + start: hunk.oldStart, + end: hunk.oldEnd, + }; + patches.push(patch); + }); + filesPatches.set(fileName, patches); + }); + return filesPatches; +} diff --git a/src/github-handler/comment-handler/patch-handler/index.ts b/src/github-handler/comment-handler/patch-handler/index.ts index 8fb99442..67baba90 100644 --- a/src/github-handler/comment-handler/patch-handler/index.ts +++ b/src/github-handler/comment-handler/patch-handler/index.ts @@ -12,8 +12,14 @@ // See the License for the specific language governing permissions and // limitations under the License. +import {generatePatches} from './hunk-to-patch-handler'; import {getValidSuggestionHunks} from './in-scope-hunks-handler'; -import {RawContent, Range, Patch} from '../../../types'; +import {Hunk, RawContent, Range, Patch} from '../../../types'; + +interface SuggestionPatches { + filePatches: Map; + outOfScopeSuggestions: Map; +} /** * Get the range of the old version of every file and the corresponding new text for that range @@ -27,9 +33,15 @@ export function getSuggestionPatches( rawChanges: Map, invalidFiles: string[], validFileLines: Map -): Map { - const filePatches: Map = new Map(); - getValidSuggestionHunks(rawChanges, invalidFiles, validFileLines); - // TODO get patches from getValidSuggestionHunks output - return filePatches; +): SuggestionPatches { + const {inScopeSuggestions, outOfScopeSuggestions} = getValidSuggestionHunks( + rawChanges, + invalidFiles, + validFileLines + ); + const filePatches: Map = generatePatches( + inScopeSuggestions, + rawChanges + ); + return {filePatches, outOfScopeSuggestions}; } diff --git a/test/hunk-to-patch.ts b/test/hunk-to-patch.ts new file mode 100644 index 00000000..eb58480b --- /dev/null +++ b/test/hunk-to-patch.ts @@ -0,0 +1,240 @@ +// Copyright 2020 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import {expect} from 'chai'; +import {describe, it, before, beforeEach} from 'mocha'; +import {setup} from './util'; +import {generatePatches} from '../src/github-handler/comment-handler/patch-handler/hunk-to-patch-handler'; +import {Hunk, RawContent} from '../src/types'; + +before(() => { + setup(); +}); + +describe('generatePatches', () => { + const rawChanges: Map = new Map(); + const filesHunks: Map = new Map(); + + const fileName1Addition = 'file-1.txt'; + const fileName2Addition = 'file-2.txt'; + const fileNameDelete = 'file-3.txt'; + const fileNameSpecialChar1 = 'file-4.txt'; + const fileNameEmtpy = 'file-5.txt'; + const fileNameNonEmtpy = 'file-6.txt'; + + beforeEach(() => { + rawChanges.clear(); + filesHunks.clear(); + }); + + it('Gets the correct substrings when there is 1 addition', () => { + rawChanges.set(fileName1Addition, { + oldContent: + 'line1\nline2\nline3\nline4\nline5\nline6\nline7\nline8\nline9\nline10\n', + newContent: + 'addition+line1\nline2\nline3\nline4\nline5\nline6\nline7\nline8\nline9\nline10\n', + }); + filesHunks.set(fileName1Addition, [ + {oldStart: 1, oldEnd: 6, newStart: 1, newEnd: 7}, + ]); + const filePatches = generatePatches(filesHunks, rawChanges); + expect(filePatches.get(fileName1Addition)!).deep.equals([ + { + start: 1, + end: 6, + newContent: 'addition+line1\nline2\nline3\nline4\nline5\nline6', + }, + ]); + }); + + it('Gets the correct substrings when there is 2 additions', () => { + rawChanges.set(fileName2Addition, { + oldContent: + 'line1\nline2\nline3\nline4\nline5\nline6\nline7\nline8\nline9\nline10\n', + newContent: + 'line0\nline1\nline2\nline3\nline4\nline5\nline6\nline7\nline8\nline9\nline10+another addition\n', + }); + filesHunks.set(fileName2Addition, [ + {oldStart: 1, oldEnd: 6, newStart: 1, newEnd: 7}, + {oldStart: 9, oldEnd: 12, newStart: 10, newEnd: 13}, + ]); + const filePatches = generatePatches(filesHunks, rawChanges); + expect(filePatches.get(fileName2Addition)!).deep.equals([ + { + start: 1, + end: 6, + newContent: 'line0\nline1\nline2\nline3\nline4\nline5', + }, + { + start: 9, + end: 12, + newContent: 'line9\nline10+another addition\n', + }, + ]); + }); + + it('Gets the correct substrings when there is 1 deletion', () => { + rawChanges.set(fileNameDelete, { + oldContent: + 'line1\nline2\nline3\nline4\nline5\nline6\nline7\nline8\nline9\nline10\n', + newContent: + 'line1\nline2\nline3\nline4\nline5\nline6\nline7\nline8\nline9\n', + }); + filesHunks.set(fileNameDelete, [ + {oldStart: 9, oldEnd: 12, newStart: 9, newEnd: 11}, + ]); + const filePatches = generatePatches(filesHunks, rawChanges); + expect(filePatches.get(fileNameDelete)!).deep.equals([ + { + start: 9, + end: 12, + newContent: 'line9\n', + }, + ]); + }); + + it('Gets the correct substrings when there is a special patch char prepending the text', () => { + rawChanges.set(fileNameSpecialChar1, { + oldContent: + 'line1\nline2\nline3\nline4\nline5\nline6\nline7\nline8\nline9\nline10\n', + newContent: + '+line1\nline2\nline3\nline4\nline5\nline6\nline7\nline8\nline9\nline10\n', + }); + + filesHunks.set(fileNameSpecialChar1, [ + {oldStart: 1, oldEnd: 2, newStart: 1, newEnd: 2}, + ]); + const filePatches = generatePatches(filesHunks, rawChanges); + expect(filePatches.get(fileNameSpecialChar1)!).deep.equals([ + { + start: 1, + end: 2, + newContent: '+line1', + }, + ]); + }); + + it('Gets the correct substrings when the file is now an empty string', () => { + rawChanges.set(fileNameEmtpy, { + oldContent: 'line1', + newContent: '', + }); + + filesHunks.set(fileNameEmtpy, [ + {oldStart: 1, oldEnd: 2, newStart: 1, newEnd: 1}, + ]); + const filePatches = generatePatches(filesHunks, rawChanges); + expect(filePatches.get(fileNameEmtpy)!).deep.equals([ + { + start: 1, + end: 2, + newContent: '', + }, + ]); + }); + + it('Gets the correct substrings when the empty string file is now has text', () => { + rawChanges.set(fileNameNonEmtpy, { + oldContent: '', + newContent: + 'line1\nline2\nline3\nline4\nline5\nline6\nline7\nline8\nline9\nline10\n', + }); + + filesHunks.set(fileNameNonEmtpy, [ + {oldStart: 1, oldEnd: 1, newStart: 1, newEnd: 12}, + ]); + const filePatches = generatePatches(filesHunks, rawChanges); + expect(filePatches.get(fileNameNonEmtpy)!).deep.equals([ + { + start: 1, + end: 1, + newContent: + 'line1\nline2\nline3\nline4\nline5\nline6\nline7\nline8\nline9\nline10\n', + }, + ]); + }); + + it('Throws an error when the new start hunk line is 0', () => { + rawChanges.set(fileNameNonEmtpy, { + oldContent: '', + newContent: + 'line1\nline2\nline3\nline4\nline5\nline6\nline7\nline8\nline9\nline10\n', + }); + filesHunks.set(fileNameNonEmtpy, [ + {oldStart: 1, oldEnd: 1, newStart: 0, newEnd: 12}, + ]); + try { + generatePatches(filesHunks, rawChanges); + expect.fail( + 'Should have errored because the new start line is < 1. Value should be >= 1' + ); + } catch (err) { + expect(err instanceof RangeError).equals(true); + } + }); + it('Throws an error when the new end hunk line is 0', () => { + rawChanges.set(fileNameNonEmtpy, { + oldContent: '', + newContent: + 'line1\nline2\nline3\nline4\nline5\nline6\nline7\nline8\nline9\nline10\n', + }); + filesHunks.set(fileNameNonEmtpy, [ + {oldStart: 2, oldEnd: 1, newStart: 1, newEnd: 0}, + ]); + try { + generatePatches(filesHunks, rawChanges); + expect.fail( + 'Should have errored because the new end line is < 1. Value should be >= 1' + ); + } catch (err) { + expect(err instanceof RangeError).equals(true); + } + }); + it('Throws an error when the old start hunk line is 0', () => { + rawChanges.set(fileNameNonEmtpy, { + oldContent: '', + newContent: + 'line1\nline2\nline3\nline4\nline5\nline6\nline7\nline8\nline9\nline10\n', + }); + filesHunks.set(fileNameNonEmtpy, [ + {oldStart: 0, oldEnd: 1, newStart: 1, newEnd: 12}, + ]); + try { + generatePatches(filesHunks, rawChanges); + expect.fail( + 'Should have errored because the old start line is < 1. Value should be >= 1' + ); + } catch (err) { + expect(err instanceof RangeError).equals(true); + } + }); + it('Throws an error when theold end hunk line is 0', () => { + rawChanges.set(fileNameNonEmtpy, { + oldContent: '', + newContent: + 'line1\nline2\nline3\nline4\nline5\nline6\nline7\nline8\nline9\nline10\n', + }); + filesHunks.set(fileNameNonEmtpy, [ + {oldStart: 2, oldEnd: 0, newStart: 1, newEnd: 2}, + ]); + try { + generatePatches(filesHunks, rawChanges); + expect.fail( + 'Should have errored because the old end line is < 1. Value should be >= 1' + ); + } catch (err) { + expect(err instanceof RangeError).equals(true); + } + }); +}); From ecdb61521ad475b6426a1b6f24f81c7ec1751e37 Mon Sep 17 00:00:00 2001 From: Jeff Ching Date: Fri, 21 Aug 2020 14:27:01 -0700 Subject: [PATCH 06/10] feat: build failure message from invalid hunks (#90) * test: add failing stub and test for building the failure message * fix: implement message building * fix: use original line numbers in error message * docs: add docstring * docs: add note about empty input returning empty string --- .../invalid-hunk-handler/index.ts | 40 ++++++++++++ test/invalid-hunks.ts | 65 +++++++++++++++++++ 2 files changed, 105 insertions(+) create mode 100644 src/github-handler/comment-handler/invalid-hunk-handler/index.ts create mode 100644 test/invalid-hunks.ts diff --git a/src/github-handler/comment-handler/invalid-hunk-handler/index.ts b/src/github-handler/comment-handler/invalid-hunk-handler/index.ts new file mode 100644 index 00000000..f61b8b1d --- /dev/null +++ b/src/github-handler/comment-handler/invalid-hunk-handler/index.ts @@ -0,0 +1,40 @@ +// Copyright 2020 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import {Hunk} from '../../../types'; + +function hunkErrorMessage(hunk: Hunk): string { + return ` * lines ${hunk.oldStart}-${hunk.oldEnd}`; +} + +function fileErrorMessage(filename: string, hunks: Hunk[]): string { + return `* ${filename}\n` + hunks.map(hunkErrorMessage).join('\n'); +} + +/** + * Build an error message based on invalid hunks. + * Returns an empty string if the provided hunks are empty. + * @param invalidHunks a map of filename to hunks that are not suggestable + */ +export function buildErrorMessage(invalidHunks: Map): string { + if (invalidHunks.size === 0) { + return ''; + } + return ( + 'Some suggestions could not be made:\n' + + Array.from(invalidHunks, ([filename, hunks]) => + fileErrorMessage(filename, hunks) + ).join('\n') + ); +} diff --git a/test/invalid-hunks.ts b/test/invalid-hunks.ts new file mode 100644 index 00000000..5202a8c5 --- /dev/null +++ b/test/invalid-hunks.ts @@ -0,0 +1,65 @@ +// Copyright 2020 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import {expect} from 'chai'; +import {describe, it, before} from 'mocha'; +import {setup} from './util'; +import {buildErrorMessage} from '../src/github-handler/comment-handler/invalid-hunk-handler'; + +before(() => { + setup(); +}); + +describe('buildErrorMessage', () => { + it('should handle an empty list of failures', () => { + const invalidHunks = new Map(); + const expectedMessage = ''; + + const errorMessage = buildErrorMessage(invalidHunks); + expect(errorMessage).to.be.equal(expectedMessage); + }); + + it('should handle multiple file entries', () => { + const invalidHunks = new Map(); + invalidHunks.set('foo.txt', [ + {oldStart: 1, oldEnd: 2, newStart: 1, newEnd: 2}, + ]); + invalidHunks.set('bar.txt', [ + {oldStart: 3, oldEnd: 4, newStart: 3, newEnd: 4}, + ]); + const expectedMessage = `Some suggestions could not be made: +* foo.txt + * lines 1-2 +* bar.txt + * lines 3-4`; + + const errorMessage = buildErrorMessage(invalidHunks); + expect(errorMessage).to.be.equal(expectedMessage); + }); + + it('should handle multiple entries for a file', () => { + const invalidHunks = new Map(); + invalidHunks.set('foo.txt', [ + {oldStart: 1, oldEnd: 2, newStart: 1, newEnd: 2}, + {oldStart: 3, oldEnd: 4, newStart: 3, newEnd: 4}, + ]); + const expectedMessage = `Some suggestions could not be made: +* foo.txt + * lines 1-2 + * lines 3-4`; + + const errorMessage = buildErrorMessage(invalidHunks); + expect(errorMessage).to.be.equal(expectedMessage); + }); +}); From ba0c7643919e210d7d19ff5b30c08595df6a4524 Mon Sep 17 00:00:00 2001 From: TomKristie <65685417+TomKristie@users.noreply.github.com> Date: Wed, 2 Sep 2020 16:35:38 -0400 Subject: [PATCH 07/10] feat(framework-core): comment on prs given suggestions (#93) --- src/github-handler/comment-handler/index.ts | 36 +- .../invalid-hunk-handler/index.ts | 50 +- .../invalid-hunk-handler/message-handler.ts | 40 ++ .../hunk-to-patch-handler.ts | 0 .../in-scope-hunks-handler.ts | 0 .../index.ts | 0 .../raw-hunk-handler.ts | 0 .../valid-patch-handler/index.ts | 15 + .../upload-comments-handler.ts | 115 ++++ test/fixtures/get-pull-request-response.json | 524 ++++++++++++++++++ test/hunk-to-patch.ts | 2 +- test/inline-suggest.ts | 221 ++++++++ test/invalid-hunks.ts | 2 +- test/scope-suggestion-hunks.ts | 2 +- test/suggestion-hunk.ts | 2 +- test/suggestion-patch.ts | 2 +- test/timeline-comment-invalid-hunks.ts | 52 ++ 17 files changed, 1028 insertions(+), 35 deletions(-) create mode 100644 src/github-handler/comment-handler/invalid-hunk-handler/message-handler.ts rename src/github-handler/comment-handler/{patch-handler => raw-patch-handler}/hunk-to-patch-handler.ts (100%) rename src/github-handler/comment-handler/{patch-handler => raw-patch-handler}/in-scope-hunks-handler.ts (100%) rename src/github-handler/comment-handler/{patch-handler => raw-patch-handler}/index.ts (100%) rename src/github-handler/comment-handler/{patch-handler => raw-patch-handler}/raw-hunk-handler.ts (100%) create mode 100644 src/github-handler/comment-handler/valid-patch-handler/index.ts create mode 100644 src/github-handler/comment-handler/valid-patch-handler/upload-comments-handler.ts create mode 100644 test/fixtures/get-pull-request-response.json create mode 100644 test/inline-suggest.ts create mode 100644 test/timeline-comment-invalid-hunks.ts diff --git a/src/github-handler/comment-handler/index.ts b/src/github-handler/comment-handler/index.ts index 5e99f78a..6078060e 100644 --- a/src/github-handler/comment-handler/index.ts +++ b/src/github-handler/comment-handler/index.ts @@ -15,7 +15,10 @@ import {RawContent, RepoDomain} from '../../types'; import {getPullRequestScope} from './get-hunk-scope-handler/remote-patch-ranges-handler'; import {Octokit} from '@octokit/rest'; -import {getSuggestionPatches} from './patch-handler'; +import {getSuggestionPatches} from './raw-patch-handler'; +import {makeInlineSuggestion} from './valid-patch-handler'; +import {makeTimeLineComment} from './invalid-hunk-handler'; +import {logger} from '../../logger'; /** * Comment on a Pull Request @@ -32,12 +35,27 @@ export async function comment( pageSize: number, rawChanges: Map ): Promise { - const {invalidFiles, validFileLines} = await getPullRequestScope( - octokit, - remote, - pullNumber, - pageSize - ); - getSuggestionPatches(rawChanges, invalidFiles, validFileLines); - // TODO get the raw file content from the new range of the hunk and make a PR with that + try { + const {invalidFiles, validFileLines} = await getPullRequestScope( + octokit, + remote, + pullNumber, + pageSize + ); + const {filePatches, outOfScopeSuggestions} = getSuggestionPatches( + rawChanges, + invalidFiles, + validFileLines + ); + await makeInlineSuggestion(octokit, filePatches, remote, pullNumber); + await makeTimeLineComment( + octokit, + outOfScopeSuggestions, + remote, + pullNumber + ); + } catch (err) { + logger.error('Failed to suggest'); + throw err; + } } diff --git a/src/github-handler/comment-handler/invalid-hunk-handler/index.ts b/src/github-handler/comment-handler/invalid-hunk-handler/index.ts index f61b8b1d..385e0e10 100644 --- a/src/github-handler/comment-handler/invalid-hunk-handler/index.ts +++ b/src/github-handler/comment-handler/invalid-hunk-handler/index.ts @@ -12,29 +12,37 @@ // See the License for the specific language governing permissions and // limitations under the License. -import {Hunk} from '../../../types'; - -function hunkErrorMessage(hunk: Hunk): string { - return ` * lines ${hunk.oldStart}-${hunk.oldEnd}`; -} - -function fileErrorMessage(filename: string, hunks: Hunk[]): string { - return `* ${filename}\n` + hunks.map(hunkErrorMessage).join('\n'); -} +import {Octokit} from '@octokit/rest'; +import {Hunk, RepoDomain} from '../../../types'; +import {buildErrorMessage} from './message-handler'; +import {logger} from '../../../logger'; /** - * Build an error message based on invalid hunks. - * Returns an empty string if the provided hunks are empty. - * @param invalidHunks a map of filename to hunks that are not suggestable + * From the invalid hunks provided, make a comment on the pull request + * timeline detailing what hunks and files could not be commented on. + * @param octokit + * @param invalidHunks + * @param remote + * @param pullNumber */ -export function buildErrorMessage(invalidHunks: Map): string { - if (invalidHunks.size === 0) { - return ''; +export async function makeTimeLineComment( + octokit: Octokit, + invalidHunks: Map, + remote: RepoDomain, + pullNumber: number +): Promise { + try { + const errorMessage = buildErrorMessage(invalidHunks); + if (errorMessage) { + await octokit.issues.createComment({ + owner: remote.owner, + repo: remote.repo, + issue_number: pullNumber, + body: errorMessage, + }); + } + } catch (err) { + logger.error('Failed to make a timeline comment'); + throw err; } - return ( - 'Some suggestions could not be made:\n' + - Array.from(invalidHunks, ([filename, hunks]) => - fileErrorMessage(filename, hunks) - ).join('\n') - ); } diff --git a/src/github-handler/comment-handler/invalid-hunk-handler/message-handler.ts b/src/github-handler/comment-handler/invalid-hunk-handler/message-handler.ts new file mode 100644 index 00000000..f61b8b1d --- /dev/null +++ b/src/github-handler/comment-handler/invalid-hunk-handler/message-handler.ts @@ -0,0 +1,40 @@ +// Copyright 2020 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import {Hunk} from '../../../types'; + +function hunkErrorMessage(hunk: Hunk): string { + return ` * lines ${hunk.oldStart}-${hunk.oldEnd}`; +} + +function fileErrorMessage(filename: string, hunks: Hunk[]): string { + return `* ${filename}\n` + hunks.map(hunkErrorMessage).join('\n'); +} + +/** + * Build an error message based on invalid hunks. + * Returns an empty string if the provided hunks are empty. + * @param invalidHunks a map of filename to hunks that are not suggestable + */ +export function buildErrorMessage(invalidHunks: Map): string { + if (invalidHunks.size === 0) { + return ''; + } + return ( + 'Some suggestions could not be made:\n' + + Array.from(invalidHunks, ([filename, hunks]) => + fileErrorMessage(filename, hunks) + ).join('\n') + ); +} diff --git a/src/github-handler/comment-handler/patch-handler/hunk-to-patch-handler.ts b/src/github-handler/comment-handler/raw-patch-handler/hunk-to-patch-handler.ts similarity index 100% rename from src/github-handler/comment-handler/patch-handler/hunk-to-patch-handler.ts rename to src/github-handler/comment-handler/raw-patch-handler/hunk-to-patch-handler.ts diff --git a/src/github-handler/comment-handler/patch-handler/in-scope-hunks-handler.ts b/src/github-handler/comment-handler/raw-patch-handler/in-scope-hunks-handler.ts similarity index 100% rename from src/github-handler/comment-handler/patch-handler/in-scope-hunks-handler.ts rename to src/github-handler/comment-handler/raw-patch-handler/in-scope-hunks-handler.ts diff --git a/src/github-handler/comment-handler/patch-handler/index.ts b/src/github-handler/comment-handler/raw-patch-handler/index.ts similarity index 100% rename from src/github-handler/comment-handler/patch-handler/index.ts rename to src/github-handler/comment-handler/raw-patch-handler/index.ts diff --git a/src/github-handler/comment-handler/patch-handler/raw-hunk-handler.ts b/src/github-handler/comment-handler/raw-patch-handler/raw-hunk-handler.ts similarity index 100% rename from src/github-handler/comment-handler/patch-handler/raw-hunk-handler.ts rename to src/github-handler/comment-handler/raw-patch-handler/raw-hunk-handler.ts diff --git a/src/github-handler/comment-handler/valid-patch-handler/index.ts b/src/github-handler/comment-handler/valid-patch-handler/index.ts new file mode 100644 index 00000000..8291bda1 --- /dev/null +++ b/src/github-handler/comment-handler/valid-patch-handler/index.ts @@ -0,0 +1,15 @@ +// Copyright 2020 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +export {makeInlineSuggestion} from './upload-comments-handler'; diff --git a/src/github-handler/comment-handler/valid-patch-handler/upload-comments-handler.ts b/src/github-handler/comment-handler/valid-patch-handler/upload-comments-handler.ts new file mode 100644 index 00000000..190d6980 --- /dev/null +++ b/src/github-handler/comment-handler/valid-patch-handler/upload-comments-handler.ts @@ -0,0 +1,115 @@ +// Copyright 2020 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import {Octokit} from '@octokit/rest'; +import {Patch, RepoDomain} from '../../../types'; +import {logger} from '../../../logger'; + +const COMFORT_PREVIEW_HEADER = + 'application/vnd.github.comfort-fade-preview+json'; + +/** + * Multiline comment GitHub API parameters + * For more information see + * https://developer.github.com/v3/pulls/comments/#create-a-review-comment-for-a-pull-request + */ +interface MultilineComment { + body: string; + path: string; + start_line: number; + line: number; + side: 'RIGHT' | 'LEFT'; + start_side: 'RIGHT' | 'LEFT'; +} + +/** + * GitHub-defined type. The Octokit library/docs are probably behind since create review already + * accept multi-line code comments. However, the API does not reflect that. + */ +export type PullsCreateReviewParamsComments = { + path: string; + position: number; + body: string; +}; + +/** + * Convert the patch suggestions into GitHub parameter objects. + * Use this to generate review comments + * For information see: + * https://developer.github.com/v3/pulls/comments/#create-a-review-comment-for-a-pull-request + * @param suggestions + */ +export function buildReviewComments( + suggestions: Map +): MultilineComment[] { + const fileComments: MultilineComment[] = []; + suggestions.forEach((patches: Patch[], fileName: string) => { + patches.forEach(patch => { + const comment: MultilineComment = { + path: fileName, + body: `\`\`\`suggestion\n${patch.newContent}\`\`\``, + start_line: patch.start, + line: patch.end, + side: 'RIGHT', + start_side: 'RIGHT', + }; + fileComments.push(comment); + }); + }); + return fileComments; +} + +/** + * Make a request to GitHub to make review comments + * @param octokit + * @param suggestions + * @param remote + * @param pullNumber + */ +export async function makeInlineSuggestion( + octokit: Octokit, + suggestions: Map, + remote: RepoDomain, + pullNumber: number +): Promise { + if (!suggestions.size) { + logger.info('No suggestions to make. Exiting...'); + return; + } + // apply the suggestions to the latest sha + // the latest Pull Request hunk range includes + // all previous commit valid hunk ranges + const headSha = ( + await octokit.pulls.get({ + owner: remote.owner, + repo: remote.repo, + pull_number: pullNumber, + }) + ).data.head.sha; + const comments = buildReviewComments(suggestions); + if (!comments.length) { + logger.info('No suggestions to make. Exiting...'); + return; + } + await octokit.pulls.createReview({ + owner: remote.owner, + repo: remote.repo, + pull_number: pullNumber, + commit_id: headSha, + event: 'COMMENT', + headers: {accept: COMFORT_PREVIEW_HEADER}, + // Octokit type definitions doesn't support mulitiline comments, but the GitHub API does + comments: (comments as unknown) as PullsCreateReviewParamsComments[], + }); +} diff --git a/test/fixtures/get-pull-request-response.json b/test/fixtures/get-pull-request-response.json new file mode 100644 index 00000000..fa7420e3 --- /dev/null +++ b/test/fixtures/get-pull-request-response.json @@ -0,0 +1,524 @@ +{ + "url": "https://api.github.com/repos/octocat/Hello-World/pulls/1347", + "id": 1, + "node_id": "MDExOlB1bGxSZXF1ZXN0MQ==", + "html_url": "https://github.com/octocat/Hello-World/pull/1347", + "diff_url": "https://github.com/octocat/Hello-World/pull/1347.diff", + "patch_url": "https://github.com/octocat/Hello-World/pull/1347.patch", + "issue_url": "https://api.github.com/repos/octocat/Hello-World/issues/1347", + "commits_url": "https://api.github.com/repos/octocat/Hello-World/pulls/1347/commits", + "review_comments_url": "https://api.github.com/repos/octocat/Hello-World/pulls/1347/comments", + "review_comment_url": "https://api.github.com/repos/octocat/Hello-World/pulls/comments{/number}", + "comments_url": "https://api.github.com/repos/octocat/Hello-World/issues/1347/comments", + "statuses_url": "https://api.github.com/repos/octocat/Hello-World/statuses/6dcb09b5b57875f334f61aebed695e2e4193db5e", + "number": 1347, + "state": "open", + "locked": true, + "title": "Amazing new feature", + "user": { + "login": "octocat", + "id": 1, + "node_id": "MDQ6VXNlcjE=", + "avatar_url": "https://github.com/images/error/octocat_happy.gif", + "gravatar_id": "", + "url": "https://api.github.com/users/octocat", + "html_url": "https://github.com/octocat", + "followers_url": "https://api.github.com/users/octocat/followers", + "following_url": "https://api.github.com/users/octocat/following{/other_user}", + "gists_url": "https://api.github.com/users/octocat/gists{/gist_id}", + "starred_url": "https://api.github.com/users/octocat/starred{/owner}{/repo}", + "subscriptions_url": "https://api.github.com/users/octocat/subscriptions", + "organizations_url": "https://api.github.com/users/octocat/orgs", + "repos_url": "https://api.github.com/users/octocat/repos", + "events_url": "https://api.github.com/users/octocat/events{/privacy}", + "received_events_url": "https://api.github.com/users/octocat/received_events", + "type": "User", + "site_admin": false + }, + "body": "Please pull these awesome changes in!", + "labels": [ + { + "id": 208045946, + "node_id": "MDU6TGFiZWwyMDgwNDU5NDY=", + "url": "https://api.github.com/repos/octocat/Hello-World/labels/bug", + "name": "bug", + "description": "Something isn't working", + "color": "f29513", + "default": true + } + ], + "milestone": { + "url": "https://api.github.com/repos/octocat/Hello-World/milestones/1", + "html_url": "https://github.com/octocat/Hello-World/milestones/v1.0", + "labels_url": "https://api.github.com/repos/octocat/Hello-World/milestones/1/labels", + "id": 1002604, + "node_id": "MDk6TWlsZXN0b25lMTAwMjYwNA==", + "number": 1, + "state": "open", + "title": "v1.0", + "description": "Tracking milestone for version 1.0", + "creator": { + "login": "octocat", + "id": 1, + "node_id": "MDQ6VXNlcjE=", + "avatar_url": "https://github.com/images/error/octocat_happy.gif", + "gravatar_id": "", + "url": "https://api.github.com/users/octocat", + "html_url": "https://github.com/octocat", + "followers_url": "https://api.github.com/users/octocat/followers", + "following_url": "https://api.github.com/users/octocat/following{/other_user}", + "gists_url": "https://api.github.com/users/octocat/gists{/gist_id}", + "starred_url": "https://api.github.com/users/octocat/starred{/owner}{/repo}", + "subscriptions_url": "https://api.github.com/users/octocat/subscriptions", + "organizations_url": "https://api.github.com/users/octocat/orgs", + "repos_url": "https://api.github.com/users/octocat/repos", + "events_url": "https://api.github.com/users/octocat/events{/privacy}", + "received_events_url": "https://api.github.com/users/octocat/received_events", + "type": "User", + "site_admin": false + }, + "open_issues": 4, + "closed_issues": 8, + "created_at": "2011-04-10T20:09:31Z", + "updated_at": "2014-03-03T18:58:10Z", + "closed_at": "2013-02-12T13:22:01Z", + "due_on": "2012-10-09T23:39:01Z" + }, + "active_lock_reason": "too heated", + "created_at": "2011-01-26T19:01:12Z", + "updated_at": "2011-01-26T19:01:12Z", + "closed_at": "2011-01-26T19:01:12Z", + "merged_at": "2011-01-26T19:01:12Z", + "merge_commit_sha": "e5bd3914e2e596debea16f433f57875b5b90bcd6", + "assignee": { + "login": "octocat", + "id": 1, + "node_id": "MDQ6VXNlcjE=", + "avatar_url": "https://github.com/images/error/octocat_happy.gif", + "gravatar_id": "", + "url": "https://api.github.com/users/octocat", + "html_url": "https://github.com/octocat", + "followers_url": "https://api.github.com/users/octocat/followers", + "following_url": "https://api.github.com/users/octocat/following{/other_user}", + "gists_url": "https://api.github.com/users/octocat/gists{/gist_id}", + "starred_url": "https://api.github.com/users/octocat/starred{/owner}{/repo}", + "subscriptions_url": "https://api.github.com/users/octocat/subscriptions", + "organizations_url": "https://api.github.com/users/octocat/orgs", + "repos_url": "https://api.github.com/users/octocat/repos", + "events_url": "https://api.github.com/users/octocat/events{/privacy}", + "received_events_url": "https://api.github.com/users/octocat/received_events", + "type": "User", + "site_admin": false + }, + "assignees": [ + { + "login": "octocat", + "id": 1, + "node_id": "MDQ6VXNlcjE=", + "avatar_url": "https://github.com/images/error/octocat_happy.gif", + "gravatar_id": "", + "url": "https://api.github.com/users/octocat", + "html_url": "https://github.com/octocat", + "followers_url": "https://api.github.com/users/octocat/followers", + "following_url": "https://api.github.com/users/octocat/following{/other_user}", + "gists_url": "https://api.github.com/users/octocat/gists{/gist_id}", + "starred_url": "https://api.github.com/users/octocat/starred{/owner}{/repo}", + "subscriptions_url": "https://api.github.com/users/octocat/subscriptions", + "organizations_url": "https://api.github.com/users/octocat/orgs", + "repos_url": "https://api.github.com/users/octocat/repos", + "events_url": "https://api.github.com/users/octocat/events{/privacy}", + "received_events_url": "https://api.github.com/users/octocat/received_events", + "type": "User", + "site_admin": false + }, + { + "login": "hubot", + "id": 1, + "node_id": "MDQ6VXNlcjE=", + "avatar_url": "https://github.com/images/error/hubot_happy.gif", + "gravatar_id": "", + "url": "https://api.github.com/users/hubot", + "html_url": "https://github.com/hubot", + "followers_url": "https://api.github.com/users/hubot/followers", + "following_url": "https://api.github.com/users/hubot/following{/other_user}", + "gists_url": "https://api.github.com/users/hubot/gists{/gist_id}", + "starred_url": "https://api.github.com/users/hubot/starred{/owner}{/repo}", + "subscriptions_url": "https://api.github.com/users/hubot/subscriptions", + "organizations_url": "https://api.github.com/users/hubot/orgs", + "repos_url": "https://api.github.com/users/hubot/repos", + "events_url": "https://api.github.com/users/hubot/events{/privacy}", + "received_events_url": "https://api.github.com/users/hubot/received_events", + "type": "User", + "site_admin": true + } + ], + "requested_reviewers": [ + { + "login": "other_user", + "id": 1, + "node_id": "MDQ6VXNlcjE=", + "avatar_url": "https://github.com/images/error/other_user_happy.gif", + "gravatar_id": "", + "url": "https://api.github.com/users/other_user", + "html_url": "https://github.com/other_user", + "followers_url": "https://api.github.com/users/other_user/followers", + "following_url": "https://api.github.com/users/other_user/following{/other_user}", + "gists_url": "https://api.github.com/users/other_user/gists{/gist_id}", + "starred_url": "https://api.github.com/users/other_user/starred{/owner}{/repo}", + "subscriptions_url": "https://api.github.com/users/other_user/subscriptions", + "organizations_url": "https://api.github.com/users/other_user/orgs", + "repos_url": "https://api.github.com/users/other_user/repos", + "events_url": "https://api.github.com/users/other_user/events{/privacy}", + "received_events_url": "https://api.github.com/users/other_user/received_events", + "type": "User", + "site_admin": false + } + ], + "requested_teams": [ + { + "id": 1, + "node_id": "MDQ6VGVhbTE=", + "url": "https://api.github.com/teams/1", + "html_url": "https://api.github.com/teams/justice-league", + "name": "Justice League", + "slug": "justice-league", + "description": "A great team.", + "privacy": "closed", + "permission": "admin", + "members_url": "https://api.github.com/teams/1/members{/member}", + "repositories_url": "https://api.github.com/teams/1/repos", + "parent": {} + } + ], + "head": { + "label": "octocat:new-topic", + "ref": "new-topic", + "sha": "6dcb09b5b57875f334f61aebed695e2e4193db5e", + "user": { + "login": "octocat", + "id": 1, + "node_id": "MDQ6VXNlcjE=", + "avatar_url": "https://github.com/images/error/octocat_happy.gif", + "gravatar_id": "", + "url": "https://api.github.com/users/octocat", + "html_url": "https://github.com/octocat", + "followers_url": "https://api.github.com/users/octocat/followers", + "following_url": "https://api.github.com/users/octocat/following{/other_user}", + "gists_url": "https://api.github.com/users/octocat/gists{/gist_id}", + "starred_url": "https://api.github.com/users/octocat/starred{/owner}{/repo}", + "subscriptions_url": "https://api.github.com/users/octocat/subscriptions", + "organizations_url": "https://api.github.com/users/octocat/orgs", + "repos_url": "https://api.github.com/users/octocat/repos", + "events_url": "https://api.github.com/users/octocat/events{/privacy}", + "received_events_url": "https://api.github.com/users/octocat/received_events", + "type": "User", + "site_admin": false + }, + "repo": { + "id": 1296269, + "node_id": "MDEwOlJlcG9zaXRvcnkxMjk2MjY5", + "name": "Hello-World", + "full_name": "octocat/Hello-World", + "owner": { + "login": "octocat", + "id": 1, + "node_id": "MDQ6VXNlcjE=", + "avatar_url": "https://github.com/images/error/octocat_happy.gif", + "gravatar_id": "", + "url": "https://api.github.com/users/octocat", + "html_url": "https://github.com/octocat", + "followers_url": "https://api.github.com/users/octocat/followers", + "following_url": "https://api.github.com/users/octocat/following{/other_user}", + "gists_url": "https://api.github.com/users/octocat/gists{/gist_id}", + "starred_url": "https://api.github.com/users/octocat/starred{/owner}{/repo}", + "subscriptions_url": "https://api.github.com/users/octocat/subscriptions", + "organizations_url": "https://api.github.com/users/octocat/orgs", + "repos_url": "https://api.github.com/users/octocat/repos", + "events_url": "https://api.github.com/users/octocat/events{/privacy}", + "received_events_url": "https://api.github.com/users/octocat/received_events", + "type": "User", + "site_admin": false + }, + "private": false, + "html_url": "https://github.com/octocat/Hello-World", + "description": "This your first repo!", + "fork": false, + "url": "https://api.github.com/repos/octocat/Hello-World", + "archive_url": "http://api.github.com/repos/octocat/Hello-World/{archive_format}{/ref}", + "assignees_url": "http://api.github.com/repos/octocat/Hello-World/assignees{/user}", + "blobs_url": "http://api.github.com/repos/octocat/Hello-World/git/blobs{/sha}", + "branches_url": "http://api.github.com/repos/octocat/Hello-World/branches{/branch}", + "collaborators_url": "http://api.github.com/repos/octocat/Hello-World/collaborators{/collaborator}", + "comments_url": "http://api.github.com/repos/octocat/Hello-World/comments{/number}", + "commits_url": "http://api.github.com/repos/octocat/Hello-World/commits{/sha}", + "compare_url": "http://api.github.com/repos/octocat/Hello-World/compare/{base}...{head}", + "contents_url": "http://api.github.com/repos/octocat/Hello-World/contents/{+path}", + "contributors_url": "http://api.github.com/repos/octocat/Hello-World/contributors", + "deployments_url": "http://api.github.com/repos/octocat/Hello-World/deployments", + "downloads_url": "http://api.github.com/repos/octocat/Hello-World/downloads", + "events_url": "http://api.github.com/repos/octocat/Hello-World/events", + "forks_url": "http://api.github.com/repos/octocat/Hello-World/forks", + "git_commits_url": "http://api.github.com/repos/octocat/Hello-World/git/commits{/sha}", + "git_refs_url": "http://api.github.com/repos/octocat/Hello-World/git/refs{/sha}", + "git_tags_url": "http://api.github.com/repos/octocat/Hello-World/git/tags{/sha}", + "git_url": "git:github.com/octocat/Hello-World.git", + "issue_comment_url": "http://api.github.com/repos/octocat/Hello-World/issues/comments{/number}", + "issue_events_url": "http://api.github.com/repos/octocat/Hello-World/issues/events{/number}", + "issues_url": "http://api.github.com/repos/octocat/Hello-World/issues{/number}", + "keys_url": "http://api.github.com/repos/octocat/Hello-World/keys{/key_id}", + "labels_url": "http://api.github.com/repos/octocat/Hello-World/labels{/name}", + "languages_url": "http://api.github.com/repos/octocat/Hello-World/languages", + "merges_url": "http://api.github.com/repos/octocat/Hello-World/merges", + "milestones_url": "http://api.github.com/repos/octocat/Hello-World/milestones{/number}", + "notifications_url": "http://api.github.com/repos/octocat/Hello-World/notifications{?since,all,participating}", + "pulls_url": "http://api.github.com/repos/octocat/Hello-World/pulls{/number}", + "releases_url": "http://api.github.com/repos/octocat/Hello-World/releases{/id}", + "ssh_url": "git@github.com:octocat/Hello-World.git", + "stargazers_url": "http://api.github.com/repos/octocat/Hello-World/stargazers", + "statuses_url": "http://api.github.com/repos/octocat/Hello-World/statuses/{sha}", + "subscribers_url": "http://api.github.com/repos/octocat/Hello-World/subscribers", + "subscription_url": "http://api.github.com/repos/octocat/Hello-World/subscription", + "tags_url": "http://api.github.com/repos/octocat/Hello-World/tags", + "teams_url": "http://api.github.com/repos/octocat/Hello-World/teams", + "trees_url": "http://api.github.com/repos/octocat/Hello-World/git/trees{/sha}", + "clone_url": "https://github.com/octocat/Hello-World.git", + "mirror_url": "git:git.example.com/octocat/Hello-World", + "hooks_url": "http://api.github.com/repos/octocat/Hello-World/hooks", + "svn_url": "https://svn.github.com/octocat/Hello-World", + "homepage": "https://github.com", + "language": "", + "forks_count": 9, + "stargazers_count": 80, + "watchers_count": 80, + "size": 108, + "default_branch": "master", + "open_issues_count": 0, + "is_template": true, + "topics": [ + "octocat", + "atom", + "electron", + "api" + ], + "has_issues": true, + "has_projects": true, + "has_wiki": true, + "has_pages": false, + "has_downloads": true, + "archived": false, + "disabled": false, + "visibility": "public", + "pushed_at": "2011-01-26T19:06:43Z", + "created_at": "2011-01-26T19:01:12Z", + "updated_at": "2011-01-26T19:14:43Z", + "permissions": { + "admin": false, + "push": false, + "pull": true + }, + "allow_rebase_merge": true, + "template_repository": {}, + "temp_clone_token": "ABTLWHOULUVAXGTRYU7OC2876QJ2O", + "allow_squash_merge": true, + "delete_branch_on_merge": true, + "allow_merge_commit": true, + "subscribers_count": 42, + "network_count": 0 + } + }, + "base": { + "label": "octocat:master", + "ref": "master", + "sha": "6dcb09b5b57875f334f61aebed695e2e4193db5e", + "user": { + "login": "octocat", + "id": 1, + "node_id": "MDQ6VXNlcjE=", + "avatar_url": "https://github.com/images/error/octocat_happy.gif", + "gravatar_id": "", + "url": "https://api.github.com/users/octocat", + "html_url": "https://github.com/octocat", + "followers_url": "https://api.github.com/users/octocat/followers", + "following_url": "https://api.github.com/users/octocat/following{/other_user}", + "gists_url": "https://api.github.com/users/octocat/gists{/gist_id}", + "starred_url": "https://api.github.com/users/octocat/starred{/owner}{/repo}", + "subscriptions_url": "https://api.github.com/users/octocat/subscriptions", + "organizations_url": "https://api.github.com/users/octocat/orgs", + "repos_url": "https://api.github.com/users/octocat/repos", + "events_url": "https://api.github.com/users/octocat/events{/privacy}", + "received_events_url": "https://api.github.com/users/octocat/received_events", + "type": "User", + "site_admin": false + }, + "repo": { + "id": 1296269, + "node_id": "MDEwOlJlcG9zaXRvcnkxMjk2MjY5", + "name": "Hello-World", + "full_name": "octocat/Hello-World", + "owner": { + "login": "octocat", + "id": 1, + "node_id": "MDQ6VXNlcjE=", + "avatar_url": "https://github.com/images/error/octocat_happy.gif", + "gravatar_id": "", + "url": "https://api.github.com/users/octocat", + "html_url": "https://github.com/octocat", + "followers_url": "https://api.github.com/users/octocat/followers", + "following_url": "https://api.github.com/users/octocat/following{/other_user}", + "gists_url": "https://api.github.com/users/octocat/gists{/gist_id}", + "starred_url": "https://api.github.com/users/octocat/starred{/owner}{/repo}", + "subscriptions_url": "https://api.github.com/users/octocat/subscriptions", + "organizations_url": "https://api.github.com/users/octocat/orgs", + "repos_url": "https://api.github.com/users/octocat/repos", + "events_url": "https://api.github.com/users/octocat/events{/privacy}", + "received_events_url": "https://api.github.com/users/octocat/received_events", + "type": "User", + "site_admin": false + }, + "private": false, + "html_url": "https://github.com/octocat/Hello-World", + "description": "This your first repo!", + "fork": false, + "url": "https://api.github.com/repos/octocat/Hello-World", + "archive_url": "http://api.github.com/repos/octocat/Hello-World/{archive_format}{/ref}", + "assignees_url": "http://api.github.com/repos/octocat/Hello-World/assignees{/user}", + "blobs_url": "http://api.github.com/repos/octocat/Hello-World/git/blobs{/sha}", + "branches_url": "http://api.github.com/repos/octocat/Hello-World/branches{/branch}", + "collaborators_url": "http://api.github.com/repos/octocat/Hello-World/collaborators{/collaborator}", + "comments_url": "http://api.github.com/repos/octocat/Hello-World/comments{/number}", + "commits_url": "http://api.github.com/repos/octocat/Hello-World/commits{/sha}", + "compare_url": "http://api.github.com/repos/octocat/Hello-World/compare/{base}...{head}", + "contents_url": "http://api.github.com/repos/octocat/Hello-World/contents/{+path}", + "contributors_url": "http://api.github.com/repos/octocat/Hello-World/contributors", + "deployments_url": "http://api.github.com/repos/octocat/Hello-World/deployments", + "downloads_url": "http://api.github.com/repos/octocat/Hello-World/downloads", + "events_url": "http://api.github.com/repos/octocat/Hello-World/events", + "forks_url": "http://api.github.com/repos/octocat/Hello-World/forks", + "git_commits_url": "http://api.github.com/repos/octocat/Hello-World/git/commits{/sha}", + "git_refs_url": "http://api.github.com/repos/octocat/Hello-World/git/refs{/sha}", + "git_tags_url": "http://api.github.com/repos/octocat/Hello-World/git/tags{/sha}", + "git_url": "git:github.com/octocat/Hello-World.git", + "issue_comment_url": "http://api.github.com/repos/octocat/Hello-World/issues/comments{/number}", + "issue_events_url": "http://api.github.com/repos/octocat/Hello-World/issues/events{/number}", + "issues_url": "http://api.github.com/repos/octocat/Hello-World/issues{/number}", + "keys_url": "http://api.github.com/repos/octocat/Hello-World/keys{/key_id}", + "labels_url": "http://api.github.com/repos/octocat/Hello-World/labels{/name}", + "languages_url": "http://api.github.com/repos/octocat/Hello-World/languages", + "merges_url": "http://api.github.com/repos/octocat/Hello-World/merges", + "milestones_url": "http://api.github.com/repos/octocat/Hello-World/milestones{/number}", + "notifications_url": "http://api.github.com/repos/octocat/Hello-World/notifications{?since,all,participating}", + "pulls_url": "http://api.github.com/repos/octocat/Hello-World/pulls{/number}", + "releases_url": "http://api.github.com/repos/octocat/Hello-World/releases{/id}", + "ssh_url": "git@github.com:octocat/Hello-World.git", + "stargazers_url": "http://api.github.com/repos/octocat/Hello-World/stargazers", + "statuses_url": "http://api.github.com/repos/octocat/Hello-World/statuses/{sha}", + "subscribers_url": "http://api.github.com/repos/octocat/Hello-World/subscribers", + "subscription_url": "http://api.github.com/repos/octocat/Hello-World/subscription", + "tags_url": "http://api.github.com/repos/octocat/Hello-World/tags", + "teams_url": "http://api.github.com/repos/octocat/Hello-World/teams", + "trees_url": "http://api.github.com/repos/octocat/Hello-World/git/trees{/sha}", + "clone_url": "https://github.com/octocat/Hello-World.git", + "mirror_url": "git:git.example.com/octocat/Hello-World", + "hooks_url": "http://api.github.com/repos/octocat/Hello-World/hooks", + "svn_url": "https://svn.github.com/octocat/Hello-World", + "homepage": "https://github.com", + "language": "", + "forks_count": 9, + "stargazers_count": 80, + "watchers_count": 80, + "size": 108, + "default_branch": "master", + "open_issues_count": 0, + "is_template": true, + "topics": [ + "octocat", + "atom", + "electron", + "api" + ], + "has_issues": true, + "has_projects": true, + "has_wiki": true, + "has_pages": false, + "has_downloads": true, + "archived": false, + "disabled": false, + "visibility": "public", + "pushed_at": "2011-01-26T19:06:43Z", + "created_at": "2011-01-26T19:01:12Z", + "updated_at": "2011-01-26T19:14:43Z", + "permissions": { + "admin": false, + "push": false, + "pull": true + }, + "allow_rebase_merge": true, + "template_repository": {}, + "temp_clone_token": "ABTLWHOULUVAXGTRYU7OC2876QJ2O", + "allow_squash_merge": true, + "delete_branch_on_merge": true, + "allow_merge_commit": true, + "subscribers_count": 42, + "network_count": 0 + } + }, + "_links": { + "self": { + "href": "https://api.github.com/repos/octocat/Hello-World/pulls/1347" + }, + "html": { + "href": "https://github.com/octocat/Hello-World/pull/1347" + }, + "issue": { + "href": "https://api.github.com/repos/octocat/Hello-World/issues/1347" + }, + "comments": { + "href": "https://api.github.com/repos/octocat/Hello-World/issues/1347/comments" + }, + "review_comments": { + "href": "https://api.github.com/repos/octocat/Hello-World/pulls/1347/comments" + }, + "review_comment": { + "href": "https://api.github.com/repos/octocat/Hello-World/pulls/comments{/number}" + }, + "commits": { + "href": "https://api.github.com/repos/octocat/Hello-World/pulls/1347/commits" + }, + "statuses": { + "href": "https://api.github.com/repos/octocat/Hello-World/statuses/6dcb09b5b57875f334f61aebed695e2e4193db5e" + } + }, + "author_association": "OWNER", + "draft": false, + "merged": false, + "mergeable": true, + "rebaseable": true, + "mergeable_state": "clean", + "merged_by": { + "login": "octocat", + "id": 1, + "node_id": "MDQ6VXNlcjE=", + "avatar_url": "https://github.com/images/error/octocat_happy.gif", + "gravatar_id": "", + "url": "https://api.github.com/users/octocat", + "html_url": "https://github.com/octocat", + "followers_url": "https://api.github.com/users/octocat/followers", + "following_url": "https://api.github.com/users/octocat/following{/other_user}", + "gists_url": "https://api.github.com/users/octocat/gists{/gist_id}", + "starred_url": "https://api.github.com/users/octocat/starred{/owner}{/repo}", + "subscriptions_url": "https://api.github.com/users/octocat/subscriptions", + "organizations_url": "https://api.github.com/users/octocat/orgs", + "repos_url": "https://api.github.com/users/octocat/repos", + "events_url": "https://api.github.com/users/octocat/events{/privacy}", + "received_events_url": "https://api.github.com/users/octocat/received_events", + "type": "User", + "site_admin": false + }, + "comments": 10, + "review_comments": 0, + "maintainer_can_modify": true, + "commits": 3, + "additions": 100, + "deletions": 3, + "changed_files": 5 + } \ No newline at end of file diff --git a/test/hunk-to-patch.ts b/test/hunk-to-patch.ts index eb58480b..fcd91846 100644 --- a/test/hunk-to-patch.ts +++ b/test/hunk-to-patch.ts @@ -15,7 +15,7 @@ import {expect} from 'chai'; import {describe, it, before, beforeEach} from 'mocha'; import {setup} from './util'; -import {generatePatches} from '../src/github-handler/comment-handler/patch-handler/hunk-to-patch-handler'; +import {generatePatches} from '../src/github-handler/comment-handler/raw-patch-handler/hunk-to-patch-handler'; import {Hunk, RawContent} from '../src/types'; before(() => { diff --git a/test/inline-suggest.ts b/test/inline-suggest.ts new file mode 100644 index 00000000..24fa22f6 --- /dev/null +++ b/test/inline-suggest.ts @@ -0,0 +1,221 @@ +// Copyright 2020 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import {expect} from 'chai'; +import {describe, it, before, afterEach} from 'mocha'; +import {octokit, setup} from './util'; +import * as sinon from 'sinon'; +import { + makeInlineSuggestion, + buildReviewComments, + PullsCreateReviewParamsComments, +} from '../src/github-handler/comment-handler/valid-patch-handler/upload-comments-handler'; +import {Patch} from '../src/types'; + +before(() => { + setup(); +}); + +describe('buildFileComments', () => { + it('Maps patches to GitHub comment object types', () => { + const suggestions: Map = new Map(); + const fileName1 = 'foo.txt'; + const patch1: Patch = { + start: 1, + end: 2, + newContent: 'Foo', + }; + suggestions.set(fileName1, [patch1]); + const comments = buildReviewComments(suggestions); + expect(comments).deep.equals([ + { + body: '```suggestion\nFoo```', + path: 'foo.txt', + start_line: 1, + line: 2, + side: 'RIGHT', + start_side: 'RIGHT', + }, + ]); + }); + it('Maps multiple patches to GitHub comment object types', () => { + const suggestions: Map = new Map(); + const fileName1 = 'foo.txt'; + const fileName2 = 'bar.txt'; + const patch1: Patch = { + start: 1, + end: 2, + newContent: 'Foo', + }; + const patch2: Patch = { + start: 3, + end: 4, + newContent: 'Bar', + }; + suggestions.set(fileName2, [patch1]); + suggestions.set(fileName1, [patch1, patch2]); + const comments = buildReviewComments(suggestions); + expect(comments).deep.equals([ + { + body: '```suggestion\nFoo```', + path: 'bar.txt', + start_line: 1, + line: 2, + side: 'RIGHT', + start_side: 'RIGHT', + }, + { + body: '```suggestion\nFoo```', + path: 'foo.txt', + start_line: 1, + line: 2, + side: 'RIGHT', + start_side: 'RIGHT', + }, + { + body: '```suggestion\nBar```', + path: 'foo.txt', + start_line: 3, + line: 4, + side: 'RIGHT', + start_side: 'RIGHT', + }, + ]); + }); + it('Maps empty suggestion to empty list', () => { + const suggestions: Map = new Map(); + const comments = buildReviewComments(suggestions); + expect(comments.length).deep.equals(0); + }); +}); + +describe('makeInlineSuggestion', () => { + const sandbox = sinon.createSandbox(); + const suggestions: Map = new Map(); + afterEach(() => { + sandbox.restore(); + suggestions.clear(); + }); + const fileName1 = 'foo.txt'; + const patch1: Patch = { + start: 1, + end: 2, + newContent: 'Foo', + }; + const remote = {owner: 'upstream-owner', repo: 'upstream-repo'}; + const pullNumber = 711; + it('Calls Octokit with the correct values', async () => { + suggestions.set(fileName1, [patch1]); + const responseData = await import( + './fixtures/get-pull-request-response.json' + ); + const getPullRequestResponse = { + headers: {}, + status: 200, + url: 'http://fake-url.com', + data: responseData, + }; + // setup + const stubGetPulls = sandbox + .stub(octokit.pulls, 'get') + .resolves(getPullRequestResponse); + + const stubCreateReview = sandbox.stub(octokit.pulls, 'createReview'); + // tests + + await makeInlineSuggestion(octokit, suggestions, remote, pullNumber); + sandbox.assert.calledOnceWithExactly(stubGetPulls, { + owner: remote.owner, + repo: remote.repo, + pull_number: pullNumber, + }); + sandbox.assert.calledOnceWithExactly(stubCreateReview, { + owner: remote.owner, + repo: remote.repo, + pull_number: pullNumber, + commit_id: '6dcb09b5b57875f334f61aebed695e2e4193db5e', + comments: ([ + { + body: '```suggestion\nFoo```', + path: 'foo.txt', + start_line: 1, + line: 2, + side: 'RIGHT', + start_side: 'RIGHT', + }, + ] as unknown) as PullsCreateReviewParamsComments[], + event: 'COMMENT', + headers: {accept: 'application/vnd.github.comfort-fade-preview+json'}, + }); + }); + it('Does not call octokit at all when there are no suggestions', async () => { + // setup + const stubGetPulls = sandbox.stub(octokit.pulls, 'get'); + + const stubCreateReview = sandbox.stub(octokit.pulls, 'createReview'); + // tests + + await makeInlineSuggestion(octokit, new Map(), remote, pullNumber); + sandbox.assert.notCalled(stubGetPulls); + sandbox.assert.notCalled(stubCreateReview); + }); + it('Throws and does not continue when get pull request fails', async () => { + // setup + suggestions.set(fileName1, [patch1]); + const stubGetPulls = sandbox + .stub(octokit.pulls, 'get') + .rejects(new Error()); + + const stubCreateReview = sandbox.stub(octokit.pulls, 'createReview'); + // tests + try { + await makeInlineSuggestion(octokit, suggestions, remote, pullNumber); + expect.fail('Should have failed because get pull request failed'); + } catch (err) { + sandbox.assert.called(stubGetPulls); + sandbox.assert.notCalled(stubCreateReview); + } + }); + it('Throws when create review comments fails', async () => { + // setup + suggestions.set(fileName1, [patch1]); + const responseData = await import( + './fixtures/get-pull-request-response.json' + ); + const getPullRequestResponse = { + headers: {}, + status: 200, + url: 'http://fake-url.com', + data: responseData, + }; + // setup + const stubGetPulls = sandbox + .stub(octokit.pulls, 'get') + .resolves(getPullRequestResponse); + + const stubCreateReview = sandbox + .stub(octokit.pulls, 'createReview') + .rejects(new Error()); + // tests + try { + await makeInlineSuggestion(octokit, suggestions, remote, pullNumber); + expect.fail( + 'Should have failed because create pull request review failed' + ); + } catch (err) { + sandbox.assert.called(stubGetPulls); + sandbox.assert.called(stubCreateReview); + } + }); +}); diff --git a/test/invalid-hunks.ts b/test/invalid-hunks.ts index 5202a8c5..f0ec3a5c 100644 --- a/test/invalid-hunks.ts +++ b/test/invalid-hunks.ts @@ -15,7 +15,7 @@ import {expect} from 'chai'; import {describe, it, before} from 'mocha'; import {setup} from './util'; -import {buildErrorMessage} from '../src/github-handler/comment-handler/invalid-hunk-handler'; +import {buildErrorMessage} from '../src/github-handler/comment-handler/invalid-hunk-handler/message-handler'; before(() => { setup(); diff --git a/test/scope-suggestion-hunks.ts b/test/scope-suggestion-hunks.ts index b42dc221..1525a199 100644 --- a/test/scope-suggestion-hunks.ts +++ b/test/scope-suggestion-hunks.ts @@ -21,7 +21,7 @@ import { getInScopeByFileName, getOutOfScopeByFileName, mergeOutOfScopeSuggestions, -} from '../src/github-handler/comment-handler/patch-handler/in-scope-hunks-handler'; +} from '../src/github-handler/comment-handler/raw-patch-handler/in-scope-hunks-handler'; import {Range, Hunk} from '../src/types'; before(() => { diff --git a/test/suggestion-hunk.ts b/test/suggestion-hunk.ts index 4d1eff3c..62048db4 100644 --- a/test/suggestion-hunk.ts +++ b/test/suggestion-hunk.ts @@ -18,7 +18,7 @@ import {setup} from './util'; import { generateHunks, getRawSuggestionHunks, -} from '../src/github-handler/comment-handler/patch-handler/raw-hunk-handler'; +} from '../src/github-handler/comment-handler/raw-patch-handler/raw-hunk-handler'; import {RawContent} from '../src/types'; before(() => { diff --git a/test/suggestion-patch.ts b/test/suggestion-patch.ts index 7b1c3c4d..0b33b2fe 100644 --- a/test/suggestion-patch.ts +++ b/test/suggestion-patch.ts @@ -15,7 +15,7 @@ import {expect} from 'chai'; import {describe, it, before, beforeEach} from 'mocha'; import {setup} from './util'; -import {getValidSuggestionHunks} from '../src/github-handler/comment-handler/patch-handler/in-scope-hunks-handler'; +import {getValidSuggestionHunks} from '../src/github-handler/comment-handler/raw-patch-handler/in-scope-hunks-handler'; import {Range, RawContent} from '../src/types'; before(() => { diff --git a/test/timeline-comment-invalid-hunks.ts b/test/timeline-comment-invalid-hunks.ts new file mode 100644 index 00000000..6f512afd --- /dev/null +++ b/test/timeline-comment-invalid-hunks.ts @@ -0,0 +1,52 @@ +// Copyright 2020 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import {describe, it, before, afterEach} from 'mocha'; +import {octokit, setup} from './util'; +import * as sinon from 'sinon'; +import {makeTimeLineComment} from '../src/github-handler/comment-handler/invalid-hunk-handler'; +import {Hunk} from '../src/types'; + +before(() => { + setup(); +}); + +describe('makeTimeLineComment', () => { + const sandbox = sinon.createSandbox(); + const remote = {owner: 'upstream-owner', repo: 'upstream-repo'}; + const pullNumber = 711; + afterEach(() => { + sandbox.restore(); + }); + it('Invokes the octokit create issues comment when there are invalid hunks', async () => { + const invalidHunks: Map = new Map(); + const fileName1 = 'foo.txt'; + const hunk1: Hunk = { + oldStart: 1, + oldEnd: 2, + newStart: 1, + newEnd: 2, + }; + invalidHunks.set(fileName1, [hunk1]); + const stubGetPulls = sandbox.stub(octokit.issues, 'createComment'); + await makeTimeLineComment(octokit, invalidHunks, remote, pullNumber); + sandbox.assert.called(stubGetPulls); + }); + it('Does not invoke the octokit create issues comment when there are no invalid hunks', async () => { + const invalidHunks: Map = new Map(); + const stubGetPulls = sandbox.stub(octokit.issues, 'createComment'); + await makeTimeLineComment(octokit, invalidHunks, remote, pullNumber); + sandbox.assert.notCalled(stubGetPulls); + }); +}); From e759059572cf5a1287c994e3549fe38704edda1f Mon Sep 17 00:00:00 2001 From: TomKristie <65685417+TomKristie@users.noreply.github.com> Date: Fri, 11 Sep 2020 01:00:31 -0400 Subject: [PATCH 08/10] feat(framework-core): main interface for create review on a pull request (#114) * feat(framework-core): main interface for create review on a pull request * docs: fix typo * nits and typos... * gts lint warning fix --- README.md | 50 ++ src/default-options-handler.ts | 31 +- .../get-hunk-scope-handler/index.ts | 1 + src/github-handler/comment-handler/index.ts | 8 +- .../valid-patch-handler/index.ts | 2 +- .../upload-comments-handler.ts | 2 +- src/github-handler/index.ts | 1 + src/index.ts | 61 ++- src/types/index.ts | 28 ++ test/helper-review-pull-request.ts | 446 ++++++++++++++++++ test/inline-suggest.ts | 12 +- test/main-pr-option-defaults.ts | 41 +- test/main-review-pull-request.ts | 150 ++++++ 13 files changed, 813 insertions(+), 20 deletions(-) create mode 100644 test/helper-review-pull-request.ts create mode 100644 test/main-review-pull-request.ts diff --git a/README.md b/README.md index 8d9e6731..aa7109d7 100644 --- a/README.md +++ b/README.md @@ -51,6 +51,56 @@ async function main() { } ``` +### reviewPullRequest(options) + +The `reviewPullRequest()` method creates a code suggestion review on a GitHub Pull request with the files given as input. +From these files, calculate the hunk diff and make all the multiline code suggestion review comments on a given pull request with these hunks given +that they are in scope of the pull request. Outof scope suggestions are not made. + +In-scope suggestions are specifically: suggestions whose file is in-scope of the pull request, +and suggestions whose diff hunk is a subset of the pull request's files hunks. + + If a file is too large to load in the review, it is skipped in the suggestion phase. + + If the program terminates without exception, a timeline comment will be made with all errors or suggestions that could not be made. + +#### Syntax +`reviewPullRequest(octokit, rawChanges, config [, logger])` + +#### Parameters +#### `octokit` +*octokit*
+**Required.** An authenticated [octokit](https://github.com/octokit/rest.js/) instance. + +#### `rawChanges` +*Map | null | undefined*
+**Required.** A set of files with their respective original raw file content and the new file content. If it is null, the empty map, or undefined, a review is not made. + +**RawContent Object** +| field | type | description | +|--------------- |----------- |------------- | +| oldContent | `string` | **Required.** The older version of a file. | +| newContent | `string` | **Required.** The newer version of a file. | + +#### `options` +*Review Pull Request Options Object*
+**Required.** + +**Review Pull Request Options Object** +| field | type | description | +|--------------- |----------- |------------- | +| repo | `string` | **Required.** The repository containing the pull request. | +| owner | `string` | **Required.** The owner of the repository. | +| pullNumber | `number` | **Required.** The GitHub Pull Request number. | +| pageSize | `number` | **Required.** The number of files to return in the pull request list files query. Used when getting data on the remote PR's files. | + +#### `logger` +*[Logger](https://www.npmjs.com/package/@types/pino)*
+The default logger is [Pino](https://github.com/pinojs/pino). You can plug in any logger that conforms to [Pino's interface](https://www.npmjs.com/package/@types/pino) + + +#### Exceptions +The core-library will throw an exception if the [GitHub V3 API](https://developer.github.com/v3/) returns an error response, or if the response data format did not come back as expected.
### createPullRequest(options) diff --git a/src/default-options-handler.ts b/src/default-options-handler.ts index 19399942..ca281699 100644 --- a/src/default-options-handler.ts +++ b/src/default-options-handler.ts @@ -12,10 +12,16 @@ // See the License for the specific language governing permissions and // limitations under the License. -import {CreatePullRequest, CreatePullRequestUserOptions} from './types'; +import { + CreatePullRequest, + CreatePullRequestUserOptions, + CreateReviewComment, + CreateReviewCommentUserOptions, +} from './types'; const DEFAULT_BRANCH_NAME = 'code-suggestions'; const DEFAULT_PRIMARY_BRANCH = 'master'; +const DEFAULT_PAGE_SIZE = 100; /** * Add defaults to GitHub Pull Request options. @@ -25,7 +31,7 @@ const DEFAULT_PRIMARY_BRANCH = 'master'; * @param {PullRequestUserOptions} options the user-provided github pull request options * @returns {CreatePullRequest} git hub context with defaults applied */ -function addPullRequestDefaults( +export function addPullRequestDefaults( options: CreatePullRequestUserOptions ): CreatePullRequest { const pullRequestSettings: CreatePullRequest = { @@ -46,4 +52,23 @@ function addPullRequestDefaults( return pullRequestSettings; } -export {addPullRequestDefaults}; +/** + * Format user input for pull request review comments + * @param options The user's options input for review comments + * @returns the formatted version of user input for pull request review comments + */ +export function addReviewCommentsDefaults( + options: CreateReviewCommentUserOptions +): CreateReviewComment { + const createReviewComment: CreateReviewComment = { + repo: options.repo, + owner: options.owner, + pullNumber: options.pullNumber, + // if zero set as 0 + pageSize: + options.pageSize === null || options.pageSize === undefined + ? DEFAULT_PAGE_SIZE + : options.pageSize, + }; + return createReviewComment; +} diff --git a/src/github-handler/comment-handler/get-hunk-scope-handler/index.ts b/src/github-handler/comment-handler/get-hunk-scope-handler/index.ts index ee7ed517..0c8d3f12 100644 --- a/src/github-handler/comment-handler/get-hunk-scope-handler/index.ts +++ b/src/github-handler/comment-handler/get-hunk-scope-handler/index.ts @@ -13,3 +13,4 @@ // limitations under the License. export {getPullRequestScope} from './remote-patch-ranges-handler'; +export {getGitHubPatchRanges} from './github-patch-text-handler'; diff --git a/src/github-handler/comment-handler/index.ts b/src/github-handler/comment-handler/index.ts index 6078060e..ac63f590 100644 --- a/src/github-handler/comment-handler/index.ts +++ b/src/github-handler/comment-handler/index.ts @@ -13,10 +13,10 @@ // limitations under the License. import {RawContent, RepoDomain} from '../../types'; -import {getPullRequestScope} from './get-hunk-scope-handler/remote-patch-ranges-handler'; +import {getPullRequestScope} from './get-hunk-scope-handler/'; import {Octokit} from '@octokit/rest'; import {getSuggestionPatches} from './raw-patch-handler'; -import {makeInlineSuggestion} from './valid-patch-handler'; +import {makeInlineSuggestions} from './valid-patch-handler'; import {makeTimeLineComment} from './invalid-hunk-handler'; import {logger} from '../../logger'; @@ -28,7 +28,7 @@ import {logger} from '../../logger'; * @param {number} pageSize the number of files to comment on // TODO pagination * @param {Map} rawChanges the old and new contents of the files to suggest */ -export async function comment( +export async function reviewPullRequest( octokit: Octokit, remote: RepoDomain, pullNumber: number, @@ -47,7 +47,7 @@ export async function comment( invalidFiles, validFileLines ); - await makeInlineSuggestion(octokit, filePatches, remote, pullNumber); + await makeInlineSuggestions(octokit, filePatches, remote, pullNumber); await makeTimeLineComment( octokit, outOfScopeSuggestions, diff --git a/src/github-handler/comment-handler/valid-patch-handler/index.ts b/src/github-handler/comment-handler/valid-patch-handler/index.ts index 8291bda1..e304d640 100644 --- a/src/github-handler/comment-handler/valid-patch-handler/index.ts +++ b/src/github-handler/comment-handler/valid-patch-handler/index.ts @@ -12,4 +12,4 @@ // See the License for the specific language governing permissions and // limitations under the License. -export {makeInlineSuggestion} from './upload-comments-handler'; +export {makeInlineSuggestions} from './upload-comments-handler'; diff --git a/src/github-handler/comment-handler/valid-patch-handler/upload-comments-handler.ts b/src/github-handler/comment-handler/valid-patch-handler/upload-comments-handler.ts index 190d6980..9f060a61 100644 --- a/src/github-handler/comment-handler/valid-patch-handler/upload-comments-handler.ts +++ b/src/github-handler/comment-handler/valid-patch-handler/upload-comments-handler.ts @@ -77,7 +77,7 @@ export function buildReviewComments( * @param remote * @param pullNumber */ -export async function makeInlineSuggestion( +export async function makeInlineSuggestions( octokit: Octokit, suggestions: Map, remote: RepoDomain, diff --git a/src/github-handler/index.ts b/src/github-handler/index.ts index 2e8b0c5a..617fd3dc 100644 --- a/src/github-handler/index.ts +++ b/src/github-handler/index.ts @@ -16,3 +16,4 @@ export * from './fork-handler'; export {branch} from './branch-handler'; export {commitAndPush} from './commit-and-push-handler'; export * from './pull-request-handler'; +export * from './comment-handler'; diff --git a/src/index.ts b/src/index.ts index b230b1f0..c775e1f3 100644 --- a/src/index.ts +++ b/src/index.ts @@ -20,14 +20,71 @@ import { RepoDomain, BranchDomain, FileData, + RawContent, + CreateReviewCommentUserOptions, } from './types'; export {Changes} from './types'; import {Octokit} from '@octokit/rest'; import {Logger, LoggerOptions} from 'pino'; import {logger, setupLogger} from './logger'; -import {addPullRequestDefaults} from './default-options-handler'; +import { + addPullRequestDefaults, + addReviewCommentsDefaults, +} from './default-options-handler'; import * as retry from 'async-retry'; +/** + * Given a set of suggestions, make all the multiline inline review comments on a given pull request given + * that they are in scope of the pull request. Outof scope suggestions are not made. + * + * In-scope suggestions are specifically: the suggestion for a file must correspond to a file in the remote pull request + * and the diff hunk computed for a raw file must produce a range that is a subset of the pull request's files hunks. + * + * If a file is too large to load in the review, it is skipped in the suggestion phase. + * + * If changes are empty then the workflow will not run. + * Rethrows an HttpError if Octokit GitHub API returns an error. HttpError Octokit access_token and client_secret headers redact all sensitive information. + * @param octokit The authenticated octokit instance, instantiated with an access token having permissiong to create a fork on the target repository. + * @param rawChanges A set of changes. The changes may be empty. + * @param options The configuration for interacting with GitHub provided by the user. + * @param loggerOption The logger instance (optional). + */ +export async function reviewPullRequest( + octokit: Octokit, + rawChanges: Map, + options: CreateReviewCommentUserOptions, + loggerOption?: Logger | LoggerOptions +): Promise { + setupLogger(loggerOption); + // if null undefined, or the empty map then no changes have been provided. + // Do not execute GitHub workflow + if ( + rawChanges === null || + rawChanges === undefined || + rawChanges.size === 0 + ) { + logger.info( + 'Empty changes provided. No suggestions to be made. Cancelling workflow.' + ); + return; + } + const gitHubConfigs = addReviewCommentsDefaults(options); + const remote: RepoDomain = { + owner: gitHubConfigs.owner, + repo: gitHubConfigs.repo, + }; + logger.info( + `Successfully created a review on pull request: ${gitHubConfigs.pullNumber}.` + ); + handler.reviewPullRequest( + octokit, + remote, + gitHubConfigs.pullNumber, + gitHubConfigs.pageSize, + rawChanges + ); +} + /** * Make a new GitHub Pull Request with a set of changes applied on top of primary branch HEAD. * The changes are committed into a new branch based on the upstream repository options using the authenticated Octokit account. @@ -49,7 +106,7 @@ import * as retry from 'async-retry'; * @param {Changes | null | undefined} changes A set of changes. The changes may be empty * @param {CreatePullRequestUserOptions} options The configuration for interacting with GitHub provided by the user. * @param {Logger} logger The logger instance (optional). - * @returns {Promise} a void promise + * @returns {Promise} the pull request number. Returns 0 if unsuccessful. */ async function createPullRequest( octokit: Octokit, diff --git a/src/types/index.ts b/src/types/index.ts index a40a873d..0956e57b 100644 --- a/src/types/index.ts +++ b/src/types/index.ts @@ -119,6 +119,34 @@ export interface CreatePullRequest { maintainersCanModify: boolean; } +/** + * The user options for creating GitHub PR review comment + */ +export interface CreateReviewCommentUserOptions { + // the owner of the target fork repository + owner: string; + // the name of the target fork repository + repo: string; + // The pull request number + pullNumber: number; + // The number of files to return per pull request list files query. Used when getting data on the remote PR's files. + pageSize?: number; +} + +/** + * The user options for creating GitHub PR review comment + */ +export interface CreateReviewComment { + // the owner of the target fork repository + owner: string; + // the name of the target fork repository + repo: string; + // The pull request number + pullNumber: number; + // The number of files to return per pull request list files query. Used when getting data on the remote PR's files. + pageSize: number; +} + export class PatchSyntaxError extends Error { constructor(message: string) { super(message); diff --git a/test/helper-review-pull-request.ts b/test/helper-review-pull-request.ts new file mode 100644 index 00000000..e410384e --- /dev/null +++ b/test/helper-review-pull-request.ts @@ -0,0 +1,446 @@ +// Copyright 2020 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import {assert, expect} from 'chai'; +import {describe, it, before} from 'mocha'; +import {octokit, setup} from './util'; +import {RepoDomain, RawContent, Patch, Hunk} from '../src/types'; +import {Octokit} from '@octokit/rest'; +import * as proxyquire from 'proxyquire'; +before(() => { + setup(); +}); + +describe('reviewPullRequest', () => { + const rawChanges: Map = new Map(); + rawChanges.set('src/index.ts', { + newContent: 'hello world', + oldContent: 'hello', + }); + const pageSize = 3; + const pullNumber = 100; + const repo = 'helper-comment-review-repo'; + const owner = 'helper-comment-review-owner'; + const remote: RepoDomain = {repo, owner}; + + it('All values are passed as expected', async () => { + let numMockedHelpersCalled = 0; + const validFileLines = new Map(); + const filePatches = new Map(); + const outOfScopeSuggestions = new Map(); + const invalidFiles = [ + 'invalid-file1.txt', + 'invalid-file2.txt', + 'invalid-file3.txt', + ]; + const stubMakePr = proxyquire.noCallThru()( + '../src/github-handler/comment-handler', + { + './get-hunk-scope-handler': { + getPullRequestScope: ( + testOctokit: Octokit, + testRemote: RepoDomain, + testPullNumber: number, + testPageSize: number + ) => { + expect(testOctokit).equals(octokit); + expect(testRemote.owner).equals(owner); + expect(testOctokit).equals(octokit); + expect(testRemote.repo).equals(repo); + expect(testPullNumber).equals(pullNumber); + expect(testPageSize).equals(pageSize); + numMockedHelpersCalled += 1; + return {invalidFiles, validFileLines}; + }, + }, + './raw-patch-handler': { + getSuggestionPatches: ( + testRawChanges: Map, + testInvalidFiles: string[], + testValidFileLines: Map + ) => { + expect(testRawChanges).equals(rawChanges); + expect(testInvalidFiles).equals(invalidFiles); + expect(testValidFileLines).equals(validFileLines); + numMockedHelpersCalled += 1; + return {filePatches, outOfScopeSuggestions}; + }, + }, + './valid-patch-handler': { + makeInlineSuggestions: ( + testOctokit: Octokit, + testFilePatches: Map, + testRemote: RepoDomain, + testPullNumber: number + ) => { + expect(testOctokit).equals(octokit); + expect(testFilePatches).equals(filePatches); + expect(testRemote).equals(remote); + expect(testPullNumber).equals(pullNumber); + numMockedHelpersCalled += 1; + }, + }, + './invalid-hunk-handler': { + makeTimeLineComment: ( + testOctokit: Octokit, + testOutOfScopeSuggestions: Map, + testRemote: RepoDomain, + testPullNumber: number + ) => { + expect(testOctokit).equals(octokit); + expect(testOutOfScopeSuggestions).equals(outOfScopeSuggestions); + expect(testRemote).equals(remote); + expect(testPullNumber).equals(pullNumber); + numMockedHelpersCalled += 1; + }, + }, + } + ); + await stubMakePr.reviewPullRequest( + octokit, + remote, + pullNumber, + pageSize, + rawChanges + ); + expect(numMockedHelpersCalled).equals(4); + }); + + it('Executes patch handler without error when valid patch is passed', async () => { + let numMockedHelpersCalled = 0; + const validFileLines = new Map(); + const invalidFiles = [ + 'invalid-file1.txt', + 'invalid-file2.txt', + 'invalid-file3.txt', + ]; + const stubMakePr = proxyquire.noCallThru()( + '../src/github-handler/comment-handler', + { + './get-hunk-scope-handler': { + getPullRequestScope: ( + testOctokit: Octokit, + testRemote: RepoDomain, + testPullNumber: number, + testPageSize: number + ) => { + expect(testOctokit).equals(octokit); + expect(testRemote.owner).equals(owner); + expect(testOctokit).equals(octokit); + expect(testRemote.repo).equals(repo); + expect(testPullNumber).equals(pullNumber); + expect(testPageSize).equals(pageSize); + numMockedHelpersCalled += 1; + return {invalidFiles, validFileLines}; + }, + }, + './valid-patch-handler': { + makeInlineSuggestions: ( + testOctokit: Octokit, + testFilePatches: Map, + testRemote: RepoDomain, + testPullNumber: number + ) => { + expect(testOctokit).equals(octokit); + expect(testRemote).equals(remote); + expect(testPullNumber).equals(pullNumber); + numMockedHelpersCalled += 1; + }, + }, + './invalid-hunk-handler': { + makeTimeLineComment: ( + testOctokit: Octokit, + testOutOfScopeSuggestions: Map, + testRemote: RepoDomain, + testPullNumber: number + ) => { + expect(testOctokit).equals(octokit); + expect(testRemote).equals(remote); + expect(testPullNumber).equals(pullNumber); + numMockedHelpersCalled += 1; + }, + }, + } + ); + await stubMakePr.reviewPullRequest( + octokit, + remote, + pullNumber, + pageSize, + rawChanges + ); + expect(numMockedHelpersCalled).equals(3); + }); + + it('Passes up the error message when getPullRequestScope helper fails', async () => { + let numMockedHelpersCalled = 0; + const stubMakePr = proxyquire.noCallThru()( + '../src/github-handler/comment-handler', + { + './get-hunk-scope-handler': { + getPullRequestScope: ( + testOctokit: Octokit, + testRemote: RepoDomain, + testPullNumber: number, + testPageSize: number + ) => { + expect(testOctokit).equals(octokit); + expect(testRemote.owner).equals(owner); + expect(testOctokit).equals(octokit); + expect(testRemote.repo).equals(repo); + expect(testPullNumber).equals(pullNumber); + expect(testPageSize).equals(pageSize); + numMockedHelpersCalled += 1; + throw new Error('getPullRequestScope failed'); + }, + }, + } + ); + try { + await stubMakePr.reviewPullRequest( + octokit, + remote, + pullNumber, + pageSize, + rawChanges + ); + assert.ok(false); + } catch (err) { + expect(numMockedHelpersCalled).equals(1); + expect(err.message).equals('getPullRequestScope failed'); + } + }); + + it('Passes up the error message when getSuggestionPatches helper fails', async () => { + let numMockedHelpersCalled = 0; + const validFileLines = new Map(); + const invalidFiles = [ + 'invalid-file1.txt', + 'invalid-file2.txt', + 'invalid-file3.txt', + ]; + const stubMakePr = proxyquire.noCallThru()( + '../src/github-handler/comment-handler', + { + './get-hunk-scope-handler': { + getPullRequestScope: ( + testOctokit: Octokit, + testRemote: RepoDomain, + testPullNumber: number, + testPageSize: number + ) => { + expect(testOctokit).equals(octokit); + expect(testRemote.owner).equals(owner); + expect(testOctokit).equals(octokit); + expect(testRemote.repo).equals(repo); + expect(testPullNumber).equals(pullNumber); + expect(testPageSize).equals(pageSize); + numMockedHelpersCalled += 1; + return {invalidFiles, validFileLines}; + }, + }, + './raw-patch-handler': { + getSuggestionPatches: ( + testRawChanges: Map, + testInvalidFiles: string[], + testValidFileLines: Map + ) => { + expect(testRawChanges).equals(rawChanges); + expect(testInvalidFiles).equals(invalidFiles); + expect(testValidFileLines).equals(validFileLines); + numMockedHelpersCalled += 1; + throw new Error('getSuggestionPatches failed'); + }, + }, + } + ); + try { + await stubMakePr.reviewPullRequest( + octokit, + remote, + pullNumber, + pageSize, + rawChanges + ); + assert.ok(false); + } catch (err) { + expect(numMockedHelpersCalled).equals(2); + expect(err.message).equals('getSuggestionPatches failed'); + } + }); + + it('Passes up the error message when get makeInlineSuggestions helper fails', async () => { + let numMockedHelpersCalled = 0; + const validFileLines = new Map(); + const filePatches = new Map(); + const outOfScopeSuggestions = new Map(); + const invalidFiles = [ + 'invalid-file1.txt', + 'invalid-file2.txt', + 'invalid-file3.txt', + ]; + const stubMakePr = proxyquire.noCallThru()( + '../src/github-handler/comment-handler', + { + './get-hunk-scope-handler': { + getPullRequestScope: ( + testOctokit: Octokit, + testRemote: RepoDomain, + testPullNumber: number, + testPageSize: number + ) => { + expect(testOctokit).equals(octokit); + expect(testRemote.owner).equals(owner); + expect(testOctokit).equals(octokit); + expect(testRemote.repo).equals(repo); + expect(testPullNumber).equals(pullNumber); + expect(testPageSize).equals(pageSize); + numMockedHelpersCalled += 1; + return {invalidFiles, validFileLines}; + }, + }, + './raw-patch-handler': { + getSuggestionPatches: ( + testRawChanges: Map, + testInvalidFiles: string[], + testValidFileLines: Map + ) => { + expect(testRawChanges).equals(rawChanges); + expect(testInvalidFiles).equals(invalidFiles); + expect(testValidFileLines).equals(validFileLines); + numMockedHelpersCalled += 1; + return {filePatches, outOfScopeSuggestions}; + }, + }, + './valid-patch-handler': { + makeInlineSuggestions: ( + testOctokit: Octokit, + testFilePatches: Map, + testRemote: RepoDomain, + testPullNumber: number + ) => { + expect(testOctokit).equals(octokit); + expect(testFilePatches).equals(filePatches); + expect(testRemote).equals(remote); + expect(testPullNumber).equals(pullNumber); + numMockedHelpersCalled += 1; + throw new Error('makeInlineSuggestions failed'); + }, + }, + } + ); + try { + await stubMakePr.reviewPullRequest( + octokit, + remote, + pullNumber, + pageSize, + rawChanges + ); + assert.ok(false); + } catch (err) { + expect(numMockedHelpersCalled).equals(3); + expect(err.message).equals('makeInlineSuggestions failed'); + } + }); + + it('Passes up the error message when makeTimeLineComment helper fails', async () => { + let numMockedHelpersCalled = 0; + const validFileLines = new Map(); + const filePatches = new Map(); + const outOfScopeSuggestions = new Map(); + const invalidFiles = [ + 'invalid-file1.txt', + 'invalid-file2.txt', + 'invalid-file3.txt', + ]; + const stubMakePr = proxyquire.noCallThru()( + '../src/github-handler/comment-handler', + { + './get-hunk-scope-handler': { + getPullRequestScope: ( + testOctokit: Octokit, + testRemote: RepoDomain, + testPullNumber: number, + testPageSize: number + ) => { + expect(testOctokit).equals(octokit); + expect(testRemote.owner).equals(owner); + expect(testOctokit).equals(octokit); + expect(testRemote.repo).equals(repo); + expect(testPullNumber).equals(pullNumber); + expect(testPageSize).equals(pageSize); + numMockedHelpersCalled += 1; + return {invalidFiles, validFileLines}; + }, + }, + './raw-patch-handler': { + getSuggestionPatches: ( + testRawChanges: Map, + testInvalidFiles: string[], + testValidFileLines: Map + ) => { + expect(testRawChanges).equals(rawChanges); + expect(testInvalidFiles).equals(invalidFiles); + expect(testValidFileLines).equals(validFileLines); + numMockedHelpersCalled += 1; + return {filePatches, outOfScopeSuggestions}; + }, + }, + './valid-patch-handler': { + makeInlineSuggestions: ( + testOctokit: Octokit, + testFilePatches: Map, + testRemote: RepoDomain, + testPullNumber: number + ) => { + expect(testOctokit).equals(octokit); + expect(testFilePatches).equals(filePatches); + expect(testRemote).equals(remote); + expect(testPullNumber).equals(pullNumber); + numMockedHelpersCalled += 1; + }, + }, + './invalid-hunk-handler': { + makeTimeLineComment: ( + testOctokit: Octokit, + testOutOfScopeSuggestions: Map, + testRemote: RepoDomain, + testPullNumber: number + ) => { + expect(testOctokit).equals(octokit); + expect(testOutOfScopeSuggestions).equals(outOfScopeSuggestions); + expect(testRemote).equals(remote); + expect(testPullNumber).equals(pullNumber); + numMockedHelpersCalled += 1; + throw new Error('makeTimeLineComment failed'); + }, + }, + } + ); + try { + await stubMakePr.reviewPullRequest( + octokit, + remote, + pullNumber, + pageSize, + rawChanges + ); + assert.ok(false); + } catch (err) { + expect(numMockedHelpersCalled).equals(4); + expect(err.message).equals('makeTimeLineComment failed'); + } + }); +}); diff --git a/test/inline-suggest.ts b/test/inline-suggest.ts index 24fa22f6..e83e243c 100644 --- a/test/inline-suggest.ts +++ b/test/inline-suggest.ts @@ -17,7 +17,7 @@ import {describe, it, before, afterEach} from 'mocha'; import {octokit, setup} from './util'; import * as sinon from 'sinon'; import { - makeInlineSuggestion, + makeInlineSuggestions, buildReviewComments, PullsCreateReviewParamsComments, } from '../src/github-handler/comment-handler/valid-patch-handler/upload-comments-handler'; @@ -100,7 +100,7 @@ describe('buildFileComments', () => { }); }); -describe('makeInlineSuggestion', () => { +describe('makeInlineSuggestions', () => { const sandbox = sinon.createSandbox(); const suggestions: Map = new Map(); afterEach(() => { @@ -134,7 +134,7 @@ describe('makeInlineSuggestion', () => { const stubCreateReview = sandbox.stub(octokit.pulls, 'createReview'); // tests - await makeInlineSuggestion(octokit, suggestions, remote, pullNumber); + await makeInlineSuggestions(octokit, suggestions, remote, pullNumber); sandbox.assert.calledOnceWithExactly(stubGetPulls, { owner: remote.owner, repo: remote.repo, @@ -166,7 +166,7 @@ describe('makeInlineSuggestion', () => { const stubCreateReview = sandbox.stub(octokit.pulls, 'createReview'); // tests - await makeInlineSuggestion(octokit, new Map(), remote, pullNumber); + await makeInlineSuggestions(octokit, new Map(), remote, pullNumber); sandbox.assert.notCalled(stubGetPulls); sandbox.assert.notCalled(stubCreateReview); }); @@ -180,7 +180,7 @@ describe('makeInlineSuggestion', () => { const stubCreateReview = sandbox.stub(octokit.pulls, 'createReview'); // tests try { - await makeInlineSuggestion(octokit, suggestions, remote, pullNumber); + await makeInlineSuggestions(octokit, suggestions, remote, pullNumber); expect.fail('Should have failed because get pull request failed'); } catch (err) { sandbox.assert.called(stubGetPulls); @@ -209,7 +209,7 @@ describe('makeInlineSuggestion', () => { .rejects(new Error()); // tests try { - await makeInlineSuggestion(octokit, suggestions, remote, pullNumber); + await makeInlineSuggestions(octokit, suggestions, remote, pullNumber); expect.fail( 'Should have failed because create pull request review failed' ); diff --git a/test/main-pr-option-defaults.ts b/test/main-pr-option-defaults.ts index 46d194f5..1ee58a9a 100644 --- a/test/main-pr-option-defaults.ts +++ b/test/main-pr-option-defaults.ts @@ -15,14 +15,20 @@ import {expect} from 'chai'; import {describe, it, before} from 'mocha'; import {setup} from './util'; -import {CreatePullRequestUserOptions} from '../src/types'; -import {addPullRequestDefaults} from '../src/default-options-handler'; +import { + CreatePullRequestUserOptions, + CreateReviewCommentUserOptions, +} from '../src/types'; +import { + addPullRequestDefaults, + addReviewCommentsDefaults, +} from '../src/default-options-handler'; before(() => { setup(); }); -describe('Create with defaults', () => { +describe('addPullRequestDefaults', () => { it('Populates all un-specified parameters with a default', () => { const upstreamOnly: CreatePullRequestUserOptions = { upstreamOwner: 'owner', @@ -99,3 +105,32 @@ describe('Create with defaults', () => { expect(gitHubPr).to.deep.equal(options); }); }); + +describe('addReviewCommentsDefaults', () => { + it('Populates all un-specified parameters with a default', () => { + const reviewOptionsWithDefaultPageSize: CreateReviewCommentUserOptions = { + owner: 'owner', + repo: 'repo', + pullNumber: 12345678, + }; + const gitHubPrReview = addReviewCommentsDefaults( + reviewOptionsWithDefaultPageSize + ); + expect(gitHubPrReview).to.deep.equal({ + owner: 'owner', + repo: 'repo', + pullNumber: 12345678, + pageSize: 100, + }); + }); + it("Uses all of user's provided options", () => { + const reviewOptions: CreateReviewCommentUserOptions = { + owner: 'owner', + repo: 'repo', + pullNumber: 12345678, + pageSize: 4321, + }; + const gitHubPrReview = addReviewCommentsDefaults(reviewOptions); + expect(gitHubPrReview).to.deep.equal(reviewOptions); + }); +}); diff --git a/test/main-review-pull-request.ts b/test/main-review-pull-request.ts new file mode 100644 index 00000000..21767cd8 --- /dev/null +++ b/test/main-review-pull-request.ts @@ -0,0 +1,150 @@ +// Copyright 2020 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import {assert, expect} from 'chai'; +import {describe, it, before} from 'mocha'; +import {octokit, setup} from './util'; +import {CreateReviewComment, RepoDomain, RawContent} from '../src/types'; +import {Octokit} from '@octokit/rest'; +import * as proxyquire from 'proxyquire'; +before(() => { + setup(); +}); + +/* eslint-disable @typescript-eslint/no-unused-vars */ +// tslint:disable:no-unused-expression +// .true triggers ts-lint failure, but is valid chai +describe('reviewPullRequest', () => { + const rawChanges: Map = new Map(); + rawChanges.set('src/index.ts', { + newContent: 'hello world', + oldContent: 'hello', + }); + const repo = 'main-comment-review-repo'; + const owner = 'main-comment-review-owner'; + const pullNumber = 1232143242; + const pageSize = 321; + const options: CreateReviewComment = { + repo, + owner, + pullNumber, + pageSize, + }; + + it('Does not error on success', async () => { + let numMockedHelpersCalled = 0; + const stubHelperHandlers = { + reviewPullRequest: ( + octokit: Octokit, + remote: RepoDomain, + testPullNumber: number, + testPPageSize: number, + testChanges: Map + ) => { + expect(remote.owner).equals(owner); + expect(remote.repo).equals(repo); + expect(testPullNumber).equals(pullNumber); + expect(testPPageSize).equals(pageSize); + expect(testChanges).equals(rawChanges); + numMockedHelpersCalled += 1; + }, + }; + const stubReviewPr = proxyquire.noCallThru()('../src/', { + './github-handler': stubHelperHandlers, + }); + await stubReviewPr.reviewPullRequest(octokit, rawChanges, options); + expect(numMockedHelpersCalled).equals(1); + }); + + it('Does not call the github handlers when there are no changes to make because user passed null', async () => { + const stubHelperHandlers = { + reviewPullRequest: ( + octokit: Octokit, + remote: RepoDomain, + pullNumber: number, + pageSize: number, + testChanges: Map + ) => { + assert.isOk(false); + }, + }; + const stubReviewPr = proxyquire.noCallThru()('../src/', { + './github-handler': stubHelperHandlers, + }); + await stubReviewPr.reviewPullRequest(octokit, null, options); + }); + + it('Does not call the github handlers when there are no changes to make because user passed undefined', async () => { + const stubHelperHandlers = { + reviewPullRequest: ( + octokit: Octokit, + remote: RepoDomain, + pullNumber: number, + pageSize: number, + testChanges: Map + ) => { + assert.isOk(false); + }, + }; + const stubReviewPr = proxyquire.noCallThru()('../src/', { + './github-handler': stubHelperHandlers, + }); + await stubReviewPr.reviewPullRequest(octokit, undefined, options); + }); + + it('Does not call the github handlers when there are no changes to make because user passed an empty changeset', async () => { + const stubHelperHandlers = { + reviewPullRequest: ( + octokit: Octokit, + remote: RepoDomain, + pullNumber: number, + pageSize: number, + testChanges: Map + ) => { + assert.isOk(false); + }, + }; + const stubReviewPr = proxyquire.noCallThru()('../src/', { + './github-handler': stubHelperHandlers, + }); + await stubReviewPr.reviewPullRequest(octokit, new Map(), options); + }); + + it('Passes up the error message when the create review comment helper fails', async () => { + // setup + + const stubHelperHandlers = { + reviewPullRequest: ( + octokit: Octokit, + remote: RepoDomain, + pullNumber: number, + pageSize: number, + testChanges: Map + ) => { + throw Error('Review pull request helper failed'); + }, + }; + const stubReviewPr = proxyquire.noCallThru()('../src/', { + './github-handler': stubHelperHandlers, + }); + try { + await stubReviewPr.reviewPullRequest(octokit, rawChanges, options); + expect.fail( + 'The main function should have errored because the sub-function failed.' + ); + } catch (err) { + expect(err.message).equals('Review pull request helper failed'); + } + }); +}); From 731e8c5316140109668cf5deee866ad1f0e4092c Mon Sep 17 00:00:00 2001 From: TomKristie <65685417+TomKristie@users.noreply.github.com> Date: Tue, 15 Sep 2020 18:46:06 -0400 Subject: [PATCH 09/10] fix(framework-core): combine review comments (#116) * fix(framework-core): collapsing timeline and inline comments into single review * test: fixed imports * added case when there are out of scope suggestions and no valid suggestions --- src/github-handler/comment-handler/index.ts | 7 +- .../invalid-hunk-handler/index.ts | 48 ------- .../index.ts | 0 .../message-handler.ts | 2 +- .../upload-comments-handler.ts | 22 +-- test/helper-review-pull-request.ts | 133 ++---------------- test/inline-suggest.ts | 55 ++++++-- test/invalid-hunks.ts | 8 +- test/timeline-comment-invalid-hunks.ts | 52 ------- 9 files changed, 80 insertions(+), 247 deletions(-) delete mode 100644 src/github-handler/comment-handler/invalid-hunk-handler/index.ts rename src/github-handler/comment-handler/{valid-patch-handler => make-review-handler}/index.ts (100%) rename src/github-handler/comment-handler/{invalid-hunk-handler => make-review-handler}/message-handler.ts (94%) rename src/github-handler/comment-handler/{valid-patch-handler => make-review-handler}/upload-comments-handler.ts (86%) delete mode 100644 test/timeline-comment-invalid-hunks.ts diff --git a/src/github-handler/comment-handler/index.ts b/src/github-handler/comment-handler/index.ts index ac63f590..42b112cc 100644 --- a/src/github-handler/comment-handler/index.ts +++ b/src/github-handler/comment-handler/index.ts @@ -16,8 +16,7 @@ import {RawContent, RepoDomain} from '../../types'; import {getPullRequestScope} from './get-hunk-scope-handler/'; import {Octokit} from '@octokit/rest'; import {getSuggestionPatches} from './raw-patch-handler'; -import {makeInlineSuggestions} from './valid-patch-handler'; -import {makeTimeLineComment} from './invalid-hunk-handler'; +import {makeInlineSuggestions} from './make-review-handler'; import {logger} from '../../logger'; /** @@ -47,9 +46,9 @@ export async function reviewPullRequest( invalidFiles, validFileLines ); - await makeInlineSuggestions(octokit, filePatches, remote, pullNumber); - await makeTimeLineComment( + await makeInlineSuggestions( octokit, + filePatches, outOfScopeSuggestions, remote, pullNumber diff --git a/src/github-handler/comment-handler/invalid-hunk-handler/index.ts b/src/github-handler/comment-handler/invalid-hunk-handler/index.ts deleted file mode 100644 index 385e0e10..00000000 --- a/src/github-handler/comment-handler/invalid-hunk-handler/index.ts +++ /dev/null @@ -1,48 +0,0 @@ -// Copyright 2020 Google LLC -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// https://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -import {Octokit} from '@octokit/rest'; -import {Hunk, RepoDomain} from '../../../types'; -import {buildErrorMessage} from './message-handler'; -import {logger} from '../../../logger'; - -/** - * From the invalid hunks provided, make a comment on the pull request - * timeline detailing what hunks and files could not be commented on. - * @param octokit - * @param invalidHunks - * @param remote - * @param pullNumber - */ -export async function makeTimeLineComment( - octokit: Octokit, - invalidHunks: Map, - remote: RepoDomain, - pullNumber: number -): Promise { - try { - const errorMessage = buildErrorMessage(invalidHunks); - if (errorMessage) { - await octokit.issues.createComment({ - owner: remote.owner, - repo: remote.repo, - issue_number: pullNumber, - body: errorMessage, - }); - } - } catch (err) { - logger.error('Failed to make a timeline comment'); - throw err; - } -} diff --git a/src/github-handler/comment-handler/valid-patch-handler/index.ts b/src/github-handler/comment-handler/make-review-handler/index.ts similarity index 100% rename from src/github-handler/comment-handler/valid-patch-handler/index.ts rename to src/github-handler/comment-handler/make-review-handler/index.ts diff --git a/src/github-handler/comment-handler/invalid-hunk-handler/message-handler.ts b/src/github-handler/comment-handler/make-review-handler/message-handler.ts similarity index 94% rename from src/github-handler/comment-handler/invalid-hunk-handler/message-handler.ts rename to src/github-handler/comment-handler/make-review-handler/message-handler.ts index f61b8b1d..4d24a12b 100644 --- a/src/github-handler/comment-handler/invalid-hunk-handler/message-handler.ts +++ b/src/github-handler/comment-handler/make-review-handler/message-handler.ts @@ -27,7 +27,7 @@ function fileErrorMessage(filename: string, hunks: Hunk[]): string { * Returns an empty string if the provided hunks are empty. * @param invalidHunks a map of filename to hunks that are not suggestable */ -export function buildErrorMessage(invalidHunks: Map): string { +export function buildSummaryComment(invalidHunks: Map): string { if (invalidHunks.size === 0) { return ''; } diff --git a/src/github-handler/comment-handler/valid-patch-handler/upload-comments-handler.ts b/src/github-handler/comment-handler/make-review-handler/upload-comments-handler.ts similarity index 86% rename from src/github-handler/comment-handler/valid-patch-handler/upload-comments-handler.ts rename to src/github-handler/comment-handler/make-review-handler/upload-comments-handler.ts index 9f060a61..d0b8236b 100644 --- a/src/github-handler/comment-handler/valid-patch-handler/upload-comments-handler.ts +++ b/src/github-handler/comment-handler/make-review-handler/upload-comments-handler.ts @@ -13,8 +13,9 @@ // limitations under the License. import {Octokit} from '@octokit/rest'; -import {Patch, RepoDomain} from '../../../types'; +import {Hunk, Patch, RepoDomain} from '../../../types'; import {logger} from '../../../logger'; +import {buildSummaryComment} from './message-handler'; const COMFORT_PREVIEW_HEADER = 'application/vnd.github.comfort-fade-preview+json'; @@ -80,13 +81,22 @@ export function buildReviewComments( export async function makeInlineSuggestions( octokit: Octokit, suggestions: Map, + outOfScopeSuggestions: Map, remote: RepoDomain, pullNumber: number ): Promise { - if (!suggestions.size) { - logger.info('No suggestions to make. Exiting...'); + const comments = buildReviewComments(suggestions); + if (!comments.length) { + logger.info('No valid suggestions to make'); + } + if (!comments.length && !outOfScopeSuggestions.size) { + logger.info('No suggestions were generated. Exiting...'); return; } + const summaryComment = buildSummaryComment(outOfScopeSuggestions); + if (summaryComment) { + logger.warn('Some suggestions could not be made'); + } // apply the suggestions to the latest sha // the latest Pull Request hunk range includes // all previous commit valid hunk ranges @@ -97,17 +107,13 @@ export async function makeInlineSuggestions( pull_number: pullNumber, }) ).data.head.sha; - const comments = buildReviewComments(suggestions); - if (!comments.length) { - logger.info('No suggestions to make. Exiting...'); - return; - } await octokit.pulls.createReview({ owner: remote.owner, repo: remote.repo, pull_number: pullNumber, commit_id: headSha, event: 'COMMENT', + body: summaryComment, headers: {accept: COMFORT_PREVIEW_HEADER}, // Octokit type definitions doesn't support mulitiline comments, but the GitHub API does comments: (comments as unknown) as PullsCreateReviewParamsComments[], diff --git a/test/helper-review-pull-request.ts b/test/helper-review-pull-request.ts index e410384e..cccbe27b 100644 --- a/test/helper-review-pull-request.ts +++ b/test/helper-review-pull-request.ts @@ -34,7 +34,7 @@ describe('reviewPullRequest', () => { const owner = 'helper-comment-review-owner'; const remote: RepoDomain = {repo, owner}; - it('All values are passed as expected', async () => { + it('Succeeds when all values are passed as expected', async () => { let numMockedHelpersCalled = 0; const validFileLines = new Map(); const filePatches = new Map(); @@ -77,30 +77,18 @@ describe('reviewPullRequest', () => { return {filePatches, outOfScopeSuggestions}; }, }, - './valid-patch-handler': { + './make-review-handler': { makeInlineSuggestions: ( testOctokit: Octokit, testFilePatches: Map, + testoutOfScopeSuggestions: Map, testRemote: RepoDomain, testPullNumber: number ) => { expect(testOctokit).equals(octokit); expect(testFilePatches).equals(filePatches); expect(testRemote).equals(remote); - expect(testPullNumber).equals(pullNumber); - numMockedHelpersCalled += 1; - }, - }, - './invalid-hunk-handler': { - makeTimeLineComment: ( - testOctokit: Octokit, - testOutOfScopeSuggestions: Map, - testRemote: RepoDomain, - testPullNumber: number - ) => { - expect(testOctokit).equals(octokit); - expect(testOutOfScopeSuggestions).equals(outOfScopeSuggestions); - expect(testRemote).equals(remote); + expect(testoutOfScopeSuggestions).equals(outOfScopeSuggestions); expect(testPullNumber).equals(pullNumber); numMockedHelpersCalled += 1; }, @@ -114,7 +102,7 @@ describe('reviewPullRequest', () => { pageSize, rawChanges ); - expect(numMockedHelpersCalled).equals(4); + expect(numMockedHelpersCalled).equals(3); }); it('Executes patch handler without error when valid patch is passed', async () => { @@ -145,23 +133,11 @@ describe('reviewPullRequest', () => { return {invalidFiles, validFileLines}; }, }, - './valid-patch-handler': { + './make-review-handler': { makeInlineSuggestions: ( testOctokit: Octokit, testFilePatches: Map, - testRemote: RepoDomain, - testPullNumber: number - ) => { - expect(testOctokit).equals(octokit); - expect(testRemote).equals(remote); - expect(testPullNumber).equals(pullNumber); - numMockedHelpersCalled += 1; - }, - }, - './invalid-hunk-handler': { - makeTimeLineComment: ( - testOctokit: Octokit, - testOutOfScopeSuggestions: Map, + testoutOfScopeSuggestions: Map, testRemote: RepoDomain, testPullNumber: number ) => { @@ -180,7 +156,7 @@ describe('reviewPullRequest', () => { pageSize, rawChanges ); - expect(numMockedHelpersCalled).equals(3); + expect(numMockedHelpersCalled).equals(2); }); it('Passes up the error message when getPullRequestScope helper fails', async () => { @@ -323,16 +299,18 @@ describe('reviewPullRequest', () => { return {filePatches, outOfScopeSuggestions}; }, }, - './valid-patch-handler': { + './make-review-handler': { makeInlineSuggestions: ( testOctokit: Octokit, testFilePatches: Map, + testoutOfScopeSuggestions: Map, testRemote: RepoDomain, testPullNumber: number ) => { expect(testOctokit).equals(octokit); expect(testFilePatches).equals(filePatches); expect(testRemote).equals(remote); + expect(testoutOfScopeSuggestions).equals(outOfScopeSuggestions); expect(testPullNumber).equals(pullNumber); numMockedHelpersCalled += 1; throw new Error('makeInlineSuggestions failed'); @@ -354,93 +332,4 @@ describe('reviewPullRequest', () => { expect(err.message).equals('makeInlineSuggestions failed'); } }); - - it('Passes up the error message when makeTimeLineComment helper fails', async () => { - let numMockedHelpersCalled = 0; - const validFileLines = new Map(); - const filePatches = new Map(); - const outOfScopeSuggestions = new Map(); - const invalidFiles = [ - 'invalid-file1.txt', - 'invalid-file2.txt', - 'invalid-file3.txt', - ]; - const stubMakePr = proxyquire.noCallThru()( - '../src/github-handler/comment-handler', - { - './get-hunk-scope-handler': { - getPullRequestScope: ( - testOctokit: Octokit, - testRemote: RepoDomain, - testPullNumber: number, - testPageSize: number - ) => { - expect(testOctokit).equals(octokit); - expect(testRemote.owner).equals(owner); - expect(testOctokit).equals(octokit); - expect(testRemote.repo).equals(repo); - expect(testPullNumber).equals(pullNumber); - expect(testPageSize).equals(pageSize); - numMockedHelpersCalled += 1; - return {invalidFiles, validFileLines}; - }, - }, - './raw-patch-handler': { - getSuggestionPatches: ( - testRawChanges: Map, - testInvalidFiles: string[], - testValidFileLines: Map - ) => { - expect(testRawChanges).equals(rawChanges); - expect(testInvalidFiles).equals(invalidFiles); - expect(testValidFileLines).equals(validFileLines); - numMockedHelpersCalled += 1; - return {filePatches, outOfScopeSuggestions}; - }, - }, - './valid-patch-handler': { - makeInlineSuggestions: ( - testOctokit: Octokit, - testFilePatches: Map, - testRemote: RepoDomain, - testPullNumber: number - ) => { - expect(testOctokit).equals(octokit); - expect(testFilePatches).equals(filePatches); - expect(testRemote).equals(remote); - expect(testPullNumber).equals(pullNumber); - numMockedHelpersCalled += 1; - }, - }, - './invalid-hunk-handler': { - makeTimeLineComment: ( - testOctokit: Octokit, - testOutOfScopeSuggestions: Map, - testRemote: RepoDomain, - testPullNumber: number - ) => { - expect(testOctokit).equals(octokit); - expect(testOutOfScopeSuggestions).equals(outOfScopeSuggestions); - expect(testRemote).equals(remote); - expect(testPullNumber).equals(pullNumber); - numMockedHelpersCalled += 1; - throw new Error('makeTimeLineComment failed'); - }, - }, - } - ); - try { - await stubMakePr.reviewPullRequest( - octokit, - remote, - pullNumber, - pageSize, - rawChanges - ); - assert.ok(false); - } catch (err) { - expect(numMockedHelpersCalled).equals(4); - expect(err.message).equals('makeTimeLineComment failed'); - } - }); }); diff --git a/test/inline-suggest.ts b/test/inline-suggest.ts index e83e243c..f3e0d052 100644 --- a/test/inline-suggest.ts +++ b/test/inline-suggest.ts @@ -20,8 +20,8 @@ import { makeInlineSuggestions, buildReviewComments, PullsCreateReviewParamsComments, -} from '../src/github-handler/comment-handler/valid-patch-handler/upload-comments-handler'; -import {Patch} from '../src/types'; +} from '../src/github-handler/comment-handler/make-review-handler/upload-comments-handler'; +import {Patch, Hunk} from '../src/types'; before(() => { setup(); @@ -103,6 +103,7 @@ describe('buildFileComments', () => { describe('makeInlineSuggestions', () => { const sandbox = sinon.createSandbox(); const suggestions: Map = new Map(); + const outOfScopeSuggestions: Map = new Map(); afterEach(() => { sandbox.restore(); suggestions.clear(); @@ -134,7 +135,13 @@ describe('makeInlineSuggestions', () => { const stubCreateReview = sandbox.stub(octokit.pulls, 'createReview'); // tests - await makeInlineSuggestions(octokit, suggestions, remote, pullNumber); + await makeInlineSuggestions( + octokit, + suggestions, + outOfScopeSuggestions, + remote, + pullNumber + ); sandbox.assert.calledOnceWithExactly(stubGetPulls, { owner: remote.owner, repo: remote.repo, @@ -145,6 +152,7 @@ describe('makeInlineSuggestions', () => { repo: remote.repo, pull_number: pullNumber, commit_id: '6dcb09b5b57875f334f61aebed695e2e4193db5e', + body: '', comments: ([ { body: '```suggestion\nFoo```', @@ -159,17 +167,36 @@ describe('makeInlineSuggestions', () => { headers: {accept: 'application/vnd.github.comfort-fade-preview+json'}, }); }); - it('Does not call octokit at all when there are no suggestions', async () => { + + it('Does not create a review when there are no suggestions and no out of scope suggestions generated', async () => { + const responseData = await import( + './fixtures/get-pull-request-response.json' + ); + const getPullRequestResponse = { + headers: {}, + status: 200, + url: 'http://fake-url.com', + data: responseData, + }; // setup - const stubGetPulls = sandbox.stub(octokit.pulls, 'get'); + const stubGetPulls = sandbox + .stub(octokit.pulls, 'get') + .resolves(getPullRequestResponse); const stubCreateReview = sandbox.stub(octokit.pulls, 'createReview'); // tests - await makeInlineSuggestions(octokit, new Map(), remote, pullNumber); + await makeInlineSuggestions( + octokit, + suggestions, + outOfScopeSuggestions, + remote, + pullNumber + ); sandbox.assert.notCalled(stubGetPulls); sandbox.assert.notCalled(stubCreateReview); }); + it('Throws and does not continue when get pull request fails', async () => { // setup suggestions.set(fileName1, [patch1]); @@ -180,7 +207,13 @@ describe('makeInlineSuggestions', () => { const stubCreateReview = sandbox.stub(octokit.pulls, 'createReview'); // tests try { - await makeInlineSuggestions(octokit, suggestions, remote, pullNumber); + await makeInlineSuggestions( + octokit, + suggestions, + outOfScopeSuggestions, + remote, + pullNumber + ); expect.fail('Should have failed because get pull request failed'); } catch (err) { sandbox.assert.called(stubGetPulls); @@ -209,7 +242,13 @@ describe('makeInlineSuggestions', () => { .rejects(new Error()); // tests try { - await makeInlineSuggestions(octokit, suggestions, remote, pullNumber); + await makeInlineSuggestions( + octokit, + suggestions, + outOfScopeSuggestions, + remote, + pullNumber + ); expect.fail( 'Should have failed because create pull request review failed' ); diff --git a/test/invalid-hunks.ts b/test/invalid-hunks.ts index f0ec3a5c..ce4c0530 100644 --- a/test/invalid-hunks.ts +++ b/test/invalid-hunks.ts @@ -15,7 +15,7 @@ import {expect} from 'chai'; import {describe, it, before} from 'mocha'; import {setup} from './util'; -import {buildErrorMessage} from '../src/github-handler/comment-handler/invalid-hunk-handler/message-handler'; +import {buildSummaryComment} from '../src/github-handler/comment-handler/make-review-handler/message-handler'; before(() => { setup(); @@ -26,7 +26,7 @@ describe('buildErrorMessage', () => { const invalidHunks = new Map(); const expectedMessage = ''; - const errorMessage = buildErrorMessage(invalidHunks); + const errorMessage = buildSummaryComment(invalidHunks); expect(errorMessage).to.be.equal(expectedMessage); }); @@ -44,7 +44,7 @@ describe('buildErrorMessage', () => { * bar.txt * lines 3-4`; - const errorMessage = buildErrorMessage(invalidHunks); + const errorMessage = buildSummaryComment(invalidHunks); expect(errorMessage).to.be.equal(expectedMessage); }); @@ -59,7 +59,7 @@ describe('buildErrorMessage', () => { * lines 1-2 * lines 3-4`; - const errorMessage = buildErrorMessage(invalidHunks); + const errorMessage = buildSummaryComment(invalidHunks); expect(errorMessage).to.be.equal(expectedMessage); }); }); diff --git a/test/timeline-comment-invalid-hunks.ts b/test/timeline-comment-invalid-hunks.ts deleted file mode 100644 index 6f512afd..00000000 --- a/test/timeline-comment-invalid-hunks.ts +++ /dev/null @@ -1,52 +0,0 @@ -// Copyright 2020 Google LLC -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -import {describe, it, before, afterEach} from 'mocha'; -import {octokit, setup} from './util'; -import * as sinon from 'sinon'; -import {makeTimeLineComment} from '../src/github-handler/comment-handler/invalid-hunk-handler'; -import {Hunk} from '../src/types'; - -before(() => { - setup(); -}); - -describe('makeTimeLineComment', () => { - const sandbox = sinon.createSandbox(); - const remote = {owner: 'upstream-owner', repo: 'upstream-repo'}; - const pullNumber = 711; - afterEach(() => { - sandbox.restore(); - }); - it('Invokes the octokit create issues comment when there are invalid hunks', async () => { - const invalidHunks: Map = new Map(); - const fileName1 = 'foo.txt'; - const hunk1: Hunk = { - oldStart: 1, - oldEnd: 2, - newStart: 1, - newEnd: 2, - }; - invalidHunks.set(fileName1, [hunk1]); - const stubGetPulls = sandbox.stub(octokit.issues, 'createComment'); - await makeTimeLineComment(octokit, invalidHunks, remote, pullNumber); - sandbox.assert.called(stubGetPulls); - }); - it('Does not invoke the octokit create issues comment when there are no invalid hunks', async () => { - const invalidHunks: Map = new Map(); - const stubGetPulls = sandbox.stub(octokit.issues, 'createComment'); - await makeTimeLineComment(octokit, invalidHunks, remote, pullNumber); - sandbox.assert.notCalled(stubGetPulls); - }); -}); From 17959306f44b730008a5f99eadc823a047e02753 Mon Sep 17 00:00:00 2001 From: TomKristie <65685417+TomKristie@users.noreply.github.com> Date: Thu, 24 Sep 2020 19:59:58 -0400 Subject: [PATCH 10/10] feat(framework-core): return review number and variable renaming (#117) * feat(framework-core): return review number and variable renaming * lint --- README.md | 15 ++-- src/github-handler/comment-handler/index.ts | 14 +-- .../upload-comments-handler.ts | 38 ++++---- .../hunk-to-patch-handler.ts | 8 +- .../in-scope-hunks-handler.ts | 8 +- .../raw-patch-handler/index.ts | 10 +-- .../raw-patch-handler/raw-hunk-handler.ts | 20 ++--- src/index.ts | 27 +++--- src/types/index.ts | 4 +- test/helper-review-pull-request.ts | 28 +++--- test/hunk-to-patch.ts | 46 +++++----- test/inline-suggest.ts | 90 ++++++++++++++++++- test/main-review-pull-request.ts | 22 ++--- test/suggestion-hunk.ts | 62 +++++++------ test/suggestion-patch.ts | 38 ++++---- 15 files changed, 265 insertions(+), 165 deletions(-) diff --git a/README.md b/README.md index be41b565..35122a01 100644 --- a/README.md +++ b/README.md @@ -65,18 +65,21 @@ and suggestions whose diff hunk is a subset of the pull request's files hunks. If the program terminates without exception, a timeline comment will be made with all errors or suggestions that could not be made. #### Syntax -`reviewPullRequest(octokit, rawChanges, config [, logger])` +`reviewPullRequest(octokit, diffContents, config [, logger])` #### Parameters #### `octokit` *octokit*
**Required.** An authenticated [octokit](https://github.com/octokit/rest.js/) instance. -#### `rawChanges` -*Map | null | undefined*
-**Required.** A set of files with their respective original raw file content and the new file content. If it is null, the empty map, or undefined, a review is not made. +#### `diffContents` +*Map | null | undefined*
+**Required.** A set of files with their respective original text file content and the new file content. +If the map is null, the empty map, or undefined, a review is not made. +A review is also not made when forall FileDiffContent objects, f, +f.oldContent == f.newContent -**RawContent Object** +**FileDiffContent Object** | field | type | description | |--------------- |----------- |------------- | | oldContent | `string` | **Required.** The older version of a file. | @@ -98,6 +101,8 @@ and suggestions whose diff hunk is a subset of the pull request's files hunks. *[Logger](https://www.npmjs.com/package/@types/pino)*
The default logger is [Pino](https://github.com/pinojs/pino). You can plug in any logger that conforms to [Pino's interface](https://www.npmjs.com/package/@types/pino) +#### returns +returns the review number if a review was created, or null if a review was not made and an exception was not thrown. #### Exceptions The core-library will throw an exception if the [GitHub V3 API](https://developer.github.com/v3/) returns an error response, or if the response data format did not come back as expected.
diff --git a/src/github-handler/comment-handler/index.ts b/src/github-handler/comment-handler/index.ts index 42b112cc..70ece1f0 100644 --- a/src/github-handler/comment-handler/index.ts +++ b/src/github-handler/comment-handler/index.ts @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -import {RawContent, RepoDomain} from '../../types'; +import {FileDiffContent, RepoDomain} from '../../types'; import {getPullRequestScope} from './get-hunk-scope-handler/'; import {Octokit} from '@octokit/rest'; import {getSuggestionPatches} from './raw-patch-handler'; @@ -25,15 +25,16 @@ import {logger} from '../../logger'; * @param {RepoDomain} remote the Pull Request repository * @param {number} pullNumber the Pull Request number * @param {number} pageSize the number of files to comment on // TODO pagination - * @param {Map} rawChanges the old and new contents of the files to suggest + * @param {Map} diffContents the old and new contents of the files to suggest + * @returns the created review's id, or null if no review was made */ export async function reviewPullRequest( octokit: Octokit, remote: RepoDomain, pullNumber: number, pageSize: number, - rawChanges: Map -): Promise { + diffContents: Map +): Promise { try { const {invalidFiles, validFileLines} = await getPullRequestScope( octokit, @@ -42,17 +43,18 @@ export async function reviewPullRequest( pageSize ); const {filePatches, outOfScopeSuggestions} = getSuggestionPatches( - rawChanges, + diffContents, invalidFiles, validFileLines ); - await makeInlineSuggestions( + const reviewNumber = await makeInlineSuggestions( octokit, filePatches, outOfScopeSuggestions, remote, pullNumber ); + return reviewNumber; } catch (err) { logger.error('Failed to suggest'); throw err; diff --git a/src/github-handler/comment-handler/make-review-handler/upload-comments-handler.ts b/src/github-handler/comment-handler/make-review-handler/upload-comments-handler.ts index d0b8236b..541d2dda 100644 --- a/src/github-handler/comment-handler/make-review-handler/upload-comments-handler.ts +++ b/src/github-handler/comment-handler/make-review-handler/upload-comments-handler.ts @@ -73,10 +73,10 @@ export function buildReviewComments( /** * Make a request to GitHub to make review comments - * @param octokit - * @param suggestions - * @param remote - * @param pullNumber + * @param octokit an authenticated octokit instance + * @param suggestions code suggestions patches + * @param remote the repository domain + * @param pullNumber the pull request number to make a review on */ export async function makeInlineSuggestions( octokit: Octokit, @@ -84,14 +84,14 @@ export async function makeInlineSuggestions( outOfScopeSuggestions: Map, remote: RepoDomain, pullNumber: number -): Promise { +): Promise { const comments = buildReviewComments(suggestions); if (!comments.length) { logger.info('No valid suggestions to make'); } if (!comments.length && !outOfScopeSuggestions.size) { logger.info('No suggestions were generated. Exiting...'); - return; + return null; } const summaryComment = buildSummaryComment(outOfScopeSuggestions); if (summaryComment) { @@ -107,15 +107,19 @@ export async function makeInlineSuggestions( pull_number: pullNumber, }) ).data.head.sha; - await octokit.pulls.createReview({ - owner: remote.owner, - repo: remote.repo, - pull_number: pullNumber, - commit_id: headSha, - event: 'COMMENT', - body: summaryComment, - headers: {accept: COMFORT_PREVIEW_HEADER}, - // Octokit type definitions doesn't support mulitiline comments, but the GitHub API does - comments: (comments as unknown) as PullsCreateReviewParamsComments[], - }); + const reviewNumber = ( + await octokit.pulls.createReview({ + owner: remote.owner, + repo: remote.repo, + pull_number: pullNumber, + commit_id: headSha, + event: 'COMMENT', + body: summaryComment, + headers: {accept: COMFORT_PREVIEW_HEADER}, + // Octokit type definitions doesn't support mulitiline comments, but the GitHub API does + comments: (comments as unknown) as PullsCreateReviewParamsComments[], + }) + ).data.id; + logger.info(`Successfully created a review on pull request: ${pullNumber}.`); + return reviewNumber; } diff --git a/src/github-handler/comment-handler/raw-patch-handler/hunk-to-patch-handler.ts b/src/github-handler/comment-handler/raw-patch-handler/hunk-to-patch-handler.ts index 6e8b0ded..5f61e900 100644 --- a/src/github-handler/comment-handler/raw-patch-handler/hunk-to-patch-handler.ts +++ b/src/github-handler/comment-handler/raw-patch-handler/hunk-to-patch-handler.ts @@ -12,18 +12,18 @@ // See the License for the specific language governing permissions and // limitations under the License. -import {Hunk, Patch, RawContent} from '../../../types'; +import {Hunk, Patch, FileDiffContent} from '../../../types'; /** * From the each file's hunk, generate each file's hunk's old version range * and the new content (from the user's) to update that range with. * @param filesHunks a list of hunks for each file where the lines are at least 1 - * @param rawChanges + * @param diffContents * @returns patches to upload to octokit */ export function generatePatches( filesHunks: Map, - rawChanges: Map + diffContents: Map ): Map { const filesPatches: Map = new Map(); filesHunks.forEach((hunks, fileName) => { @@ -31,7 +31,7 @@ export function generatePatches( // we expect all hunk lists to not be empty hunks.forEach(hunk => { // returns a copy of the hashmap value, then creates a new list with new substrings - const lines = rawChanges.get(fileName)!.newContent.split('\n'); + const lines = diffContents.get(fileName)!.newContent.split('\n'); // creates a new shallow-copied subarray // we assume the hunk new start and new end to be within the domain of the lines length if ( diff --git a/src/github-handler/comment-handler/raw-patch-handler/in-scope-hunks-handler.ts b/src/github-handler/comment-handler/raw-patch-handler/in-scope-hunks-handler.ts index 1a21ecb1..55623dd2 100644 --- a/src/github-handler/comment-handler/raw-patch-handler/in-scope-hunks-handler.ts +++ b/src/github-handler/comment-handler/raw-patch-handler/in-scope-hunks-handler.ts @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -import {Hunk, Range, RawContent} from '../../../types'; +import {Hunk, Range, FileDiffContent} from '../../../types'; import {getRawSuggestionHunks} from './raw-hunk-handler'; /** @@ -165,19 +165,19 @@ export function mergeOutOfScopeSuggestions( /** * Get the in scope (of the corresponding pull request's) hunks and files - * @param {Map} rawChanges the raw old content and new content of a file + * @param {Map} diffContents the old text content and new text content of a file * @param {string[]} invalidFiles list of invalid files * @param {Map} validFileLines a map of each file's in scope lines for a Pull Request */ export function getValidSuggestionHunks( - rawChanges: Map, + diffContents: Map, invalidFiles: string[], validFileLines: Map ): { inScopeSuggestions: Map; outOfScopeSuggestions: Map; } { - const totalfileHunks = getRawSuggestionHunks(rawChanges); + const totalfileHunks = getRawSuggestionHunks(diffContents); const outofScopeByFilename = getOutOfScopeByFileName( invalidFiles, totalfileHunks diff --git a/src/github-handler/comment-handler/raw-patch-handler/index.ts b/src/github-handler/comment-handler/raw-patch-handler/index.ts index 67baba90..10955df4 100644 --- a/src/github-handler/comment-handler/raw-patch-handler/index.ts +++ b/src/github-handler/comment-handler/raw-patch-handler/index.ts @@ -14,7 +14,7 @@ import {generatePatches} from './hunk-to-patch-handler'; import {getValidSuggestionHunks} from './in-scope-hunks-handler'; -import {Hunk, RawContent, Range, Patch} from '../../../types'; +import {Hunk, FileDiffContent, Range, Patch} from '../../../types'; interface SuggestionPatches { filePatches: Map; @@ -25,23 +25,23 @@ interface SuggestionPatches { * Get the range of the old version of every file and the corresponding new text for that range * whose old and new contents differ, under the constraints that the file * is in scope for Pull Request, as well as its lines. - * @param {Map} rawChanges the raw old content and new content of a file + * @param {Map} diffContents the old text content and new text content of a file * @param {string[]} invalidFiles list of invalid files * @param {Map} validFileLines a map of each file's in scope lines for a Pull Request */ export function getSuggestionPatches( - rawChanges: Map, + diffContents: Map, invalidFiles: string[], validFileLines: Map ): SuggestionPatches { const {inScopeSuggestions, outOfScopeSuggestions} = getValidSuggestionHunks( - rawChanges, + diffContents, invalidFiles, validFileLines ); const filePatches: Map = generatePatches( inScopeSuggestions, - rawChanges + diffContents ); return {filePatches, outOfScopeSuggestions}; } diff --git a/src/github-handler/comment-handler/raw-patch-handler/raw-hunk-handler.ts b/src/github-handler/comment-handler/raw-patch-handler/raw-hunk-handler.ts index bd5967c7..e979a1df 100644 --- a/src/github-handler/comment-handler/raw-patch-handler/raw-hunk-handler.ts +++ b/src/github-handler/comment-handler/raw-patch-handler/raw-hunk-handler.ts @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -import {Hunk, RawContent} from '../../../types'; +import {Hunk, FileDiffContent} from '../../../types'; import {logger} from '../../../logger'; import {structuredPatch, Hunk as RawHunk} from 'diff'; @@ -20,19 +20,19 @@ import {structuredPatch, Hunk as RawHunk} from 'diff'; * Given the old content and new content of a file * compute the diff and extract the necessary hunk data. * The hunk list is a list of disjoint ascending intervals. - * @param rawContent + * @param fileDiffContent * @param fileName * @returns the hunks that are generated in ascending sorted order */ export function generateHunks( - rawContent: RawContent, + fileDiffContent: FileDiffContent, fileName: string ): Hunk[] { const rawHunks: RawHunk[] = structuredPatch( fileName, // old file name fileName, // new file name - rawContent.oldContent, - rawContent.newContent + fileDiffContent.oldContent, + fileDiffContent.newContent ).hunks; const hunks: Hunk[] = rawHunks.map(rawHunk => ({ oldStart: rawHunk.oldStart, @@ -49,19 +49,19 @@ export function generateHunks( * compute the hunk for each file whose old and new contents differ. * Do not compute the hunk if the old content is the same as the new content. * The hunk list is sorted and each interval is disjoint. - * @param {Map} rawChanges a map of the original file contents and the new file contents + * @param {Map} diffContents a map of the original file contents and the new file contents * @returns the hunks for each file whose old and new contents differ */ export function getRawSuggestionHunks( - rawChanges: Map + diffContents: Map ): Map { const fileHunks: Map = new Map(); - rawChanges.forEach((rawContent: RawContent, fileName: string) => { + diffContents.forEach((fileDiffContent: FileDiffContent, fileName: string) => { // if identical don't calculate the hunk and continue in the loop - if (rawContent.oldContent === rawContent.newContent) { + if (fileDiffContent.oldContent === fileDiffContent.newContent) { return; } - const hunks = generateHunks(rawContent, fileName); + const hunks = generateHunks(fileDiffContent, fileName); fileHunks.set(fileName, hunks); }); logger.info('Parsed ranges of old and new patch'); diff --git a/src/index.ts b/src/index.ts index c775e1f3..2f95ff1d 100644 --- a/src/index.ts +++ b/src/index.ts @@ -20,7 +20,7 @@ import { RepoDomain, BranchDomain, FileData, - RawContent, + FileDiffContent, CreateReviewCommentUserOptions, } from './types'; export {Changes} from './types'; @@ -38,51 +38,50 @@ import * as retry from 'async-retry'; * that they are in scope of the pull request. Outof scope suggestions are not made. * * In-scope suggestions are specifically: the suggestion for a file must correspond to a file in the remote pull request - * and the diff hunk computed for a raw file must produce a range that is a subset of the pull request's files hunks. + * and the diff hunk computed for a file's contents must produce a range that is a subset of the pull request's files hunks. * * If a file is too large to load in the review, it is skipped in the suggestion phase. * * If changes are empty then the workflow will not run. * Rethrows an HttpError if Octokit GitHub API returns an error. HttpError Octokit access_token and client_secret headers redact all sensitive information. * @param octokit The authenticated octokit instance, instantiated with an access token having permissiong to create a fork on the target repository. - * @param rawChanges A set of changes. The changes may be empty. + * @param diffContents A set of changes. The changes may be empty. * @param options The configuration for interacting with GitHub provided by the user. * @param loggerOption The logger instance (optional). + * @returns the created review's id number, or null if there are no changes to be made. */ export async function reviewPullRequest( octokit: Octokit, - rawChanges: Map, + diffContents: Map, options: CreateReviewCommentUserOptions, loggerOption?: Logger | LoggerOptions -): Promise { +): Promise { setupLogger(loggerOption); // if null undefined, or the empty map then no changes have been provided. // Do not execute GitHub workflow if ( - rawChanges === null || - rawChanges === undefined || - rawChanges.size === 0 + diffContents === null || + diffContents === undefined || + diffContents.size === 0 ) { logger.info( 'Empty changes provided. No suggestions to be made. Cancelling workflow.' ); - return; + return null; } const gitHubConfigs = addReviewCommentsDefaults(options); const remote: RepoDomain = { owner: gitHubConfigs.owner, repo: gitHubConfigs.repo, }; - logger.info( - `Successfully created a review on pull request: ${gitHubConfigs.pullNumber}.` - ); - handler.reviewPullRequest( + const reviewNumber = await handler.reviewPullRequest( octokit, remote, gitHubConfigs.pullNumber, gitHubConfigs.pageSize, - rawChanges + diffContents ); + return reviewNumber; } /** diff --git a/src/types/index.ts b/src/types/index.ts index 0956e57b..68ce4dba 100644 --- a/src/types/index.ts +++ b/src/types/index.ts @@ -157,7 +157,7 @@ export class PatchSyntaxError extends Error { /** * The file content of the original content and the patched content */ -export interface RawContent { +export interface FileDiffContent { readonly oldContent: string; readonly newContent: string; } @@ -178,7 +178,7 @@ export interface Hunk { } /** - * The range of a patch along with the raw file content + * The range of a patch along with the raw text file content */ export interface Patch extends Range { readonly newContent: string; diff --git a/test/helper-review-pull-request.ts b/test/helper-review-pull-request.ts index cccbe27b..6e62637b 100644 --- a/test/helper-review-pull-request.ts +++ b/test/helper-review-pull-request.ts @@ -15,7 +15,7 @@ import {assert, expect} from 'chai'; import {describe, it, before} from 'mocha'; import {octokit, setup} from './util'; -import {RepoDomain, RawContent, Patch, Hunk} from '../src/types'; +import {RepoDomain, FileDiffContent, Patch, Hunk} from '../src/types'; import {Octokit} from '@octokit/rest'; import * as proxyquire from 'proxyquire'; before(() => { @@ -23,8 +23,8 @@ before(() => { }); describe('reviewPullRequest', () => { - const rawChanges: Map = new Map(); - rawChanges.set('src/index.ts', { + const diffContents: Map = new Map(); + diffContents.set('src/index.ts', { newContent: 'hello world', oldContent: 'hello', }); @@ -66,11 +66,11 @@ describe('reviewPullRequest', () => { }, './raw-patch-handler': { getSuggestionPatches: ( - testRawChanges: Map, + testDiffContents: Map, testInvalidFiles: string[], testValidFileLines: Map ) => { - expect(testRawChanges).equals(rawChanges); + expect(testDiffContents).equals(diffContents); expect(testInvalidFiles).equals(invalidFiles); expect(testValidFileLines).equals(validFileLines); numMockedHelpersCalled += 1; @@ -100,7 +100,7 @@ describe('reviewPullRequest', () => { remote, pullNumber, pageSize, - rawChanges + diffContents ); expect(numMockedHelpersCalled).equals(3); }); @@ -154,7 +154,7 @@ describe('reviewPullRequest', () => { remote, pullNumber, pageSize, - rawChanges + diffContents ); expect(numMockedHelpersCalled).equals(2); }); @@ -189,7 +189,7 @@ describe('reviewPullRequest', () => { remote, pullNumber, pageSize, - rawChanges + diffContents ); assert.ok(false); } catch (err) { @@ -228,11 +228,11 @@ describe('reviewPullRequest', () => { }, './raw-patch-handler': { getSuggestionPatches: ( - testRawChanges: Map, + testDiffContents: Map, testInvalidFiles: string[], testValidFileLines: Map ) => { - expect(testRawChanges).equals(rawChanges); + expect(testDiffContents).equals(diffContents); expect(testInvalidFiles).equals(invalidFiles); expect(testValidFileLines).equals(validFileLines); numMockedHelpersCalled += 1; @@ -247,7 +247,7 @@ describe('reviewPullRequest', () => { remote, pullNumber, pageSize, - rawChanges + diffContents ); assert.ok(false); } catch (err) { @@ -288,11 +288,11 @@ describe('reviewPullRequest', () => { }, './raw-patch-handler': { getSuggestionPatches: ( - testRawChanges: Map, + testDiffContents: Map, testInvalidFiles: string[], testValidFileLines: Map ) => { - expect(testRawChanges).equals(rawChanges); + expect(testDiffContents).equals(diffContents); expect(testInvalidFiles).equals(invalidFiles); expect(testValidFileLines).equals(validFileLines); numMockedHelpersCalled += 1; @@ -324,7 +324,7 @@ describe('reviewPullRequest', () => { remote, pullNumber, pageSize, - rawChanges + diffContents ); assert.ok(false); } catch (err) { diff --git a/test/hunk-to-patch.ts b/test/hunk-to-patch.ts index fcd91846..3342f530 100644 --- a/test/hunk-to-patch.ts +++ b/test/hunk-to-patch.ts @@ -16,14 +16,14 @@ import {expect} from 'chai'; import {describe, it, before, beforeEach} from 'mocha'; import {setup} from './util'; import {generatePatches} from '../src/github-handler/comment-handler/raw-patch-handler/hunk-to-patch-handler'; -import {Hunk, RawContent} from '../src/types'; +import {Hunk, FileDiffContent} from '../src/types'; before(() => { setup(); }); describe('generatePatches', () => { - const rawChanges: Map = new Map(); + const diffContents: Map = new Map(); const filesHunks: Map = new Map(); const fileName1Addition = 'file-1.txt'; @@ -34,12 +34,12 @@ describe('generatePatches', () => { const fileNameNonEmtpy = 'file-6.txt'; beforeEach(() => { - rawChanges.clear(); + diffContents.clear(); filesHunks.clear(); }); it('Gets the correct substrings when there is 1 addition', () => { - rawChanges.set(fileName1Addition, { + diffContents.set(fileName1Addition, { oldContent: 'line1\nline2\nline3\nline4\nline5\nline6\nline7\nline8\nline9\nline10\n', newContent: @@ -48,7 +48,7 @@ describe('generatePatches', () => { filesHunks.set(fileName1Addition, [ {oldStart: 1, oldEnd: 6, newStart: 1, newEnd: 7}, ]); - const filePatches = generatePatches(filesHunks, rawChanges); + const filePatches = generatePatches(filesHunks, diffContents); expect(filePatches.get(fileName1Addition)!).deep.equals([ { start: 1, @@ -59,7 +59,7 @@ describe('generatePatches', () => { }); it('Gets the correct substrings when there is 2 additions', () => { - rawChanges.set(fileName2Addition, { + diffContents.set(fileName2Addition, { oldContent: 'line1\nline2\nline3\nline4\nline5\nline6\nline7\nline8\nline9\nline10\n', newContent: @@ -69,7 +69,7 @@ describe('generatePatches', () => { {oldStart: 1, oldEnd: 6, newStart: 1, newEnd: 7}, {oldStart: 9, oldEnd: 12, newStart: 10, newEnd: 13}, ]); - const filePatches = generatePatches(filesHunks, rawChanges); + const filePatches = generatePatches(filesHunks, diffContents); expect(filePatches.get(fileName2Addition)!).deep.equals([ { start: 1, @@ -85,7 +85,7 @@ describe('generatePatches', () => { }); it('Gets the correct substrings when there is 1 deletion', () => { - rawChanges.set(fileNameDelete, { + diffContents.set(fileNameDelete, { oldContent: 'line1\nline2\nline3\nline4\nline5\nline6\nline7\nline8\nline9\nline10\n', newContent: @@ -94,7 +94,7 @@ describe('generatePatches', () => { filesHunks.set(fileNameDelete, [ {oldStart: 9, oldEnd: 12, newStart: 9, newEnd: 11}, ]); - const filePatches = generatePatches(filesHunks, rawChanges); + const filePatches = generatePatches(filesHunks, diffContents); expect(filePatches.get(fileNameDelete)!).deep.equals([ { start: 9, @@ -105,7 +105,7 @@ describe('generatePatches', () => { }); it('Gets the correct substrings when there is a special patch char prepending the text', () => { - rawChanges.set(fileNameSpecialChar1, { + diffContents.set(fileNameSpecialChar1, { oldContent: 'line1\nline2\nline3\nline4\nline5\nline6\nline7\nline8\nline9\nline10\n', newContent: @@ -115,7 +115,7 @@ describe('generatePatches', () => { filesHunks.set(fileNameSpecialChar1, [ {oldStart: 1, oldEnd: 2, newStart: 1, newEnd: 2}, ]); - const filePatches = generatePatches(filesHunks, rawChanges); + const filePatches = generatePatches(filesHunks, diffContents); expect(filePatches.get(fileNameSpecialChar1)!).deep.equals([ { start: 1, @@ -126,7 +126,7 @@ describe('generatePatches', () => { }); it('Gets the correct substrings when the file is now an empty string', () => { - rawChanges.set(fileNameEmtpy, { + diffContents.set(fileNameEmtpy, { oldContent: 'line1', newContent: '', }); @@ -134,7 +134,7 @@ describe('generatePatches', () => { filesHunks.set(fileNameEmtpy, [ {oldStart: 1, oldEnd: 2, newStart: 1, newEnd: 1}, ]); - const filePatches = generatePatches(filesHunks, rawChanges); + const filePatches = generatePatches(filesHunks, diffContents); expect(filePatches.get(fileNameEmtpy)!).deep.equals([ { start: 1, @@ -145,7 +145,7 @@ describe('generatePatches', () => { }); it('Gets the correct substrings when the empty string file is now has text', () => { - rawChanges.set(fileNameNonEmtpy, { + diffContents.set(fileNameNonEmtpy, { oldContent: '', newContent: 'line1\nline2\nline3\nline4\nline5\nline6\nline7\nline8\nline9\nline10\n', @@ -154,7 +154,7 @@ describe('generatePatches', () => { filesHunks.set(fileNameNonEmtpy, [ {oldStart: 1, oldEnd: 1, newStart: 1, newEnd: 12}, ]); - const filePatches = generatePatches(filesHunks, rawChanges); + const filePatches = generatePatches(filesHunks, diffContents); expect(filePatches.get(fileNameNonEmtpy)!).deep.equals([ { start: 1, @@ -166,7 +166,7 @@ describe('generatePatches', () => { }); it('Throws an error when the new start hunk line is 0', () => { - rawChanges.set(fileNameNonEmtpy, { + diffContents.set(fileNameNonEmtpy, { oldContent: '', newContent: 'line1\nline2\nline3\nline4\nline5\nline6\nline7\nline8\nline9\nline10\n', @@ -175,7 +175,7 @@ describe('generatePatches', () => { {oldStart: 1, oldEnd: 1, newStart: 0, newEnd: 12}, ]); try { - generatePatches(filesHunks, rawChanges); + generatePatches(filesHunks, diffContents); expect.fail( 'Should have errored because the new start line is < 1. Value should be >= 1' ); @@ -184,7 +184,7 @@ describe('generatePatches', () => { } }); it('Throws an error when the new end hunk line is 0', () => { - rawChanges.set(fileNameNonEmtpy, { + diffContents.set(fileNameNonEmtpy, { oldContent: '', newContent: 'line1\nline2\nline3\nline4\nline5\nline6\nline7\nline8\nline9\nline10\n', @@ -193,7 +193,7 @@ describe('generatePatches', () => { {oldStart: 2, oldEnd: 1, newStart: 1, newEnd: 0}, ]); try { - generatePatches(filesHunks, rawChanges); + generatePatches(filesHunks, diffContents); expect.fail( 'Should have errored because the new end line is < 1. Value should be >= 1' ); @@ -202,7 +202,7 @@ describe('generatePatches', () => { } }); it('Throws an error when the old start hunk line is 0', () => { - rawChanges.set(fileNameNonEmtpy, { + diffContents.set(fileNameNonEmtpy, { oldContent: '', newContent: 'line1\nline2\nline3\nline4\nline5\nline6\nline7\nline8\nline9\nline10\n', @@ -211,7 +211,7 @@ describe('generatePatches', () => { {oldStart: 0, oldEnd: 1, newStart: 1, newEnd: 12}, ]); try { - generatePatches(filesHunks, rawChanges); + generatePatches(filesHunks, diffContents); expect.fail( 'Should have errored because the old start line is < 1. Value should be >= 1' ); @@ -220,7 +220,7 @@ describe('generatePatches', () => { } }); it('Throws an error when theold end hunk line is 0', () => { - rawChanges.set(fileNameNonEmtpy, { + diffContents.set(fileNameNonEmtpy, { oldContent: '', newContent: 'line1\nline2\nline3\nline4\nline5\nline6\nline7\nline8\nline9\nline10\n', @@ -229,7 +229,7 @@ describe('generatePatches', () => { {oldStart: 2, oldEnd: 0, newStart: 1, newEnd: 2}, ]); try { - generatePatches(filesHunks, rawChanges); + generatePatches(filesHunks, diffContents); expect.fail( 'Should have errored because the old end line is < 1. Value should be >= 1' ); diff --git a/test/inline-suggest.ts b/test/inline-suggest.ts index f3e0d052..046109d1 100644 --- a/test/inline-suggest.ts +++ b/test/inline-suggest.ts @@ -116,7 +116,7 @@ describe('makeInlineSuggestions', () => { }; const remote = {owner: 'upstream-owner', repo: 'upstream-repo'}; const pullNumber = 711; - it('Calls Octokit with the correct values', async () => { + it("Calls Octokit with the correct values and returns the successfully created review's number", async () => { suggestions.set(fileName1, [patch1]); const responseData = await import( './fixtures/get-pull-request-response.json' @@ -127,21 +127,75 @@ describe('makeInlineSuggestions', () => { url: 'http://fake-url.com', data: responseData, }; + const createReviewResponse = { + headers: {}, + status: 200, + url: 'http://fake-url.com', + data: { + id: 80, + node_id: 'MDE3OlB1bGxSZXF1ZXN0UmV2aWV3ODA=', + user: { + login: 'octocat', + id: 1, + node_id: 'MDQ6VXNlcjE=', + avatar_url: 'https://github.com/images/error/octocat_happy.gif', + gravatar_id: '', + url: 'https://api.github.com/users/octocat', + html_url: 'https://github.com/octocat', + followers_url: 'https://api.github.com/users/octocat/followers', + following_url: + 'https://api.github.com/users/octocat/following{/other_user}', + gists_url: 'https://api.github.com/users/octocat/gists{/gist_id}', + starred_url: + 'https://api.github.com/users/octocat/starred{/owner}{/repo}', + subscriptions_url: + 'https://api.github.com/users/octocat/subscriptions', + organizations_url: 'https://api.github.com/users/octocat/orgs', + repos_url: 'https://api.github.com/users/octocat/repos', + events_url: 'https://api.github.com/users/octocat/events{/privacy}', + received_events_url: + 'https://api.github.com/users/octocat/received_events', + type: 'User', + site_admin: false, + }, + body: + 'This is close to perfect! Please address the suggested inline change.', + state: 'CHANGES_REQUESTED', + html_url: + 'https://github.com/octocat/Hello-World/pull/12#pullrequestreview-80', + pull_request_url: + 'https://api.github.com/repos/octocat/Hello-World/pulls/12', + _links: { + html: { + href: + 'https://github.com/octocat/Hello-World/pull/12#pullrequestreview-80', + }, + pull_request: { + href: 'https://api.github.com/repos/octocat/Hello-World/pulls/12', + }, + }, + submitted_at: '2019-11-17T17:43:43Z', + commit_id: 'ecdd80bb57125d7ba9641ffaa4d7d2c19d3f3091', + }, + }; // setup const stubGetPulls = sandbox .stub(octokit.pulls, 'get') .resolves(getPullRequestResponse); - const stubCreateReview = sandbox.stub(octokit.pulls, 'createReview'); + const stubCreateReview = sandbox + .stub(octokit.pulls, 'createReview') + .resolves(createReviewResponse); // tests - await makeInlineSuggestions( + const reivewNumber = await makeInlineSuggestions( octokit, suggestions, outOfScopeSuggestions, remote, pullNumber ); + expect(reivewNumber).equals(80); sandbox.assert.calledOnceWithExactly(stubGetPulls, { owner: remote.owner, repo: remote.repo, @@ -197,6 +251,36 @@ describe('makeInlineSuggestions', () => { sandbox.assert.notCalled(stubCreateReview); }); + it('Returns null when a review is not made', async () => { + const responseData = await import( + './fixtures/get-pull-request-response.json' + ); + const getPullRequestResponse = { + headers: {}, + status: 200, + url: 'http://fake-url.com', + data: responseData, + }; + // setup + const stubGetPulls = sandbox + .stub(octokit.pulls, 'get') + .resolves(getPullRequestResponse); + + const stubCreateReview = sandbox.stub(octokit.pulls, 'createReview'); + // tests + + const reviewNumber = await makeInlineSuggestions( + octokit, + suggestions, + outOfScopeSuggestions, + remote, + pullNumber + ); + sandbox.assert.notCalled(stubGetPulls); + sandbox.assert.notCalled(stubCreateReview); + expect(reviewNumber).equals(null); + }); + it('Throws and does not continue when get pull request fails', async () => { // setup suggestions.set(fileName1, [patch1]); diff --git a/test/main-review-pull-request.ts b/test/main-review-pull-request.ts index 21767cd8..76c37556 100644 --- a/test/main-review-pull-request.ts +++ b/test/main-review-pull-request.ts @@ -15,7 +15,7 @@ import {assert, expect} from 'chai'; import {describe, it, before} from 'mocha'; import {octokit, setup} from './util'; -import {CreateReviewComment, RepoDomain, RawContent} from '../src/types'; +import {CreateReviewComment, RepoDomain, FileDiffContent} from '../src/types'; import {Octokit} from '@octokit/rest'; import * as proxyquire from 'proxyquire'; before(() => { @@ -26,8 +26,8 @@ before(() => { // tslint:disable:no-unused-expression // .true triggers ts-lint failure, but is valid chai describe('reviewPullRequest', () => { - const rawChanges: Map = new Map(); - rawChanges.set('src/index.ts', { + const diffContents: Map = new Map(); + diffContents.set('src/index.ts', { newContent: 'hello world', oldContent: 'hello', }); @@ -50,20 +50,20 @@ describe('reviewPullRequest', () => { remote: RepoDomain, testPullNumber: number, testPPageSize: number, - testChanges: Map + testDiffContents: Map ) => { expect(remote.owner).equals(owner); expect(remote.repo).equals(repo); expect(testPullNumber).equals(pullNumber); expect(testPPageSize).equals(pageSize); - expect(testChanges).equals(rawChanges); + expect(testDiffContents).equals(diffContents); numMockedHelpersCalled += 1; }, }; const stubReviewPr = proxyquire.noCallThru()('../src/', { './github-handler': stubHelperHandlers, }); - await stubReviewPr.reviewPullRequest(octokit, rawChanges, options); + await stubReviewPr.reviewPullRequest(octokit, diffContents, options); expect(numMockedHelpersCalled).equals(1); }); @@ -74,7 +74,7 @@ describe('reviewPullRequest', () => { remote: RepoDomain, pullNumber: number, pageSize: number, - testChanges: Map + testDiffContents: Map ) => { assert.isOk(false); }, @@ -92,7 +92,7 @@ describe('reviewPullRequest', () => { remote: RepoDomain, pullNumber: number, pageSize: number, - testChanges: Map + testDiffContents: Map ) => { assert.isOk(false); }, @@ -110,7 +110,7 @@ describe('reviewPullRequest', () => { remote: RepoDomain, pullNumber: number, pageSize: number, - testChanges: Map + testDiffContents: Map ) => { assert.isOk(false); }, @@ -130,7 +130,7 @@ describe('reviewPullRequest', () => { remote: RepoDomain, pullNumber: number, pageSize: number, - testChanges: Map + testDiffContents: Map ) => { throw Error('Review pull request helper failed'); }, @@ -139,7 +139,7 @@ describe('reviewPullRequest', () => { './github-handler': stubHelperHandlers, }); try { - await stubReviewPr.reviewPullRequest(octokit, rawChanges, options); + await stubReviewPr.reviewPullRequest(octokit, diffContents, options); expect.fail( 'The main function should have errored because the sub-function failed.' ); diff --git a/test/suggestion-hunk.ts b/test/suggestion-hunk.ts index 62048db4..a93611cb 100644 --- a/test/suggestion-hunk.ts +++ b/test/suggestion-hunk.ts @@ -19,23 +19,26 @@ import { generateHunks, getRawSuggestionHunks, } from '../src/github-handler/comment-handler/raw-patch-handler/raw-hunk-handler'; -import {RawContent} from '../src/types'; +import {FileDiffContent} from '../src/types'; before(() => { setup(); }); describe('generateHunks', () => { - const rawContent: RawContent = {oldContent: 'foo', newContent: 'FOO'}; + const fileDiffContent: FileDiffContent = { + oldContent: 'foo', + newContent: 'FOO', + }; const fileName = 'README.md'; - it('Does not update the user raw change input', () => { - generateHunks(rawContent, fileName); - expect(rawContent.oldContent).equals('foo'); - expect(rawContent.newContent).equals('FOO'); + it("Does not update the user's input of text file diffs contents", () => { + generateHunks(fileDiffContent, fileName); + expect(fileDiffContent.oldContent).equals('foo'); + expect(fileDiffContent.newContent).equals('FOO'); }); it('Generates the hunks that are produced by the diff library given one file', () => { - const hunk = generateHunks(rawContent, fileName); + const hunk = generateHunks(fileDiffContent, fileName); expect(hunk.length).equals(1); expect(hunk[0].oldStart).equals(1); expect(hunk[0].oldEnd).equals(2); @@ -44,7 +47,7 @@ describe('generateHunks', () => { }); it('Generates the hunks that are produced by the diff library given one file which was empty but now has content', () => { - const addingContentToEmpty: RawContent = { + const addingContentToEmpty: FileDiffContent = { oldContent: '', newContent: 'FOO', }; @@ -58,9 +61,12 @@ describe('generateHunks', () => { }); describe('getRawSuggestionHunks', () => { - const rawChange: Map = new Map(); - const rawContent1: RawContent = {oldContent: 'foo', newContent: 'FOO'}; - const rawContent2: RawContent = { + const diffContents: Map = new Map(); + const fileDiffContent1: FileDiffContent = { + oldContent: 'foo', + newContent: 'FOO', + }; + const fileDiffContent2: FileDiffContent = { oldContent: 'bar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar', newContent: @@ -68,31 +74,31 @@ describe('getRawSuggestionHunks', () => { }; const fileName1 = 'README.md'; const fileName2 = 'bars.txt'; - rawChange.set(fileName1, rawContent1); - rawChange.set(fileName2, rawContent2); + diffContents.set(fileName1, fileDiffContent1); + diffContents.set(fileName2, fileDiffContent2); - it('Does not update the user raw change input', () => { - getRawSuggestionHunks(rawChange); - expect(rawContent1.oldContent).equals('foo'); - expect(rawContent1.newContent).equals('FOO'); - expect(rawChange.get(fileName1)!.oldContent).equals('foo'); - expect(rawChange.get(fileName1)!.newContent).equals('FOO'); - expect(rawContent2.oldContent).equals( + it("Does not update the user's input of text file diff contents", () => { + getRawSuggestionHunks(diffContents); + expect(fileDiffContent1.oldContent).equals('foo'); + expect(fileDiffContent1.newContent).equals('FOO'); + expect(diffContents.get(fileName1)!.oldContent).equals('foo'); + expect(diffContents.get(fileName1)!.newContent).equals('FOO'); + expect(fileDiffContent2.oldContent).equals( 'bar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar' ); - expect(rawContent2.newContent).equals( + expect(fileDiffContent2.newContent).equals( 'foo\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nfoo' ); - expect(rawChange.get(fileName2)!.oldContent).equals( + expect(diffContents.get(fileName2)!.oldContent).equals( 'bar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar' ); - expect(rawChange.get(fileName2)!.newContent).equals( + expect(diffContents.get(fileName2)!.newContent).equals( 'foo\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nfoo' ); }); it('Generates the hunks that are produced by the diff library for all files that are updated', () => { - const fileHunks = getRawSuggestionHunks(rawChange); + const fileHunks = getRawSuggestionHunks(diffContents); expect(fileHunks.size).equals(2); expect(fileHunks.get(fileName1)!.length).equals(1); expect(fileHunks.get(fileName1)![0].oldStart).equals(1); @@ -111,13 +117,13 @@ describe('getRawSuggestionHunks', () => { }); it('Does not generate hunks for changes that contain no updates', () => { - const sameRawChanges = new Map(); - sameRawChanges.set('unchanged-1.txt', {oldContent: '', newContent: ''}); - sameRawChanges.set('unchanged-2.txt', { + const samediffContents = new Map(); + samediffContents.set('unchanged-1.txt', {oldContent: '', newContent: ''}); + samediffContents.set('unchanged-2.txt', { oldContent: 'same', newContent: 'same', }); - const fileHunks = getRawSuggestionHunks(sameRawChanges); + const fileHunks = getRawSuggestionHunks(samediffContents); expect(fileHunks.size).equals(0); }); }); diff --git a/test/suggestion-patch.ts b/test/suggestion-patch.ts index 0b33b2fe..341b06cc 100644 --- a/test/suggestion-patch.ts +++ b/test/suggestion-patch.ts @@ -16,7 +16,7 @@ import {expect} from 'chai'; import {describe, it, before, beforeEach} from 'mocha'; import {setup} from './util'; import {getValidSuggestionHunks} from '../src/github-handler/comment-handler/raw-patch-handler/in-scope-hunks-handler'; -import {Range, RawContent} from '../src/types'; +import {Range, FileDiffContent} from '../src/types'; before(() => { setup(); @@ -30,21 +30,21 @@ describe('getValidSuggestionHunks', () => { const invalidFiles = [invalidFile]; scope.set(fileName1, [{start: 1, end: 2}]); scope.set(fileName2, [{start: 2, end: 11}]); - const rawChanges: Map = new Map(); + const diffContents: Map = new Map(); beforeEach(() => { - rawChanges.clear(); + diffContents.clear(); }); it('Finds file hunks that are in scope for the pull request', () => { - rawChanges.set(fileName2, { + diffContents.set(fileName2, { oldContent: 'bar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar', newContent: 'bar\nbar\nbar\nbar\nbar\nbar\nfoo\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar', }); const suggestions = getValidSuggestionHunks( - rawChanges, + diffContents, invalidFiles, scope ); @@ -52,14 +52,14 @@ describe('getValidSuggestionHunks', () => { }); it('Excludes file hunks that are not in scope for the pull request', () => { - rawChanges.set(fileName1, { + diffContents.set(fileName1, { oldContent: 'bar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nfoo', newContent: 'bar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nfoo', }); const suggestions = getValidSuggestionHunks( - rawChanges, + diffContents, invalidFiles, scope ); @@ -67,12 +67,12 @@ describe('getValidSuggestionHunks', () => { }); it('Excludes suggestion files that are not in scope because the file is not in scope', () => { - rawChanges.set('non-existant-file-that-is-not-invalid.txt', { + diffContents.set('non-existant-file-that-is-not-invalid.txt', { oldContent: 'foo', newContent: 'bar', }); const suggestions = getValidSuggestionHunks( - rawChanges, + diffContents, invalidFiles, scope ); @@ -80,9 +80,9 @@ describe('getValidSuggestionHunks', () => { }); it('Excludes suggestion files that are not in scope because the file is invalid', () => { - rawChanges.set(invalidFile, {oldContent: 'foo', newContent: 'bar'}); + diffContents.set(invalidFile, {oldContent: 'foo', newContent: 'bar'}); const suggestions = getValidSuggestionHunks( - rawChanges, + diffContents, invalidFiles, scope ); @@ -90,9 +90,9 @@ describe('getValidSuggestionHunks', () => { }); it('Does not include suggestion files that have no change', () => { - rawChanges.set(fileName1, {oldContent: 'foo', newContent: 'foo'}); + diffContents.set(fileName1, {oldContent: 'foo', newContent: 'foo'}); const suggestions = getValidSuggestionHunks( - rawChanges, + diffContents, invalidFiles, scope ); @@ -101,38 +101,38 @@ describe('getValidSuggestionHunks', () => { it('Calculates in scope and out of scope files that are mutually exclusive', () => { // in scope hunk - rawChanges.set(fileName2, { + diffContents.set(fileName2, { oldContent: 'bar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar', newContent: 'bar\nbar\nbar\nbar\nbar\nbar\nfoo\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar', }); // out of scope hunks - rawChanges.set(fileName1, { + diffContents.set(fileName1, { oldContent: 'bar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nfoo', newContent: 'bar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar', }); // same before and after - rawChanges.set('same-before-and-after.text', { + diffContents.set('same-before-and-after.text', { oldContent: 'bar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nfoo', newContent: 'bar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nbar\nfoo', }); // out of scope file name - rawChanges.set('non-existant-file-that-is-not-invalid.txt', { + diffContents.set('non-existant-file-that-is-not-invalid.txt', { oldContent: 'foo', newContent: 'bar', }); // out of scope file name - rawChanges.set(invalidFile, { + diffContents.set(invalidFile, { oldContent: 'foo', newContent: 'bar', }); const suggestions = getValidSuggestionHunks( - rawChanges, + diffContents, invalidFiles, scope );