XS✔ ◾ v1.4.2: Bug fix for GitHub API restrictions (#221)

The GitHub API will fail to add a comment to a file in a PR if the file diff is larger than a specific threshold. In these cases, PR Metrics will throw an error, preventing further execution.

The error is now caught and handled in the cases where it is expected, but still thrown for other errors. There has also been some refactoring to extract out shared logic. Tests have been updated.

In addition, the version of PR Metrics has been updated to v1.4.2 to allow this change to be released to the marketplaces.

README.md

@@ -98,7 +98,7 @@ The default input values are expected to be appropriate for most builds.
 Therefore, the following YAML definition is recommended:
 
 ```YAML
-uses: microsoft/[email protected].1
+uses: microsoft/[email protected].2
 name: PR Metrics
 env:
   PR_METRICS_ACCESS_TOKEN: ${{ secrets.GITHUB_TOKEN }}
@@ -108,7 +108,7 @@ continue-on-error: true
 If you wish to modify the inputs, YAML akin the to the following can be used:
 
 ```YAML
-uses: microsoft/[email protected].1
+uses: microsoft/[email protected].2
 name: PR Metrics
 env:
   PR_METRICS_ACCESS_TOKEN: ${{ secrets.GITHUB_TOKEN }}

dist/resources.resjson

@@ -2,7 +2,7 @@
   "$schema": "https://json.schemastore.org/resjson.json",
   "loc.description": "Augments pull request titles to let reviewers quickly determine PR size and test coverage.",
   "loc.description.comment": "The description of the task.",
-  "loc.friendlyName": "PR Metrics 1.4.1",
+  "loc.friendlyName": "PR Metrics 1.4.2",
   "loc.friendlyName.comment": "The name of the task.",
   "loc.helpMarkDown": "[More information](https://aka.ms/PRMetrics/README)",
   "loc.helpMarkDown.comment": "The Markdown-formatted help text of the task.",

package.json

@@ -1,7 +1,7 @@
 {
   "$schema": "https://json.schemastore.org/package.json",
   "name": "prmetrics",
-  "version": "1.4.1",
+  "version": "1.4.2",
   "description": "Augments pull request titles to let reviewers quickly determine PR size and test coverage.",
   "main": "dist/index.js",
   "scripts": {

src/task/Strings/resources.resjson/en-US/resources.resjson

@@ -2,7 +2,7 @@
   "$schema": "https://json.schemastore.org/resjson.json",
   "loc.description": "Augments pull request titles to let reviewers quickly determine PR size and test coverage.",
   "loc.description.comment": "The description of the task.",
-  "loc.friendlyName": "PR Metrics 1.4.1",
+  "loc.friendlyName": "PR Metrics 1.4.2",
   "loc.friendlyName.comment": "The name of the task.",
   "loc.helpMarkDown": "[More information](https://aka.ms/PRMetrics/README)",
   "loc.helpMarkDown.comment": "The Markdown-formatted help text of the task.",

src/task/index.ts

@@ -42,16 +42,10 @@ async function run (): Promise<void> {
     runnerInvoker.setStatusSucceeded(runnerInvoker.loc('index.succeeded'))
   } catch (error: any) {
     const logger: Logger = container.resolve(Logger)
-    const runnerInvoker: RunnerInvoker = container.resolve(RunnerInvoker)
-    const properties: string[] = Object.getOwnPropertyNames(error)
-    properties.forEach((property: string): void => {
-      if (property !== 'message') {
-        const name: string = error.name
-        logger.logInfo(`${name} – ${property}: ${JSON.stringify(error[property])}`)
-      }
-    })
-
+    logger.logErrorObject(error)
     logger.replay()
+
+    const runnerInvoker: RunnerInvoker = container.resolve(RunnerInvoker)
     runnerInvoker.setStatusFailed(error.message)
   }
 }

src/task/src/repos/gitHubReposInvoker.ts

@@ -137,8 +137,17 @@ export default class GitHubReposInvoker extends BaseReposInvoker {
       }
 
       await this.invokeApiCall(async (): Promise<void> => {
-        const result: CreateReviewCommentResponse = await this._octokitWrapper.createReviewComment(this._owner, this._repo, this._pullRequestId, content, fileName, this._commitId)
-        this._logger.logDebug(JSON.stringify(result))
+        try {
+          const result: CreateReviewCommentResponse = await this._octokitWrapper.createReviewComment(this._owner, this._repo, this._pullRequestId, content, fileName, this._commitId)
+          this._logger.logDebug(JSON.stringify(result))
+        } catch (error: any) {
+          if (error.status === 422 && error.message === 'pull_request_review_thread.path diff too large') {
+            this._logger.logInfo('GitHub createReviewComment() threw a 422 error related to a large diff. Ignoring as this is expected.')
+            this._logger.logErrorObject(error)
+          } else {
+            throw error
+          }
+        }
       })
     } else {
       await this.invokeApiCall(async (): Promise<void> => {
@@ -183,7 +192,7 @@ export default class GitHubReposInvoker extends BaseReposInvoker {
 
     const options: OctokitOptions = {
       auth: process.env.PR_METRICS_ACCESS_TOKEN,
-      userAgent: 'PRMetrics/v1.4.1',
+      userAgent: 'PRMetrics/v1.4.2',
       log: {
         debug: (message: string): void => this._logger.logDebug(`Octokit – ${message}`),
         info: (message: string): void => this._logger.logInfo(`Octokit – ${message}`),

src/task/src/utilities/logger.ts

@@ -61,6 +61,18 @@ export default class Logger {
     this._runnerInvoker.logError(message)
   }
 
+  /**
+   * Logs an error object.
+   * @param error The error object to log.
+   */
+  public logErrorObject (error: any): void {
+    const name: string = error.name
+    const properties: string[] = Object.getOwnPropertyNames(error)
+    properties.forEach((property: string): void => {
+      this.logInfo(`${name} – ${property}: ${JSON.stringify(error[property])}`)
+    })
+  }
+
   /**
    * Replays the messages logged.
    */

src/task/task.json

@@ -2,7 +2,7 @@
   "$schema": "https://raw.githubusercontent.com/microsoft/azure-pipelines-task-lib/master/tasks.schema.json",
   "id": "907d3b28-6b37-4ac7-ac75-9631ee53e512",
   "name": "PRMetrics",
-  "friendlyName": "PR Metrics 1.4.1",
+  "friendlyName": "PR Metrics 1.4.2",
   "description": "Augments pull request titles to let reviewers quickly determine PR size and test coverage.",
   "helpUrl": "https://aka.ms/PRMetrics/README",
   "helpMarkDown": "[More information](https://aka.ms/PRMetrics/README)",
@@ -15,7 +15,7 @@
   "version": {
     "Major": 1,
     "Minor": 4,
-    "Patch": 1
+    "Patch": 2
   },
   "instanceNameFormat": "PR Metrics",
   "showEnvironmentVariables": true,

src/task/task.loc.json

@@ -15,7 +15,7 @@
   "version": {
     "Major": 1,
     "Minor": 4,
-    "Patch": 1
+    "Patch": 2
   },
   "instanceNameFormat": "ms-resource:loc.instanceNameFormat",
   "showEnvironmentVariables": true,

src/task/tests/repos/gitHubReposInvoker.spec.ts

@@ -14,6 +14,7 @@ import GetIssueCommentsResponse from '../../src/wrappers/octokitInterfaces/getIs
 import GetPullResponse from '../../src/wrappers/octokitInterfaces/getPullResponse'
 import GitHubReposInvoker from '../../src/repos/gitHubReposInvoker'
 import GitInvoker from '../../src/git/gitInvoker'
+import HttpError from '../testUtilities/httpError'
 import Logger from '../../src/utilities/logger'
 import OctokitLogObject from '../wrappers/octokitLogObject'
 import OctokitWrapper from '../../src/wrappers/octokitWrapper'
@@ -26,7 +27,7 @@ describe('gitHubReposInvoker.ts', function (): void {
   let octokitWrapper: OctokitWrapper
   let runnerInvoker: RunnerInvoker
 
-  const expectedUserAgent: string = 'PRMetrics/v1.4.1'
+  const expectedUserAgent: string = 'PRMetrics/v1.4.2'
 
   beforeEach((): void => {
     process.env.PR_METRICS_ACCESS_TOKEN = 'PAT'
@@ -953,6 +954,77 @@ describe('gitHubReposInvoker.ts', function (): void {
       verify(logger.logDebug('null')).twice()
     })
 
+    it('should succeed when a HTTP 422 error occurs due to having a too large path diff', async (): Promise<void> => {
+      // Arrange
+      when(octokitWrapper.initialize(anything())).thenCall((options: any): void => {
+        expect(options.auth).to.equal('PAT')
+        expect(options.userAgent).to.equal(expectedUserAgent)
+        expect(options.log).to.not.equal(null)
+        expect(options.log.debug).to.not.equal(null)
+        expect(options.log.info).to.not.equal(null)
+        expect(options.log.warn).to.not.equal(null)
+        expect(options.log.error).to.not.equal(null)
+      })
+      const error: HttpError = new HttpError(422, 'pull_request_review_thread.path diff too large')
+      when(octokitWrapper.createReviewComment('microsoft', 'PR-Metrics', 12345, 'Content', 'file.ts', 'sha54321')).thenCall((): void => {
+        throw error
+      })
+      const gitHubReposInvoker: GitHubReposInvoker = new GitHubReposInvoker(instance(gitInvoker), instance(logger), instance(octokitWrapper), instance(runnerInvoker))
+
+      // Act
+      await gitHubReposInvoker.createComment('Content', CommentThreadStatus.Unknown, 'file.ts')
+
+      // Assert
+      verify(octokitWrapper.initialize(anything())).once()
+      verify(octokitWrapper.listCommits('microsoft', 'PR-Metrics', 12345, 1)).once()
+      verify(octokitWrapper.createReviewComment('microsoft', 'PR-Metrics', 12345, 'Content', 'file.ts', 'sha54321')).once()
+      verify(logger.logDebug('* GitHubReposInvoker.createComment()')).once()
+      verify(logger.logDebug('* GitHubReposInvoker.initialize()')).once()
+      verify(logger.logDebug('* GitHubReposInvoker.initializeForAzureDevOps()')).once()
+      verify(logger.logDebug('* GitHubReposInvoker.getCommitId()')).once()
+      verify(logger.logInfo('GitHub createReviewComment() threw a 422 error related to a large diff. Ignoring as this is expected.')).once()
+      verify(logger.logErrorObject(error)).once()
+    })
+
+    {
+      const testCases: HttpError[] = [
+        new HttpError(400, 'pull_request_review_thread.path diff too large'),
+        new HttpError(422, 'Unprocessable Entity')
+      ]
+
+      testCases.forEach((error: HttpError): void => {
+        it('should throw when an error occurs that is not a HTTP 422 or is not due to having a too large path diff', async (): Promise<void> => {
+          // Arrange
+          when(octokitWrapper.initialize(anything())).thenCall((options: any): void => {
+            expect(options.auth).to.equal('PAT')
+            expect(options.userAgent).to.equal(expectedUserAgent)
+            expect(options.log).to.not.equal(null)
+            expect(options.log.debug).to.not.equal(null)
+            expect(options.log.info).to.not.equal(null)
+            expect(options.log.warn).to.not.equal(null)
+            expect(options.log.error).to.not.equal(null)
+          })
+          when(octokitWrapper.createReviewComment('microsoft', 'PR-Metrics', 12345, 'Content', 'file.ts', 'sha54321')).thenCall((): void => {
+            throw error
+          })
+          const gitHubReposInvoker: GitHubReposInvoker = new GitHubReposInvoker(instance(gitInvoker), instance(logger), instance(octokitWrapper), instance(runnerInvoker))
+
+          // Act
+          const func: () => Promise<void> = async () => await gitHubReposInvoker.createComment('Content', CommentThreadStatus.Unknown, 'file.ts')
+
+          // Assert
+          await ExpectExtensions.toThrowAsync(func, error.message)
+          verify(octokitWrapper.initialize(anything())).once()
+          verify(octokitWrapper.listCommits('microsoft', 'PR-Metrics', 12345, 1)).once()
+          verify(octokitWrapper.createReviewComment('microsoft', 'PR-Metrics', 12345, 'Content', 'file.ts', 'sha54321')).once()
+          verify(logger.logDebug('* GitHubReposInvoker.createComment()')).once()
+          verify(logger.logDebug('* GitHubReposInvoker.initialize()')).once()
+          verify(logger.logDebug('* GitHubReposInvoker.initializeForAzureDevOps()')).once()
+          verify(logger.logDebug('* GitHubReposInvoker.getCommitId()')).once()
+        })
+      })
+    }
+
     it('should succeed when no file name is specified', async (): Promise<void> => {
       // Arrange
       when(octokitWrapper.initialize(anything())).thenCall((options: any): void => {

src/task/tests/testUtilities/httpError.ts

@@ -0,0 +1,34 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT License.
+
+/**
+ * A class representing a HTTP error.
+ */
+export default class HttpError extends Error {
+  /**
+   * Initializes a new instance of the `HttpError` class.
+   * @param status The HTTP status code.
+   * @param message The error message.
+   */
+  public constructor (status: number, message: string) {
+    super()
+
+    this.status = status
+    this.message = message
+  }
+
+  /**
+   * Gets the name of the object type.
+   */
+  public name: string = 'HttpError'
+
+  /**
+   * Gets the HTTP status code.
+   */
+  public status: number
+
+  /**
+   * Gets the error message.
+   */
+  public message: string
+}

src/task/tests/utilities/logger.spec.ts

@@ -4,6 +4,7 @@
 import 'reflect-metadata'
 import { instance, mock, verify } from 'ts-mockito'
 import ConsoleWrapper from '../../src/wrappers/consoleWrapper'
+import HttpError from '../testUtilities/httpError'
 import Logger from '../../src/utilities/logger'
 import RunnerInvoker from '../../src/runners/runnerInvoker'
 
@@ -68,6 +69,40 @@ describe('logger.ts', (): void => {
     })
   })
 
+  describe('logErrorObject()', (): void => {
+    it('should log all properties of the error object', (): void => {
+      // Arrange
+      const logger: Logger = new Logger(instance(consoleWrapper), instance(runnerInvoker))
+      const error: Error = new Error('Message')
+      error.name = 'Error'
+      error.stack = 'Stack contents'
+
+      // Act
+      logger.logErrorObject(error)
+
+      // Assert
+      verify(consoleWrapper.log('Error – name: "Error"')).once()
+      verify(consoleWrapper.log('Error – message: "Message"')).once()
+      verify(consoleWrapper.log('Error – stack: "Stack contents"')).once()
+    })
+
+    it('should log all properties of a complex error object', (): void => {
+      // Arrange
+      const logger: Logger = new Logger(instance(consoleWrapper), instance(runnerInvoker))
+      const error: HttpError = new HttpError(404, 'Not Found')
+      error.stack = 'Stack contents'
+
+      // Act
+      logger.logErrorObject(error)
+
+      // Assert
+      verify(consoleWrapper.log('HttpError – name: "HttpError"')).once()
+      verify(consoleWrapper.log('HttpError – message: "Not Found"')).once()
+      verify(consoleWrapper.log('HttpError – stack: "Stack contents"')).once()
+      verify(consoleWrapper.log('HttpError – status: 404')).once()
+    })
+  })
+
   describe('replay()', (): void => {
     it('should replay all messages', (): void => {
       // Arrange

src/vss-extension.json

@@ -3,7 +3,7 @@
   "manifestVersion": 1,
   "id": "PRMetrics",
   "name": "PR Metrics",
-  "version": "1.4.1",
+  "version": "1.4.2",
   "publisher": "ms-omex",
   "description": "Augments pull request titles to let reviewers quickly determine PR size and test coverage.",
   "public": true,