Modify jobs detection (oppia#294)

* Modify jobs detection * Fix tests
visioninhope · Oct 23, 2021 · 3760568 · 3760568
1 parent 1f685e1
commit 3760568
Show file tree

Hide file tree

Showing 3 changed files with 114 additions and 198 deletions.
diff --git a/lib/checkPullRequestJob.js b/lib/checkPullRequestJob.js
@@ -25,19 +25,19 @@ const {
   pingAndAssignUsers
 } = require('./utils');
 const REGISTRY_FILENAME = 'jobs_registry.py';
-const TEST_DIR_PREFIX = 'core/tests/';
+const JOBS_DIR_PREFIX = 'core/jobs/';
 const JOB_REGEX = new RegExp(
   [
     '(?<addition>\\+)(?<classDefinition>class\\s)',
-    '(?<name>[a-zA-Z]{2,256})(?<suffix>OneOffJob)(?<funDef>\\()'
+    '(?<name>[a-zA-Z]{2,80})(?<suffix>Job)(?<funDef>\\()'
   ].join('')
 );
 
 /**
  * @param {string} filename
  */
-const isInTestDir = (filename) => {
-  return filename.startsWith(TEST_DIR_PREFIX);
+const isInJobsDir = (filename) => {
+  return filename.startsWith(JOBS_DIR_PREFIX);
 };
 
 /**
@@ -55,63 +55,6 @@ const addsNewJob = (file) => {
   return newJobs.length > 0;
 };
 
-/**
- * Returns the name of the job module from it's file name.
- * @param {string} filename
- *
- * @example
- * getJobModuleName('core/domain/user_jobs_one_off.py')
- * // returns user_jobs_one_off
- */
-const getJobModuleName = (filename) => {
-  // Filename is the relative path to the file, we need to
-  // get the exact file name.
-  const substrings = filename.split('/');
-  const actualFileNameWithExtension = substrings[substrings.length - 1];
-  const extensionIndex = actualFileNameWithExtension.indexOf('.py');
-
-  return actualFileNameWithExtension.substring(0, extensionIndex);
-};
-
-/**
- * @param {string} job
- * @param {import('probot').Octokit.PullsListFilesResponse} files
- */
-const isRegistryUpdated = (job, files) => {
-  const registryFile = files.find(({ filename }) => {
-    return filename.includes(REGISTRY_FILENAME);
-  });
-
-  return registryFile && registryFile.patch.includes(job);
-};
-
-/**
- * @param {import('probot').Octokit.PullsListFilesResponseItem[]} newJobFiles
- * @param {import('probot').Octokit.PullsListFilesResponseItem[]} changedFiles
- */
-const getRegistryReminder = (newJobFiles, changedFiles) => {
-  let registryReminder = ', ';
-
-  const hasNotAddedFileToRegistry = newJobFiles.some(
-    (file) => {
-      const jobs = getNewJobsFromFile(file);
-      const jobModuleName = getJobModuleName(file.filename);
-      return jobs.some((job) => !isRegistryUpdated(
-        `${jobModuleName}.${job}`, changedFiles));
-    }
-  );
-
-  if (hasNotAddedFileToRegistry) {
-    const jobText = newJobFiles.length > 1 ? 'jobs' : 'job';
-    const jobRegistryFile = 'job registry'.link(
-      'https://github.com/oppia/oppia/blob/develop/core/jobs_registry.py');
-    registryReminder +=
-      'please add the new ' + jobText + ' to the ' + jobRegistryFile + ' and ';
-  }
-
-  return registryReminder;
-};
-
 /**
  * @param {import('probot').Octokit.PullsListFilesResponseItem[]} newJobFiles
  */
@@ -132,7 +75,6 @@ const getJobNameString = (newJobFiles) => {
  * @param {string} prAuthor - Author of the Pull Request
  */
 const getCommentBody = (newJobFiles, changedFiles, prAuthor) => {
-  const registryReminder = getRegistryReminder(newJobFiles, changedFiles);
   const jobNameString = getJobNameString(newJobFiles);
   const newLineFeed = '<br>';
   const serverJobsForm = 'server jobs form'.link(
@@ -144,30 +86,35 @@ const getCommentBody = (newJobFiles, changedFiles, prAuthor) => {
   if (newJobFiles.length === 1) {
     const serverAdminPing = (
       'Hi @' + serverJobAdmins.join(', @') +
-      ', PTAL at this PR, ' + 'it adds a new one off job.' + jobNameString
+      ', PTAL at this PR, ' + 'it adds a new job.' + jobNameString
     );
 
-    const prAuthorPing = 'Also @' + prAuthor + registryReminder +
-      'please make sure to fill in the ' + serverJobsForm +
-      ' for the new job to be tested on the backup server. ' +
+    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!');
+      '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 one off jobs.' + jobNameString);
+      'Hi @' + serverJobAdmins.join(', @') + ', PTAL at this PR, ' +
+      'it adds new jobs.' + jobNameString
+    );
 
-    const prAuthorPing = 'Also @' + prAuthor + registryReminder +
-      'please make sure to fill in the ' + serverJobsForm +
-      ' for the new jobs to be tested on the backup server. ' +
+    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!');
+      'Please refer to ' + wikiLinkText + ' for details.'
+    );
+    return (
+      serverAdminPing + newLineFeed + prAuthorPing + newLineFeed + 'Thanks!'
+    );
   }
 };
 
@@ -185,7 +132,7 @@ const checkForNewJob = async (context) => {
 
     // Get new jobs that were created in the PR.
     const newJobFiles = changedFiles.filter((file) => {
-      return !isInTestDir(file.filename) && addsNewJob(file);
+      return isInJobsDir(file.filename) && addsNewJob(file);
     });
 
     if (newJobFiles.length > 0) {

diff --git a/lib/utils.js b/lib/utils.js
@@ -91,8 +91,7 @@ const getNewItemsFromFileByRegex = (regex, file) => {
   // This represents an new line added from the git diff.
   // This is done because classes can only be created in a new line.
   // '+' represents an addition while '-' represents a deletion.
-  const newLine = '\n';
-  const changesArray = file.patch ? file.patch.split(newLine) : [];
+  const changesArray = file.patch ? file.patch.split('\n') : [];
 
   const newDefinitions = changesArray.filter((change) => {
     const matches = regex.exec(change);