diff --git a/lib/pr_data.js b/lib/pr_data.js index 3dda6ebd..bb6d4ee7 100644 --- a/lib/pr_data.js +++ b/lib/pr_data.js @@ -60,11 +60,30 @@ export default class PRData { this.getThreadData(), this.getCommits() ]).then(() => { + this.mergeHeadCommitChecks(); this.cli.stopSpinner(`Done loading data for ${prStr}`); }); this.analyzeReviewers(); } + mergeHeadCommitChecks() { + const headCommit = this.pr.headCommit?.nodes?.[0]?.commit; + delete this.pr.headCommit; + + const lastCommit = this.commits.at(-1)?.commit; + if (headCommit === undefined && lastCommit === undefined) { + return; + } + + if (headCommit?.oid !== lastCommit?.oid) { + throw new Error( + 'Unable to match pull request head commit while loading data'); + } + + lastCommit.checkSuites = headCommit.checkSuites; + lastCommit.status = headCommit.status; + } + analyzeReviewers() { this.reviewers = new ReviewAnalyzer(this).getReviewers(); } diff --git a/lib/queries/PR.gql b/lib/queries/PR.gql index 19c86a3a..824e5c5a 100644 --- a/lib/queries/PR.gql +++ b/lib/queries/PR.gql @@ -23,6 +23,35 @@ query PR($prid: Int!, $owner: String!, $repo: String!) { path } }, + # CI data is only consumed for the pull request's head commit. Querying + # it for every commit multiplies the GraphQL rate-limit cost. + headCommit: commits(last: 1) { + nodes { + commit { + oid + checkSuites(first: 100) { + nodes { + app { + slug + } + conclusion, + status, + checkRuns(first: 40) { + nodes { + name + status + conclusion + detailsUrl + } + } + } + } + status { + state + } + } + } + }, title, baseRefName, headRefName, diff --git a/lib/queries/PRCommits.gql b/lib/queries/PRCommits.gql index 70cf2e4e..b8567e37 100644 --- a/lib/queries/PRCommits.gql +++ b/lib/queries/PRCommits.gql @@ -25,26 +25,6 @@ query Commits($prid: Int!, $owner: String!, $repo: String!, $after: String) { message messageHeadline authoredByCommitter - checkSuites(first: 100) { - nodes { - app { - slug - } - conclusion, - status, - checkRuns(first: 40) { - nodes { - name - status - conclusion - detailsUrl - } - } - } - } - status { - state - } } } } diff --git a/test/unit/graphql_queries.test.js b/test/unit/graphql_queries.test.js new file mode 100644 index 00000000..9b8d750e --- /dev/null +++ b/test/unit/graphql_queries.test.js @@ -0,0 +1,26 @@ +import { readFileSync } from 'node:fs'; +import { describe, it } from 'node:test'; +import assert from 'node:assert'; + +const prQuery = readFileSync( + new URL('../../lib/queries/PR.gql', import.meta.url), 'utf8'); +const commitsQuery = readFileSync( + new URL('../../lib/queries/PRCommits.gql', import.meta.url), 'utf8'); + +describe('GraphQL queries', () => { + it('requests CI details only for the pull request head commit', () => { + const headCommitStart = prQuery.indexOf( + 'headCommit: commits(last: 1)'); + const headCommitEnd = prQuery.indexOf('\n title,', headCommitStart); + const headCommitQuery = prQuery.slice(headCommitStart, headCommitEnd); + + assert.notStrictEqual(headCommitStart, -1); + assert.notStrictEqual(headCommitEnd, -1); + assert.match(headCommitQuery, /checkSuites\(first: 100\)/); + assert.match(headCommitQuery, /checkRuns\(first: 40\)/); + assert.match(headCommitQuery, /status \{\s+state\s+\}/); + assert.doesNotMatch(commitsQuery, /checkSuites/); + assert.doesNotMatch(commitsQuery, /checkRuns/); + assert.doesNotMatch(commitsQuery, /\sstatus\s*\{/); + }); +}); diff --git a/test/unit/pr_data.test.js b/test/unit/pr_data.test.js index 86f7b31d..7123845a 100644 --- a/test/unit/pr_data.test.js +++ b/test/unit/pr_data.test.js @@ -23,6 +23,24 @@ function toRaw(obj) { const rawPR = toRaw({ repository: { pullRequest: firstTimerPR } }); +const headCommitChecks = { + oid: oddCommits.at(-1).commit.oid, + checkSuites: { + nodes: [{ + app: { slug: 'github-actions' }, + status: 'COMPLETED', + conclusion: 'SUCCESS', + checkRuns: { nodes: [] } + }] + }, + status: { state: 'SUCCESS' } +}; +rawPR.repository.pullRequest.headCommit = { + nodes: [{ commit: toRaw(headCommitChecks) }] +}; + +const commitsWithHeadChecks = toRaw(oddCommits); +Object.assign(commitsWithHeadChecks.at(-1).commit, headCommitChecks); describe('PRData', function() { const request = { @@ -34,7 +52,8 @@ describe('PRData', function() { .withArgs('https://raw.githubusercontent.com/nodejs/node/HEAD/README.md') .returns(Promise.resolve(readme)); request.text.returns(new Error('unknown query')); - request.gql.withArgs('PR').returns(Promise.resolve(rawPR)); + request.gql.withArgs('PR').callsFake( + () => Promise.resolve(toRaw(rawPR))); request.gql.withArgs('Reviews').returns( Promise.resolve(toRaw(approvingReviews))); request.gql.withArgs('PRComments').returns( @@ -53,7 +72,45 @@ describe('PRData', function() { assert.deepStrictEqual(data.pr, firstTimerPR, 'pr'); assert.deepStrictEqual(data.reviews, approvingReviews, 'reviews'); assert.deepStrictEqual(data.comments, commentsWithLGTM, 'comments'); - assert.deepStrictEqual(data.commits, oddCommits, 'commits'); + assert.deepStrictEqual( + data.commits, commitsWithHeadChecks, 'commits'); assert.deepStrictEqual(data.reviewers, allGreenReviewers, 'reviewers'); }); + + describe('mergeHeadCommitChecks', () => { + it('allows an empty commit list', () => { + const data = new PRData(argv, new TestCLI(), request); + data.pr = { headCommit: { nodes: [] } }; + data.commits = []; + + data.mergeHeadCommitChecks(); + + assert.deepStrictEqual(data.pr, {}); + assert.deepStrictEqual(data.commits, []); + }); + + it('rejects missing head commit data', () => { + const data = new PRData(argv, new TestCLI(), request); + data.pr = {}; + data.commits = toRaw(oddCommits); + + assert.throws( + () => data.mergeHeadCommitChecks(), + /Unable to match pull request head commit/); + }); + + it('rejects a mismatched head commit', () => { + const data = new PRData(argv, new TestCLI(), request); + data.pr = { + headCommit: { + nodes: [{ commit: { ...headCommitChecks, oid: 'different' } }] + } + }; + data.commits = toRaw(oddCommits); + + assert.throws( + () => data.mergeHeadCommitChecks(), + /Unable to match pull request head commit/); + }); + }); });