From 59dc0cd3c2ebe71f920260ed49d2611dc9d2ba31 Mon Sep 17 00:00:00 2001 From: Norbert Elter <72046715+itsyoboieltr@users.noreply.github.com> Date: Mon, 14 Oct 2024 22:13:48 +0400 Subject: [PATCH] feat: Create a quality gate for typescript coverage (#27717) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/27717?quickstart=1) This PR introduces a quality gate for typescript coverage. It updates the existing fitness function to disallow the creation of new js and jsx files in the repository. ## **Related issues** Fixes: https://github.com/MetaMask/MetaMask-planning/issues/2399 ## **Manual testing steps** 1. After committing a javascript file, the fitness function should fail. ## **Screenshots/Recordings** Not applicable ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --- .github/workflows/fitness-functions.yml | 6 +- .../common/constants.test.ts | 86 ++++++++++--------- .../fitness-functions/common/constants.ts | 16 ++-- .../fitness-functions/common/shared.test.ts | 84 ++++++++---------- .../fitness-functions/common/shared.ts | 44 ++++++++-- development/fitness-functions/rules/index.ts | 18 ++-- .../rules/javascript-additions.test.ts | 12 +-- .../rules/javascript-additions.ts | 11 +-- .../rules/sinon-assert-syntax.ts | 4 +- 9 files changed, 154 insertions(+), 127 deletions(-) diff --git a/.github/workflows/fitness-functions.yml b/.github/workflows/fitness-functions.yml index b4979c8f3e7b..f8e24692e8fe 100644 --- a/.github/workflows/fitness-functions.yml +++ b/.github/workflows/fitness-functions.yml @@ -2,12 +2,14 @@ name: Fitness Functions CI on: pull_request: - types: [assigned, opened, synchronize, reopened] + types: + - opened + - reopened + - synchronize jobs: build: runs-on: ubuntu-latest - steps: - name: Checkout code uses: actions/checkout@v4 diff --git a/development/fitness-functions/common/constants.test.ts b/development/fitness-functions/common/constants.test.ts index 21912d5fa194..e0077f086594 100644 --- a/development/fitness-functions/common/constants.test.ts +++ b/development/fitness-functions/common/constants.test.ts @@ -1,8 +1,34 @@ -import { EXCLUDE_E2E_TESTS_REGEX, SHARED_FOLDER_JS_REGEX } from './constants'; +import { E2E_TESTS_REGEX, JS_REGEX } from './constants'; describe('Regular Expressions used in Fitness Functions', (): void => { - describe(`EXCLUDE_E2E_TESTS_REGEX "${EXCLUDE_E2E_TESTS_REGEX}"`, (): void => { + describe(`E2E_TESTS_REGEX "${E2E_TESTS_REGEX}"`, (): void => { const PATHS_IT_SHOULD_MATCH = [ + // JS, TS, JSX, and TSX files inside the + // test/e2e directory + 'test/e2e/file.js', + 'test/e2e/path/file.ts', + 'test/e2e/much/longer/path/file.jsx', + 'test/e2e/much/longer/path/file.tsx', + // development/fitness-functions directory + 'development/fitness-functions/file.js', + 'development/fitness-functions/path/file.ts', + 'development/fitness-functions/much/longer/path/file.jsx', + 'development/fitness-functions/much/longer/path/file.tsx', + // development/webpack directory + 'development/webpack/file.js', + 'development/webpack/path/file.ts', + 'development/webpack/much/longer/path/file.jsx', + 'development/webpack/much/longer/path/file.tsx', + ]; + + const PATHS_IT_SHOULD_NOT_MATCH = [ + // any files without JS, TS, JSX or TSX extension + 'file', + 'file.extension', + 'path/file.extension', + 'much/longer/path/file.extension', + // JS, TS, JSX, and TSX files outside + // the test/e2e, development/fitness-functions, development/webpack directories 'file.js', 'path/file.js', 'much/longer/path/file.js', @@ -12,39 +38,15 @@ describe('Regular Expressions used in Fitness Functions', (): void => { 'file.jsx', 'path/file.jsx', 'much/longer/path/file.jsx', - ]; - - const PATHS_IT_SHOULD_NOT_MATCH = [ - // any without JS, TS, JSX or TSX extension - 'file', - 'file.extension', - 'path/file.extension', - 'much/longer/path/file.extension', - // any in the test/e2e directory - 'test/e2e/file', - 'test/e2e/file.extension', - 'test/e2e/path/file.extension', - 'test/e2e/much/longer/path/file.extension', - 'test/e2e/file.js', - 'test/e2e/path/file.ts', - 'test/e2e/much/longer/path/file.jsx', - 'test/e2e/much/longer/path/file.tsx', - // any in the development/fitness-functions directory - 'development/fitness-functions/file', - 'development/fitness-functions/file.extension', - 'development/fitness-functions/path/file.extension', - 'development/fitness-functions/much/longer/path/file.extension', - 'development/fitness-functions/file.js', - 'development/fitness-functions/path/file.ts', - 'development/fitness-functions/much/longer/path/file.jsx', - 'development/fitness-functions/much/longer/path/file.tsx', + 'file.tsx', + 'path/file.tsx', + 'much/longer/path/file.tsx', ]; describe('included paths', (): void => { PATHS_IT_SHOULD_MATCH.forEach((path: string): void => { it(`should match "${path}"`, (): void => { - const result = new RegExp(EXCLUDE_E2E_TESTS_REGEX, 'u').test(path); - + const result = E2E_TESTS_REGEX.test(path); expect(result).toStrictEqual(true); }); }); @@ -53,22 +55,23 @@ describe('Regular Expressions used in Fitness Functions', (): void => { describe('excluded paths', (): void => { PATHS_IT_SHOULD_NOT_MATCH.forEach((path: string): void => { it(`should not match "${path}"`, (): void => { - const result = new RegExp(EXCLUDE_E2E_TESTS_REGEX, 'u').test(path); - + const result = E2E_TESTS_REGEX.test(path); expect(result).toStrictEqual(false); }); }); }); }); - describe(`SHARED_FOLDER_JS_REGEX "${SHARED_FOLDER_JS_REGEX}"`, (): void => { + describe(`JS_REGEX "${JS_REGEX}"`, (): void => { const PATHS_IT_SHOULD_MATCH = [ + 'app/much/longer/path/file.js', + 'app/much/longer/path/file.jsx', + 'offscreen/path/file.js', + 'offscreen/path/file.jsx', 'shared/file.js', - 'shared/path/file.js', - 'shared/much/longer/path/file.js', 'shared/file.jsx', - 'shared/path/file.jsx', - 'shared/much/longer/path/file.jsx', + 'ui/much/longer/path/file.js', + 'ui/much/longer/path/file.jsx', ]; const PATHS_IT_SHOULD_NOT_MATCH = [ @@ -80,13 +83,15 @@ describe('Regular Expressions used in Fitness Functions', (): void => { 'file.ts', 'path/file.ts', 'much/longer/path/file.tsx', + // any JS or JSX files outside the app, offscreen, shared, and ui directories + 'test/longer/path/file.js', + 'random/longer/path/file.jsx', ]; describe('included paths', (): void => { PATHS_IT_SHOULD_MATCH.forEach((path: string): void => { it(`should match "${path}"`, (): void => { - const result = new RegExp(SHARED_FOLDER_JS_REGEX, 'u').test(path); - + const result = JS_REGEX.test(path); expect(result).toStrictEqual(true); }); }); @@ -95,8 +100,7 @@ describe('Regular Expressions used in Fitness Functions', (): void => { describe('excluded paths', (): void => { PATHS_IT_SHOULD_NOT_MATCH.forEach((path: string): void => { it(`should not match "${path}"`, (): void => { - const result = new RegExp(SHARED_FOLDER_JS_REGEX, 'u').test(path); - + const result = JS_REGEX.test(path); expect(result).toStrictEqual(false); }); }); diff --git a/development/fitness-functions/common/constants.ts b/development/fitness-functions/common/constants.ts index 5758d4e2a6e1..f3996a294a5a 100644 --- a/development/fitness-functions/common/constants.ts +++ b/development/fitness-functions/common/constants.ts @@ -1,10 +1,12 @@ -// include JS, TS, JSX, TSX files only excluding files in the e2e tests and -// fitness functions directories -const EXCLUDE_E2E_TESTS_REGEX = - '^(?!test/e2e)(?!development/fitness|development/webpack).*.(js|ts|jsx|tsx)$'; +// include JS, TS, JSX, TSX files only in the +// test/e2e +// development/fitness-functions +// development/webpack directories +const E2E_TESTS_REGEX = + /^(test\/e2e|development\/fitness-functions|development\/webpack).*\.(js|ts|jsx|tsx)$/u; -// include JS and JSX files in the shared directory only -const SHARED_FOLDER_JS_REGEX = '^(shared).*.(js|jsx)$'; +// include JS and JSX files only in the app, offscreen, shared, and ui directories +const JS_REGEX = /^(app|offscreen|shared|ui)\/.*\.(js|jsx)$/u; enum AUTOMATION_TYPE { CI = 'ci', @@ -12,4 +14,4 @@ enum AUTOMATION_TYPE { PRE_PUSH_HOOK = 'pre-push-hook', } -export { EXCLUDE_E2E_TESTS_REGEX, SHARED_FOLDER_JS_REGEX, AUTOMATION_TYPE }; +export { E2E_TESTS_REGEX, JS_REGEX, AUTOMATION_TYPE }; diff --git a/development/fitness-functions/common/shared.test.ts b/development/fitness-functions/common/shared.test.ts index 92306b9d4751..66af337fbfc8 100644 --- a/development/fitness-functions/common/shared.test.ts +++ b/development/fitness-functions/common/shared.test.ts @@ -30,13 +30,13 @@ describe('filterDiffFileCreations()', (): void => { const actualResult = filterDiffFileCreations(testFileDiff); expect(actualResult).toMatchInlineSnapshot(` - "diff --git a/old-file.js b/old-file.js - new file mode 100644 - index 000000000..30d74d258 - --- /dev/null - +++ b/old-file.js - @@ -0,0 +1 @@ - +ping" + "diff --git a/old-file.js b/old-file.js + new file mode 100644 + index 000000000..30d74d258 + --- /dev/null + +++ b/old-file.js + @@ -0,0 +1 @@ + +ping" `); }); }); @@ -44,9 +44,9 @@ describe('filterDiffFileCreations()', (): void => { describe('hasNumberOfCodeBlocksIncreased()', (): void => { it('should show which code blocks have increased', (): void => { const testDiffFragment = ` - +foo - +bar - +baz`; + +foo + +bar + +baz`; const testCodeBlocks = ['code block 1', 'foo', 'baz']; const actualResult = hasNumberOfCodeBlocksIncreased( @@ -69,7 +69,7 @@ describe('filterDiffByFilePath()', (): void => { it('should return the right diff for a generic matcher', (): void => { const actualResult = filterDiffByFilePath( testFileDiff, - '.*/.*.(js|ts)$|.*.(js|ts)$', + /^(.*\/)?.*\.(jsx)$/u, // Exclude jsx files ); expect(actualResult).toMatchInlineSnapshot(` @@ -93,35 +93,17 @@ describe('filterDiffByFilePath()', (): void => { }); it('should return the right diff for a specific file in any dir matcher', (): void => { - const actualResult = filterDiffByFilePath(testFileDiff, '.*old-file.js$'); + const actualResult = filterDiffByFilePath(testFileDiff, /.*old-file\.js$/u); // Exclude old-file.js expect(actualResult).toMatchInlineSnapshot(` - "diff --git a/old-file.js b/old-file.js - index 57d5de75c..808d8ba37 100644 - --- a/old-file.js - +++ b/old-file.js - @@ -1,3 +1,8 @@ - +ping - @@ -34,33 +39,4 @@ - -pong" - `); - }); - - it('should return the right diff for a multiple file extension (OR) matcher', (): void => { - const actualResult = filterDiffByFilePath( - testFileDiff, - '^(./)*old-file.(js|ts|jsx)$', - ); - - expect(actualResult).toMatchInlineSnapshot(` - "diff --git a/old-file.js b/old-file.js + "diff --git a/new-file.ts b/new-file.ts index 57d5de75c..808d8ba37 100644 - --- a/old-file.js - +++ b/old-file.js + --- a/new-file.ts + +++ b/new-file.ts @@ -1,3 +1,8 @@ - +ping + +foo @@ -34,33 +39,4 @@ - -pong + -bar diff --git a/old-file.jsx b/old-file.jsx index 57d5de75c..808d8ba37 100644 --- a/old-file.jsx @@ -133,10 +115,10 @@ describe('filterDiffByFilePath()', (): void => { `); }); - it('should return the right diff for a file name negation matcher', (): void => { + it('should return the right diff for a multiple file extension (OR) matcher', (): void => { const actualResult = filterDiffByFilePath( testFileDiff, - '^(?!.*old-file.js$).*.[a-zA-Z]+$', + /^(\.\/)*old-file\.(js|ts|jsx)$/u, // Exclude files named old-file that have js, ts, or jsx extensions ); expect(actualResult).toMatchInlineSnapshot(` @@ -147,15 +129,25 @@ describe('filterDiffByFilePath()', (): void => { @@ -1,3 +1,8 @@ +foo @@ -34,33 +39,4 @@ - -bar - diff --git a/old-file.jsx b/old-file.jsx - index 57d5de75c..808d8ba37 100644 - --- a/old-file.jsx - +++ b/old-file.jsx - @@ -1,3 +1,8 @@ - +yin - @@ -34,33 +39,4 @@ - -yang" + -bar" `); }); + + it('should return the right diff for a file name negation matcher', (): void => { + const actualResult = filterDiffByFilePath( + testFileDiff, + /^(?!.*old-file\.js$).*\.[a-zA-Z]+$/u, // Exclude files that do not end with old-file.js but include all other file extensions + ); + + expect(actualResult).toMatchInlineSnapshot(` + "diff --git a/old-file.js b/old-file.js + index 57d5de75c..808d8ba37 100644 + --- a/old-file.js + +++ b/old-file.js + @@ -1,3 +1,8 @@ + +ping + @@ -34,33 +39,4 @@ + -pong" + `); + }); }); diff --git a/development/fitness-functions/common/shared.ts b/development/fitness-functions/common/shared.ts index f7f22101378d..e96073ab5b27 100644 --- a/development/fitness-functions/common/shared.ts +++ b/development/fitness-functions/common/shared.ts @@ -1,11 +1,11 @@ -function filterDiffByFilePath(diff: string, regex: string): string { +function filterDiffByFilePath(diff: string, regex: RegExp): string { // split by `diff --git` and remove the first element which is empty const diffBlocks = diff.split(`diff --git`).slice(1); const filteredDiff = diffBlocks .map((block) => block.trim()) .filter((block) => { - let didAPathInBlockMatchRegEx = false; + let shouldCheckBlock = false; block // get the first line of the block which has the paths @@ -18,12 +18,13 @@ function filterDiffByFilePath(diff: string, regex: string): string { // if at least one of the two paths matches the regex, filter the // corresponding diff block in .forEach((path) => { - if (new RegExp(regex, 'u').test(path)) { - didAPathInBlockMatchRegEx = true; + if (!regex.test(path)) { + // Not excluded, include in check + shouldCheckBlock = true; } }); - return didAPathInBlockMatchRegEx; + return shouldCheckBlock; }) // prepend `git --diff` to each block .map((block) => `diff --git ${block}`) @@ -32,6 +33,34 @@ function filterDiffByFilePath(diff: string, regex: string): string { return filteredDiff; } +function restrictedFilePresent(diff: string, regex: RegExp): boolean { + // split by `diff --git` and remove the first element which is empty + const diffBlocks = diff.split(`diff --git`).slice(1); + let jsOrJsxFilePresent = false; + diffBlocks + .map((block) => block.trim()) + .filter((block) => { + block + // get the first line of the block which has the paths + .split('\n')[0] + .trim() + // split the two paths + .split(' ') + // remove `a/` and `b/` from the paths + .map((path) => path.substring(2)) + // if at least one of the two paths matches the regex, filter the + // corresponding diff block in + .forEach((path) => { + if (regex.test(path)) { + // Not excluded, include in check + jsOrJsxFilePresent = true; + } + }); + return jsOrJsxFilePresent; + }); + return jsOrJsxFilePresent; +} + // This function returns all lines that are additions to files that are being // modified but that previously already existed. Example: // diff --git a/test.js b/test.js @@ -44,7 +73,9 @@ function filterDiffLineAdditions(diff: string): string { const diffLines = diff.split('\n'); const diffAdditionLines = diffLines.filter((line) => { - const isAdditionLine = line.startsWith('+') && !line.startsWith('+++'); + const trimmedLine = line.trim(); + const isAdditionLine = + trimmedLine.startsWith('+') && !trimmedLine.startsWith('+++'); return isAdditionLine; }); @@ -108,6 +139,7 @@ function hasNumberOfCodeBlocksIncreased( export { filterDiffByFilePath, + restrictedFilePresent, filterDiffFileCreations, filterDiffLineAdditions, hasNumberOfCodeBlocksIncreased, diff --git a/development/fitness-functions/rules/index.ts b/development/fitness-functions/rules/index.ts index cd74d286093d..6ba0f1198684 100644 --- a/development/fitness-functions/rules/index.ts +++ b/development/fitness-functions/rules/index.ts @@ -5,23 +5,25 @@ const RULES: IRule[] = [ { name: "Don't use `sinon` or `assert` in unit tests", fn: preventSinonAssertSyntax, - docURL: - 'https://github.com/MetaMask/metamask-extension/blob/develop/docs/testing.md#favor-jest-instead-of-mocha', + errorMessage: + '`sinon` or `assert` was detected in the diff. Please use Jest instead. For more info: https://github.com/MetaMask/metamask-extension/blob/develop/docs/testing.md#favor-jest-instead-of-mocha', }, { - name: "Don't add JS or JSX files to the `shared` directory", + name: "Don't add JS or JSX files", fn: preventJavaScriptFileAdditions, + errorMessage: + 'The diff includes a newly created JS or JSX file. Please use TS or TSX instead.', }, ]; type IRule = { name: string; fn: (diff: string) => boolean; - docURL?: string; + errorMessage: string; }; function runFitnessFunctionRule(rule: IRule, diff: string): void { - const { name, fn, docURL } = rule; + const { name, fn, errorMessage } = rule; console.log(`Checking rule "${name}"...`); const hasRulePassed: boolean = fn(diff) as boolean; @@ -29,11 +31,7 @@ function runFitnessFunctionRule(rule: IRule, diff: string): void { console.log(`...OK`); } else { console.log(`...FAILED. Changes not accepted by the fitness function.`); - - if (docURL) { - console.log(`For more info: ${docURL}.`); - } - + console.log(errorMessage); process.exit(1); } } diff --git a/development/fitness-functions/rules/javascript-additions.test.ts b/development/fitness-functions/rules/javascript-additions.test.ts index db1803c1d9af..f1ae6e378e37 100644 --- a/development/fitness-functions/rules/javascript-additions.test.ts +++ b/development/fitness-functions/rules/javascript-additions.test.ts @@ -13,11 +13,11 @@ describe('preventJavaScriptFileAdditions()', (): void => { expect(hasRulePassed).toBe(true); }); - it('should pass when receiving a diff with a new TS file on the shared folder', (): void => { + it('should pass when receiving a diff with a new TS file folder', (): void => { const testDiff = [ generateModifyFilesDiff('new-file.ts', 'foo', 'bar'), generateModifyFilesDiff('old-file.js', undefined, 'pong'), - generateCreateFileDiff('shared/test.ts', 'yada yada yada yada'), + generateCreateFileDiff('app/test.ts', 'yada yada yada yada'), ].join(''); const hasRulePassed = preventJavaScriptFileAdditions(testDiff); @@ -25,11 +25,11 @@ describe('preventJavaScriptFileAdditions()', (): void => { expect(hasRulePassed).toBe(true); }); - it('should not pass when receiving a diff with a new JS file on the shared folder', (): void => { + it('should not pass when receiving a diff with a new JS file', (): void => { const testDiff = [ generateModifyFilesDiff('new-file.ts', 'foo', 'bar'), generateModifyFilesDiff('old-file.js', undefined, 'pong'), - generateCreateFileDiff('shared/test.js', 'yada yada yada yada'), + generateCreateFileDiff('app/test.js', 'yada yada yada yada'), ].join(''); const hasRulePassed = preventJavaScriptFileAdditions(testDiff); @@ -37,11 +37,11 @@ describe('preventJavaScriptFileAdditions()', (): void => { expect(hasRulePassed).toBe(false); }); - it('should not pass when receiving a diff with a new JSX file on the shared folder', (): void => { + it('should not pass when receiving a diff with a new JSX file', (): void => { const testDiff = [ generateModifyFilesDiff('new-file.ts', 'foo', 'bar'), generateModifyFilesDiff('old-file.js', undefined, 'pong'), - generateCreateFileDiff('shared/test.jsx', 'yada yada yada yada'), + generateCreateFileDiff('app/test.jsx', 'yada yada yada yada'), ].join(''); const hasRulePassed = preventJavaScriptFileAdditions(testDiff); diff --git a/development/fitness-functions/rules/javascript-additions.ts b/development/fitness-functions/rules/javascript-additions.ts index 3e3705ea30f0..0d7c39b07110 100644 --- a/development/fitness-functions/rules/javascript-additions.ts +++ b/development/fitness-functions/rules/javascript-additions.ts @@ -1,15 +1,12 @@ -import { SHARED_FOLDER_JS_REGEX } from '../common/constants'; +import { JS_REGEX } from '../common/constants'; import { - filterDiffByFilePath, filterDiffFileCreations, + restrictedFilePresent, } from '../common/shared'; function preventJavaScriptFileAdditions(diff: string): boolean { - const sharedFolderDiff = filterDiffByFilePath(diff, SHARED_FOLDER_JS_REGEX); - const sharedFolderCreationDiff = filterDiffFileCreations(sharedFolderDiff); - - const hasCreatedAtLeastOneJSFileInShared = sharedFolderCreationDiff !== ''; - if (hasCreatedAtLeastOneJSFileInShared) { + const diffAdditions = filterDiffFileCreations(diff); + if (restrictedFilePresent(diffAdditions, JS_REGEX)) { return false; } return true; diff --git a/development/fitness-functions/rules/sinon-assert-syntax.ts b/development/fitness-functions/rules/sinon-assert-syntax.ts index 2cc56ec37762..a40c0768ad06 100644 --- a/development/fitness-functions/rules/sinon-assert-syntax.ts +++ b/development/fitness-functions/rules/sinon-assert-syntax.ts @@ -1,4 +1,4 @@ -import { EXCLUDE_E2E_TESTS_REGEX } from '../common/constants'; +import { E2E_TESTS_REGEX } from '../common/constants'; import { filterDiffByFilePath, filterDiffFileCreations, @@ -15,7 +15,7 @@ const codeBlocks = [ ]; function preventSinonAssertSyntax(diff: string): boolean { - const diffByFilePath = filterDiffByFilePath(diff, EXCLUDE_E2E_TESTS_REGEX); + const diffByFilePath = filterDiffByFilePath(diff, E2E_TESTS_REGEX); const diffAdditions = filterDiffFileCreations(diffByFilePath); const hashmap = hasNumberOfCodeBlocksIncreased(diffAdditions, codeBlocks);