From 750df3745e0aa26b488cc1556bd996798f259194 Mon Sep 17 00:00:00 2001 From: James James Date: Sun, 20 Dec 2020 01:22:19 +0100 Subject: [PATCH] Automatically remove stale build label after the PR has been updated (#173) * automatically remove stale build label after the PR has been updated * address review comments * move from synchronize to push --- constants.js | 8 +- index.js | 6 +- lib/periodicChecks.js | 53 +--- lib/staleBuildChecks.js | 104 +++++++ lib/utils.js | 2 + spec/checkBranchPushSpec.js | 2 + spec/checkForNewCodeownerSpec.js | 2 +- spec/periodicChecksSpec.js | 298 +-------------------- spec/staleBuildChecksSpec.js | 446 +++++++++++++++++++++++++++++++ 9 files changed, 574 insertions(+), 347 deletions(-) create mode 100644 lib/staleBuildChecks.js create mode 100644 spec/staleBuildChecksSpec.js diff --git a/constants.js b/constants.js index 8230683c..85f72e49 100644 --- a/constants.js +++ b/constants.js @@ -46,10 +46,11 @@ const issuesLabelCheck = 'issues-labeled-check'; const issuesAssignedCheck = 'issues-assigned-check'; const forcePushCheck = 'force-push-check'; const pullRequestReviewCheck = 'pr-review-check'; -const codeOwnerCheck = 'code-owner-check' +const codeOwnerCheck = 'code-owner-check'; const ciFailureCheck = 'ci-failure-check'; const updateWithDevelopCheck = 'update-with-develop-check'; const respondToReviewCheck = 'respond-to-review-check'; +const oldBuildLabelCheck = 'old-build-label-check'; const checksWhitelist = { 'oppia-android': { @@ -92,7 +93,7 @@ const checksWhitelist = { [issuesLabelEvent]: [issuesLabelCheck], [issuesAssignedEvent]: [issuesAssignedCheck], [unlabelEvent]: [datastoreLabelCheck], - [pushEvent]: [forcePushCheck], + [pushEvent]: [forcePushCheck, oldBuildLabelCheck], [periodicCheckEvent]: [periodicCheck], [pullRequestReviewEvent]: [pullRequestReviewCheck], [checkCompletedEvent]: [ciFailureCheck], @@ -147,7 +148,8 @@ module.exports.pullRequestReviewCheck = pullRequestReviewCheck; module.exports.codeOwnerCheck = codeOwnerCheck; module.exports.ciFailureCheck = ciFailureCheck; module.exports.updateWithDevelopCheck = updateWithDevelopCheck; -module.exports.respondToReviewCheck = respondToReviewCheck +module.exports.respondToReviewCheck = respondToReviewCheck; +module.exports.oldBuildLabelCheck = oldBuildLabelCheck; module.exports.getBlacklistedAuthors = function() { return blacklistedAuthors; diff --git a/index.js b/index.js index c3456bd5..45f7ed7f 100644 --- a/index.js +++ b/index.js @@ -38,6 +38,7 @@ const periodicCheckModule = require('./lib/periodicChecks'); const constants = require('./constants'); const checkIssueAssigneeModule = require('./lib/checkIssueAssignee'); +const staleBuildModule = require('./lib/staleBuildChecks'); const whitelistedAccounts = (process.env.WHITELISTED_ACCOUNTS || '') .toLowerCase() @@ -128,12 +129,15 @@ const runChecks = async (context, checkEvent) => { await Promise.all([ periodicCheckModule.ensureAllPullRequestsAreAssigned(context), periodicCheckModule.ensureAllIssuesHaveProjects(context), - periodicCheckModule.checkAndTagPRsWithOldBuilds(context) + staleBuildModule.checkAndTagPRsWithOldBuilds(context) ]); break; case constants.respondToReviewCheck: await checkPullRequestReviewModule.handleResponseToReview(context); break; + case constants.oldBuildLabelCheck: + await staleBuildModule.removeOldBuildLabel(context); + break; } } } diff --git a/lib/periodicChecks.js b/lib/periodicChecks.js index f5ac47c0..98e11b43 100644 --- a/lib/periodicChecks.js +++ b/lib/periodicChecks.js @@ -24,9 +24,8 @@ const { pingAndAssignUsers, getAllOpenIssues, getAllProjectCards, - hasPendingReviews + hasPendingReviews, } = require('./utils'); -const utilityModule = require('./utils'); const mergeConflictModule = require('./checkMergeConflicts'); const pullRequestReviewModule = require('./checkPullRequestReview'); const { teamLeads, oppiaMaintainers } = require('../userWhitelist.json'); @@ -175,57 +174,7 @@ const ensureAllIssuesHaveProjects = async (context) => { }); }; -/** - * This function comments on and tags pull requests in which the last - * commit was created 2 days ago - * @param {import('probot').Context} context - */ -const checkAndTagPRsWithOldBuilds = async (context) => { - const OLD_BUILD_LABEL = "PR: don't merge - STALE BUILD"; - - const allOpenPullRequests = await getAllOpenPullRequests(context); - - allOpenPullRequests.forEach(async (pullRequest) => { - const containsOldBuildLabel = pullRequest.labels.some( - label => label.name === OLD_BUILD_LABEL - ); - // If label has already been added, do nothing. - if (containsOldBuildLabel) { - return; - } - // Get last commit for pull request. - const {data: lastCommit} = await context.github.repos.getCommit( - context.repo({ - ref: pullRequest.head.sha - }) - ); - - const lastCommitDate = new Date(lastCommit.commit.author.date); - // Commit is older than 2 days - if (utilityModule.MIN_BUILD_DATE > lastCommitDate) { - const labelParams = context.repo({ - issue_number: pullRequest.number, - labels: [OLD_BUILD_LABEL], - }); - context.github.issues.addLabels(labelParams); - - // Ping PR author. - const commentParams = context.repo({ - issue_number: pullRequest.number, - body: - 'Hi @' + pullRequest.user.login + ', the build of this PR is ' + - 'stale and this could result in tests failing in develop. Please ' + - 'update this pull request with the latest changes from develop. ' + - 'Thanks!', - }); - context.github.issues.createComment(commentParams); - } - } - ); -}; - module.exports = { ensureAllPullRequestsAreAssigned, ensureAllIssuesHaveProjects, - checkAndTagPRsWithOldBuilds }; diff --git a/lib/staleBuildChecks.js b/lib/staleBuildChecks.js new file mode 100644 index 00000000..96242b83 --- /dev/null +++ b/lib/staleBuildChecks.js @@ -0,0 +1,104 @@ +const utils = require('./utils'); + +/** + * This function comments on and tags pull requests in which the last + * commit was created 2 days ago + * @param {import('probot').Context} context + */ +const checkAndTagPRsWithOldBuilds = async (context) => { + const allOpenPullRequests = await utils.getAllOpenPullRequests( + context + ); + + allOpenPullRequests.forEach(async (pullRequest) => { + const containsOldBuildLabel = pullRequest.labels.some( + label => label.name === utils.OLD_BUILD_LABEL + ); + // If label has already been added, do nothing. + if (containsOldBuildLabel) { + return; + } + // Get last commit for pull request. + const {data: lastCommit} = await context.github.repos.getCommit( + context.repo({ + ref: pullRequest.head.sha + }) + ); + + const lastCommitDate = new Date(lastCommit.commit.author.date); + // Commit is older than 2 days + if (utils.MIN_BUILD_DATE > lastCommitDate) { + const labelParams = context.repo({ + issue_number: pullRequest.number, + labels: [utils.OLD_BUILD_LABEL], + }); + context.github.issues.addLabels(labelParams); + + // Ping PR author. + const commentParams = context.repo({ + issue_number: pullRequest.number, + body: + 'Hi @' + pullRequest.user.login + ', the build of this PR is ' + + 'stale and this could result in tests failing in develop. Please ' + + 'update this pull request with the latest changes from develop. ' + + 'Thanks!', + }); + context.github.issues.createComment(commentParams); + } + } + ); +}; + +/** + * This function removes the old build label when a PR has a new commit. + * @param {import('probot').Context} context + */ +const removeOldBuildLabel = async (context) => { + // Get the commit SHA after the push. + const sha = context.payload.after; + const maxWaitTime = 10000; + const waitTime = 2000; + /** + * @type {import('probot').Octokit.PullsGetResponse} pullRequest + */ + let pullRequest; + let totalWaitTime = 0; + + do { + // Wait for 2 seconds before searching so that the pull request + // would have been synchronized with the new commit. + await utils.sleep(waitTime); + totalWaitTime += waitTime; + if (totalWaitTime >= maxWaitTime) { + return; + } + + const pullRequestData = await context.github.search.issuesAndPullRequests( + { + q: `${sha} repo:${context.payload.repository.full_name}`, + } + ); + // Since we are searching via the sha, only one PR will be found, + // which is the PR that we are looking for. + pullRequest = pullRequestData.data.items[0]; + } while (pullRequest === undefined); + + const containsOldBuildLabel = pullRequest.labels.some( + label => label.name === utils.OLD_BUILD_LABEL + ); + + if (!containsOldBuildLabel) { + return; + } + + context.github.issues.removeLabel( + context.repo({ + issue_number: pullRequest.number, + name: utils.OLD_BUILD_LABEL, + }) + ); +}; +module.exports = { + checkAndTagPRsWithOldBuilds, + removeOldBuildLabel +}; diff --git a/lib/utils.js b/lib/utils.js index 0c5ff704..b849853b 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -31,6 +31,7 @@ const SUCCESS_STATUS = 204; const THREE_MINUTES = 60 * 1000 * 3; let MIN_BUILD_DATE = new Date(); MIN_BUILD_DATE.setDate(MIN_BUILD_DATE.getDate() - 2); // 2 days prior. +const OLD_BUILD_LABEL = "PR: don't merge - STALE BUILD"; /** * Gets all the changed files in the repo @@ -419,6 +420,7 @@ module.exports = { THREE_MINUTES, LABELS_EXCLUDED_FROM_CODEOWNER_ASSIGNMENT, DATASTORE_LABEL, + OLD_BUILD_LABEL, getAllChangedFiles, getNameString, getNewItemsFromFileByRegex, diff --git a/spec/checkBranchPushSpec.js b/spec/checkBranchPushSpec.js index 78d0de8e..a9ff8c28 100644 --- a/spec/checkBranchPushSpec.js +++ b/spec/checkBranchPushSpec.js @@ -20,6 +20,7 @@ const scheduler = require('../lib/scheduler'); const pushPayload = require('../fixtures/push.json'); const pullRequestPayload = require('../fixtures/pullRequestPayload.json'); const checkBranchPushModule = require('../lib/checkBranchPush'); +const staleBuildModule = require('../lib/staleBuildChecks'); describe('Force Push Check', () => { /** @@ -39,6 +40,7 @@ describe('Force Push Check', () => { beforeEach(() => { spyOn(scheduler, 'createScheduler').and.callFake(() => {}); + spyOn(staleBuildModule, 'removeOldBuildLabel').and.callFake(() => {}); github = { issues: { createComment: jasmine.createSpy('createComment').and.returnValue({}), diff --git a/spec/checkForNewCodeownerSpec.js b/spec/checkForNewCodeownerSpec.js index f00a883a..aa337fd9 100644 --- a/spec/checkForNewCodeownerSpec.js +++ b/spec/checkForNewCodeownerSpec.js @@ -52,7 +52,7 @@ describe('check for new code owner', () => { /core/templates/google-analytics.initializer.ts @jameesjohn @vojtechjelinek /core/templates/base-components/ @jameesjohn @vojtechjelinek /core/templates/pages/Base.ts @jameesjohn @vojtechjelinek - /core/templates/pages/oppia_footer_directive.html + /core/templates/pages/oppia_footer_directive.html @jameesjohn @vojtechjelinek /core/templates/pages/OppiaFooterDirective.ts @jameesjohn @vojtechjelinek /core/templates/pages/footer_js_libs.html @jameesjohn @vojtechjelinek diff --git a/spec/periodicChecksSpec.js b/spec/periodicChecksSpec.js index 9c93c630..80f0d249 100644 --- a/spec/periodicChecksSpec.js +++ b/spec/periodicChecksSpec.js @@ -23,7 +23,7 @@ const scheduler = require('../lib/scheduler'); const payloadData = require('../fixtures/periodicCheckPayload.json'); const periodicCheckModule = require('../lib/periodicChecks'); const mergeConflictModule = require('../lib/checkMergeConflicts'); -const utils = require('../lib/utils'); +const staleBuildModule = require('../lib/staleBuildChecks'); describe('Periodic Checks Module', () => { /** @@ -127,72 +127,6 @@ describe('Periodic Checks Module', () => { }, ], }, - - prWithOldBuild: { - number: 9, - head: { - sha: 'old-build-pr-sha', - }, - labels: [], - user: { - login: 'testuser', - id: 11153258, - node_id: 'MDQ6VXNlcjExMTUzMjU4', - avatar_url: 'https://avatars2.githubusercontent.com/u/11153258?v=4', - gravatar_id: '', - url: 'https://api.github.com/users/testuser', - html_url: 'https://github.com/testuser', - followers_url: 'https://api.github.com/users/testuser/followers', - following_url: - 'https://api.github.com/users/testuser/following{/other_user}', - gists_url: 'https://api.github.com/users/testuser/gists{/gist_id}', - starred_url: - 'https://api.github.com/users/testuser/starred{/owner}{/repo}', - subscriptions_url: - 'https://api.github.com/users/testuser/subscriptions', - organizations_url: 'https://api.github.com/users/testuser/orgs', - repos_url: 'https://api.github.com/users/testuser/repos', - events_url: - 'https://api.github.com/users/testuser/events{/privacy}', - received_events_url: - 'https://api.github.com/users/testuser/received_events', - type: 'User', - site_admin: false, - }, - }, - - prWithNewBuild: { - number: 10, - head: { - sha: 'new-build-pr-sha', - }, - labels: [], - user: { - login: 'testuser2', - id: 11153258, - node_id: 'MDQ6VXNlcjExMTUzMjU4', - avatar_url: 'https://avatars2.githubusercontent.com/u/11153258?v=4', - gravatar_id: '', - url: 'https://api.github.com/users/testuser2', - html_url: 'https://github.com/testuser2', - followers_url: 'https://api.github.com/users/testuser2/followers', - following_url: - 'https://api.github.com/users/testuser2/following{/other_user}', - gists_url: 'https://api.github.com/users/testuser2/gists{/gist_id}', - starred_url: - 'https://api.github.com/users/testuser2/starred{/owner}{/repo}', - subscriptions_url: - 'https://api.github.com/users/testuser2/subscriptions', - organizations_url: 'https://api.github.com/users/testuser2/orgs', - repos_url: 'https://api.github.com/users/testuser2/repos', - events_url: - 'https://api.github.com/users/testuser2/events{/privacy}', - received_events_url: - 'https://api.github.com/users/testuser2/received_events', - type: 'User', - site_admin: false, - }, - }, }; beforeEach(async () => { @@ -303,7 +237,7 @@ describe('Periodic Checks Module', () => { periodicCheckModule, 'ensureAllIssuesHaveProjects' ).and.callFake(() => { }); spyOn( - periodicCheckModule, 'checkAndTagPRsWithOldBuilds' + staleBuildModule, 'checkAndTagPRsWithOldBuilds' ).and.callFake(() => { }); const mergeConflictPR = pullRequests.mergeConflictPR; github.pulls.list = jasmine.createSpy('list').and.resolveTo({ @@ -374,7 +308,7 @@ describe('Periodic Checks Module', () => { periodicCheckModule, 'ensureAllIssuesHaveProjects' ).and.callFake(() => { }); spyOn( - periodicCheckModule, 'checkAndTagPRsWithOldBuilds' + staleBuildModule, 'checkAndTagPRsWithOldBuilds' ).and.callFake(() => { }); const pendingReviewPR = pullRequests.pendingReviewPR; github.pulls.list = jasmine.createSpy('list').and.resolveTo({ @@ -429,7 +363,7 @@ describe('Periodic Checks Module', () => { periodicCheckModule, 'ensureAllIssuesHaveProjects' ).and.callFake(() => { }); spyOn( - periodicCheckModule, 'checkAndTagPRsWithOldBuilds' + staleBuildModule, 'checkAndTagPRsWithOldBuilds' ).and.callFake(() => { }); const changesRequestedPR = pullRequests.hasChangesRequestedPR; github.pulls.list = jasmine.createSpy('list').and.resolveTo({ @@ -511,7 +445,7 @@ describe('Periodic Checks Module', () => { periodicCheckModule, 'ensureAllIssuesHaveProjects' ).and.callFake(() => { }); spyOn( - periodicCheckModule, 'checkAndTagPRsWithOldBuilds' + staleBuildModule, 'checkAndTagPRsWithOldBuilds' ).and.callFake(() => { }); const approvedPR = pullRequests.approvedPR; github.pulls.list = jasmine.createSpy('list').and.resolveTo({ @@ -572,7 +506,7 @@ describe('Periodic Checks Module', () => { periodicCheckModule, 'ensureAllIssuesHaveProjects' ).and.callFake(() => { }); spyOn( - periodicCheckModule, 'checkAndTagPRsWithOldBuilds' + staleBuildModule, 'checkAndTagPRsWithOldBuilds' ).and.callFake(() => { }); const approvedPR = pullRequests.approvedPRWithLabel; github.pulls.list = jasmine.createSpy('list').and.resolveTo({ @@ -638,7 +572,7 @@ describe('Periodic Checks Module', () => { periodicCheckModule, 'ensureAllIssuesHaveProjects' ).and.callFake(() => { }); spyOn( - periodicCheckModule, 'checkAndTagPRsWithOldBuilds' + staleBuildModule, 'checkAndTagPRsWithOldBuilds' ).and.callFake(() => { }); const approvedPR = pullRequests.unResolvablePR; github.pulls.list = jasmine.createSpy('list').and.resolveTo({ @@ -740,7 +674,7 @@ describe('Periodic Checks Module', () => { ]; beforeEach(() => { spyOn( - periodicCheckModule, 'checkAndTagPRsWithOldBuilds' + staleBuildModule, 'checkAndTagPRsWithOldBuilds' ).and.callFake(() => {}); spyOn( periodicCheckModule, @@ -918,220 +852,4 @@ describe('Periodic Checks Module', () => { }); }); }); - - describe('when pull request has an old build', () => { - const oldBuildPRCommitData = { - sha: 'old-build-pr-sha', - node_id: - 'MDY6Q29tbWl0MTczMDA0MDIyOmViNjk3ZTU1YTNkYTMwODUzNjBkODQz' + - 'ZGZiMTUwZjAzM2FhMTdlNjE=', - commit: { - author: { - name: 'James James', - email: 'jamesjay4199@gmail.com', - date: '2020-08-10T14:15:32Z', - }, - committer: { - name: 'James James', - email: 'jamesjay4199@gmail.com', - date: '2020-08-10T14:15:32Z', - }, - message: 'changes', - tree: { - sha: 'f5f8be9b0e4ac9970f68d8945de3474581b20d03', - url: - 'https://api.github.com/repos/jameesjohn/oppia/git/' + - 'trees/f5f8be9b0e4ac9970f68d8945de3474581b20d03', - }, - url: - 'https://api.github.com/repos/jameesjohn/oppia/git/' + - 'commits/eb697e55a3da3085360d843dfb150f033aa17e61', - comment_count: 0, - verification: {}, - }, - url: - 'https://api.github.com/repos/oppia/oppia/commits/' + - 'eb697e55a3da3085360d843dfb150f033aa17e61', - html_url: - 'https://github.com/oppia/oppia/commit/' + - 'eb697e55a3da3085360d843dfb150f033aa17e61', - comments_url: - 'https://api.github.com/repos/oppia/oppia/commits/' + - 'eb697e55a3da3085360d843dfb150f033aa17e61/comments', - author: {}, - committer: {}, - parents: [], - }; - const newBuildPRCommitData = { - sha: 'new-build-pr-sha', - node_id: - 'MDY6Q29tbWl0MTczMDA0MDIyOjUyNWQ2MDU4YTYyNmI0NjE1NGVkMz' + - 'czMTE0MWE5NWU3MGViYjBhZWY=', - commit: { - author: { - name: 'James James', - email: 'jamesjay4199@gmail.com', - date: '2020-08-13T13:43:24Z', - }, - committer: { - name: 'James James', - email: 'jamesjay4199@gmail.com', - date: '2020-08-13T13:43:24Z', - }, - message: 'new additions', - tree: { - sha: 'b5bf5af6ec0592bf3776b23d4355ff200549f427', - url: - 'https://api.github.com/repos/jameesjohn/oppia/git/' + - 'trees/b5bf5af6ec0592bf3776b23d4355ff200549f427', - }, - url: - 'https://api.github.com/repos/jameesjohn/oppia/git/' + - 'commits/525d6058a626b46154ed3731141a95e70ebb0aef', - comment_count: 0, - verification: { - verified: false, - reason: 'unsigned', - signature: null, - payload: null, - }, - }, - url: - 'https://api.github.com/repos/jameesjohn/oppia/commits/' + - '525d6058a626b46154ed3731141a95e70ebb0aef', - html_url: - 'https://github.com/jameesjohn/oppia/commit/' + - '525d6058a626b46154ed3731141a95e70ebb0aef', - comments_url: - 'https://api.github.com/repos/jameesjohn/oppia/' + - 'commits/525d6058a626b46154ed3731141a95e70ebb0aef/comments', - author: [], - committer: [], - parents: [], - }; - - beforeEach(async () => { - spyOn( - periodicCheckModule, - 'checkAndTagPRsWithOldBuilds' - ).and.callThrough(); - spyOn( - periodicCheckModule, - 'ensureAllPullRequestsAreAssigned' - ).and.callFake(() => {}); - spyOn( - periodicCheckModule, - 'ensureAllIssuesHaveProjects' - ).and.callFake(() => {}); - - github.pulls.list = jasmine.createSpy('list').and.resolveTo({ - data: [pullRequests.prWithOldBuild, pullRequests.prWithNewBuild], - }); - // Mocking the minumum build date. - utils.MIN_BUILD_DATE = new Date('2020-08-12T14:15:32Z'); - - github.repos = { - getCommit: jasmine.createSpy('getCommit').and.callFake((params) => { - if (params.ref === pullRequests.prWithOldBuild.head.sha) { - return { - data: oldBuildPRCommitData, - }; - } - return { - data: newBuildPRCommitData, - }; - }), - }; - await robot.receive(payloadData); - }); - - it('should call periodic check module', () => { - expect( - periodicCheckModule.checkAndTagPRsWithOldBuilds - ).toHaveBeenCalled(); - }); - - it('should fetch all open pull requests', () => { - expect(github.pulls.list).toHaveBeenCalled(); - }); - - it('should ping author when build is old', () => { - expect(github.issues.createComment).toHaveBeenCalled(); - expect(github.issues.createComment).toHaveBeenCalledWith( - { - owner: 'oppia', - repo: 'oppia', - issue_number: pullRequests.prWithOldBuild.number, - body: - 'Hi @' + pullRequests.prWithOldBuild.user.login + ', the build ' + - 'of this PR is stale and this could result in tests failing in ' + - 'develop. Please update this pull request with the latest ' + - 'changes from develop. Thanks!', - } - ); - }); - - it('should add old build label when build is old', () => { - expect(github.issues.addLabels).toHaveBeenCalled(); - expect(github.issues.addLabels).toHaveBeenCalledWith( - { - owner: 'oppia', - repo: 'oppia', - issue_number: pullRequests.prWithOldBuild.number, - labels: ["PR: don't merge - STALE BUILD"], - } - ); - }); - - it('should not ping author when build is new', () => { - expect(github.issues.createComment).not.toHaveBeenCalledWith( - { - owner: 'oppia', - repo: 'oppia', - issue_number: pullRequests.prWithNewBuild.number, - body: - 'Hi @' + pullRequests.prWithNewBuild.user.login + ', the build ' + - 'of this PR is stale and this could result in tests failing in ' + - 'develop. Please update this pull request with the latest ' + - 'changes from develop. Thanks!', - } - ); - }); - - describe('when pull request author has already been pinged', () => { - let oldBuildPR; - beforeAll(() => { - // Add stale build label to PR. - oldBuildPR = {...pullRequests.prWithOldBuild}; - oldBuildPR.labels.push({ - name: "PR: don't merge - STALE BUILD" - }); - }); - beforeEach(() => { - github.pulls.list = jasmine.createSpy('list').and.resolveTo({ - data: [oldBuildPR, pullRequests.prWithNewBuild], - }); - }); - - it('should not ping author', () => { - expect(github.issues.createComment).not.toHaveBeenCalled(); - expect(github.issues.createComment).not.toHaveBeenCalledWith( - { - owner: 'oppia', - repo: 'oppia', - issue_number: oldBuildPR.number, - body: - 'Hi @' + pullRequests.prWithNewBuild.user.login + ', the build ' + - 'of this PR is stale and this could result in tests failing in ' + - 'develop. Please update this pull request with the latest ' + - 'changes from develop. Thanks!', - } - ); - }); - - it('should not add old build label', () => { - expect(github.issues.addLabels).not.toHaveBeenCalled(); - }); - }); - }); }); diff --git a/spec/staleBuildChecksSpec.js b/spec/staleBuildChecksSpec.js new file mode 100644 index 00000000..2de46bb2 --- /dev/null +++ b/spec/staleBuildChecksSpec.js @@ -0,0 +1,446 @@ +require('dotenv').config(); +const { createProbot } = require('probot'); +const oppiaBot = require('../index'); +const scheduler = require('../lib/scheduler'); +const periodicCheckModule = require('../lib/periodicChecks'); +const staleBuildModule = require('../lib/staleBuildChecks'); +const periodicCheckPayload = require('../fixtures/periodicCheckPayload.json'); +const utils = require('../lib/utils'); +const pushPayload = require('../fixtures/push.json'); +const checkBranchPushModule = require('../lib/checkBranchPush'); +const pullRequestPayload = require('../fixtures/pullRequestPayload.json'); + +describe('Stale build check', () => { + /** + * @type {import('probot').Probot} robot + */ + let robot; + + /** + * @type {import('probot').Octokit} github + */ + let github; + + /** + * @type {import('probot').Application} app + */ + let app; + + let pullRequests = { + prWithOldBuild: { + number: 9, + head: { + sha: 'old-build-pr-sha', + }, + labels: [], + user: { + login: 'testuser', + id: 11153258, + node_id: 'MDQ6VXNlcjExMTUzMjU4', + avatar_url: 'https://avatars2.githubusercontent.com/u/11153258?v=4', + gravatar_id: '', + url: 'https://api.github.com/users/testuser', + html_url: 'https://github.com/testuser', + followers_url: 'https://api.github.com/users/testuser/followers', + following_url: + 'https://api.github.com/users/testuser/following{/other_user}', + gists_url: 'https://api.github.com/users/testuser/gists{/gist_id}', + starred_url: + 'https://api.github.com/users/testuser/starred{/owner}{/repo}', + subscriptions_url: + 'https://api.github.com/users/testuser/subscriptions', + organizations_url: 'https://api.github.com/users/testuser/orgs', + repos_url: 'https://api.github.com/users/testuser/repos', + events_url: + 'https://api.github.com/users/testuser/events{/privacy}', + received_events_url: + 'https://api.github.com/users/testuser/received_events', + type: 'User', + site_admin: false, + }, + }, + + prWithNewBuild: { + number: 10, + head: { + sha: 'new-build-pr-sha', + }, + labels: [], + user: { + login: 'testuser2', + id: 11153258, + node_id: 'MDQ6VXNlcjExMTUzMjU4', + avatar_url: 'https://avatars2.githubusercontent.com/u/11153258?v=4', + gravatar_id: '', + url: 'https://api.github.com/users/testuser2', + html_url: 'https://github.com/testuser2', + followers_url: 'https://api.github.com/users/testuser2/followers', + following_url: + 'https://api.github.com/users/testuser2/following{/other_user}', + gists_url: 'https://api.github.com/users/testuser2/gists{/gist_id}', + starred_url: + 'https://api.github.com/users/testuser2/starred{/owner}{/repo}', + subscriptions_url: + 'https://api.github.com/users/testuser2/subscriptions', + organizations_url: 'https://api.github.com/users/testuser2/orgs', + repos_url: 'https://api.github.com/users/testuser2/repos', + events_url: + 'https://api.github.com/users/testuser2/events{/privacy}', + received_events_url: + 'https://api.github.com/users/testuser2/received_events', + type: 'User', + site_admin: false, + }, + }, + }; + + beforeEach(async () => { + spyOn(scheduler, 'createScheduler').and.callFake(() => { }); + spyOn(checkBranchPushModule, 'handleForcePush').and.callFake(() => { }); + + github = { + issues: { + createComment: jasmine + .createSpy('createComment') + .and.callFake(() => { }), + addLabels: jasmine.createSpy('addLabels').and.callFake(() => { }), + removeLabel: jasmine.createSpy('removeLabel').and.callFake(() => { }), + }, + search:{ + issuesAndPullRequests: jasmine + .createSpy('issuesAndPullRequests') + .and.resolveTo({ + data: { + items: [pullRequestPayload.payload.pull_request], + }, + }) + }, + }; + + robot = createProbot({ + id: 1, + cert: 'test', + githubToken: 'test', + }); + + app = robot.load(oppiaBot); + spyOn(app, 'auth').and.resolveTo(github); + }); + + describe('when pull request has an old build', () => { + const oldBuildPRCommitData = { + sha: 'old-build-pr-sha', + node_id: + 'MDY6Q29tbWl0MTczMDA0MDIyOmViNjk3ZTU1YTNkYTMwODUzNjBkODQz' + + 'ZGZiMTUwZjAzM2FhMTdlNjE=', + commit: { + author: { + name: 'James James', + email: 'jamesjay4199@gmail.com', + date: '2020-08-10T14:15:32Z', + }, + committer: { + name: 'James James', + email: 'jamesjay4199@gmail.com', + date: '2020-08-10T14:15:32Z', + }, + message: 'changes', + tree: { + sha: 'f5f8be9b0e4ac9970f68d8945de3474581b20d03', + url: + 'https://api.github.com/repos/jameesjohn/oppia/git/' + + 'trees/f5f8be9b0e4ac9970f68d8945de3474581b20d03', + }, + url: + 'https://api.github.com/repos/jameesjohn/oppia/git/' + + 'commits/eb697e55a3da3085360d843dfb150f033aa17e61', + comment_count: 0, + verification: {}, + }, + url: + 'https://api.github.com/repos/oppia/oppia/commits/' + + 'eb697e55a3da3085360d843dfb150f033aa17e61', + html_url: + 'https://github.com/oppia/oppia/commit/' + + 'eb697e55a3da3085360d843dfb150f033aa17e61', + comments_url: + 'https://api.github.com/repos/oppia/oppia/commits/' + + 'eb697e55a3da3085360d843dfb150f033aa17e61/comments', + author: {}, + committer: {}, + parents: [], + }; + const newBuildPRCommitData = { + sha: 'new-build-pr-sha', + node_id: + 'MDY6Q29tbWl0MTczMDA0MDIyOjUyNWQ2MDU4YTYyNmI0NjE1NGVkMz' + + 'czMTE0MWE5NWU3MGViYjBhZWY=', + commit: { + author: { + name: 'James James', + email: 'jamesjay4199@gmail.com', + date: '2020-08-13T13:43:24Z', + }, + committer: { + name: 'James James', + email: 'jamesjay4199@gmail.com', + date: '2020-08-13T13:43:24Z', + }, + message: 'new additions', + tree: { + sha: 'b5bf5af6ec0592bf3776b23d4355ff200549f427', + url: + 'https://api.github.com/repos/jameesjohn/oppia/git/' + + 'trees/b5bf5af6ec0592bf3776b23d4355ff200549f427', + }, + url: + 'https://api.github.com/repos/jameesjohn/oppia/git/' + + 'commits/525d6058a626b46154ed3731141a95e70ebb0aef', + comment_count: 0, + verification: { + verified: false, + reason: 'unsigned', + signature: null, + payload: null, + }, + }, + url: + 'https://api.github.com/repos/jameesjohn/oppia/commits/' + + '525d6058a626b46154ed3731141a95e70ebb0aef', + html_url: + 'https://github.com/jameesjohn/oppia/commit/' + + '525d6058a626b46154ed3731141a95e70ebb0aef', + comments_url: + 'https://api.github.com/repos/jameesjohn/oppia/' + + 'commits/525d6058a626b46154ed3731141a95e70ebb0aef/comments', + author: [], + committer: [], + parents: [], + }; + + beforeEach(async () => { + spyOn( + staleBuildModule, + 'checkAndTagPRsWithOldBuilds' + ).and.callThrough(); + spyOn( + periodicCheckModule, + 'ensureAllPullRequestsAreAssigned' + ).and.callFake(() => {}); + spyOn( + periodicCheckModule, + 'ensureAllIssuesHaveProjects' + ).and.callFake(() => {}); + + github.pulls = { + list: jasmine.createSpy('list').and.resolveTo({ + data: [pullRequests.prWithOldBuild, pullRequests.prWithNewBuild], + }) + }; + // Mocking the minimum build date. + utils.MIN_BUILD_DATE = new Date('2020-08-12T14:15:32Z'); + + github.repos = { + getCommit: jasmine.createSpy('getCommit').and.callFake((params) => { + if (params.ref === pullRequests.prWithOldBuild.head.sha) { + return { + data: oldBuildPRCommitData, + }; + } + return { + data: newBuildPRCommitData, + }; + }), + }; + await robot.receive(periodicCheckPayload); + }); + + it('should call periodic check module', () => { + expect( + staleBuildModule.checkAndTagPRsWithOldBuilds + ).toHaveBeenCalled(); + }); + + it('should fetch all open pull requests', () => { + expect(github.pulls.list).toHaveBeenCalled(); + }); + + it('should ping author when build is old', () => { + expect(github.issues.createComment).toHaveBeenCalled(); + expect(github.issues.createComment).toHaveBeenCalledWith( + { + owner: 'oppia', + repo: 'oppia', + issue_number: pullRequests.prWithOldBuild.number, + body: + 'Hi @' + pullRequests.prWithOldBuild.user.login + ', the build ' + + 'of this PR is stale and this could result in tests failing in ' + + 'develop. Please update this pull request with the latest ' + + 'changes from develop. Thanks!', + } + ); + }); + + it('should add old build label when build is old', () => { + expect(github.issues.addLabels).toHaveBeenCalled(); + expect(github.issues.addLabels).toHaveBeenCalledWith( + { + owner: 'oppia', + repo: 'oppia', + issue_number: pullRequests.prWithOldBuild.number, + labels: ["PR: don't merge - STALE BUILD"], + } + ); + }); + + it('should not ping author when build is new', () => { + expect(github.issues.createComment).not.toHaveBeenCalledWith( + { + owner: 'oppia', + repo: 'oppia', + issue_number: pullRequests.prWithNewBuild.number, + body: + 'Hi @' + pullRequests.prWithNewBuild.user.login + ', the build ' + + 'of this PR is stale and this could result in tests failing in ' + + 'develop. Please update this pull request with the latest ' + + 'changes from develop. Thanks!', + } + ); + }); + + describe('when pull request author has already been pinged', () => { + let oldBuildPR; + beforeAll(() => { + // Add stale build label to PR. + oldBuildPR = {...pullRequests.prWithOldBuild}; + oldBuildPR.labels.push({ + name: "PR: don't merge - STALE BUILD" + }); + }); + beforeEach(async () => { + github.pulls.list = jasmine.createSpy('list').and.resolveTo({ + data: [oldBuildPR, pullRequests.prWithNewBuild], + }); + + await robot.receive(periodicCheckPayload); + }); + + it('should not ping author', () => { + expect(github.issues.createComment).not.toHaveBeenCalled(); + expect(github.issues.createComment).not.toHaveBeenCalledWith( + { + owner: 'oppia', + repo: 'oppia', + issue_number: oldBuildPR.number, + body: + 'Hi @' + pullRequests.prWithNewBuild.user.login + ', the build ' + + 'of this PR is stale and this could result in tests failing in ' + + 'develop. Please update this pull request with the latest ' + + 'changes from develop. Thanks!', + } + ); + }); + + it('should not add old build label', () => { + expect(github.issues.addLabels).not.toHaveBeenCalled(); + }); + }); + }); + + describe('when a pull request with an old build gets updated', () => { + const originalForcedData = pushPayload.payload.forced; + const originalPayloadLabels = ( + pullRequestPayload.payload.pull_request.labels + ); + beforeAll(() => { + // Set force push to false. + pushPayload.payload.forced = false; + // Add Old build label to PR. + pullRequestPayload.payload.pull_request.labels = [ + {name: utils.OLD_BUILD_LABEL} + ]; + }); + + afterAll(() =>{ + pushPayload.payload.forced = originalForcedData; + pullRequestPayload.payload.pull_request.labels = originalPayloadLabels; + }); + + beforeEach(async() => { + spyOn(staleBuildModule, 'removeOldBuildLabel').and.callThrough(); + await robot.receive(pushPayload); + }); + + it('should check if pull request contains old build label', () => { + expect(staleBuildModule.removeOldBuildLabel).toHaveBeenCalled(); + }); + + it('should remove old build label', () => { + expect(github.issues.removeLabel).toHaveBeenCalled(); + expect(github.issues.removeLabel).toHaveBeenCalledWith({ + issue_number: pullRequestPayload.payload.pull_request.number, + name: utils.OLD_BUILD_LABEL, + owner: pullRequestPayload.payload.repository.owner.login, + repo: pullRequestPayload.payload.repository.name + }); + }); + }); + + describe('when a pull request without an old build gets updated', () => { + const originalForcedData = pushPayload.payload.forced; + + beforeAll(() => { + // Set payload action to synchronize; + pushPayload.payload.forced = false; + }); + + afterAll(() =>{ + pushPayload.payload.forced = originalForcedData; + }); + + beforeEach(async() => { + spyOn(staleBuildModule, 'removeOldBuildLabel').and.callThrough(); + await robot.receive(pushPayload); + }); + + it('should check if pull request contains old build label', () => { + expect(staleBuildModule.removeOldBuildLabel).toHaveBeenCalled(); + }); + + it('should not remove any label', () => { + expect(github.issues.removeLabel).not.toHaveBeenCalled(); + }); + }); + + describe('when a push is made from a branch without a pr', () => { + beforeEach(async () => { + spyOn(staleBuildModule, 'removeOldBuildLabel').and.callThrough(); + + pushPayload.payload.ref = 'refs/heads/some-weird-branch'; + pushPayload.payload.forced = true; + github.search = { + issuesAndPullRequests: jasmine + .createSpy('issuesAndPullRequests') + .and.resolveTo({ + data: { + // Return an empty payload since PR can't be found. + items: [], + }, + }), + }; + await robot.receive(pushPayload); + }, 20000); + + it('should check if pull request contains old build label', () => { + expect(staleBuildModule.removeOldBuildLabel).toHaveBeenCalled(); + }); + + it('should search for pull request', () => { + expect(github.search.issuesAndPullRequests).toHaveBeenCalled(); + }); + + it('should not remove any label', () => { + expect(github.issues.removeLabel).not.toHaveBeenCalled(); + }); + }); +});