Skip to content

Commit 9c7f488

Browse files
aduh95aduh95
committed
fix(git-node): do not rely on commit date
...to display commits pushed since last approving review. It would only matter if time on the comitter machine is very different from GitHub's own time. That allows us to skip quering for `timelineItems`.
1 parent 4d07c52 commit 9c7f488

9 files changed

+28
-59
lines changed

‎lib/pr_checker.js

+13-25
Original file line numberDiff line numberDiff line change
@@ -530,6 +530,11 @@ export default class PRChecker {
530530
} = this;
531531
const { maxCommits } = argv;
532532

533+
if (commits.length === 0) {
534+
cli.warn('No commits found');
535+
return false;
536+
}
537+
533538
const reviewIndex = reviews.findLastIndex(
534539
review => review.authorCanPushToRepository && review.state === 'APPROVED'
535540
);
@@ -539,38 +544,21 @@ export default class PRChecker {
539544
return false;
540545
}
541546

542-
const reviewDate = reviews[reviewIndex].publishedAt;
543-
544-
const afterCommits = [];
545-
commits.forEach((commit) => {
546-
commit = commit.commit;
547-
if (commit.committedDate > reviewDate) {
548-
afterCommits.push(commit);
549-
}
550-
});
551-
552-
const totalCommits = afterCommits.length;
553-
if (totalCommits === 0 && this.pr.timelineItems.updatedAt > reviewDate) {
554-
// Some commits were pushed, but all the commits have a commit date prior
555-
// to the last review. It means that either that a long time elapsed
556-
// between the commit and the push, or that the clock on the dev machine
557-
// is wrong, or the commit date was forged.
558-
cli.warn('Something was pushed to the Pull Request branch since the last approving review.');
559-
return false;
560-
}
547+
const reviewedCommitIndex = commits
548+
.findLastIndex(({ commit }) => commit.oid === reviews[reviewIndex].commit.oid);
561549

562-
if (totalCommits > 0) {
550+
if (reviewedCommitIndex !== commits.length - 1) {
563551
cli.warn('Commits were pushed since the last approving review:');
564-
const sliceLength = maxCommits === 0 ? totalCommits : -maxCommits;
565-
afterCommits.slice(sliceLength)
566-
.forEach(commit => {
552+
commits.slice(Math.max(reviewedCommitIndex + 1, commits.length - maxCommits))
553+
.forEach(({ commit }) => {
567554
cli.warn(`- ${commit.messageHeadline}`);
568555
});
569556

557+
const totalCommits = commits.length - reviewedCommitIndex - 1;
570558
if (totalCommits > maxCommits) {
571559
const infoMsg = '...(use `' +
572-
`--max-commits ${totalCommits}` +
573-
'` to see the full list of commits)';
560+
`--max-commits ${totalCommits}` +
561+
'` to see the full list of commits)';
574562
cli.warn(infoMsg);
575563
}
576564

‎lib/queries/PR.gql

-3
Original file line numberDiff line numberDiff line change
@@ -23,9 +23,6 @@ query PR($prid: Int!, $owner: String!, $repo: String!) {
2323
path
2424
}
2525
},
26-
timelineItems(itemTypes: [HEAD_REF_FORCE_PUSHED_EVENT, PULL_REQUEST_COMMIT]) {
27-
updatedAt
28-
},
2926
title,
3027
baseRefName,
3128
headRefName,

‎test/fixtures/more_than_three_commits_after_review_commits.json

+3-3
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@
2121
},{
2222
"commit": {
2323
"committedDate": "2017-09-25T11:27:02Z",
24-
"oid": "f230e691459f8b0f448e1eec1b83fbf7f708eba3",
24+
"oid": "f230e691459f8b0f448e1eec1b83fbf7f708eba4",
2525
"messageHeadline": "src: add requested feature",
2626
"message": "src: new functionality\n works really great",
2727
"author": {
@@ -31,7 +31,7 @@
3131
},{
3232
"commit": {
3333
"committedDate": "2017-10-25T12:42:02Z",
34-
"oid": "9416475a6dc1b27c3e343dcbc07674a439c88db1",
34+
"oid": "9416475a6dc1b27c3e343dcbc07674a439c88db2",
3535
"messageHeadline": "nit: edit mistakes",
3636
"message": "nit: fix mistakes\n fixed requested errors",
3737
"author": {
@@ -41,7 +41,7 @@
4141
},{
4242
"commit": {
4343
"committedDate": "2017-10-25T12:42:02Z",
44-
"oid": "9416475a6dc1b27c3e343dcbc07674a439c88db1",
44+
"oid": "9416475a6dc1b27c3e343dcbc07674a439c88db3",
4545
"messageHeadline": "final: we should be good to go",
4646
"message": "final: we should be good to go\n fixed requested errors",
4747
"author": {

‎test/fixtures/more_than_three_commits_after_review_reviews.json

+1
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
"author": {
55
"login": "foo"
66
},
7+
"commit": {"oid": "f230e691459f8b0f448e1eec1b83fbf7f708eba3"},
78
"authorCanPushToRepository": true,
89
"url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-89923489",
910
"publishedAt": "2017-07-23T11:19:25Z"

‎test/fixtures/multiple_commits_after_review_reviews.json

+1
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
"author": {
55
"login": "foo"
66
},
7+
"commit": {"oid": "deadbeef"},
78
"authorCanPushToRepository": true,
89
"url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-89923489",
910
"publishedAt": "2017-09-23T11:19:25Z"

‎test/fixtures/reviews_approved.json

+7
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
"author": {
55
"login": "foo"
66
},
7+
"commit": {"oid": "deadbeef"},
78
"authorCanPushToRepository": true,
89
"url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71480624",
910
"publishedAt": "2017-10-24T11:19:00Z"
@@ -14,6 +15,7 @@
1415
"author": {
1516
"login": "Baz"
1617
},
18+
"commit": {"oid": "deadbeef"},
1719
"authorCanPushToRepository": true,
1820
"url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71488392",
1921
"publishedAt": "2017-10-24T11:50:52Z"
@@ -24,6 +26,7 @@
2426
"author": {
2527
"login": "Baz"
2628
},
29+
"commit": {"oid": "deadbeef"},
2730
"authorCanPushToRepository": true,
2831
"url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-714882992",
2932
"publishedAt": "2017-10-24T12:30:52Z"
@@ -34,6 +37,7 @@
3437
"author": {
3538
"login": "Quux"
3639
},
40+
"commit": {"oid": "deadbeef"},
3741
"authorCanPushToRepository": true,
3842
"url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71817236",
3943
"publishedAt": "2017-10-24T14:49:01Z"
@@ -44,6 +48,7 @@
4448
"author": {
4549
"login": "Baz"
4650
},
51+
"commit": {"oid": "deadbeef"},
4752
"authorCanPushToRepository": true,
4853
"url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71488236",
4954
"publishedAt": "2017-10-24T14:49:02Z"
@@ -54,6 +59,7 @@
5459
"author": {
5560
"login": "Quo"
5661
},
62+
"commit": {"oid": "deadbeef"},
5763
"authorCanPushToRepository": true,
5864
"url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71817236",
5965
"publishedAt": "2017-10-24T19:09:52Z"
@@ -64,6 +70,7 @@
6470
"author": {
6571
"login": "bot"
6672
},
73+
"commit": {"oid": "ffdef335209c77f66d933bd873950747bfe42264"},
6774
"authorCanPushToRepository": true,
6875
"url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71839232",
6976
"publishedAt": "2017-10-28T19:21:52Z"

‎test/fixtures/semver_major_pr.json

-3
Original file line numberDiff line numberDiff line change
@@ -16,9 +16,6 @@
1616
}
1717
]
1818
},
19-
"timelineItems": {
20-
"updatedAt": "2017-10-24T11:13:43Z"
21-
},
2219
"title": "lib: awesome changes",
2320
"baseRefName": "main",
2421
"headRefName": "awesome-changes"

‎test/fixtures/single_commit_after_review_reviews.json

+3
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
"author": {
55
"login": "foo"
66
},
7+
"commit": {"oid": "deadbeef"},
78
"authorCanPushToRepository": true,
89
"url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71480624",
910
"publishedAt": "2017-10-24T11:19:25Z"
@@ -13,6 +14,7 @@
1314
"author": {
1415
"login": "foo"
1516
},
17+
"commit": {"oid": "6c0945cbeea2cbbc97d13a3d9e3fe68bd145b985"},
1618
"authorCanPushToRepository": true,
1719
"url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71480625",
1820
"publishedAt": "2017-10-26T11:19:25Z"
@@ -22,6 +24,7 @@
2224
"author": {
2325
"login": "bar"
2426
},
27+
"commit": {"oid": "6c0945cbeea2cbbc97d13a3d9e3fe68bd145b985"},
2528
"authorCanPushToRepository": false,
2629
"url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71480626",
2730
"publishedAt": "2017-10-26T11:19:25Z"

‎test/unit/pr_checker.test.js

-25
Original file line numberDiff line numberDiff line change
@@ -2153,31 +2153,6 @@ describe('PRChecker', () => {
21532153
cli.assertCalledWith(expectedLogs);
21542154
});
21552155

2156-
it('should return true if PR can be landed', () => {
2157-
const expectedLogs = {
2158-
warn: [
2159-
['Something was pushed to the Pull Request branch since the last approving review.']
2160-
],
2161-
info: [],
2162-
error: []
2163-
};
2164-
2165-
const checker = new PRChecker(cli, {
2166-
pr: { ...semverMajorPR, timelineItems: { updatedAt: new Date().toISOString() } },
2167-
reviewers: allGreenReviewers,
2168-
comments: commentsWithLGTM,
2169-
reviews: approvingReviews,
2170-
commits: simpleCommits,
2171-
collaborators,
2172-
authorIsNew: () => true,
2173-
getThread: PRData.prototype.getThread
2174-
}, {}, argv);
2175-
2176-
const status = checker.checkCommitsAfterReview();
2177-
assert.deepStrictEqual(status, false);
2178-
cli.assertCalledWith(expectedLogs);
2179-
});
2180-
21812156
it('should skip the check if there are no reviews', () => {
21822157
const { commits } = multipleCommitsAfterReview;
21832158
const expectedLogs = {

0 commit comments

Comments
 (0)