diff --git a/index.js b/index.js index 878457b..397633e 100644 --- a/index.js +++ b/index.js @@ -5,10 +5,10 @@ const getDCOStatus = require("./lib/dco.js"); const hasSignoffTrailer = require("./lib/hasSignoffTrailer.js"); const requireMembers = require("./lib/requireMembers.js"); -// GitHub documents a 250-commit cap on the pull request commits REST endpoint. +// The pull request commits REST endpoint serves at most 250 commits, the +// newest ones. GraphQL's pullRequest.commits has the same cap while still +// reporting the true totalCount, so neither can list a larger pull request. const PULL_REQUEST_COMMITS_REST_LIMIT = 250; -const PULL_REQUEST_COMMITS_GRAPHQL_PAGE_SIZE = 100; -const PULL_REQUEST_COMMITS_GRAPHQL_PAGE_MARGIN = 1; // The compare endpoint returns at most 250 commits per page. const COMPARE_COMMITS_PAGE_SIZE = 100; @@ -169,7 +169,11 @@ module.exports = (app) => { // `GitHub `, committer login `web-flow`) and would be // silently downgraded from identity matching to presence only. async function inspectMergeGroup(context, pr, mergeGroup) { - const rangeCommits = await listMergeGroupRangeCommits(context, mergeGroup); + const rangeCommits = await listCompareCommits( + context, + mergeGroup.base_sha, + mergeGroup.head_sha + ); const commits = await listPullRequestCommits(context, pr); if (commits.length === 0) { @@ -185,7 +189,7 @@ module.exports = (app) => { const prShas = new Set(commits.map((commit) => commit.sha)); const synthesizedFailed = []; - for (const commit of rangeCommits) { + for (const commit of rangeCommits.values()) { // A genuine pull request commit: evaluated with full identity matching. if (prShas.has(commit.sha)) continue; // The merge strategy's two-parent head, authored by whoever enqueued the @@ -216,16 +220,16 @@ module.exports = (app) => { return { commits, synthesizedFailed }; } - // The compare endpoint caps its `commits` array, so under the merge strategy - // a large pull request truncates the range. Every commit in the range has to - // be seen for the "no synthesized commit lost its trailer" invariant to hold, - // so page until the whole range is collected. + // The compare endpoint caps its `commits` array, so a large range is + // truncated: under the merge strategy for a merge group, past 250 commits + // for a pull request. Every commit in the range has to be seen for a verdict + // to hold, so page until the whole range is collected. // // octokit's paginate() cannot be used here: this endpoint returns an object // rather than an array, and normalizePaginatedListResponse mangles it (it // mistakes `status: "ahead"` for the namespaced list key and throws). Page // manually instead. - async function listMergeGroupRangeCommits(context, mergeGroup) { + async function listCompareCommits(context, base, head) { // Keyed on sha: dedupes and preserves insertion order in one structure. // Counting raw entries instead would let a compare endpoint that ignored // the `page` parameter satisfy the completeness check with duplicates of @@ -238,8 +242,8 @@ module.exports = (app) => { for (;;) { const response = await context.octokit.rest.repos.compareCommits( context.repo({ - base: mergeGroup.base_sha, - head: mergeGroup.head_sha, + base, + head, per_page: COMPARE_COMMITS_PAGE_SIZE, page, }) @@ -254,7 +258,7 @@ module.exports = (app) => { if (commits.size >= totalCommits) break; // An iteration that adds no new commit would otherwise loop forever. // This covers both an empty page and a page repeated verbatim; the - // shortfall check below then fails the group closed. + // shortfall check below then fails the range closed. if (commits.size === seenBefore) break; page += 1; @@ -262,212 +266,50 @@ module.exports = (app) => { if (commits.size < totalCommits) { const error = new Error( - `GitHub returned ${commits.size} of ${totalCommits} merge group commits.` + `GitHub returned ${commits.size} of ${totalCommits} commits comparing ${base}...${head}.` ); - error.truncatedMergeGroupRange = true; + error.truncatedCompareRange = true; throw error; } - return [...commits.values()]; + return commits; } async function listPullRequestCommits(context, pr) { - const commits = await context.octokit.paginate( + const newest = await context.octokit.paginate( context.octokit.rest.pulls.listCommits, context.repo({ pull_number: pr.number, per_page: 100 }) ); - if (commits.length !== PULL_REQUEST_COMMITS_REST_LIMIT) { - return commits; + if (newest.length !== PULL_REQUEST_COMMITS_REST_LIMIT) { + return newest; } - return fetchCompletePullRequestCommits(context, pr); - } - - async function fetchCompletePullRequestCommits(context, pr) { + // At the cap the list may be cut short, so take the full range from the + // compare endpoint instead. Both shas resolve in the base repository even + // for a pull request from a fork. The range is `git log base..head`, so a + // base.sha behind the base branch can only add commits to it, never hide + // one: the verdict may be too strict, not too lenient. try { - const commits = []; - const commitShas = new Set(); - let cursor = null; - let hasNextPage = true; - let expectedCommitCount; - let maxPages; - let pageCount = 0; - - while (hasNextPage) { - const response = await context.octokit.graphql( - `query PullRequestCommits( - $owner: String! - $repo: String! - $pullNumber: Int! - $cursor: String - $pageSize: Int! - ) { - repository(owner: $owner, name: $repo) { - pullRequest(number: $pullNumber) { - commits(first: $pageSize, after: $cursor) { - totalCount - nodes { - commit { - oid - url - message - author { - name - email - date - user { - login - __typename - } - } - committer { - name - email - date - user { - login - __typename - } - } - signature { - isValid - state - signature - payload - } - parents(first: 100) { - nodes { - oid - url - } - } - } - } - pageInfo { - hasNextPage - endCursor - } - } - } - } - }`, - context.repo({ - pullNumber: pr.number, - cursor, - pageSize: PULL_REQUEST_COMMITS_GRAPHQL_PAGE_SIZE, - }) - ); - const connection = response.repository.pullRequest.commits; - pageCount += 1; - if (connection.totalCount < PULL_REQUEST_COMMITS_REST_LIMIT) { - throw new Error( - `GraphQL reported only ${connection.totalCount} pull request commits.` - ); - } - if (expectedCommitCount === undefined) { - expectedCommitCount = connection.totalCount; - maxPages = - Math.ceil( - expectedCommitCount / PULL_REQUEST_COMMITS_GRAPHQL_PAGE_SIZE - ) + PULL_REQUEST_COMMITS_GRAPHQL_PAGE_MARGIN; - } else if (connection.totalCount !== expectedCommitCount) { - throw new Error( - `GraphQL totalCount changed from ${expectedCommitCount} to ${connection.totalCount}.` - ); - } - - for (const node of connection.nodes) { - const commit = mapGraphQLCommit(node); - if (commitShas.has(commit.sha)) { - throw new Error( - `GraphQL returned duplicate pull request commit ${commit.sha}.` - ); - } - commitShas.add(commit.sha); - commits.push(commit); - } - - hasNextPage = connection.pageInfo.hasNextPage; - const previousCursor = cursor; - cursor = connection.pageInfo.endCursor; - - if (hasNextPage && !cursor) { - throw new Error("GraphQL pagination ended without a cursor."); - } - if (hasNextPage && cursor === previousCursor) { - throw new Error("GraphQL pagination cursor did not advance."); - } - if (hasNextPage && pageCount >= maxPages) { - throw new Error("GraphQL commit pagination exceeded expected pages."); - } - } - - if (commitShas.size !== expectedCommitCount) { + const commits = await listCompareCommits( + context, + pr.base.sha, + pr.head.sha + ); + // Guards against a complete range that is not this pull request's. + const missing = newest.find((commit) => !commits.has(commit.sha)); + if (missing) { throw new Error( - `GraphQL returned ${commitShas.size} of ${expectedCommitCount} pull request commits.` + `Comparing ${pr.base.sha}...${pr.head.sha} did not return pull request commit ${missing.sha}.` ); } - - return commits; + return [...commits.values()]; } catch (error) { error.incompleteCommitList = true; throw error; } } - function mapGraphQLCommit(node) { - const commit = node.commit; - const signature = commit.signature || { - isValid: false, - state: "unsigned", - signature: null, - payload: null, - }; - - return { - sha: commit.oid, - html_url: commit.url, - author: mapGraphQLUser(commit.author.user), - committer: mapGraphQLUser(commit.committer.user), - parents: commit.parents.nodes.map((parent) => ({ - sha: parent.oid, - html_url: parent.url, - })), - commit: { - author: { - name: commit.author.name, - email: commit.author.email, - date: commit.author.date, - }, - committer: { - name: commit.committer.name, - email: commit.committer.email, - date: commit.committer.date, - }, - message: commit.message, - verification: { - verified: signature.isValid, - reason: signature.state, - signature: signature.signature, - payload: signature.payload, - verified_at: null, - }, - }, - }; - } - - function mapGraphQLUser(user) { - if (!user) { - return null; - } - - return { - login: user.login, - type: user.login.endsWith("[bot]") ? "Bot" : user.__typename, - }; - } - async function reportDCO( context, { sha, ref, timeStart, conclusion, summary, actions } @@ -504,27 +346,27 @@ module.exports = (app) => { The sign-off certification is verified against the pull request's own commits, so without them there is no verdict to report. Please retry by re-running the check or removing and re-adding the pull request to the merge queue.`; - } - if (error.truncatedMergeGroupRange) { - // Internally detected: every compare request can have succeeded, so - // there is no meaningful HTTP status or request ID to report here. - return `The DCO check could not be evaluated because the complete merge group commit range could not be retrieved. - -Every commit in the range has to be examined before the merge group can be certified, so a partial range gives no verdict. - -Please retry by re-running the check, or by removing and re-adding the pull request to the merge queue.`; } if (error.incompleteCommitList) { return `The DCO check could not be evaluated because the complete pull request commit list could not be retrieved. -GitHub returned 250 commits from the REST API, so the app attempted to retrieve the full list through GraphQL before evaluating the pull request. That fallback did not complete, so the DCO verdict is unknown. +GitHub lists at most 250 commits for a pull request, so the app attempted to retrieve the full range by comparing the pull request's base and head. That did not complete, so the DCO verdict is unknown. -HTTP status: ${error.status || "unknown"} +${error.status ? "" : `Reason: ${error.message}\n\n`}HTTP status: ${error.status || "unknown"} GitHub request ID: ${requestId} Please retry by re-running the check or pushing a new commit.`; } + if (error.truncatedCompareRange) { + // Internally detected: every compare request can have succeeded, so + // there is no meaningful HTTP status or request ID to report here. + return `The DCO check could not be evaluated because the complete merge group commit range could not be retrieved. + +Every commit in the range has to be examined before the merge group can be certified, so a partial range gives no verdict. + +Please retry by re-running the check, or by removing and re-adding the pull request to the merge queue.`; + } return `The DCO check could not be evaluated because an error occurred while gathering or evaluating commits. HTTP status: ${error.status || "unknown"} diff --git a/test/index.test.js b/test/index.test.js index 0989a4b..48f4657 100644 --- a/test/index.test.js +++ b/test/index.test.js @@ -28,7 +28,6 @@ const compareSuccessCommits = compareSuccess.commits.map((commit, index) => ? { ...commit, sha: payloadSuccess.pull_request.head.sha } : commit ); -const belowRestLimitGraphQLTotalCounts = [249, 0]; function makeCommit(source, index) { return { @@ -37,62 +36,6 @@ function makeCommit(source, index) { }; } -function graphQLNode(restCommit, options = {}) { - const authorUser = - options.authorUser === undefined - ? { - login: restCommit.author.login, - __typename: restCommit.author.type, - } - : options.authorUser; - const committerUser = - options.committerUser === undefined - ? { - login: restCommit.committer.login, - __typename: restCommit.committer.type, - } - : options.committerUser; - - return { - commit: { - oid: restCommit.sha, - url: restCommit.html_url, - message: restCommit.commit.message, - author: { - ...restCommit.commit.author, - user: authorUser, - }, - committer: { - ...restCommit.commit.committer, - user: committerUser, - }, - signature: options.signature || null, - parents: { - nodes: restCommit.parents.map((parent) => ({ - oid: parent.sha, - url: parent.html_url, - })), - }, - }, - }; -} - -function graphQLCommits(commits, pageInfo, options = {}) { - return { - repository: { - pullRequest: { - commits: { - totalCount: options.totalCount ?? commits.length, - nodes: commits.map((commit, index) => - graphQLNode(commit, options[index]) - ), - pageInfo, - }, - }, - }, - }; -} - function expectCommitStatusNotCalled(path) { let called = false; const scope = nock("https://api.github.com") @@ -134,6 +77,8 @@ const MERGE_GROUP_HEAD_SHA = "abc123def456abc123def456abc123def456abc1"; const MERGE_GROUP_BRANCH = "gh-readonly-queue/master/pr-113-607c64cd8e37eb2db939f99a17bee5c7d1a90a31"; const MERGE_GROUP_COMPARE_PATH = `/repos/robotland/test/compare/607c64cd8e37eb2db939f99a17bee5c7d1a90a31...${MERGE_GROUP_HEAD_SHA}`; +const PULL_REQUEST_COMPARE_PATH = + "/repos/robotland/test/compare/607c64cd8e37eb2db939f99a17bee5c7d1a90a31...e76ed6025cec8879c75454a6efd6081d46de4c94"; const COMPARE_PAGE_1 = { per_page: "100", page: "1" }; const COMPARE_PAGE_2 = { per_page: "100", page: "2" }; @@ -246,620 +191,21 @@ describe("dco", () => { }); describe("pull_request event", () => { - test("creates a failing check", async () => { - const mock = nock("https://api.github.com") - .get("/repos/robotland/test/contents/.github%2Fdco.yml") - .reply(404) - .get("/repos/robotland/.github/contents/.github%2Fdco.yml") - .reply(404) - - .get("/repos/robotland/test/pulls/113/commits") - .query({ per_page: "100" }) - .reply(200, compare.commits) - - .post("/repos/robotland/test/check-runs", (body) => { - body.started_at = "2018-07-14T18:18:54.156Z"; - body.completed_at = "2018-07-14T18:18:54.156Z"; - expect(body).toMatchSnapshot(); - return true; - }) - .reply(200); - - await probot.receive({ name: "pull_request", payload }); - - expect(mock.activeMocks()).toStrictEqual([]); - }); - - test("creates a passing check", async () => { - const mock = nock("https://api.github.com") - // no config - .get("/repos/octocat/Hello-World/contents/.github%2Fdco.yml") - .reply(404) - .get("/repos/octocat/.github/contents/.github%2Fdco.yml") - .reply(404) - - .get("/repos/octocat/Hello-World/pulls/1/commits") - .query({ per_page: "100" }) - .reply(200, compareSuccessCommits) - - .post("/repos/octocat/Hello-World/check-runs", (body) => { - body.started_at = "2018-07-14T18:18:54.156Z"; - body.completed_at = "2018-07-14T18:18:54.156Z"; - expect(body).toMatchSnapshot(); - - return true; - }) - .reply(200); - - await probot.receive({ name: "pull_request", payload: payloadSuccess }); - - expect(mock.activeMocks()).toStrictEqual([]); - }); - - test("rethrows when passing check creation returns 403", async () => { - const assertCommitStatusNotCalled = expectCommitStatusNotCalled( - "/repos/octocat/Hello-World/statuses/34c5c7793cb3b279e22454cb6750c80560547b3a" - ); - const mock = nock("https://api.github.com") - .get("/repos/octocat/Hello-World/contents/.github%2Fdco.yml") - .reply(404) - .get("/repos/octocat/.github/contents/.github%2Fdco.yml") - .reply(404) - - .get("/repos/octocat/Hello-World/pulls/1/commits") - .query({ per_page: "100" }) - .reply(200, compareSuccessCommits) - - .post("/repos/octocat/Hello-World/check-runs") - .reply(403); - - await expect( - probot.receive({ name: "pull_request", payload: payloadSuccess }) - ).rejects.toMatchObject({ - errors: [expect.objectContaining({ status: 403 })], - }); - - expect(mock.activeMocks()).toStrictEqual([]); - assertCommitStatusNotCalled(); - }); - - test("creates a failing check when commits cannot be listed", async () => { - const mock = nock("https://api.github.com") - .get("/repos/robotland/test/contents/.github%2Fdco.yml") - .reply(404) - .get("/repos/robotland/.github/contents/.github%2Fdco.yml") - .reply(404) - - .get("/repos/robotland/test/pulls/113/commits") - .query({ per_page: "100" }) - .reply( - 422, - { - message: - "Server Error: Sorry, this diff is taking too long to generate.", - errors: [ - { - resource: "Comparison", - field: "diff", - code: "not_available", - }, - ], - }, - { - "x-github-request-id": "ABC1:DEF2:123456:789ABC", - } - ) - - .post("/repos/robotland/test/check-runs", (body) => { - body.started_at = "2018-07-14T18:18:54.156Z"; - body.completed_at = "2018-07-14T18:18:54.156Z"; - expect(body).toMatchObject({ - conclusion: "failure", - head_branch: "dco-test", - head_sha: "e76ed6025cec8879c75454a6efd6081d46de4c94", - name: "DCO", - output: { - title: "DCO", - }, - status: "completed", - }); - expect(body.output.summary).toContain( - "The DCO check could not be evaluated" - ); - expect(body.output.summary).toContain("HTTP status: 422"); - expect(body.output.summary).toContain( - "GitHub request ID: ABC1:DEF2:123456:789ABC" - ); - expect(body.output.summary).not.toContain("Server Error"); - expect(body.output.summary).toContain("re-running the check"); - return true; - }) - .reply(200); - - await probot.receive({ name: "pull_request", payload }); - - expect(mock.activeMocks()).toStrictEqual([]); - }); - - test("creates a failing check when config cannot be fetched", async () => { - const mock = nock("https://api.github.com") - .get("/repos/robotland/test/contents/.github%2Fdco.yml") - .reply(422, { message: "Invalid request" }) - - .post("/repos/robotland/test/check-runs", (body) => { - body.started_at = "2018-07-14T18:18:54.156Z"; - body.completed_at = "2018-07-14T18:18:54.156Z"; - expect(body).toMatchObject({ - conclusion: "failure", - head_branch: "dco-test", - head_sha: "e76ed6025cec8879c75454a6efd6081d46de4c94", - name: "DCO", - output: { - title: "DCO", - }, - status: "completed", - }); - expect(body.output.summary).toContain("HTTP status: 422"); - expect(body.output.summary).toContain( - "GitHub request ID: unavailable" - ); - return true; - }) - .reply(200); - - await probot.receive({ name: "pull_request", payload }); - - expect(mock.activeMocks()).toStrictEqual([]); - }); - - test("escalates exactly 250 REST commits to GraphQL", async () => { - const restCommits = Array.from({ length: 250 }, (_, index) => - makeCommit(compareSuccess.commits[0], index) - ); - const mock = nock("https://api.github.com") - .get("/repos/robotland/test/contents/.github%2Fdco.yml") - .reply(404) - .get("/repos/robotland/.github/contents/.github%2Fdco.yml") - .reply(404) - - .get("/repos/robotland/test/pulls/113/commits") - .query({ per_page: "100" }) - .reply(200, restCommits) - - .post("/graphql", (body) => { - expect(body.variables).toMatchObject({ - owner: "robotland", - repo: "test", - pullNumber: 113, - cursor: null, - }); - return true; - }) - .reply(200, { - data: graphQLCommits( - restCommits, - { - hasNextPage: false, - endCursor: null, - }, - { - 0: { - authorUser: null, - committerUser: null, - signature: { - isValid: true, - state: "VALID", - signature: "signature", - payload: "payload", - }, - }, - } - ), - }) - - .post("/repos/robotland/test/check-runs", (body) => { - body.started_at = "2018-07-14T18:18:54.156Z"; - body.completed_at = "2018-07-14T18:18:54.156Z"; - expect(body).toMatchObject({ - conclusion: "success", - head_branch: "dco-test", - head_sha: "e76ed6025cec8879c75454a6efd6081d46de4c94", - name: "DCO", - output: { - title: "DCO", - }, - status: "completed", - }); - expect(body.output.summary).toContain("All commits are signed off!"); - return true; - }) - .reply(200); - - await probot.receive({ name: "pull_request", payload }); - - expect(mock.activeMocks()).toStrictEqual([]); - }); - - test("fails when only GraphQL sees a missing sign-off", async () => { - const restCommits = Array.from({ length: 250 }, (_, index) => - makeCommit(compareSuccess.commits[0], index) - ); - const failingCommit = makeCommit(compare.commits[0], 250); - const commits = [...restCommits, failingCommit]; - const mock = nock("https://api.github.com") - .get("/repos/robotland/test/contents/.github%2Fdco.yml") - .reply(404) - .get("/repos/robotland/.github/contents/.github%2Fdco.yml") - .reply(404) - - .get("/repos/robotland/test/pulls/113/commits") - .query({ per_page: "100" }) - .reply(200, restCommits) - - .post("/graphql") - .reply(200, { - data: graphQLCommits(commits, { - hasNextPage: false, - endCursor: null, - }), - }) - - .post("/repos/robotland/test/check-runs", (body) => { - body.started_at = "2018-07-14T18:18:54.156Z"; - body.completed_at = "2018-07-14T18:18:54.156Z"; - expect(body).toMatchObject({ - conclusion: "action_required", - head_branch: "dco-test", - head_sha: "e76ed6025cec8879c75454a6efd6081d46de4c94", - name: "DCO", - output: { - title: "DCO", - }, - status: "completed", - }); - expect(body.output.summary).toContain("The sign-off is missing."); - expect(body.output.summary).toContain(failingCommit.sha); - return true; - }) - .reply(200); - - await probot.receive({ name: "pull_request", payload }); - - expect(mock.activeMocks()).toStrictEqual([]); - }); - - test("preserves bot author handling from GraphQL commits", async () => { - const restCommits = Array.from({ length: 250 }, (_, index) => - makeCommit(compareSuccess.commits[0], index) - ); - const botCommit = makeCommit(compare.commits[0], 250); - const commits = [...restCommits, botCommit]; - const mock = nock("https://api.github.com") - .get("/repos/robotland/test/contents/.github%2Fdco.yml") - .reply(404) - .get("/repos/robotland/.github/contents/.github%2Fdco.yml") - .reply(404) - - .get("/repos/robotland/test/pulls/113/commits") - .query({ per_page: "100" }) - .reply(200, restCommits) - - .post("/graphql") - .reply(200, { - data: graphQLCommits( - commits, - { - hasNextPage: false, - endCursor: null, - }, - { - 250: { - authorUser: { - login: "dependabot[bot]", - __typename: "User", - }, - }, - } - ), - }) - - .post("/repos/robotland/test/check-runs", (body) => { - body.started_at = "2018-07-14T18:18:54.156Z"; - body.completed_at = "2018-07-14T18:18:54.156Z"; - expect(body).toMatchObject({ - conclusion: "success", - head_branch: "dco-test", - head_sha: "e76ed6025cec8879c75454a6efd6081d46de4c94", - name: "DCO", - output: { - title: "DCO", - }, - status: "completed", - }); - expect(body.output.summary).toContain("All commits are signed off!"); - return true; - }) - .reply(200); - - await probot.receive({ name: "pull_request", payload }); - - expect(mock.activeMocks()).toStrictEqual([]); - }); - - test("paginates GraphQL commits after REST truncation", async () => { - const restCommits = Array.from({ length: 250 }, (_, index) => - makeCommit(compareSuccess.commits[0], index) - ); - const commits = Array.from({ length: 251 }, (_, index) => - makeCommit(compareSuccess.commits[0], index) - ); - const mock = nock("https://api.github.com") - .get("/repos/robotland/test/contents/.github%2Fdco.yml") - .reply(404) - .get("/repos/robotland/.github/contents/.github%2Fdco.yml") - .reply(404) - - .get("/repos/robotland/test/pulls/113/commits") - .query({ per_page: "100" }) - .reply(200, restCommits) - - .post("/graphql", (body) => { - expect(body.variables.cursor).toBeNull(); - return true; - }) - .reply(200, { - data: graphQLCommits( - commits.slice(0, 100), - { - hasNextPage: true, - endCursor: "cursor-100", - }, - { totalCount: commits.length } - ), - }) - - .post("/graphql", (body) => { - expect(body.variables.cursor).toBe("cursor-100"); - return true; - }) - .reply(200, { - data: graphQLCommits( - commits.slice(100), - { - hasNextPage: false, - endCursor: null, - }, - { totalCount: commits.length } - ), - }) - - .post("/repos/robotland/test/check-runs", (body) => { - body.started_at = "2018-07-14T18:18:54.156Z"; - body.completed_at = "2018-07-14T18:18:54.156Z"; - expect(body).toMatchObject({ - conclusion: "success", - head_branch: "dco-test", - head_sha: "e76ed6025cec8879c75454a6efd6081d46de4c94", - name: "DCO", - output: { - title: "DCO", - }, - status: "completed", - }); - expect(body.output.summary).toContain("All commits are signed off!"); - return true; - }) - .reply(200); - - await probot.receive({ name: "pull_request", payload }); - - expect(mock.activeMocks()).toStrictEqual([]); - }); - - test("fails closed when GraphQL commit fetching fails", async () => { - const restCommits = Array.from({ length: 250 }, (_, index) => - makeCommit(compareSuccess.commits[0], index) - ); - const mock = nock("https://api.github.com") - .get("/repos/robotland/test/contents/.github%2Fdco.yml") - .reply(404) - .get("/repos/robotland/.github/contents/.github%2Fdco.yml") - .reply(404) - - .get("/repos/robotland/test/pulls/113/commits") - .query({ per_page: "100" }) - .reply(200, restCommits) - - .post("/graphql") - .reply(200, { - errors: [{ message: "GraphQL unavailable" }], - }) - - .post("/repos/robotland/test/check-runs", (body) => { - body.started_at = "2018-07-14T18:18:54.156Z"; - body.completed_at = "2018-07-14T18:18:54.156Z"; - expect(body).toMatchObject({ - conclusion: "failure", - head_branch: "dco-test", - head_sha: "e76ed6025cec8879c75454a6efd6081d46de4c94", - name: "DCO", - output: { - title: "DCO", - }, - status: "completed", - }); - expect(body.output.summary).toContain( - "complete pull request commit list could not be retrieved" - ); - expect(body.output.summary).toContain("verdict is unknown"); - expect(body.output.summary).toContain("HTTP status:"); - expect(body.output.summary).toContain( - "GitHub request ID: unavailable" - ); - expect(body.output.summary).not.toContain("All commits are signed"); - return true; - }) - .reply(200); - - await probot.receive({ name: "pull_request", payload }); - - expect(mock.activeMocks()).toStrictEqual([]); - }); - - test("fails closed when GraphQL returns partial data with errors", async () => { - const restCommits = Array.from({ length: 250 }, (_, index) => - makeCommit(compareSuccess.commits[0], index) - ); - const mock = nock("https://api.github.com") - .get("/repos/robotland/test/contents/.github%2Fdco.yml") - .reply(404) - .get("/repos/robotland/.github/contents/.github%2Fdco.yml") - .reply(404) - - .get("/repos/robotland/test/pulls/113/commits") - .query({ per_page: "100" }) - .reply(200, restCommits) - - .post("/graphql") - .reply(200, { - data: graphQLCommits(restCommits, { - hasNextPage: false, - endCursor: null, - }), - errors: [{ message: "GraphQL returned partial data" }], - }) - - .post("/repos/robotland/test/check-runs", (body) => { - expectIncompleteCommitListCheck(body); - expect(body.output.summary).toContain("HTTP status:"); - expect(body.output.summary).toContain( - "GitHub request ID: unavailable" - ); - return true; - }) - .reply(200); - - await probot.receive({ name: "pull_request", payload }); - - expect(mock.activeMocks()).toStrictEqual([]); - }); - - test("fails closed when GraphQL pagination is incomplete", async () => { - const restCommits = Array.from({ length: 250 }, (_, index) => - makeCommit(compareSuccess.commits[0], index) - ); - const mock = nock("https://api.github.com") - .get("/repos/robotland/test/contents/.github%2Fdco.yml") - .reply(404) - .get("/repos/robotland/.github/contents/.github%2Fdco.yml") - .reply(404) - - .get("/repos/robotland/test/pulls/113/commits") - .query({ per_page: "100" }) - .reply(200, restCommits) - - .post("/graphql") - .reply(200, { - data: graphQLCommits( - restCommits.slice(0, 100), - { - hasNextPage: true, - endCursor: null, - }, - { totalCount: 250 } - ), - }) - - .post("/repos/robotland/test/check-runs", (body) => { - body.started_at = "2018-07-14T18:18:54.156Z"; - body.completed_at = "2018-07-14T18:18:54.156Z"; - expect(body).toMatchObject({ - conclusion: "failure", - head_branch: "dco-test", - head_sha: "e76ed6025cec8879c75454a6efd6081d46de4c94", - name: "DCO", - output: { - title: "DCO", - }, - status: "completed", - }); - expect(body.output.summary).toContain( - "complete pull request commit list could not be retrieved" - ); - expect(body.output.summary).toContain("verdict is unknown"); - return true; - }) - .reply(200); - - await probot.receive({ name: "pull_request", payload }); - - expect(mock.activeMocks()).toStrictEqual([]); - }); - - test("fails closed when GraphQL pagination exceeds expected pages", async () => { - const restCommits = Array.from({ length: 250 }, (_, index) => - makeCommit(compareSuccess.commits[0], index) - ); - const mock = nock("https://api.github.com") - .get("/repos/robotland/test/contents/.github%2Fdco.yml") - .reply(404) - .get("/repos/robotland/.github/contents/.github%2Fdco.yml") - .reply(404) - - .get("/repos/robotland/test/pulls/113/commits") - .query({ per_page: "100" }) - .reply(200, restCommits) - - .post("/graphql") - .reply(200, { - data: graphQLCommits( - [], - { - hasNextPage: true, - endCursor: "cursor-60", - }, - { totalCount: 250 } - ), - }) - - .post("/graphql") - .reply(200, { - data: graphQLCommits( - [], - { - hasNextPage: true, - endCursor: "cursor-120", - }, - { totalCount: 250 } - ), - }) - - .post("/graphql") - .reply(200, { - data: graphQLCommits( - [], - { - hasNextPage: true, - endCursor: "cursor-180", - }, - { totalCount: 250 } - ), - }) + test("creates a failing check", async () => { + const mock = nock("https://api.github.com") + .get("/repos/robotland/test/contents/.github%2Fdco.yml") + .reply(404) + .get("/repos/robotland/.github/contents/.github%2Fdco.yml") + .reply(404) - .post("/graphql") - .reply(200, { - data: graphQLCommits( - [], - { - hasNextPage: true, - endCursor: "cursor-240", - }, - { totalCount: 250 } - ), - }) + .get("/repos/robotland/test/pulls/113/commits") + .query({ per_page: "100" }) + .reply(200, compare.commits) .post("/repos/robotland/test/check-runs", (body) => { - expectIncompleteCommitListCheck(body); + body.started_at = "2018-07-14T18:18:54.156Z"; + body.completed_at = "2018-07-14T18:18:54.156Z"; + expect(body).toMatchSnapshot(); return true; }) .reply(200); @@ -867,101 +213,62 @@ describe("dco", () => { await probot.receive({ name: "pull_request", payload }); expect(mock.activeMocks()).toStrictEqual([]); - }, 10_000); + }); - for (const totalCount of belowRestLimitGraphQLTotalCounts) { - test(`fails closed when GraphQL totalCount is ${totalCount}`, async () => { - const restCommits = Array.from({ length: 250 }, (_, index) => - makeCommit(compareSuccess.commits[0], index) - ); - const graphQLCommitsPage = restCommits.slice(0, totalCount); - const mock = nock("https://api.github.com") - .get("/repos/robotland/test/contents/.github%2Fdco.yml") - .reply(404) - .get("/repos/robotland/.github/contents/.github%2Fdco.yml") - .reply(404) + test("creates a passing check", async () => { + const mock = nock("https://api.github.com") + // no config + .get("/repos/octocat/Hello-World/contents/.github%2Fdco.yml") + .reply(404) + .get("/repos/octocat/.github/contents/.github%2Fdco.yml") + .reply(404) - .get("/repos/robotland/test/pulls/113/commits") - .query({ per_page: "100" }) - .reply(200, restCommits) + .get("/repos/octocat/Hello-World/pulls/1/commits") + .query({ per_page: "100" }) + .reply(200, compareSuccessCommits) - .post("/graphql") - .reply(200, { - data: graphQLCommits( - graphQLCommitsPage, - { - hasNextPage: false, - endCursor: null, - }, - { totalCount } - ), - }) + .post("/repos/octocat/Hello-World/check-runs", (body) => { + body.started_at = "2018-07-14T18:18:54.156Z"; + body.completed_at = "2018-07-14T18:18:54.156Z"; + expect(body).toMatchSnapshot(); - .post("/repos/robotland/test/check-runs", (body) => { - expectIncompleteCommitListCheck(body); - return true; - }) - .reply(200); + return true; + }) + .reply(200); - await probot.receive({ name: "pull_request", payload }); + await probot.receive({ name: "pull_request", payload: payloadSuccess }); - expect(mock.activeMocks()).toStrictEqual([]); - }); - } + expect(mock.activeMocks()).toStrictEqual([]); + }); - test("fails closed when GraphQL totalCount changes between pages", async () => { - const restCommits = Array.from({ length: 250 }, (_, index) => - makeCommit(compareSuccess.commits[0], index) + test("rethrows when passing check creation returns 403", async () => { + const assertCommitStatusNotCalled = expectCommitStatusNotCalled( + "/repos/octocat/Hello-World/statuses/34c5c7793cb3b279e22454cb6750c80560547b3a" ); const mock = nock("https://api.github.com") - .get("/repos/robotland/test/contents/.github%2Fdco.yml") + .get("/repos/octocat/Hello-World/contents/.github%2Fdco.yml") .reply(404) - .get("/repos/robotland/.github/contents/.github%2Fdco.yml") + .get("/repos/octocat/.github/contents/.github%2Fdco.yml") .reply(404) - .get("/repos/robotland/test/pulls/113/commits") + .get("/repos/octocat/Hello-World/pulls/1/commits") .query({ per_page: "100" }) - .reply(200, restCommits) - - .post("/graphql") - .reply(200, { - data: graphQLCommits( - restCommits.slice(0, 100), - { - hasNextPage: true, - endCursor: "cursor-100", - }, - { totalCount: 250 } - ), - }) - - .post("/graphql") - .reply(200, { - data: graphQLCommits( - restCommits.slice(100, 200), - { - hasNextPage: false, - endCursor: null, - }, - { totalCount: 251 } - ), - }) + .reply(200, compareSuccessCommits) - .post("/repos/robotland/test/check-runs", (body) => { - expectIncompleteCommitListCheck(body); - return true; - }) - .reply(200); + .post("/repos/octocat/Hello-World/check-runs") + .reply(403); - await probot.receive({ name: "pull_request", payload }); + await expect( + probot.receive({ name: "pull_request", payload: payloadSuccess }) + ).rejects.toMatchObject({ + errors: [expect.objectContaining({ status: 403 })], + }); expect(mock.activeMocks()).toStrictEqual([]); + assertCommitStatusNotCalled(); }); - test("fails closed when GraphQL returns too few commits", async () => { - const restCommits = Array.from({ length: 250 }, (_, index) => - makeCommit(compareSuccess.commits[0], index) - ); + test("creates a failing check when commits cannot be listed", async () => { const mock = nock("https://api.github.com") .get("/repos/robotland/test/contents/.github%2Fdco.yml") .reply(404) @@ -970,19 +277,23 @@ describe("dco", () => { .get("/repos/robotland/test/pulls/113/commits") .query({ per_page: "100" }) - .reply(200, restCommits) - - .post("/graphql") - .reply(200, { - data: graphQLCommits( - restCommits.slice(0, 100), - { - hasNextPage: false, - endCursor: null, - }, - { totalCount: 251 } - ), - }) + .reply( + 422, + { + message: + "Server Error: Sorry, this diff is taking too long to generate.", + errors: [ + { + resource: "Comparison", + field: "diff", + code: "not_available", + }, + ], + }, + { + "x-github-request-id": "ABC1:DEF2:123456:789ABC", + } + ) .post("/repos/robotland/test/check-runs", (body) => { body.started_at = "2018-07-14T18:18:54.156Z"; @@ -998,9 +309,14 @@ describe("dco", () => { status: "completed", }); expect(body.output.summary).toContain( - "complete pull request commit list could not be retrieved" + "The DCO check could not be evaluated" ); - expect(body.output.summary).toContain("verdict is unknown"); + expect(body.output.summary).toContain("HTTP status: 422"); + expect(body.output.summary).toContain( + "GitHub request ID: ABC1:DEF2:123456:789ABC" + ); + expect(body.output.summary).not.toContain("Server Error"); + expect(body.output.summary).toContain("re-running the check"); return true; }) .reply(200); @@ -1010,46 +326,28 @@ describe("dco", () => { expect(mock.activeMocks()).toStrictEqual([]); }); - test("fails closed when GraphQL cursor does not advance", async () => { - const restCommits = Array.from({ length: 250 }, (_, index) => - makeCommit(compareSuccess.commits[0], index) - ); + test("creates a failing check when config cannot be fetched", async () => { const mock = nock("https://api.github.com") .get("/repos/robotland/test/contents/.github%2Fdco.yml") - .reply(404) - .get("/repos/robotland/.github/contents/.github%2Fdco.yml") - .reply(404) - - .get("/repos/robotland/test/pulls/113/commits") - .query({ per_page: "100" }) - .reply(200, restCommits) - - .post("/graphql") - .reply(200, { - data: graphQLCommits( - restCommits.slice(0, 100), - { - hasNextPage: true, - endCursor: "cursor-100", - }, - { totalCount: 250 } - ), - }) - - .post("/graphql") - .reply(200, { - data: graphQLCommits( - restCommits.slice(100, 200), - { - hasNextPage: true, - endCursor: "cursor-100", - }, - { totalCount: 250 } - ), - }) + .reply(422, { message: "Invalid request" }) .post("/repos/robotland/test/check-runs", (body) => { - expectIncompleteCommitListCheck(body); + body.started_at = "2018-07-14T18:18:54.156Z"; + body.completed_at = "2018-07-14T18:18:54.156Z"; + expect(body).toMatchObject({ + conclusion: "failure", + head_branch: "dco-test", + head_sha: "e76ed6025cec8879c75454a6efd6081d46de4c94", + name: "DCO", + output: { + title: "DCO", + }, + status: "completed", + }); + expect(body.output.summary).toContain("HTTP status: 422"); + expect(body.output.summary).toContain( + "GitHub request ID: unavailable" + ); return true; }) .reply(200); @@ -1059,53 +357,162 @@ describe("dco", () => { expect(mock.activeMocks()).toStrictEqual([]); }); - test("fails closed when GraphQL returns duplicate commits", async () => { - const restCommits = Array.from({ length: 250 }, (_, index) => + // pulls.listCommits serves only the newest 250 commits of a pull request, + // so a 340-commit pull request is modelled as REST returning the last 250 + // and the compare endpoint paging through all 340. + describe("with more than 250 commits", () => { + const allCommits = Array.from({ length: 340 }, (_, index) => makeCommit(compareSuccess.commits[0], index) ); - const mock = nock("https://api.github.com") - .get("/repos/robotland/test/contents/.github%2Fdco.yml") - .reply(404) - .get("/repos/robotland/.github/contents/.github%2Fdco.yml") - .reply(404) + const restCommits = allCommits.slice(-250); - .get("/repos/robotland/test/pulls/113/commits") - .query({ per_page: "100" }) - .reply(200, restCommits) + function largePullRequestMock(restPage = restCommits) { + return nock("https://api.github.com") + .get("/repos/robotland/test/contents/.github%2Fdco.yml") + .reply(404) + .get("/repos/robotland/.github/contents/.github%2Fdco.yml") + .reply(404) - .post("/graphql") - .reply(200, { - data: graphQLCommits( - restCommits.slice(0, 100), - { - hasNextPage: true, - endCursor: "cursor-100", - }, - { totalCount: 250 } - ), - }) + .get("/repos/robotland/test/pulls/113/commits") + .query({ per_page: "100" }) + .reply(200, restPage); + } - .post("/graphql") - .reply(200, { - data: graphQLCommits( - restCommits.slice(0, 100), - { - hasNextPage: false, - endCursor: null, - }, - { totalCount: 250 } - ), - }) + function comparePages(mock, commits, total = commits.length) { + for (let page = 1; (page - 1) * 100 < commits.length; page += 1) { + mock + .get(PULL_REQUEST_COMPARE_PATH) + .query({ per_page: "100", page: String(page) }) + .reply(200, { + ...compareSuccess, + total_commits: total, + commits: commits.slice((page - 1) * 100, page * 100), + }); + } + return mock; + } - .post("/repos/robotland/test/check-runs", (body) => { - expectIncompleteCommitListCheck(body); - return true; - }) - .reply(200); + function expectCheck(mock, conclusion, extra = () => {}) { + return mock + .post("/repos/robotland/test/check-runs", (body) => { + normalizeTimes(body); + expect(body).toMatchObject({ + conclusion, + head_branch: "dco-test", + head_sha: "e76ed6025cec8879c75454a6efd6081d46de4c94", + name: "DCO", + status: "completed", + }); + extra(body); + return true; + }) + .reply(200); + } - await probot.receive({ name: "pull_request", payload }); + test("pages the compare endpoint for the complete commit list", async () => { + const mock = expectCheck( + comparePages(largePullRequestMock(), allCommits), + "success", + (body) => + expect(body.output.summary).toContain("All commits are signed off!") + ); - expect(mock.activeMocks()).toStrictEqual([]); + await probot.receive({ name: "pull_request", payload }); + + expect(mock.activeMocks()).toStrictEqual([]); + }); + + test("uses the compare endpoint at exactly 250 commits", async () => { + const mock = expectCheck( + comparePages(largePullRequestMock(), restCommits), + "success" + ); + + await probot.receive({ name: "pull_request", payload }); + + expect(mock.activeMocks()).toStrictEqual([]); + }); + + test("fails a missing sign-off older than the newest 250 commits", async () => { + const failingCommit = makeCommit(compare.commits[0], 1000); + const mock = expectCheck( + comparePages(largePullRequestMock(), [ + failingCommit, + ...allCommits.slice(1), + ]), + "action_required", + (body) => { + expect(body.output.summary).toContain("The sign-off is missing."); + expect(body.output.summary).toContain(failingCommit.sha); + expect(body.output.summary).toContain("git rebase HEAD~340"); + } + ); + + await probot.receive({ name: "pull_request", payload }); + + expect(mock.activeMocks()).toStrictEqual([]); + }); + + test("fails closed when the compare request fails", async () => { + const mock = expectCheck( + largePullRequestMock() + .get(PULL_REQUEST_COMPARE_PATH) + .query(COMPARE_PAGE_1) + .reply( + 404, + { message: "Not Found" }, + { + "x-github-request-id": "REQ-1", + } + ), + "failure", + (body) => { + expectIncompleteCommitListCheck(body); + expect(body.output.summary).toContain("HTTP status: 404"); + expect(body.output.summary).toContain("GitHub request ID: REQ-1"); + } + ); + + await probot.receive({ name: "pull_request", payload }); + + expect(mock.activeMocks()).toStrictEqual([]); + }); + + test("fails closed when the compare range stays truncated", async () => { + // total_commits promises 340 but every page past the third is empty. + const mock = expectCheck( + comparePages(largePullRequestMock(), allCommits.slice(0, 300), 340) + .get(PULL_REQUEST_COMPARE_PATH) + .query({ per_page: "100", page: "4" }) + .reply(200, { ...compareSuccess, total_commits: 340, commits: [] }), + "failure", + (body) => { + expectIncompleteCommitListCheck(body); + expect(body.output.summary).toContain("300 of 340"); + } + ); + + await probot.receive({ name: "pull_request", payload }); + + expect(mock.activeMocks()).toStrictEqual([]); + }); + + test("fails closed when the compare range omits a pull request commit", async () => { + // A complete range that is not this pull request's: the commits REST + // did return must all be in it. + const mock = expectCheck( + comparePages(largePullRequestMock(), allCommits.slice(0, 300)), + "failure", + (body) => { + expectIncompleteCommitListCheck(body); + expect(body.output.summary).toContain(allCommits[300].sha); + } + ); + + await probot.receive({ name: "pull_request", payload }); + + expect(mock.activeMocks()).toStrictEqual([]); + }); }); test("creates a neutral check when no commits are returned", async () => { @@ -2645,7 +2052,7 @@ allowRemediationCommits: expect(mock.activeMocks()).toStrictEqual([]); }); - test("F10: fails closed when the GraphQL completeness path fails", async () => { + test("F10: fails closed when the pull request compare range is truncated", async () => { const restCommits = Array.from({ length: 250 }, (_, index) => makeCommit(mergeQueuePrCommits[0], index) ); @@ -2667,14 +2074,18 @@ allowRemediationCommits: .query({ per_page: "100" }) .reply(200, restCommits) - .post("/graphql") + // A truncated compare range for the pull request itself must read as + // an incomplete pull request commit list, not as a merge group fault. + .get(PULL_REQUEST_COMPARE_PATH) + .query(COMPARE_PAGE_1) .reply(200, { - data: graphQLCommits(restCommits, { - hasNextPage: false, - endCursor: null, - }), - errors: [{ message: "GraphQL returned partial data" }], + ...compareSuccess, + total_commits: 251, + commits: restCommits, }) + .get(PULL_REQUEST_COMPARE_PATH) + .query(COMPARE_PAGE_2) + .reply(200, { ...compareSuccess, total_commits: 251, commits: [] }) .post("/repos/robotland/test/check-runs", (body) => { normalizeTimes(body);