refactor: github_api/pulls:merge (#539)

* refactor: github_api/pulls:merge

* add missing awaits

* ran lint

Author: shine2lay

__fixtures__/unit/helper.js

@@ -179,7 +179,7 @@ module.exports = {
               return { status: 204 }
             }
           },
-          merge: jest.fn(),
+          merge: jest.fn().mockReturnValue(options.merge || 'merged'),
           get: jest.fn()
         },
         paginate: jest.fn(async (fn, cb) => {

__tests__/unit/github/api.test.js

@@ -560,3 +560,47 @@ describe('compareCommits', () => {
     }
   })
 })
+
+describe('checkIfMerged', () => {
+  test('return correct data if no error', async () => {
+    let res = await GithubAPI.checkIfMerged(Helper.mockContext())
+    expect(res).toEqual(true)
+
+    res = await GithubAPI.checkIfMerged(Helper.mockContext({ checkIfMerged: false }))
+    expect(res).toEqual(false)
+  })
+
+  test('that error are re-thrown', async () => {
+    const context = Helper.mockContext()
+    context.octokit.pulls.checkIfMerged = jest.fn().mockRejectedValue({ status: 402 })
+
+    try {
+      await GithubAPI.checkIfMerged(context)
+      // Fail test if above expression doesn't throw anything.
+      expect(true).toBe(false)
+    } catch (e) {
+      expect(e.status).toBe(402)
+    }
+  })
+})
+
+describe('mergePR', () => {
+  test('return correct data if no error', async () => {
+    const res = await GithubAPI.mergePR(Helper.mockContext())
+
+    expect(res).toEqual('merged')
+  })
+
+  test('that error are re-thrown', async () => {
+    const context = Helper.mockContext()
+    context.octokit.pulls.merge = jest.fn().mockRejectedValue({ status: 402 })
+
+    try {
+      await GithubAPI.mergePR(context)
+      // Fail test if above expression doesn't throw anything.
+      expect(true).toBe(false)
+    } catch (e) {
+      expect(e.status).toBe(402)
+    }
+  })
+})

lib/actions/merge.js

@@ -1,55 +1,14 @@
 const { Action } = require('./action')
-const logger = require('../logger')
 const UnSupportedSettingError = require('../errors/unSupportedSettingError')
 
 const MERGE_METHOD_OPTIONS = ['merge', 'rebase', 'squash']
 
-const checkIfMerged = async (context, prNumber) => {
-  let status = true
-
-  // return can be 204 or 404 only
-  try {
-    await context.octokit.pulls.checkIfMerged(
-      context.repo({ pull_number: prNumber })
-    )
-  } catch (err) {
-    if (err.status === 404) {
-      status = false
-    } else {
-      throw err
-    }
-  }
-
-  return status
-}
-
 const mergePR = async (context, prNumber, mergeMethod, actionObj) => {
-  const isMerged = await checkIfMerged(context, prNumber)
+  const isMerged = await actionObj.githubAPI.checkIfMerged(context, prNumber)
   if (!isMerged) {
     const pullRequest = await context.octokit.pulls.get(context.repo({ pull_number: prNumber }))
     if (pullRequest.data.mergeable_state !== 'blocked' && pullRequest.data.state === 'open') {
-      try {
-        await context.octokit.pulls.merge(context.repo({ pull_number: prNumber, merge_method: mergeMethod }))
-      } catch (err) {
-        // skip on known errors
-        // 405 === Method not allowed , 409 === Conflict
-        if (err.status === 405 || err.status === 409) {
-          // if the error is another required status check, just skip
-          // no easy way to check if all required status are done
-          if (err.message.toLowerCase().includes('required status check')) return
-
-          const errorLog = {
-            logType: logger.logTypes.MERGE_FAIL_ERROR,
-            eventId: context.eventId,
-            repo: context.payload.repository.full_name,
-            actionName: actionObj.name
-          }
-
-          actionObj.info(JSON.stringify(errorLog))
-        } else {
-          throw err
-        }
-      }
+      await actionObj.githubAPI.mergePR(context, context.repo({ pull_number: prNumber, merge_method: mergeMethod }))
     }
   }
 }

lib/github/api.js

@@ -370,6 +370,48 @@ class GithubAPI {
       return checkCommonError(err, context, callFn)
     }
   }
+
+  static async mergePR (context, callParams) {
+    const callFn = 'pulls.merge'
+    debugLog(context, callFn)
+
+    try {
+      return await context.octokit.pulls.merge(callParams)
+    } catch (err) {
+      // skip on known errors
+      // 405 === Method not allowed , 409 === Conflict
+      if (err.status === 405 || err.status === 409) {
+        // if the error is another required status check, just skip
+        // no easy way to check if all required status are done
+        if (err.message.toLowerCase().includes('required status check')) return
+
+        const errorLog = createLog(context, { callFn: callFn, errors: err.toString(), logType: logger.logTypes.MERGE_FAIL_ERROR })
+
+        const log = logger.create(`GithubAPI/${callFn}`)
+        log.info(JSON.stringify(errorLog))
+      }
+
+      return checkCommonError(err, context, callFn)
+    }
+  }
+
+  static async checkIfMerged (context, pullNumber) {
+    const callFn = 'pulls.checkIfMerged'
+
+    debugLog(context, callFn)
+
+    try {
+      await context.octokit.pulls.checkIfMerged(context.repo({ pull_number: pullNumber }))
+      // if the above doesn't throw error just return true
+      return true
+    } catch (err) {
+      if (err.status === 404) {
+        return false
+      } else {
+        return checkCommonError(err, context, callFn)
+      }
+    }
+  }
 }
 
 module.exports = GithubAPI