chore: validate page object usage in new spec files on every PR #29915

seaona · 2025-01-27T10:07:17Z

Description

This PR adds a github action job, where it checks the new files from a PR, and validates if the e2e spec files are using page object model (by checking the imports of the file).
If not, the job will fail.

The goal is to help bumping the adoption of the page object model in all e2e, currently at ~34%.

How

Re-using the get-changed-files-with-git-diff job which checks the file differences, with 2 changes
- now we store the results in artifacts, given that this job happens in Circle ci, but the present work is done in a github action (because we want to migrate everything in github actions). So if we save that as an artifact we can access it from the github action
- now we also get the status of the file (A/M/D) to distinguish between new and changed files

Re-using the existing function for filtering the e2e spec files
Using a Typescript file for the github action script, as it will be easier to re-use util functions in the future, if needed. Also it's likely more familiar than bash for most of the people

We are going to make this job required right away, as it applies to new spec files only
After some time, when page objects are more adopted, we are going to apply to both new and changed files - to see if Required or not
Eventually this should apply to new and changed files, and the job should be always required

Related issues

Fixes: https://github.com/MetaMask/MetaMask-planning/issues/4053

Manual testing steps

For testing github actions

Check github action test with a spec not using POM and another one using it https://github.com/MetaMask/metamask-extension/actions/runs/13007720258/job/36278205346?pr=29915
Alternatively, create a copy from this branch and change a couple of spec files (using and not using page objects). Then check the github action
Functionality of git diff and e2e quality gate is preserved (check this job)

For testing the local script

Create a local branch
Change a spec using page object model
Change another one not using it
Run yarn validate-e2e-page-object-usage
See results

Screenshots/Recordings

Github action result failure

Github action success

https://github.com/MetaMask/metamask-extension/actions/runs/13135918521/job/36651161856?pr=29915

Local run

Pre-merge author checklist

I've followed MetaMask Contributor Docs and MetaMask Extension Coding Standards.
I've completed the PR template to the best of my ability
I’ve included tests if applicable
I’ve documented my code using JSDoc format if applicable
I’ve applied the right labels on the PR (see labeling guidelines). 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-actions · 2025-01-27T10:07:32Z

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

.github/workflows/utils/utils.ts

.github/scripts/utils/utils.ts

seaona · 2025-01-28T10:31:20Z

.circleci/config.yml

@@ -379,6 +379,9 @@ jobs:
          root: .
          paths:
            - changed-files
+      - store_artifacts:
+          path: changed-files
+          destination: changed-files


before, we were just reading from the files, as the jobs that needed this were all in circle ci.
Now we need to store the results as an artifact, because we need to access the result from github actions too.

seaona · 2025-01-28T10:32:08Z

.circleci/scripts/test-run-e2e-timeout-minutes.ts


 fetchManifestFlagsFromPRAndGit().then((manifestFlags) => {
  let timeout;

  if (manifestFlags.circleci?.timeoutMinutes) {
    timeout = manifestFlags.circleci?.timeoutMinutes;
  } else {
-    const changedOrNewTests = filterE2eChangedFiles();
+    const changedFiles = readChangedFiles();
+    const changedOrNewTests = filterE2eChangedFiles(changedFiles);


this is due to a change in the filterE2eChangedFiles as now it accepts the files in the func argument, so it can also be re-used in the github action job

seaona · 2025-01-28T10:32:40Z

.github/scripts/utils/utils.ts

+    console.log('Owner', owner);
+    console.log('Repository', repository);
+    console.log('url', `https://circleci.com/api/v2/project/gh/${owner}/${repository}/pipeline?branch=${branch}`);
+


leaving the console logs intentionally to make debugging easier in case it's needed

DDDDDanica · 2025-01-28T13:54:24Z

test/e2e/changedFilesUtil.js

@@ -29,10 +29,10 @@ function readChangedFiles() {
 /**
 * Filters the list of changed files to include only E2E test files within the 'test/e2e/' directory.
 *
- * @returns {<string[]>} An array of filtered E2E test file paths.
+ * @param {string[]} changedFiles - An array of changed file paths to filter.


Nit: can we rename to changedFilesPaths to reflect the param more?

make sense, thank you!!

Co-authored-by: Danica Shen <[email protected]>

metamaskbot · 2025-01-28T16:21:20Z

Builds ready [4702302]

builds: chrome, firefox
builds (flask): chrome, firefox
builds (MMI): chrome
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Bundle Size Stats
mv2: E2e Actions Stats
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- content-script: 0
- offscreen: 0

Page Load Metrics (1863 ± 89 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	306	2338	1730	507	243
		domContentLoaded	1559	2254	1852	180	87
		load	1563	2280	1863	185	89
		domInteractive	26	103	44	23	11
		backgroundConnect	8	46	17	11	5
		firstReactRender	16	98	54	28	13
		getState	5	73	19	21	10
		initialActions	0	1	0	0	0
		loadScripts	1063	1692	1339	159	77
		setupStore	8	61	21	20	9
		uiStartup	1824	2672	2203	256	123

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

HowardBraham · 2025-01-28T19:18:13Z

.github/scripts/utils/utils.ts

How about calling this file .github/scripts/shared/circle-artifacts.ts?

Also, the file .devcontainer/download-builds.ts is doing extremely similar things. Can we make them both run the same shared code?

Thank you, that's a great point 🔥 pushing some changes and will ask for re-review 🙇‍♀️

HowardBraham · 2025-01-28T19:21:08Z

test/e2e/changedFilesUtil.js

If you're up for it, this looks like a great PR to migrate this file to TS. If you get into some really sticky situation, don't worry about it, but I think it might be straightforward.

👋 @HowardBraham thanks for the comment! So I changed it to TS but then I fond that we also need to change the run-all.js and possibly a couple of more files? So I think we could leave it out of scope of this PR, but let me know if you think otherwise 🙏

Yeah I was slightly scared of that, okay leave it alone 🙂

metamaskbot · 2025-01-29T17:11:46Z

Builds ready [41427bd]

builds: chrome, firefox
builds (flask): chrome, firefox
builds (MMI): chrome
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Bundle Size Stats
mv2: E2e Actions Stats
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- content-script: 0
- offscreen: 0

Page Load Metrics (1877 ± 95 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	415	2173	1803	371	178
		domContentLoaded	1546	2154	1838	181	87
		load	1554	2176	1877	197	95
		domInteractive	27	87	47	20	10
		backgroundConnect	10	86	43	27	13
		firstReactRender	16	98	37	25	12
		getState	5	58	15	15	7
		initialActions	0	1	0	0	0
		loadScripts	1090	1617	1347	162	78
		setupStore	8	58	22	19	9
		uiStartup	1789	2612	2134	227	109

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

seaona · 2025-01-29T17:18:56Z

.devcontainer/download-builds.ts

-
-  return jobs;
-}
-


ℹ️ this logic is now in circle-artifacts file, as it's shared with the validate-page-object scriptl, as per @HowardBraham 's recommendation

.devcontainer/download-builds.ts

.github/scripts/shared/circle-artifacts.ts

Co-authored-by: Howard Braham <[email protected]>

metamaskbot · 2025-01-30T12:33:46Z

Builds ready [9e15a13]

builds: chrome, firefox
builds (flask): chrome, firefox
builds (MMI): chrome
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Bundle Size Stats
mv2: E2e Actions Stats
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- content-script: 0
- offscreen: 0

Page Load Metrics (1800 ± 70 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	309	2165	1721	355	170
		domContentLoaded	1517	2120	1780	137	66
		load	1526	2166	1800	146	70
		domInteractive	25	141	49	30	14
		backgroundConnect	9	65	25	18	9
		firstReactRender	17	103	53	29	14
		getState	4	11	7	2	1
		initialActions	0	44	2	10	5
		loadScripts	1092	1660	1301	139	67
		setupStore	8	82	25	24	12
		uiStartup	1809	2585	2092	201	96

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

seaona · 2025-01-30T13:41:04Z

ℹ️ One last thing left to update, based on the platform team feedback: apply the rule only to new files (not changed) -- I'll work in this at the end of the day today and tmrr morning

metamaskbot · 2025-02-04T12:49:00Z

Builds ready [3bd0661]

builds: chrome, firefox
builds (flask): chrome, firefox
builds (MMI): chrome
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Bundle Size Stats
mv2: E2e Actions Stats
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- content-script: 0
- offscreen: 0

Page Load Metrics (1806 ± 103 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	340	2285	1726	378	182
		domContentLoaded	1485	2243	1779	210	101
		load	1490	2253	1806	215	103
		domInteractive	25	111	50	27	13
		backgroundConnect	9	83	30	23	11
		firstReactRender	15	102	51	28	13
		getState	4	52	16	14	7
		initialActions	0	1	0	0	0
		loadScripts	1035	1704	1292	178	85
		setupStore	7	84	16	19	9
		uiStartup	1719	2589	2089	272	131

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

seaona · 2025-02-04T13:18:33Z

.github/scripts/validate-e2e-page-object-usage.ts

+}
+
+// Run the verification for new files only
+verifyE2ePageObjectsUsage('A').catch((error) => {


in the future this can be changed, to 'both' whenever we want to verify both Added and Modified files

seaona · 2025-02-04T13:20:13Z

.github/scripts/validate-e2e-page-object-usage.ts

+        try {
+            const content = fs.readFileSync(file, 'utf8');
+            // Check for the presence of page object imports
+            const usesPageObjectModel = content.includes('./page-objects/');


this has an underlying assumption that all specs that use POM will have at least 1 page-objects import, and that those who don't, won't have any.
This could be 'tricked' or not apply in some cases but it's hard to tell when would this happen

agree, we can keep it like this for now and adjust it in the future if any exceptions arise

metamaskbot · 2025-02-04T13:44:36Z

Builds ready [fdea90e]

builds: chrome, firefox
builds (flask): chrome, firefox
builds (MMI): chrome
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Bundle Size Stats
mv2: E2e Actions Stats
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- content-script: 0
- offscreen: 0

Page Load Metrics (1477 ± 33 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	1370	1612	1480	67	32
		domContentLoaded	1364	1604	1461	65	31
		load	1368	1612	1477	69	33
		domInteractive	23	69	34	16	8
		backgroundConnect	8	58	21	18	9
		firstReactRender	14	96	42	28	14
		getState	4	9	5	2	1
		initialActions	0	1	0	0	0
		loadScripts	934	1126	1038	48	23
		setupStore	7	83	14	18	8
		uiStartup	1582	2174	1732	145	70

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

base work for pom validation

5ea6f0f

metamaskbot added the team-qa QA team label Jan 27, 2025

yarn version

d11f3c4

github-advanced-security bot found potential problems Jan 27, 2025

View reviewed changes

.github/workflows/utils/utils.ts Fixed Show fixed Hide fixed

seaona added 4 commits January 27, 2025 11:17

changed files as artifacts

0383d86

wait for circle ci

7c39992

some debugging

c209015

envars

451fd5d

github-advanced-security bot found potential problems Jan 27, 2025

View reviewed changes

.github/scripts/utils/utils.ts Fixed Show fixed Hide fixed

seaona added 11 commits January 27, 2025 15:09

fetch calls

b450b65

rmv wait

04c7dd4

fix script

8a9f7c1

run

ccabc5e

add env

62f8058

error handling and wait time

575b28d

fix url with envar

c8d27ab

wait

8910bc8

download buffer

725fab7

directory checks

de4975b

remove tests and clean up

da7acf5

seaona self-assigned this Jan 28, 2025

seaona added the team-extension-platform label Jan 28, 2025

seaona commented Jan 28, 2025

View reviewed changes

seaona marked this pull request as ready for review January 28, 2025 10:34

seaona requested a review from a team as a code owner January 28, 2025 10:34

seaona and others added 2 commits January 28, 2025 11:52

Merge branch 'main' into page-object-enforce

9376bf7

on workflow call

0b58c0c

DDDDDanica reviewed Jan 28, 2025

View reviewed changes

seaona and others added 3 commits January 28, 2025 16:38

review comment: update name

28a43d2

Co-authored-by: Danica Shen <[email protected]>

review: address argument name and filter includes

d96ac91

changedFilesPaths

4702302

HowardBraham reviewed Jan 28, 2025

View reviewed changes

seaona added 3 commits January 29, 2025 16:48

address more review comments

684366c

fix

3f58395

back to js

41427bd

seaona commented Jan 29, 2025

View reviewed changes

.devcontainer/download-builds.ts Outdated Show resolved Hide resolved

HowardBraham reviewed Jan 29, 2025

View reviewed changes

.github/scripts/shared/circle-artifacts.ts Outdated Show resolved Hide resolved

seaona and others added 2 commits January 30, 2025 12:56

Howard's fix .github/scripts/shared/circle-artifacts.ts

d49bd8b

Co-authored-by: Howard Braham <[email protected]>

Howard's suggestion getbranch

9e15a13

seaona added the DO-NOT-MERGE Pull requests that should not be merged label Jan 30, 2025

add file status

0792a3b

seaona changed the title ~~chore: validate page object usage in file changes on every PR~~ chore: validate page object usage in new spec files on every PR Feb 4, 2025

change workflow call for trigger

3bd0661

seaona added 2 commits February 4, 2025 13:54

test

0642693

re-add tests as check has been successful

fdea90e

seaona added area-qa Relating to QA work (Quality Assurance) and removed DO-NOT-MERGE Pull requests that should not be merged labels Feb 4, 2025

seaona commented Feb 4, 2025

View reviewed changes

DDDDDanica approved these changes Feb 4, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: validate page object usage in new spec files on every PR #29915

chore: validate page object usage in new spec files on every PR #29915

seaona commented Jan 27, 2025 •

edited

Loading

github-actions bot commented Jan 27, 2025

seaona Jan 28, 2025

seaona Jan 28, 2025 •

edited

Loading

seaona Jan 28, 2025

DDDDDanica Jan 28, 2025

seaona Jan 28, 2025

metamaskbot commented Jan 28, 2025

HowardBraham Jan 28, 2025

seaona Jan 29, 2025

HowardBraham Jan 28, 2025

seaona Jan 29, 2025 •

edited

Loading

HowardBraham Jan 29, 2025

metamaskbot commented Jan 29, 2025

seaona Jan 29, 2025

metamaskbot commented Jan 30, 2025

seaona commented Jan 30, 2025

metamaskbot commented Feb 4, 2025

seaona Feb 4, 2025

seaona Feb 4, 2025

chloeYue Feb 4, 2025

metamaskbot commented Feb 4, 2025

chore: validate page object usage in new spec files on every PR #29915

Are you sure you want to change the base?

chore: validate page object usage in new spec files on every PR #29915

Conversation

seaona commented Jan 27, 2025 • edited Loading

Description

How

Next

Related issues

Manual testing steps

Screenshots/Recordings

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Jan 27, 2025

Choose a reason for hiding this comment

seaona Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metamaskbot commented Jan 28, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seaona Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metamaskbot commented Jan 29, 2025

Choose a reason for hiding this comment

metamaskbot commented Jan 30, 2025

seaona commented Jan 30, 2025

metamaskbot commented Feb 4, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metamaskbot commented Feb 4, 2025

seaona commented Jan 27, 2025 •

edited

Loading

seaona Jan 28, 2025 •

edited

Loading

seaona Jan 29, 2025 •

edited

Loading