diff --git a/.github/workflows/frontend_tests.yml b/.github/workflows/frontend_tests.yml index c9e89b30..d2cd4f58 100644 --- a/.github/workflows/frontend_tests.yml +++ b/.github/workflows/frontend_tests.yml @@ -17,7 +17,7 @@ jobs: - uses: actions/checkout@v2 - uses: actions/setup-node@v3 with: - node-version: '14' + node-version: '16' - name: Install dependencies run: npm install - name: Run frontend tests diff --git a/constants.js b/constants.js index f8060ef5..1ef2907a 100644 --- a/constants.js +++ b/constants.js @@ -52,7 +52,6 @@ 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 ciFailureCheck = 'ci-failure-check'; const updateWithDevelopCheck = 'update-with-develop-check'; const respondToReviewCheck = 'respond-to-review-check'; @@ -79,7 +78,6 @@ const checksWhitelist = { [openEvent]: [ assigneeCheck, claCheck, - codeOwnerCheck, branchCheck, jobCheck, cronJobCheck, @@ -102,7 +100,6 @@ const checksWhitelist = { jobCheck, cronJobCheck, modelCheck, - codeOwnerCheck, ], [closeEvent]: [allMergeConflictCheck, updateWithDevelopCheck], [editEvent]: [wipCheck], @@ -169,7 +166,6 @@ module.exports.hotfixLabelCheck = hotfixLabelCheck; module.exports.prTemplateCheck = prTemplateCheck; module.exports.forcePushCheck = forcePushCheck; module.exports.pullRequestReviewCheck = pullRequestReviewCheck; -module.exports.codeOwnerCheck = codeOwnerCheck; module.exports.ciFailureCheck = ciFailureCheck; module.exports.updateWithDevelopCheck = updateWithDevelopCheck; module.exports.respondToReviewCheck = respondToReviewCheck; diff --git a/index.js b/index.js index c4eeddb4..cc83d7c5 100644 --- a/index.js +++ b/index.js @@ -32,7 +32,6 @@ const checkCriticalPullRequestModule = require( ); const checkBranchPushModule = require('./lib/checkBranchPush'); const checkPullRequestReviewModule = require('./lib/checkPullRequestReview'); -const newCodeOwnerModule = require('./lib/checkForNewCodeowner'); const ciCheckModule = require('./lib/ciChecks'); const periodicCheckModule = require('./lib/periodicChecks'); @@ -84,7 +83,8 @@ const runChecks = async (context, checkEvent) => { ); break; case constants.jobCheck: - callable.push(checkPullRequestJobModule.checkForNewJob(context)); + callable.push( + checkPullRequestJobModule.checkForModificationsToFiles(context)); break; case constants.cronJobCheck: callable.push(checkCronJobModule.checkForNewCronJob(context)); @@ -132,9 +132,6 @@ const runChecks = async (context, checkEvent) => { checkPullRequestReviewModule.handlePullRequestReview(context) ); break; - case constants.codeOwnerCheck: - callable.push(newCodeOwnerModule.checkForNewCodeowner(context)); - break; case constants.ciFailureCheck: callable.push(ciCheckModule.handleFailure(context)); break; diff --git a/lib/checkForNewCodeowner.js b/lib/checkForNewCodeowner.js deleted file mode 100644 index 02995af0..00000000 --- a/lib/checkForNewCodeowner.js +++ /dev/null @@ -1,163 +0,0 @@ -// Copyright 2020 The Oppia Authors. All Rights Reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS-IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -/** - * @fileoverview File to handle checks when a new code owner gets added. - */ - -const utilsModule = require('./utils'); -const userWhitelist = require('../userWhitelist.json'); -const CODE_OWNER_FILE_NAME = '.github/CODEOWNERS'; -const newAddition = '+'; -const newCommentAddition = '+#'; -const newLine = '\n'; -const usernameDefiner = '@'; - -/** - * This function returns true if new added line starts with '+' - * and not as '+#' which is comment line and false if otherwise. - * - * @param {string} change - */ -const checkNewLineAddition = (change) => { - return ( - change.startsWith(newAddition) && !change.startsWith(newCommentAddition) - ); -}; - -/** - * This function returns all the new codeowners added to the codeowners file. - * @param {import('probot').Octokit.PullsListFilesResponseItem} file - * @returns {string[]} - */ -const getNewCodeOwners = (file) => { - const changesArray = file.patch.split(newLine); - const newAdditions = changesArray.filter((change) => { - return checkNewLineAddition(change); - }); - - /** - * @type {string[]} addedCodeOwners - */ - let addedCodeOwners = []; - newAdditions.forEach((addition) => { - const usernames = addition.split(usernameDefiner); - // The first item of the array will be the path to the file, and should - // be removed to get only the usernames. - usernames.shift(); - addedCodeOwners.push(...usernames); - }); - // Trim and add @ to the front of the username. - addedCodeOwners = addedCodeOwners.map( - (username) => usernameDefiner + username.trim() - ); - - // Remove all duplicates and return the array. - return Array.from(new Set(addedCodeOwners)); -}; - -/** - * This function returns all the files added/modified to the codeowner file. - * @param {import('probot').Octokit.PullsListFilesResponseItem} file - * @returns {string[]} - */ -const getNewCodeOwnerFiles = (file) => { - const changesArray = file.patch.split(newLine); - const newAdditions = changesArray.filter((change) => { - return checkNewLineAddition(change); - }); - - const addedFiles = newAdditions.map((addition) => { - const additionArray = addition.split(usernameDefiner); - // The first item of the array will be the path to the file, and should - // be returned. - const filePath = additionArray.shift().trim(); - // Remove '+' from the file path. - return filePath.substring(1); - }); - - return addedFiles; -}; - -/** - * This function checks if a new codeowner has been added to the codeowner - * file and responds as required. - * - * @param {import('probot').Context} context - */ -const checkForNewCodeowner = async (context) => { - /** - * @type {import('probot').Octokit.PullsGetResponse} pullRequest - */ - const pullRequest = context.payload.pull_request; - - const changedFiles = await utilsModule.getAllChangedFiles(context); - - const changedCodeOwnerFile = changedFiles.find( - (file) => file.filename === CODE_OWNER_FILE_NAME - ); - - if (changedCodeOwnerFile) { - const mainCodeOwnerFile = await utilsModule.getMainCodeOwnerfile(); - - const newlyAddedCodeOwners = getNewCodeOwners(changedCodeOwnerFile); - - // Remove all exisiting codeowners. - const newCodeOwners = newlyAddedCodeOwners.filter( - (user) => !mainCodeOwnerFile.includes(user) - ); - - if (newCodeOwners.length > 0) { - let codeOwnerString = ''; - if (newCodeOwners.length > 1) { - codeOwnerString = - 'the following new code owners ' + newCodeOwners.join(', '); - } else { - codeOwnerString = 'a new code owner, ' + newCodeOwners[0]; - } - - const filePaths = getNewCodeOwnerFiles(changedCodeOwnerFile); - let codeOwnerFileString = ''; - if (filePaths.length > 1) { - codeOwnerFileString = ' to the following files ' + filePaths.join(', '); - } else { - codeOwnerFileString = ' to ' + filePaths[0]; - } - - let commentBody = ''; - // Ping the code owner of CODEOWNERS file since we just want to flag - // that a new code-owner is added and bring it to his attention. - commentBody = - 'Hi @' + - userWhitelist.codeOwnerFileReviewer + - ', this PR adds ' + - codeOwnerString + - codeOwnerFileString + - '. Please make sure the changes are ' + - 'verified by the previous codeowner(s) of the file. Thanks!'; - - context.github.issues.createComment( - context.repo({ - issue_number: pullRequest.number, - body: commentBody, - }) - ); - } - } -}; - -module.exports = { - checkForNewCodeowner, - getNewCodeOwners -}; diff --git a/lib/checkNewCronJobs.js b/lib/checkNewCronJobs.js index 3cdfc20d..303719bb 100644 --- a/lib/checkNewCronJobs.js +++ b/lib/checkNewCronJobs.js @@ -17,7 +17,7 @@ const { } = require('../userWhitelist.json'); const { DATASTORE_LABEL, - JOBS_AND_FETURES_TESTING_WIKI_LINK, + JOBS_AND_FEATURES_TESTING_WIKI_LINK, getAllChangedFiles, hasDatastoreLabel, pingAndAssignUsers @@ -79,7 +79,7 @@ const getCommentBody = (changedFiles, prAuthor) => { const serverJobsForm = 'server jobs form'.link( 'https://goo.gl/forms/XIj00RJ2h5L55XzU2'); const wikiLinkText = 'this guide'.link( - JOBS_AND_FETURES_TESTING_WIKI_LINK); + JOBS_AND_FEATURES_TESTING_WIKI_LINK); const serverAdminPing = ( 'Hi @' + serverJobAdmins.join(', @') + diff --git a/lib/checkPullRequestJob.js b/lib/checkPullRequestJob.js index 2bf0999d..02aa1fc6 100644 --- a/lib/checkPullRequestJob.js +++ b/lib/checkPullRequestJob.js @@ -16,113 +16,59 @@ const { serverJobAdmins } = require('../userWhitelist.json'); const { - JOBS_AND_FETURES_TESTING_WIKI_LINK, + JOBS_AND_FEATURES_TESTING_WIKI_LINK, DATASTORE_LABEL, getAllChangedFiles, - getNameString, - getNewItemsFromFileByRegex, hasDatastoreLabel, pingAndAssignUsers } = require('./utils'); -const REGISTRY_FILENAME = 'jobs_registry.py'; const JOBS_DIR_PREFIX = 'core/jobs/'; -const JOB_REGEX = new RegExp( - [ - '(?\\+)(?class\\s)', - '(?[a-zA-Z]{2,80})(?Job)(?\\()' - ].join('') -); +const PLATFORM_DIR_PREFIX = 'core/platform/'; /** * @param {string} filename */ -const isInJobsDir = (filename) => { - return filename.startsWith(JOBS_DIR_PREFIX); -}; - -/** - * @param {import('probot').Octokit.PullsListFilesResponseItem} file - */ -const getNewJobsFromFile = (file) => { - return getNewItemsFromFileByRegex(JOB_REGEX, file); -}; - -/** - * @param {import('probot').Octokit.PullsListFilesResponseItem} file - */ -const addsNewJob = (file) => { - const newJobs = getNewJobsFromFile(file); - return newJobs.length > 0; -}; - -/** - * @param {import('probot').Octokit.PullsListFilesResponseItem[]} newJobFiles - */ -const getJobNameString = (newJobFiles) => { - return getNameString( - newJobFiles, - { - singular: 'job', - plural: 'jobs' - }, - JOB_REGEX +const shouldFileChangesBeVerified = (filename) => { + return ( + ( + filename.startsWith(JOBS_DIR_PREFIX) || + filename.startsWith(PLATFORM_DIR_PREFIX) + ) && + !filename.endsWith('test.py') ); }; /** - * @param {import('probot').Octokit.PullsListFilesResponseItem[]} newJobFiles - * @param {import('probot').Octokit.PullsListFilesResponseItem[]} changedFiles * @param {string} prAuthor - Author of the Pull Request */ -const getCommentBody = (newJobFiles, changedFiles, prAuthor) => { - const jobNameString = getJobNameString(newJobFiles); +const getCommentBody = (prAuthor) => { const newLineFeed = '
'; const serverJobsForm = 'server jobs form'.link( 'https://goo.gl/forms/XIj00RJ2h5L55XzU2'); const wikiLinkText = 'this guide'.link( - JOBS_AND_FETURES_TESTING_WIKI_LINK); - - // This function will never be called when there are no job files. - if (newJobFiles.length === 1) { - const serverAdminPing = ( - 'Hi @' + serverJobAdmins.join(', @') + - ', PTAL at this PR, ' + 'it adds a new job.' + jobNameString - ); + JOBS_AND_FEATURES_TESTING_WIKI_LINK); - const prAuthorPing = ( - 'Also @' + prAuthor + ', ' + - 'please make sure to fill in the ' + serverJobsForm + ' ' + - 'for the new job to be tested on the backup server. ' + - 'This PR can be merged only after the test run is successful. ' + - 'Please refer to ' + wikiLinkText + ' for details.' - ); - return ( - serverAdminPing + newLineFeed + prAuthorPing + newLineFeed + 'Thanks!' - ); - } else { - const serverAdminPing = ( - 'Hi @' + serverJobAdmins.join(', @') + ', PTAL at this PR, ' + - 'it adds new jobs.' + jobNameString - ); - - const prAuthorPing = ( - 'Also @' + prAuthor + ', ' + - 'please make sure to fill in the ' + serverJobsForm + ' ' + - 'for the new jobs to be tested on the backup server. ' + - 'This PR can be merged only after the test run is successful. ' + - 'Please refer to ' + wikiLinkText + ' for details.' - ); - return ( - serverAdminPing + newLineFeed + prAuthorPing + newLineFeed + 'Thanks!' - ); - } + const serverAdminPing = ( + 'Hi @' + serverJobAdmins.join(', @') + + ', PTAL at this PR, it modifies files in jobs or platform folders.' + ); + const prAuthorPing = ( + 'Also @' + prAuthor + ', ' + + 'please make sure to fill in the ' + serverJobsForm + ' ' + + 'for the new job or feature to be tested on the backup server. ' + + 'This PR can be merged only after the test run is successful. ' + + 'Please refer to ' + wikiLinkText + ' for details.' + ); + return ( + serverAdminPing + newLineFeed + prAuthorPing + newLineFeed + 'Thanks!' + ); }; /** * @param {import('probot').Context} context */ -const checkForNewJob = async (context) => { +const checkForModificationsToFiles = async (context) => { /** * @type {import('probot').Octokit.PullsGetResponse} pullRequest */ @@ -131,16 +77,12 @@ const checkForNewJob = async (context) => { const changedFiles = await getAllChangedFiles(context); // Get new jobs that were created in the PR. - const newJobFiles = changedFiles.filter((file) => { - return isInJobsDir(file.filename) && addsNewJob(file); + const modifiedFilesToBeTested = changedFiles.filter((file) => { + return shouldFileChangesBeVerified(file.filename); }); - if (newJobFiles.length > 0) { - const commentBody = getCommentBody( - newJobFiles, - changedFiles, - pullRequest.user.login - ); + if (modifiedFilesToBeTested.length > 0) { + const commentBody = getCommentBody(pullRequest.user.login); await pingAndAssignUsers( context, @@ -159,8 +101,7 @@ const checkForNewJob = async (context) => { }; module.exports = { - checkForNewJob, - getNewJobsFromFile, + checkForModificationsToFiles, hasDatastoreLabel, DATASTORE_LABEL }; diff --git a/lib/utils.js b/lib/utils.js index 302dfa32..501da296 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -22,7 +22,7 @@ const DATASTORE_LABEL = 'PR: Affects datastore layer'; const HOTFIX_LABEL = 'PR: Needs to be hotfixed'; const CODE_OWNERS_FILE_URL = 'https://raw.githubusercontent.com/oppia/oppia/develop/.github/CODEOWNERS'; -const JOBS_AND_FETURES_TESTING_WIKI_LINK = ( +const JOBS_AND_FEATURES_TESTING_WIKI_LINK = ( 'https://github.com/oppia/oppia/wiki/' + 'Testing-jobs-and-other-features-on-production' ); @@ -448,7 +448,7 @@ module.exports = { DATASTORE_LABEL, HOTFIX_LABEL, OLD_BUILD_LABEL, - JOBS_AND_FETURES_TESTING_WIKI_LINK, + JOBS_AND_FEATURES_TESTING_WIKI_LINK, getAllChangedFiles, getNameString, getNewItemsFromFileByRegex, diff --git a/spec/apiForSheetsSpec.js b/spec/apiForSheetsSpec.js index 649bdb8b..4d25fe71 100644 --- a/spec/apiForSheetsSpec.js +++ b/spec/apiForSheetsSpec.js @@ -30,7 +30,6 @@ const checkCriticalPullRequestModule = require('../lib/checkCriticalPullRequest'); const checkPullRequestTemplateModule = require('../lib/checkPullRequestTemplate'); -const newCodeOwnerModule = require('../lib/checkForNewCodeowner'); const { google } = require('googleapis'); const { OAuth2Client } = require('google-auth-library'); @@ -52,13 +51,14 @@ describe('Api For Sheets Module', () => { beforeEach(function (done) { spyOn(scheduler, 'createScheduler').and.callFake(() => { }); - spyOn(checkPullRequestJobModule, 'checkForNewJob').and.callFake(() => { }); + spyOn( + checkPullRequestJobModule, 'checkForModificationsToFiles' + ).and.callFake(() => { }); spyOn(checkPullRequestBranchModule, 'checkBranch').and.callFake(() => { }); spyOn(checkCriticalPullRequestModule, 'checkIfPRAffectsDatastoreLayer') .and.callFake(() => { }); spyOn(checkPullRequestTemplateModule, 'checkTemplate') .and.callFake(() => { }); - spyOn(newCodeOwnerModule, 'checkForNewCodeowner').and.callFake(() => { }); github = { issues: { @@ -124,7 +124,9 @@ describe('Api For Sheets Module', () => { it('should call other checks', () => { expect(checkPullRequestBranchModule.checkBranch).toHaveBeenCalled(); - expect(checkPullRequestJobModule.checkForNewJob).toHaveBeenCalled(); + expect( + checkPullRequestJobModule.checkForModificationsToFiles + ).toHaveBeenCalled(); }); it('should be called for the given payload', () => { @@ -432,7 +434,9 @@ describe('Api For Sheets Module', () => { it('should not call any checks', () => { expect(checkPullRequestBranchModule.checkBranch).not.toHaveBeenCalled(); expect(apiForSheetsModule.checkClaStatus).not.toHaveBeenCalled(); - expect(checkPullRequestJobModule.checkForNewJob).not.toHaveBeenCalled(); + expect( + checkPullRequestJobModule.checkForModificationsToFiles + ).not.toHaveBeenCalled(); }); }); @@ -450,7 +454,9 @@ describe('Api For Sheets Module', () => { it('should not call any checks', () => { expect(checkPullRequestBranchModule.checkBranch).not.toHaveBeenCalled(); expect(apiForSheetsModule.checkClaStatus).not.toHaveBeenCalled(); - expect(checkPullRequestJobModule.checkForNewJob).not.toHaveBeenCalled(); + expect( + checkPullRequestJobModule.checkForModificationsToFiles + ).not.toHaveBeenCalled(); }); }); }); diff --git a/spec/checkCriticalPullRequestSpec.js b/spec/checkCriticalPullRequestSpec.js index 110864ba..ee6182f7 100644 --- a/spec/checkCriticalPullRequestSpec.js +++ b/spec/checkCriticalPullRequestSpec.js @@ -29,7 +29,6 @@ const checkCriticalPullRequestModule = require('../lib/checkCriticalPullRequest'); const checkPullRequestTemplateModule = require('../lib/checkPullRequestTemplate'); -const newCodeOwnerModule = require('../lib/checkForNewCodeowner'); const scheduler = require('../lib/scheduler'); let payloadData = JSON.parse( @@ -191,14 +190,15 @@ describe('Critical Pull Request Spec', () => { app = robot.load(oppiaBot); spyOn(app, 'auth').and.resolveTo(github); - spyOn(checkPullRequestJobModule, 'checkForNewJob').and.callFake(() => { }); + spyOn( + checkPullRequestJobModule, 'checkForModificationsToFiles' + ).and.callFake(() => { }); spyOn(checkCronJobModule, 'checkForNewCronJob').and.callFake(() => { }); spyOn(apiForSheetsModule, 'checkClaStatus').and.callFake(() => { }); spyOn(checkPullRequestBranchModule, 'checkBranch').and.callFake(() => { }); spyOn( checkPullRequestTemplateModule, 'checkTemplate' ).and.callFake(() => { }); - spyOn(newCodeOwnerModule, 'checkForNewCodeowner').and.callFake(() => { }); spyOn( checkCriticalPullRequestModule, 'checkIfPRAffectsDatastoreLayer' diff --git a/spec/checkForNewCodeownerSpec.js b/spec/checkForNewCodeownerSpec.js deleted file mode 100644 index 1afedab8..00000000 --- a/spec/checkForNewCodeownerSpec.js +++ /dev/null @@ -1,420 +0,0 @@ -// Copyright 2020 The Oppia Authors. All Rights Reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS-IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -/** - * @fileoverview Spec for new code owner checks. - */ - -require('dotenv').config(); -const { createProbot } = require('probot'); -// The plugin refers to the actual app in index.js. -const oppiaBot = require('../index'); -const checkPullRequestJobModule = require('../lib/checkPullRequestJob'); -const checkCronJobModule = require('../lib/checkNewCronJobs'); -const checkCriticalPullRequestModule = - require('../lib/checkCriticalPullRequest'); -const newCodeOwnerModule = require('../lib/checkForNewCodeowner'); -const mergeConflictModule = require('../lib/checkMergeConflicts'); -const utils = require('../lib/utils'); -const scheduler = require('../lib/scheduler'); -let payloadData = JSON.parse( - JSON.stringify(require('../fixtures/pullRequestPayload.json')) -); - -describe('Check for new code owner', () => { - /** - * @type {import('probot').Probot} robot - */ - let robot; - - /** - * @type {import('probot').Octokit} github - */ - let github; - - /** - * @type {import('probot').Application} app - */ - let app; - - const mainCodeOwnerFile = `# Speed Improvement team. - /app_dev.yaml @vojtechjelinek - /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 - @jameesjohn @vojtechjelinek - /core/templates/pages/OppiaFooterDirective.ts @jameesjohn @vojtechjelinek - /core/templates/pages/footer_js_libs.html @jameesjohn @vojtechjelinek - /core/templates/pages/header_css_libs.html @jameesjohn @vojtechjelinek - /core/templates/services/csrf-token.service*.ts @jameesjohn @vojtechjelinek - /core/templates/third-party-imports/ @nishantwrp @vojtechjelinek - /jinja_utils*.py @jameesjohn @vojtechjelinek - /.lighthouserc.json @jimbyo @jameesjohn @vojtechjelinek - /puppeteer-login-script.js @jimbyo - /scripts/run_lighthouse_tests.py @jameesjohn @vojtechjelinek - /webpack.common.config.ts @jameesjohn @vojtechjelinek - /webpack.common.macros.ts @jameesjohn @vojtechjelinek - /webpack.dev.config.ts @jameesjohn @vojtechjelinek - /webpack.prod.config.ts @jameesjohn @vojtechjelinek - /webpack.terser.config.ts @nishantwrp @vojtechjelinek`; - - const nonCodeOwnerFile = { - sha: 'd144f32b9812373d5f1bc9f94d9af795f09023ff', - filename: 'core/domain/exp_fetchers.py', - status: 'added', - additions: 1, - deletions: 0, - changes: 1, - blob_url: - 'https://github.com/oppia/oppia/blob/67fb4a973b318882af3b5a894130' + - 'e110d7e9833c/core/domain/exp_fetchers.py', - raw_url: - 'https://github.com/oppia/oppia/raw/67fb4a973b318882af3b5a894130e' + - '110d7e9833c/core/domain/exp_fetchers.py', - contents_url: - 'https://api.github.com/repos/oppia/oppia/contents/core/domain/exp' + - '_fetchers.py?ref=67fb4a973b318882af3b5a894130e110d7e9833c', - patch: - '@@ -0,0 +1 @@\n+# def _migrate_states_schema(versioned_exploration' + - '_states, exploration_id):', - }; - - const codeOwnerFileWithNewUser = { - sha: '6911619e95dfcb11eb2204fba88987e5abf02352', - filename: '.github/CODEOWNERS', - status: 'modified', - additions: 2, - deletions: 0, - changes: 2, - blob_url: - 'https://github.com/oppia/oppia/blob/c876111ec9c743179483d7ca75e348' + - 'b8680b13ec/.github/CODEOWNERS', - raw_url: - 'https://github.com/oppia/oppia/raw/c876111ec9c743179483d7ca75e348b8' + - '680b13ec/.github/CODEOWNERS', - contents_url: - 'https://api.github.com/repos/oppia/oppia/contents/.github/CODEOWNERS' + - '?ref=c876111ec9c743179483d7ca75e348b8680b13ec', - patch: - '@@ -523,7 +523,9 @@\n' + - ' /core/templates/pages/pending-account-deletion-page/ ' + - '@jameesjohn @vojtechjelinek\n' + - ' /core/templates/pages/preferences-page/ @jameesjohn @vojtechjelinek\n' + - '+/core/templates/pages/signup-page/ @testuser @vojtechjelinek\n' + - ' /core/templates/pages/splash-page/ @jameesjohn @vojtechjelinek\n' + - ' /core/templates/pages/teach-page/ @jameesjohn @vojtechjelinek\n' + - ' /core/templates/pages/thanks-page/ @jameesjohn @vojtechjelinek\n' + - ' ', - }; - - const codeOwnerFileWithMultipleNewUsers = { - sha: '6911619e95dfcb11eb2204fba88987e5abf02352', - filename: '.github/CODEOWNERS', - status: 'modified', - additions: 2, - deletions: 0, - changes: 2, - blob_url: - 'https://github.com/oppia/oppia/blob/c876111ec9c743179483d7ca75e348b' + - '8680b13ec/.github/CODEOWNERS', - raw_url: - 'https://github.com/oppia/oppia/raw/c876111ec9c743179483d7ca75e348b8' + - '680b13ec/.github/CODEOWNERS', - contents_url: - 'https://api.github.com/repos/oppia/oppia/contents/.github/CODEOWNERS' + - '?ref=c876111ec9c743179483d7ca75e348b8680b13ec', - patch: - '@@ -523,7 +523,9 @@\n' + - ' /core/templates/pages/pending-account-deletion-page/ ' + - '@jameesjohn @vojtechjelinek\n' + - ' /core/templates/pages/preferences-page/ @jameesjohn @vojtechjelinek\n' + - ' /core/templates/pages/signup-page/ @jameesjohn @vojtechjelinek\n' + - '+/core/templates/pages/signup-page/ @testuser @vojtechjelinek\n' + - ' /core/templates/pages/splash-page/ @jameesjohn @vojtechjelinek\n' + - '+/core/templates/pages/signdown-page/ @testuser2 @jameesjohn\n' + - ' /core/templates/pages/teach-page/ @jameesjohn @vojtechjelinek\n' + - ' /core/templates/pages/thanks-page/ @jameesjohn @vojtechjelinek\n' + - ' ', - }; - - const codeOwnerFileWithNewExistingUser = { - sha: '6911619e95dfcb11eb2204fba88987e5abf02352', - filename: '.github/CODEOWNERS', - status: 'modified', - additions: 2, - deletions: 0, - changes: 2, - blob_url: - 'https://github.com/oppia/oppia/blob/c876111ec9c743179483d7ca75e348b86' + - '80b13ec/.github/CODEOWNERS', - raw_url: - 'https://github.com/oppia/oppia/raw/c876111ec9c743179483d7ca75e348b868' + - '0b13ec/.github/CODEOWNERS', - contents_url: - 'https://api.github.com/repos/oppia/oppia/contents/.github/CODEOWNERS' + - '?ref=c876111ec9c743179483d7ca75e348b8680b13ec', - patch: - '@@ -523,7 +523,9 @@\n' + - ' /core/templates/pages/pending-account-deletion-page/ ' + - '@jameesjohn @vojtechjelinek\n' + - ' /core/templates/pages/preferences-page/ @jameesjohn @vojtechjelinek\n' + - ' /core/templates/pages/signup-page/ @jameesjohn @vojtechjelinek\n' + - '+/core/templates/pages/signup-page/ @jameesjohn @vojtechjelinek\n' + - ' /core/templates/pages/splash-page/ @jameesjohn @vojtechjelinek\n' + - '+/core/templates/pages/signup-page/ @vojtechjelinek @jameesjohn\n' + - ' /core/templates/pages/teach-page/ @jameesjohn @vojtechjelinek\n' + - ' /core/templates/pages/thanks-page/ @jameesjohn @vojtechjelinek\n' + - ' ', - }; - beforeAll(() => { - payloadData.payload.action = 'synchronize'; - }); - - beforeEach(() => { - spyOn(scheduler, 'createScheduler').and.callFake(() => { }); - - github = { - issues: { - createComment: jasmine.createSpy('createComment').and.returnValue({}), - }, - }; - - robot = createProbot({ - id: 1, - cert: 'test', - githubToken: 'test', - }); - - app = robot.load(oppiaBot); - spyOn(app, 'auth').and.resolveTo(github); - spyOn(checkPullRequestJobModule, 'checkForNewJob').and.callFake(() => { }); - spyOn(checkCronJobModule, 'checkForNewCronJob').and.callFake(() => { }); - spyOn( - checkCriticalPullRequestModule, - 'checkIfPRAffectsDatastoreLayer' - ).and.callFake(() => { }); - spyOn(mergeConflictModule, 'checkMergeConflictsInPullRequest') - .and.callFake(() => { }); - spyOn(newCodeOwnerModule, 'checkForNewCodeowner').and.callThrough(); - spyOn(utils, 'getMainCodeOwnerfile').and.resolveTo(mainCodeOwnerFile); - }); - - describe('When a new code owner gets added', () => { - beforeEach(() => { - github.pulls = { - listFiles: jasmine.createSpy('listFiles').and.resolveTo({ - data: [nonCodeOwnerFile, codeOwnerFileWithNewUser], - }), - }; - payloadData.payload.pull_request.changed_files = 2; - }); - - describe('and there is no changelog label', () => { - beforeEach(async () => { - const reviewer = { - login: 'reviewer' - }; - payloadData.payload.pull_request.requested_reviewers.push(reviewer); - await robot.receive(payloadData); - }); - - it('should check for new code owner', () => { - expect(newCodeOwnerModule.checkForNewCodeowner).toHaveBeenCalled(); - }); - - it('should get all modified files', () => { - expect(github.pulls.listFiles).toHaveBeenCalled(); - }); - - it('should get codeowner file from develop', () => { - expect(utils.getMainCodeOwnerfile).toHaveBeenCalled(); - }); - - it('should ping reviewer', () => { - expect(github.issues.createComment).toHaveBeenCalledWith({ - repo: payloadData.payload.repository.name, - owner: payloadData.payload.repository.owner.login, - issue_number: payloadData.payload.pull_request.number, - body: - 'Hi @DubeySandeep, this PR adds a new code owner, @testuser to ' + - '/core/templates/pages/signup-page/. Please make sure the changes' + - ' are verified by the previous codeowner(s) of the file. Thanks!' - }); - }); - - afterEach(() => { - payloadData.payload.pull_request.requested_reviewers.pop(); - }); - }); - }); - - describe('When multiple new code owners get added', () => { - beforeEach(async () => { - github.pulls = { - listFiles: jasmine.createSpy('listFiles').and.resolveTo({ - data: [nonCodeOwnerFile, codeOwnerFileWithMultipleNewUsers], - }), - }; - payloadData.payload.pull_request.changed_files = 2; - await robot.receive(payloadData); - }); - it('should check for new code owner', () => { - expect(newCodeOwnerModule.checkForNewCodeowner).toHaveBeenCalled(); - }); - - it('should get all modified files', () => { - expect(github.pulls.listFiles).toHaveBeenCalled(); - }); - - it('should get codeowner file from develop', () => { - expect(utils.getMainCodeOwnerfile).toHaveBeenCalled(); - }); - - it('should ping project owner', () => { - expect(github.issues.createComment).toHaveBeenCalled(); - expect(github.issues.createComment).toHaveBeenCalledWith({ - repo: payloadData.payload.repository.name, - owner: payloadData.payload.repository.owner.login, - issue_number: payloadData.payload.pull_request.number, - body: - 'Hi @DubeySandeep, this PR adds the following new code owners ' + - '@testuser, @testuser2 to the following files ' + - '/core/templates/pages/signup-page/, ' + - '/core/templates/pages/signdown-page/' + - '. Please make sure the changes are ' + - 'verified by the previous codeowner(s) of the file. Thanks!' - }); - }); - - afterEach(() => { - payloadData.payload.pull_request.labels.pop(); - }); - }); - - describe('When an exisiting code owner gets added to another file', () => { - beforeEach(async () => { - github.pulls = { - listFiles: jasmine.createSpy('listFiles').and.resolveTo({ - data: [nonCodeOwnerFile, codeOwnerFileWithNewExistingUser], - }), - }; - payloadData.payload.pull_request.changed_files = 2; - await robot.receive(payloadData); - }); - it('should check for new code owner', () => { - expect(newCodeOwnerModule.checkForNewCodeowner).toHaveBeenCalled(); - }); - - it('should get all modified files', () => { - expect(github.pulls.listFiles).toHaveBeenCalled(); - }); - - it('should get codeowner file from develop', () => { - expect(utils.getMainCodeOwnerfile).toHaveBeenCalled(); - }); - - it('should not ping project owner', () => { - expect(github.issues.createComment).not.toHaveBeenCalled(); - }); - - afterEach(() => { - payloadData.payload.pull_request.labels.pop(); - }); - }); - - describe('When codeowner file is not modified', () => { - beforeEach(async () => { - github.pulls = { - listFiles: jasmine.createSpy('listFiles').and.resolveTo({ - data: [nonCodeOwnerFile], - }), - }; - payloadData.payload.pull_request.changed_files = 1; - await robot.receive(payloadData); - }); - it('should check for new code owner', () => { - expect(newCodeOwnerModule.checkForNewCodeowner).toHaveBeenCalled(); - }); - - it('should get all modified files', () => { - expect(github.pulls.listFiles).toHaveBeenCalled(); - }); - - it('should not get codeowner file from develop', () => { - expect(utils.getMainCodeOwnerfile).not.toHaveBeenCalled(); - }); - - it('should not ping project owner', () => { - expect(github.issues.createComment).not.toHaveBeenCalled(); - }); - - afterEach(() => { - payloadData.payload.pull_request.labels.pop(); - }); - }); - - it('Should get new codeowner from files', () => { - spyOn(newCodeOwnerModule, 'getNewCodeOwners').and.callThrough(); - - // Payload from https://github.com/oppia/oppia/pull/10534. - const fileWithComments = { - sha: '6911619e95dfcb11eb2204fba88987e5abf02352', - filename: '.github/CODEOWNERS', - status: 'modified', - additions: 82, - deletions: 58, - changes: 140, - blob_url: - 'https://github.com/oppia/oppia/blob/c876111ec9c743179483d7ca75e348b' + - '8680b13ec/.github/CODEOWNERS', - raw_url: - 'https://github.com/oppia/oppia/raw/c876111ec9c743179483d7ca75e348b8' + - '680b13ec/.github/CODEOWNERS', - contents_url: - 'https://api.github.com/repos/oppia/oppia/contents/.github/CODEOWNERS' + - '?ref=c876111ec9c743179483d7ca75e348b8680b13ec', - patch: - '-/extensions/ @vojtechjelinek\r\n+# TODO(#10538): Revert ownership ' + - 'to @vojtechjelinek after 2020-09-06.\r\n+/extensions/ @seanlip\r\n/' + - 'core/templates/services/UpgradedServices.ts @bansalnitish ' + - '@srijanreddy98\r\n/typings/ @ankita240796\r\n/tsconfig.json ' + - '@ankita240796\r\n-/tsconfig-strict.json @nishantwrp @vojtechjelinek' + - '\r\n+# TODO(#10538): Add @vojtechjelinek as an owner after 2020-09-' + - '06.\r\n+/tsconfig-strict.json @nishantwrp\r\n-/core/controllers/' + - 'resources*.py @vojtechjelinek\r\n+# TODO(#10538): Revert ownership ' + - 'to @vojtechjelinek after 2020-09-06.\r\n-/core/controllers/' + - 'resources*.py @seanlip' - }; - - let codeowners = newCodeOwnerModule.getNewCodeOwners(fileWithComments); - expect(codeowners.length).toBe(2); - expect(codeowners).toEqual(['@seanlip', '@nishantwrp']); - - codeowners = newCodeOwnerModule.getNewCodeOwners(nonCodeOwnerFile); - expect(codeowners.length).toBe(0); - - codeowners = newCodeOwnerModule.getNewCodeOwners(codeOwnerFileWithNewUser); - expect(codeowners.length).toBe(2); - expect(codeowners).toEqual(['@testuser', '@vojtechjelinek']); - - codeowners = newCodeOwnerModule.getNewCodeOwners( - codeOwnerFileWithMultipleNewUsers - ); - expect(codeowners.length).toBe(4); - expect(codeowners).toEqual([ - '@testuser', '@vojtechjelinek', '@testuser2', '@jameesjohn' - ]); - }); -}); diff --git a/spec/checkMergeConflictSpec.js b/spec/checkMergeConflictSpec.js index 16ce71a7..774adc71 100644 --- a/spec/checkMergeConflictSpec.js +++ b/spec/checkMergeConflictSpec.js @@ -20,7 +20,6 @@ const checkMergeConflictModule = require('../lib/checkMergeConflicts'); const scheduler = require('../lib/scheduler'); const checkPullRequestJobModule = require('../lib/checkPullRequestJob'); const checkCronJobModule = require('../lib/checkNewCronJobs'); -const newCodeOwnerModule = require('../lib/checkForNewCodeowner'); const checkCriticalPullRequestModule = require( '../lib/checkCriticalPullRequest' ); @@ -74,9 +73,10 @@ describe('Merge Conflict Check', () => { app = robot.load(oppiaBot); spyOn(app, 'auth').and.resolveTo(github); - spyOn(checkPullRequestJobModule, 'checkForNewJob').and.callFake(() => { }); + spyOn( + checkPullRequestJobModule, 'checkForModificationsToFiles' + ).and.callFake(() => { }); spyOn(checkCronJobModule, 'checkForNewCronJob').and.callFake(() => { }); - spyOn(newCodeOwnerModule, 'checkForNewCodeowner').and.callFake(() => { }); spyOn( checkCriticalPullRequestModule, 'checkIfPRAffectsDatastoreLayer' diff --git a/spec/checkNewCronJobSpec.js b/spec/checkNewCronJobSpec.js index 6f7a4766..19003431 100644 --- a/spec/checkNewCronJobSpec.js +++ b/spec/checkNewCronJobSpec.js @@ -26,9 +26,8 @@ const checkCriticalPullRequestModule = require('../lib/checkCriticalPullRequest'); const checkPullRequestTemplateModule = require('../lib/checkPullRequestTemplate'); -const newCodeOwnerModule = require('../lib/checkForNewCodeowner'); const scheduler = require('../lib/scheduler'); -const { JOBS_AND_FETURES_TESTING_WIKI_LINK } = require('../lib/utils'); +const { JOBS_AND_FEATURES_TESTING_WIKI_LINK } = require('../lib/utils'); let payloadData = JSON.parse( JSON.stringify(require('../fixtures/pullRequestPayload.json')) ); @@ -200,7 +199,8 @@ describe('Cron Job Spec', () => { spyOn(app, 'auth').and.resolveTo(github); spyOn(checkCronJobModule, 'checkForNewCronJob').and.callThrough(); spyOn( - checkPullRequestJobModule, 'checkForNewJob').and.callFake(() => { }); + checkPullRequestJobModule, 'checkForModificationsToFiles' + ).and.callFake(() => { }); spyOn(apiForSheetsModule, 'checkClaStatus').and.callFake(() => { }); spyOn( checkCriticalPullRequestModule, 'checkIfPRAffectsDatastoreLayer' @@ -209,7 +209,6 @@ describe('Cron Job Spec', () => { spyOn( checkPullRequestTemplateModule, 'checkTemplate' ).and.callFake(() => { }); - spyOn(newCodeOwnerModule, 'checkForNewCodeowner').and.callFake(() => { }); }); describe('When a new cron job is added in a pull request', () => { @@ -242,7 +241,7 @@ describe('Cron Job Spec', () => { ); const newLineFeed = '
'; const wikiLinkText = 'this guide'.link( - JOBS_AND_FETURES_TESTING_WIKI_LINK); + JOBS_AND_FEATURES_TESTING_WIKI_LINK); expect(github.issues.createComment).toHaveBeenCalledWith({ issue_number: payloadData.payload.pull_request.number, @@ -309,7 +308,7 @@ describe('Cron Job Spec', () => { ); const newLineFeed = '
'; const wikiLinkText = 'this guide'.link( - JOBS_AND_FETURES_TESTING_WIKI_LINK); + JOBS_AND_FEATURES_TESTING_WIKI_LINK); expect(github.issues.createComment).toHaveBeenCalledWith({ issue_number: payloadData.payload.pull_request.number, @@ -406,7 +405,7 @@ describe('Cron Job Spec', () => { ); const newLineFeed = '
'; const wikiLinkText = 'this guide'.link( - JOBS_AND_FETURES_TESTING_WIKI_LINK); + JOBS_AND_FEATURES_TESTING_WIKI_LINK); expect(github.issues.createComment).toHaveBeenCalledWith({ issue_number: payloadData.payload.pull_request.number, @@ -462,7 +461,7 @@ describe('Cron Job Spec', () => { ); const newLineFeed = '
'; const wikiLinkText = 'this guide'.link( - JOBS_AND_FETURES_TESTING_WIKI_LINK); + JOBS_AND_FEATURES_TESTING_WIKI_LINK); expect(github.issues.createComment).toHaveBeenCalledWith({ issue_number: payloadData.payload.pull_request.number, diff --git a/spec/checkPullRequestBranchSpec.js b/spec/checkPullRequestBranchSpec.js index befcdb02..0ffe5a4f 100644 --- a/spec/checkPullRequestBranchSpec.js +++ b/spec/checkPullRequestBranchSpec.js @@ -26,7 +26,6 @@ const checkCriticalPullRequestModule = require('../lib/checkCriticalPullRequest'); const checkPullRequestTemplateModule = require('../lib/checkPullRequestTemplate'); -const newCodeOwnerModule = require('../lib/checkForNewCodeowner'); describe('Pull Request Branch Check', () => { /** @@ -49,13 +48,13 @@ describe('Pull Request Branch Check', () => { // Spy on other modules that will be triggered by the payload. spyOn(apiForSheetsModule, 'checkClaStatus').and.callFake(() => { }); - spyOn(checkPullRequestJobModule, 'checkForNewJob') - .and.callFake(() => { }); + spyOn( + checkPullRequestJobModule, 'checkForModificationsToFiles' + ).and.callFake(() => { }); spyOn(checkCriticalPullRequestModule, 'checkIfPRAffectsDatastoreLayer') .and.callFake(() => { }); spyOn(checkPullRequestTemplateModule, 'checkTemplate') .and.callFake(() => { }); - spyOn(newCodeOwnerModule, 'checkForNewCodeowner').and.callFake(() => { }); github = { issues: { diff --git a/spec/checkPullRequestJobSpec.js b/spec/checkPullRequestJobSpec.js index 521f3868..7d9d14e6 100644 --- a/spec/checkPullRequestJobSpec.js +++ b/spec/checkPullRequestJobSpec.js @@ -1,4 +1,3 @@ -// Copyright 2020 The Oppia Authors. All Rights Reserved. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -24,10 +23,9 @@ const checkCriticalPullRequestModule = require('../lib/checkCriticalPullRequest'); const checkPullRequestTemplateModule = require('../lib/checkPullRequestTemplate'); -const newCodeOwnerModule = require('../lib/checkForNewCodeowner'); const scheduler = require('../lib/scheduler'); const checkCronJobModule = require('../lib/checkNewCronJobs'); -const { JOBS_AND_FETURES_TESTING_WIKI_LINK } = require('../lib/utils'); +const { JOBS_AND_FEATURES_TESTING_WIKI_LINK } = require('../lib/utils'); let payloadData = JSON.parse( JSON.stringify(require('../fixtures/pullRequestPayload.json')) @@ -432,7 +430,9 @@ describe('Pull Request Job Spec', () => { app = robot.load(oppiaBot); spyOn(app, 'auth').and.resolveTo(github); - spyOn(checkPullRequestJobModule, 'checkForNewJob').and.callThrough(); + spyOn( + checkPullRequestJobModule, 'checkForModificationsToFiles' + ).and.callThrough(); spyOn(apiForSheetsModule, 'checkClaStatus').and.callFake(() => { }); spyOn( checkCriticalPullRequestModule, 'checkIfPRAffectsDatastoreLayer' @@ -442,7 +442,6 @@ describe('Pull Request Job Spec', () => { spyOn( checkPullRequestTemplateModule, 'checkTemplate' ).and.callFake(() => { }); - spyOn(newCodeOwnerModule, 'checkForNewCodeowner').and.callFake(() => { }); }); describe('When a new job file is created in a pull request', () => { @@ -457,7 +456,9 @@ describe('Pull Request Job Spec', () => { }); it('should check for jobs', () => { - expect(checkPullRequestJobModule.checkForNewJob).toHaveBeenCalled(); + expect( + checkPullRequestJobModule.checkForModificationsToFiles + ).toHaveBeenCalled(); }); it('should get modified files', () => { @@ -472,7 +473,7 @@ describe('Pull Request Job Spec', () => { ); const newLineFeed = '
'; const wikiLinkText = 'this guide'.link( - JOBS_AND_FETURES_TESTING_WIKI_LINK); + JOBS_AND_FEATURES_TESTING_WIKI_LINK); const jobNameLink = ( 'FirstTestJob'.link(firstNewJobFileObj.blob_url) ); @@ -481,10 +482,9 @@ describe('Pull Request Job Spec', () => { issue_number: payloadData.payload.pull_request.number, body: 'Hi @vojtechjelinek, @DubeySandeep, @kevintab95, PTAL at this PR, ' + - 'it adds a new job. The name of the job is ' + jobNameLink + '.' + - newLineFeed + + 'it modifies files in jobs or platform folders.' + newLineFeed + 'Also @' + author + ', please make sure to fill in the ' + formText + - ' for the new job to be tested on the backup server. ' + + ' for the new job or feature to be tested on the backup server. ' + 'This PR can be merged only after the test run is successful. ' + 'Please refer to ' + wikiLinkText + ' for details.' + newLineFeed + 'Thanks!', @@ -531,7 +531,9 @@ describe('Pull Request Job Spec', () => { }); it('should check for jobs', () => { - expect(checkPullRequestJobModule.checkForNewJob).toHaveBeenCalled(); + expect( + checkPullRequestJobModule.checkForModificationsToFiles + ).toHaveBeenCalled(); }); it('should get modified files', () => { @@ -546,7 +548,7 @@ describe('Pull Request Job Spec', () => { ); const newLineFeed = '
'; const wikiLinkText = 'this guide'.link( - JOBS_AND_FETURES_TESTING_WIKI_LINK); + JOBS_AND_FEATURES_TESTING_WIKI_LINK); const jobRegistryLink = ( 'job registry'.link( 'https://github.com/oppia/oppia/blob/develop/core/jobs_registry.py') @@ -561,10 +563,9 @@ describe('Pull Request Job Spec', () => { issue_number: payloadData.payload.pull_request.number, body: 'Hi @vojtechjelinek, @DubeySandeep, @kevintab95, PTAL at this PR, ' + - 'it adds new jobs. The jobs are ' + firstJobNameLink + - ', ' + secondJobNameLink + '.' + newLineFeed + 'Also @' + - author + ', please make sure to fill in the ' + - formText + ' for the new jobs to be tested on the backup server. ' + + 'it modifies files in jobs or platform folders.' + newLineFeed + + 'Also @' + author + ', please make sure to fill in the ' + formText + + ' for the new job or feature to be tested on the backup server. ' + 'This PR can be merged only after the test run is successful. ' + 'Please refer to ' + wikiLinkText + ' for details.' + newLineFeed + 'Thanks!', @@ -611,7 +612,9 @@ describe('Pull Request Job Spec', () => { }); it('should check for jobs', () => { - expect(checkPullRequestJobModule.checkForNewJob).toHaveBeenCalled(); + expect( + checkPullRequestJobModule.checkForModificationsToFiles + ).toHaveBeenCalled(); }); it('should get modified files', () => { @@ -626,7 +629,7 @@ describe('Pull Request Job Spec', () => { ); const newLineFeed = '
'; const wikiLinkText = 'this guide'.link( - JOBS_AND_FETURES_TESTING_WIKI_LINK); + JOBS_AND_FEATURES_TESTING_WIKI_LINK); const jobNameLink = ( 'FirstTestJob'.link(firstNewJobFileObj.blob_url) ); @@ -634,12 +637,11 @@ describe('Pull Request Job Spec', () => { issue_number: payloadData.payload.pull_request.number, body: 'Hi @vojtechjelinek, @DubeySandeep, @kevintab95, PTAL at this ' + - 'PR, it adds a new job. The name of the job is ' + jobNameLink + - '.' + newLineFeed + 'Also @' + author + - ', please make sure to fill in the ' + formText + - ' for the new job to be tested on the backup server. ' + - 'This PR can be merged only after the test run is successful. ' + - 'Please refer to ' + wikiLinkText + ' for details.' + + 'PR, it modifies files in jobs or platform folders.' + newLineFeed + + 'Also @' + author + ', please make sure to fill in the ' + + formText + ' for the new job or feature to be tested on the ' + + 'backup server. This PR can be merged only after the test run ' + + 'is successful. Please refer to ' + wikiLinkText + ' for details.' + newLineFeed + 'Thanks!', repo: payloadData.payload.repository.name, owner: payloadData.payload.repository.owner.login, @@ -682,7 +684,9 @@ describe('Pull Request Job Spec', () => { }); it('should check for jobs', () => { - expect(checkPullRequestJobModule.checkForNewJob).toHaveBeenCalled(); + expect( + checkPullRequestJobModule.checkForModificationsToFiles + ).toHaveBeenCalled(); }); it('should get modified files', () => { @@ -698,7 +702,7 @@ describe('Pull Request Job Spec', () => { ); const newLineFeed = '
'; const wikiLinkText = 'this guide'.link( - JOBS_AND_FETURES_TESTING_WIKI_LINK); + JOBS_AND_FEATURES_TESTING_WIKI_LINK); const jobRegistryLink = ( 'job registry'.link( 'https://github.com/oppia/oppia/blob/develop/core/jobs_registry.py') @@ -711,11 +715,11 @@ describe('Pull Request Job Spec', () => { issue_number: payloadData.payload.pull_request.number, body: 'Hi @vojtechjelinek, @DubeySandeep, @kevintab95, PTAL at this PR, ' + - 'it adds a new job. The name of the job is ' + jobNameLink + - '.' + newLineFeed + 'Also @' + author + ', please make sure to ' + - 'fill in the ' + formText + ' for the new job to be tested on the ' + - 'backup server. ' + 'This PR can be merged only after the test ' + - 'run is successful. ' + 'Please refer to ' + wikiLinkText + + 'it modifies files in jobs or platform folders.' + newLineFeed + + 'Also @' + author + ', please make sure to ' + 'fill in the ' + + formText + ' for the new job or feature to be tested on the ' + + 'backup server. This PR can be merged only after the test ' + + 'run is successful. Please refer to ' + wikiLinkText + ' for details.' + newLineFeed + 'Thanks!', repo: payloadData.payload.repository.name, owner: payloadData.payload.repository.owner.login, @@ -743,44 +747,6 @@ describe('Pull Request Job Spec', () => { }); }); - describe('When an existing job file is modified with no new job', () => { - beforeEach(async () => { - github.pulls = { - listFiles: jasmine.createSpy('listFiles').and.resolveTo({ - data: [ - { - ...firstNewJobFileObj, - status: 'modified', - patch: '\n+# No job files present in the changes', - }, - ], - }), - }; - - payloadData.payload.pull_request.changed_files = 1; - await robot.receive(payloadData); - }); - - it('should check for jobs', () => { - expect(checkPullRequestJobModule.checkForNewJob).toHaveBeenCalled(); - }); - - it('should get modified files', () => { - expect(github.pulls.listFiles).toHaveBeenCalled(); - }); - - - it('should not ping server jobs admin', () => { - expect(github.issues.createComment).not.toHaveBeenCalled(); - }); - it('should not add datastore label', () => { - expect(github.issues.addLabels).not.toHaveBeenCalled(); - }); - it('should not assign server jobs admin', () => { - expect(github.issues.addAssignees).not.toHaveBeenCalled(); - }); - }); - describe('When no job file is modified in a pull request', () => { beforeEach(async () => { github.pulls = { @@ -796,7 +762,9 @@ describe('Pull Request Job Spec', () => { }); it('should check for jobs', () => { - expect(checkPullRequestJobModule.checkForNewJob).toHaveBeenCalled(); + expect( + checkPullRequestJobModule.checkForModificationsToFiles + ).toHaveBeenCalled(); }); it('should not get modified files', () => { @@ -823,7 +791,9 @@ describe('Pull Request Job Spec', () => { }); it('should check for jobs', () => { - expect(checkPullRequestJobModule.checkForNewJob).toHaveBeenCalled(); + expect( + checkPullRequestJobModule.checkForModificationsToFiles + ).toHaveBeenCalled(); }); it('should get modified files', () => { @@ -850,7 +820,9 @@ describe('Pull Request Job Spec', () => { }); it('should check for jobs', () => { - expect(checkPullRequestJobModule.checkForNewJob).toHaveBeenCalled(); + expect( + checkPullRequestJobModule.checkForModificationsToFiles + ).toHaveBeenCalled(); }); it('should get modified files', () => { @@ -880,7 +852,9 @@ describe('Pull Request Job Spec', () => { }); it('should check for jobs', () => { - expect(checkPullRequestJobModule.checkForNewJob).toHaveBeenCalled(); + expect( + checkPullRequestJobModule.checkForModificationsToFiles + ).toHaveBeenCalled(); }); it('should not get modified files', () => { @@ -891,41 +865,4 @@ describe('Pull Request Job Spec', () => { expect(github.issues.createComment).not.toHaveBeenCalled(); }); }); - - describe('Returns appropriate job name', () => { - it('should return the correct job created in the file', () => { - let jobs = checkPullRequestJobModule.getNewJobsFromFile( - firstNewJobFileObj - ); - expect(jobs.length).toBe(1); - expect(jobs[0]).toBe('FirstTestJob'); - - jobs = checkPullRequestJobModule.getNewJobsFromFile(secondNewJobFileObj); - expect(jobs.length).toBe(1); - expect(jobs[0]).toBe('SecondTestJob'); - - jobs = checkPullRequestJobModule.getNewJobsFromFile( - modifiedExistingJobFileObj - ); - expect(jobs.length).toBe(1); - expect(jobs[0]).toBe('OppiabotContributionsJob'); - - jobs = checkPullRequestJobModule.getNewJobsFromFile(fileWithMultipleJobs); - expect(jobs.length).toBe(2); - expect(jobs[0]).toBe('TestJob'); - expect(jobs[1]).toBe('AnotherTestJob'); - - jobs = checkPullRequestJobModule.getNewJobsFromFile(jobTestFile); - expect(jobs.length).toBe(0); - - jobs = checkPullRequestJobModule.getNewJobsFromFile( - modifiedExistingJobFileObjWithJobClassInPatch); - expect(jobs.length).toBe(0); - - jobs = checkPullRequestJobModule.getNewJobsFromFile( - jobFileObjWithJobClassInPatchAndNewJob); - expect(jobs.length).toBe(1); - expect(jobs[0]).toBe('AnotherTestJob'); - }); - }); }); diff --git a/spec/checkPullRequestLabelsSpec.js b/spec/checkPullRequestLabelsSpec.js index 50662e90..15df0f1f 100644 --- a/spec/checkPullRequestLabelsSpec.js +++ b/spec/checkPullRequestLabelsSpec.js @@ -81,7 +81,9 @@ describe('Pull Request Label Check', () => { app = robot.load(oppiaBot); spyOn(app, 'auth').and.resolveTo(github); - spyOn(checkPullRequestJobModule, 'checkForNewJob').and.callFake(() => { }); + spyOn( + checkPullRequestJobModule, 'checkForModificationsToFiles' + ).and.callFake(() => { }); spyOn( checkCriticalPullRequestModule, 'checkIfPRAffectsDatastoreLayer' ).and.callFake(() => { }); diff --git a/spec/checkPullRequestTemplateSpec.js b/spec/checkPullRequestTemplateSpec.js index 02d393a0..4a438a95 100644 --- a/spec/checkPullRequestTemplateSpec.js +++ b/spec/checkPullRequestTemplateSpec.js @@ -27,7 +27,6 @@ const checkPullRequestLabelsModule = require('../lib/checkPullRequestLabels'); const checkPullRequestBranchModule = require('../lib/checkPullRequestBranch'); const checkCriticalPullRequestModule = require('../lib/checkCriticalPullRequest'); -const newCodeOwnerModule = require('../lib/checkForNewCodeowner'); const scheduler = require('../lib/scheduler'); const payloadData = JSON.parse( JSON.stringify(require('../fixtures/pullRequestPayload.json')) @@ -289,7 +288,9 @@ describe('Pull Request Template', () => { app = robot.load(oppiaBot); spyOn(app, 'auth').and.resolveTo(github); - spyOn(checkPullRequestJobModule, 'checkForNewJob').and.callFake(() => { }); + spyOn( + checkPullRequestJobModule, 'checkForModificationsToFiles' + ).and.callFake(() => { }); spyOn(checkCronJobModule, 'checkForNewCronJob').and.callFake(() => { }); spyOn(apiForSheetsModule, 'checkClaStatus').and.callFake(() => { }); spyOn( @@ -298,7 +299,6 @@ describe('Pull Request Template', () => { ).and.callFake(() => { }); spyOn(checkPullRequestBranchModule, 'checkBranch').and.callFake(() => { }); spyOn(checkPullRequestTemplateModule, 'checkTemplate').and.callThrough(); - spyOn(newCodeOwnerModule, 'checkForNewCodeowner').and.callFake(() => { }); }); describe('when pull request with invalid template is created', () => { diff --git a/userWhitelist.json b/userWhitelist.json index f265951a..5ad67a45 100644 --- a/userWhitelist.json +++ b/userWhitelist.json @@ -27,7 +27,6 @@ "aks681", "srijanreddy98" ], - "codeOwnerFileReviewer":"DubeySandeep", "teamLeads": { "onboardingTeam": "DubeySandeep", "releaseTeam": "vojtechjelinek",