chore: validate page object usage in file changes on every PR #29915

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

Description

This PR adds a github action job, where it checks the changed 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 a small change: 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
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

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

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

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

metamaskbot · 2025-01-28T13:25:20Z

Builds ready [ba02294]

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 (1652 ± 78 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	1446	2040	1650	159	76
		domContentLoaded	1432	1960	1629	146	70
		load	1448	2041	1652	162	78
		domInteractive	24	97	41	20	10
		backgroundConnect	9	82	26	23	11
		firstReactRender	15	99	49	29	14
		getState	4	57	10	11	5
		initialActions	0	44	2	10	5
		loadScripts	996	1447	1180	124	60
		setupStore	7	92	26	27	13
		uiStartup	1650	2785	1927	268	129

Bundle size diffs

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

.github/workflows/validate-page-object-usage.yml

DDDDDanica · 2025-01-28T13:52:48Z

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

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


Can we use include '/page-objects' so we don't need to worry about path ?

oh yeahh, definitely better! I've updated the filter 🙏

DDDDDanica · 2025-01-28T13:53:44Z

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

+                                    content.includes("import") && content.includes("from '../../page-objects/");
+
+        if (!usesPageObjectModel) {
+            console.error(`\x1b[31mFailure: You need to use Page Object Model in ${file}\x1b[0m`);


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

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

seaona added 4 commits January 28, 2025 13:11

lint

c501046

reset envars again

2adb681

env

a65e2eb

smaller sleep

ba02294

seaona changed the title ~~chore: validate page object usage~~ chore: validate page object usage in every PR Jan 28, 2025

seaona changed the title ~~chore: validate page object usage in every PR~~ chore: validate page object usage in file changes on every PR Jan 28, 2025

DDDDDanica reviewed Jan 28, 2025

View reviewed changes

.github/workflows/validate-page-object-usage.yml Outdated Show resolved Hide resolved

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: validate page object usage in file changes on every PR #29915

chore: validate page object usage in file changes 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

metamaskbot commented Jan 28, 2025

DDDDDanica Jan 28, 2025

seaona Jan 28, 2025

DDDDDanica 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

chore: validate page object usage in file changes on every PR #29915

Are you sure you want to change the base?

chore: validate page object usage in file changes 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

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

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

seaona commented Jan 27, 2025 •

edited

Loading

seaona Jan 28, 2025 •

edited

Loading

seaona Jan 29, 2025 •

edited

Loading