From 65d7a1bf58319356b25f0ee2f40a4b1c475318dc Mon Sep 17 00:00:00 2001 From: James James Date: Mon, 1 Jun 2020 12:54:58 +0100 Subject: [PATCH] Update WIP / Draft check to check for skip prefix in commit messages (#74) * update wip check to check for skip prefix in commit messages * address review comments --- lib/checkWipDraftPR.js | 63 ++++++++----- spec/checkWipDraftPRSpec.js | 174 +++++++++++++++++++++++++++++++++--- 2 files changed, 205 insertions(+), 32 deletions(-) diff --git a/lib/checkWipDraftPR.js b/lib/checkWipDraftPR.js index 2a0a0b5d..5ad69005 100644 --- a/lib/checkWipDraftPR.js +++ b/lib/checkWipDraftPR.js @@ -30,6 +30,26 @@ const isWIPPr = ({ title, body }) => { ); }; +/** + * @param {import('probot').Context} context + * + */ +const isSkipCICommit = async (context) => { + /** + * @type {import('probot').Octokit.PullsGetResponse} pullRequest + */ + const pullRequest = context.payload.pull_request; + + const commitParams = context.repo({ + commit_sha: pullRequest.head.sha + }); + const commitResponse = await context.github.git.getCommit(commitParams); + + return ( + commitResponse.data.message.startsWith('[ci skip]') || + commitResponse.data.message.startsWith('[skip ci]')); +}; + /** * @param {import('probot').Context} context */ @@ -41,26 +61,29 @@ module.exports.checkWIP = async (context) => { const prAuthor = pullRequest.user.login; if (isDraftPr(pullRequest) || isWIPPr(pullRequest)) { - const link = 'here'.link( - 'https://github.com/oppia/oppia/wiki/Contributing-code-to-Oppia' + - '#wip--draft-pull-requests'); - // Comment on Pull Request. - const commentBody = ( - 'Hi @' + prAuthor + ', WIP/Draft PRs are highly discouraged. You can ' + - 'learn more about it ' + link + '. You can reopen it when it\'s ' + - 'ready to be reviewed and ensure that it is without any WIP phrase ' + - 'in title or body. Thanks!'); - const commentParams = context.repo({ - issue_number: pullRequest.number, - body: commentBody, - }); - await context.github.issues.createComment(commentParams); + const hasSkipCIMessage = await isSkipCICommit(context); + if (!hasSkipCIMessage) { + const link = 'here'.link( + 'https://github.com/oppia/oppia/wiki/Contributing-code-to-Oppia' + + '#wip--draft-pull-requests'); + // Comment on Pull Request. + const commentBody = ( + 'Hi @' + prAuthor + ', when creating WIP/Draft PRs, ensure that ' + + 'your commit messages are prefixed with **[ci skip]** or ' + + '**[skip ci]** to prevent CI checks from running. ' + + 'You can learn more about it ' + link + '.'); + const commentParams = context.repo({ + issue_number: pullRequest.number, + body: commentBody, + }); + await context.github.issues.createComment(commentParams); - // Close Pull Request. - const closePRParams = context.repo({ - issue_number: pullRequest.number, - state: 'closed', - }); - await context.github.issues.update(closePRParams); + // Close Pull Request. + const closePRParams = context.repo({ + issue_number: pullRequest.number, + state: 'closed', + }); + await context.github.issues.update(closePRParams); + } } }; diff --git a/spec/checkWipDraftPRSpec.js b/spec/checkWipDraftPRSpec.js index ed45d266..3b00eb32 100644 --- a/spec/checkWipDraftPRSpec.js +++ b/spec/checkWipDraftPRSpec.js @@ -66,8 +66,15 @@ describe('Oppiabot\'s', () => { done(); }); - describe('WIP PRs', () => { + describe('WIP PRs without skip prefix', () => { beforeEach((done) => { + github.git = { + getCommit: jasmine.createSpy('getCommit').and.resolveTo({ + data: { + message: 'commit without skip prefix' + } + }) + }; spyOn(checkWipDraftPRModule, 'checkWIP').and.callThrough(); robot.receive(pullRequestEditedPayload); done(); @@ -81,6 +88,16 @@ describe('Oppiabot\'s', () => { expect(checkWipDraftPRModule.checkWIP).toHaveBeenCalledTimes(1); }); + it('calls get commit', () => { + expect(github.git.getCommit).toHaveBeenCalled(); + expect(github.git.getCommit).toHaveBeenCalledTimes(1); + expect(github.git.getCommit).toHaveBeenCalledWith({ + owner: pullRequestEditedPayload.payload.repository.owner.login, + repo: pullRequestEditedPayload.payload.repository.name, + commit_sha: pullRequestEditedPayload.payload.pull_request.head.sha + }); + }); + it('creates comment for WIP PRs', () => { expect(github.issues.createComment).toHaveBeenCalled(); expect(github.issues.createComment).toHaveBeenCalledTimes(1); @@ -91,10 +108,10 @@ describe('Oppiabot\'s', () => { '#wip--draft-pull-requests'); const author = pullRequestEditedPayload.payload.pull_request.user.login; const commentBody = ( - 'Hi @' + author + ', WIP/Draft PRs are highly discouraged. You can ' + - 'learn more about it ' + link + '. You can reopen it when it\'s ' + - 'ready to be reviewed and ensure that it is without any WIP phrase ' + - 'in title or body. Thanks!'); + 'Hi @' + author + ', when creating WIP/Draft PRs, ensure that ' + + 'your commit messages are prefixed with **[ci skip]** or ' + + '**[skip ci]** to prevent CI checks from running. ' + + 'You can learn more about it ' + link + '.'); expect(github.issues.createComment).toHaveBeenCalledWith({ issue_number: pullRequestEditedPayload.payload.pull_request.number, @@ -117,16 +134,63 @@ describe('Oppiabot\'s', () => { }); }); + describe('WIP PRs with skip prefix', () => { + beforeEach((done) => { + github.git = { + getCommit: jasmine.createSpy('getCommit').and.resolveTo({ + data: { + message: '[ci skip] commit with skip prefix' + } + }) + }; + spyOn(checkWipDraftPRModule, 'checkWIP').and.callThrough(); + robot.receive(pullRequestEditedPayload); + done(); + }); + + it('calls checkWIP function', () =>{ + expect(checkWipDraftPRModule.checkWIP).toHaveBeenCalled(); + }); + + it('calls checkWIP once', () =>{ + expect(checkWipDraftPRModule.checkWIP).toHaveBeenCalledTimes(1); + }); + + it('calls get commit', () => { + expect(github.git.getCommit).toHaveBeenCalled(); + expect(github.git.getCommit).toHaveBeenCalledTimes(1); + expect(github.git.getCommit).toHaveBeenCalledWith({ + owner: pullRequestEditedPayload.payload.repository.owner.login, + repo: pullRequestEditedPayload.payload.repository.name, + commit_sha: pullRequestEditedPayload.payload.pull_request.head.sha + }); + }); + + it('does not create comment for WIP PRs', () => { + expect(github.issues.createComment).not.toHaveBeenCalled(); + }); + + it('does not close WIP PRs', () => { + expect(github.issues.update).not.toHaveBeenCalled(); + }); + }); describe('Checks when PR is opened or reopened', () => { it('should check when PR is opened', async () => { + github.git = { + getCommit: jasmine.createSpy('getCommit').and.resolveTo({ + data: { + message: 'changes' + } + }) + }; spyOn(checkWipDraftPRModule, 'checkWIP').and.callThrough(); - // Trigger pull_request.opened event. pullRequestEditedPayload.payload.action = 'opened'; await robot.receive(pullRequestEditedPayload); expect(checkWipDraftPRModule.checkWIP).toHaveBeenCalled(); + expect(github.git.getCommit).toHaveBeenCalled(); expect(github.issues.createComment).toHaveBeenCalled(); expect(github.issues.createComment).toHaveBeenCalledTimes(1); expect(github.issues.update).toHaveBeenCalled(); @@ -135,12 +199,19 @@ describe('Oppiabot\'s', () => { it('should check when PR is reopnend', async() => { spyOn(checkWipDraftPRModule, 'checkWIP').and.callThrough(); - + github.git = { + getCommit: jasmine.createSpy('getCommit').and.resolveTo({ + data: { + message: 'commit without skip prefix' + } + }) + }; // Trigger pull_request.opened event. pullRequestEditedPayload.payload.action = 'reopened'; await robot.receive(pullRequestEditedPayload); expect(checkWipDraftPRModule.checkWIP).toHaveBeenCalled(); + expect(github.git.getCommit).toHaveBeenCalled(); expect(github.issues.createComment).toHaveBeenCalled(); expect(github.issues.createComment).toHaveBeenCalledTimes(1); expect(github.issues.update).toHaveBeenCalled(); @@ -150,6 +221,13 @@ describe('Oppiabot\'s', () => { describe('Draft PRs', () => { beforeEach((done) => { + github.git = { + getCommit: jasmine.createSpy('getCommit').and.resolveTo({ + data: { + message: 'commit without skip prefix' + } + }) + }; spyOn(checkWipDraftPRModule, 'checkWIP').and.callThrough(); // Receive a draft payload and remove WIP from title. pullRequestEditedPayload.payload.pull_request.draft = true; @@ -162,6 +240,16 @@ describe('Oppiabot\'s', () => { expect(checkWipDraftPRModule.checkWIP).toHaveBeenCalled(); }); + it('calls get commit', () => { + expect(github.git.getCommit).toHaveBeenCalled(); + expect(github.git.getCommit).toHaveBeenCalledTimes(1); + expect(github.git.getCommit).toHaveBeenCalledWith({ + owner: pullRequestEditedPayload.payload.repository.owner.login, + repo: pullRequestEditedPayload.payload.repository.name, + commit_sha: pullRequestEditedPayload.payload.pull_request.head.sha + }); + }); + it('calls checkWIP once', () => { expect(checkWipDraftPRModule.checkWIP).toHaveBeenCalledTimes(1); }); @@ -181,10 +269,10 @@ describe('Oppiabot\'s', () => { '#wip--draft-pull-requests'); const author = pullRequestEditedPayload.payload.pull_request.user.login; const commentBody = ( - 'Hi @' + author + ', WIP/Draft PRs are highly discouraged. You can ' + - 'learn more about it ' + link + '. You can reopen it when it\'s ' + - 'ready to be reviewed and ensure that it is without any WIP phrase ' + - 'in title or body. Thanks!'); + 'Hi @' + author + ', when creating WIP/Draft PRs, ensure that ' + + 'your commit messages are prefixed with **[ci skip]** or ' + + '**[skip ci]** to prevent CI checks from running. ' + + 'You can learn more about it ' + link + '.'); expect(github.issues.createComment).toHaveBeenCalledWith({ issue_number: pullRequestEditedPayload.payload.pull_request.number, @@ -207,8 +295,65 @@ describe('Oppiabot\'s', () => { }); }); + describe('Draft PRs with skip prefix', () => { + beforeEach((done) => { + github.git = { + getCommit: jasmine.createSpy('getCommit').and.resolveTo({ + data: { + message: '[skip ci] commit with skip prefix' + } + }) + }; + spyOn(checkWipDraftPRModule, 'checkWIP').and.callThrough(); + // Receive a draft payload and remove WIP from title. + pullRequestEditedPayload.payload.pull_request.draft = true; + pullRequestEditedPayload.payload.pull_request.title = 'Testing Draft'; + robot.receive(pullRequestEditedPayload); + done(); + }); + + it('calls checkWIP function', () => { + expect(checkWipDraftPRModule.checkWIP).toHaveBeenCalled(); + }); + + it('calls checkWIP once', () => { + expect(checkWipDraftPRModule.checkWIP).toHaveBeenCalledTimes(1); + }); + + it('calls get commit', () => { + expect(github.git.getCommit).toHaveBeenCalled(); + expect(github.git.getCommit).toHaveBeenCalledTimes(1); + expect(github.git.getCommit).toHaveBeenCalledWith({ + owner: pullRequestEditedPayload.payload.repository.owner.login, + repo: pullRequestEditedPayload.payload.repository.name, + commit_sha: pullRequestEditedPayload.payload.pull_request.head.sha + }); + }); + + it('calls with draft payload', () => { + const args = checkWipDraftPRModule.checkWIP.calls.argsFor(0)[0]; + expect(args.payload.pull_request.draft).toBe(true); + expect(args.payload.pull_request.title).toBe('Testing Draft'); + }); + + it('does not create comment for draft PRs', () => { + expect(github.issues.createComment).not.toHaveBeenCalled(); + }); + + it('does not close draft PRs', () => { + expect(github.issues.update).not.toHaveBeenCalled(); + }); + }); + describe('Neither Draft nor WIP PRs', () => { beforeEach((done) => { + github.git = { + getCommit: jasmine.createSpy('getCommit').and.resolveTo({ + data: { + message: '[skip ci] commit with skip prefix' + } + }) + }; spyOn(checkWipDraftPRModule, 'checkWIP').and.callThrough(); // Receive a neither draft nor WIP payload. pullRequestEditedPayload.payload.pull_request.draft = false; @@ -220,12 +365,17 @@ describe('Oppiabot\'s', () => { it('calls checkWIP function', () => { expect(checkWipDraftPRModule.checkWIP).toHaveBeenCalled(); }); + + it('does not call get commit', () => { + expect(github.git.getCommit).not.toHaveBeenCalled(); + }); + it('does not create a comment', () => { expect(github.issues.createComment).not.toHaveBeenCalled(); }); + it('does not close the PR', ()=> { expect(github.issues.update).not.toHaveBeenCalled(); }); }); - });