Skip to content

Commit

Permalink
feat: Create a quality gate for typescript coverage (#27717)
Browse files Browse the repository at this point in the history
<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**

<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?
-->

[![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: MetaMask/MetaMask-planning#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.
  • Loading branch information
itsyoboieltr authored Oct 14, 2024
1 parent cedabc6 commit 59dc0cd
Show file tree
Hide file tree
Showing 9 changed files with 154 additions and 127 deletions.
6 changes: 4 additions & 2 deletions .github/workflows/fitness-functions.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
86 changes: 45 additions & 41 deletions development/fitness-functions/common/constants.test.ts
Original file line number Diff line number Diff line change
@@ -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',
Expand All @@ -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);
});
});
Expand All @@ -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 = [
Expand All @@ -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);
});
});
Expand All @@ -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);
});
});
Expand Down
16 changes: 9 additions & 7 deletions development/fitness-functions/common/constants.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,17 @@
// 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',
PRE_COMMIT_HOOK = 'pre-commit-hook',
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 };
84 changes: 38 additions & 46 deletions development/fitness-functions/common/shared.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,23 +30,23 @@ 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"
`);
});
});

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(
Expand All @@ -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(`
Expand All @@ -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
Expand All @@ -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(`
Expand All @@ -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"
`);
});
});
44 changes: 38 additions & 6 deletions development/fitness-functions/common/shared.ts
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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}`)
Expand All @@ -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
Expand All @@ -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;
});
Expand Down Expand Up @@ -108,6 +139,7 @@ function hasNumberOfCodeBlocksIncreased(

export {
filterDiffByFilePath,
restrictedFilePresent,
filterDiffFileCreations,
filterDiffLineAdditions,
hasNumberOfCodeBlocksIncreased,
Expand Down
Loading

0 comments on commit 59dc0cd

Please sign in to comment.