From 016611d92f8da69624c2aabf72bf7e858a7db595 Mon Sep 17 00:00:00 2001
From: Niranjan Jayakar <nija@amazon.com>
Date: Wed, 21 Apr 2021 13:07:51 +0100
Subject: [PATCH] chore(prlint): stricter validation around breaking changes
 (#14278)

Increase the scrutiny on how breaking changes are formatted.

Motivation
A previous PR - #14227 - incorrectly specified breaking changes that
standard-release failed to record in the changelog.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
---
 tools/prlint/index.js           | 11 +++++-
 tools/prlint/package.json       |  5 ++-
 tools/prlint/test/index.test.js | 69 +++++++++++++++++++++++++++++++++
 3 files changed, 82 insertions(+), 3 deletions(-)
 create mode 100644 tools/prlint/test/index.test.js

diff --git a/tools/prlint/index.js b/tools/prlint/index.js
index addb026ec6924..6f8de5cb38dbb 100755
--- a/tools/prlint/index.js
+++ b/tools/prlint/index.js
@@ -83,13 +83,20 @@ function hasLabel(issue, labelName) {
  * to be said note, but got misspelled as "BREAKING CHANGES:" or
  * "BREAKING CHANGES(module):"
  */
-function validateBreakingChangeFormat(body) {
+function validateBreakingChangeFormat(title, body) {
     const re = /^BREAKING.*$/m;
     const m = re.exec(body);
     if (m) {
         if (!m[0].startsWith('BREAKING CHANGE: ')) {
             throw new LinterError(`Breaking changes should be indicated by starting a line with 'BREAKING CHANGE: ', variations are not allowed. (found: '${m[0]}')`);
         }
+        if (m[0].substr('BREAKING CHANGE:'.length).trim().length === 0) {
+            throw new LinterError("The description of the first breaking change should immediately follow the 'BREAKING CHANGE: ' clause")
+        }
+        const titleRe = /^[a-z]+\([0-9a-z-_]+\)/;
+        if (!titleRe.exec(title)) {
+            throw new LinterError("The title of this PR must specify the module name that the first breaking change should be associated to");
+        }
     }
 }
 
@@ -125,7 +132,7 @@ async function validatePr(number) {
         fixContainsTest(issue, files);
     }
 
-    validateBreakingChangeFormat(issue.body);
+    validateBreakingChangeFormat(issue.title, issue.body);
 
     console.log("✅  Success")
 
diff --git a/tools/prlint/package.json b/tools/prlint/package.json
index c9e6e895d56d4..22ddafef09426 100644
--- a/tools/prlint/package.json
+++ b/tools/prlint/package.json
@@ -13,9 +13,12 @@
   "dependencies": {
     "github-api": "^3.4.0"
   },
+  "devDependencies": {
+    "jest": "^26.6.3"
+  },
   "scripts": {
     "build": "echo success",
-    "test": "echo success",
+    "test": "jest",
     "build+test": "npm run build test && npm run test"
   }
 }
diff --git a/tools/prlint/test/index.test.js b/tools/prlint/test/index.test.js
new file mode 100644
index 0000000000000..eda53bb5ce137
--- /dev/null
+++ b/tools/prlint/test/index.test.js
@@ -0,0 +1,69 @@
+const GitHub = require('github-api');
+jest.mock('github-api');
+const linter = require('../index');
+
+beforeEach(() => {
+  GitHub.mockClear();
+});
+
+describe('breaking changes format', () => {
+  test('disallow variations to "BREAKING CHANGE:"', async () => {
+    const issue = {
+      body: 'BREAKING CHANGES:',
+      labels: [{ name: 'pr-linter/exempt-test' }, { name: 'pr-linter/exempt-readme' }]
+    };
+    configureMock(issue, undefined);
+    await expect(linter.validatePr(1000)).rejects.toThrow(/'BREAKING CHANGE: ', variations are not allowed/);
+  });
+
+  test('the first breaking change should immediately follow "BREAKING CHANGE:"', async () => {
+    const issue = {
+      body: `BREAKING CHANGE: 
+             * **module:** another change`,
+      labels: [{ name: 'pr-linter/exempt-test' }, { name: 'pr-linter/exempt-readme' }]
+    };
+    configureMock(issue, undefined);
+    await expect(linter.validatePr(1000)).rejects.toThrow(/description of the first breaking change should immediately follow/);
+  });
+
+  test('invalid title', async () => {
+    const issue = {
+      title: 'chore(foo/bar): some title',
+      body: 'BREAKING CHANGE: this breaking change',
+      labels: [{ name: 'pr-linter/exempt-test' }, { name: 'pr-linter/exempt-readme' }]
+    };
+    configureMock(issue, undefined);
+    await expect(linter.validatePr(1000)).rejects.toThrow(/must specify the module name that the first breaking change/);
+  });
+
+  test('valid title', async () => {
+    const issue = {
+      title: 'chore(foo): some title',
+      body: 'BREAKING CHANGE: this breaking change',
+      labels: [{ name: 'pr-linter/exempt-test' }, { name: 'pr-linter/exempt-readme' }]
+    };
+    configureMock(issue, undefined);
+    await linter.validatePr(1000); // not throw
+  });
+});
+
+function configureMock(issue, prFiles) {
+  GitHub.mockImplementation(() => {
+    return {
+      getIssues: () => {
+        return {
+          getIssue: () => {
+            return { data: issue };
+          },
+        };
+      },
+      getRepo: () => {
+        return {
+          listPullRequestFiles: () => {
+            return { data: prFiles };
+          }
+        }
+      }
+    };
+  });
+}
\ No newline at end of file