Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 35 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
5ea6f0f
base work for pom validation
seaona Jan 27, 2025
d11f3c4
yarn version
seaona Jan 27, 2025
0383d86
changed files as artifacts
seaona Jan 27, 2025
7c39992
wait for circle ci
seaona Jan 27, 2025
c209015
some debugging
seaona Jan 27, 2025
451fd5d
envars
seaona Jan 27, 2025
b450b65
fetch calls
seaona Jan 27, 2025
04c7dd4
rmv wait
seaona Jan 27, 2025
8a9f7c1
fix script
seaona Jan 27, 2025
ccabc5e
run
seaona Jan 27, 2025
62f8058
add env
seaona Jan 27, 2025
575b28d
error handling and wait time
seaona Jan 27, 2025
c8d27ab
fix url with envar
seaona Jan 28, 2025
8910bc8
wait
seaona Jan 28, 2025
725fab7
download buffer
seaona Jan 28, 2025
de4975b
directory checks
seaona Jan 28, 2025
da7acf5
remove tests and clean up
seaona Jan 28, 2025
9376bf7
Merge branch 'main' into page-object-enforce
seaona Jan 28, 2025
0b58c0c
on workflow call
seaona Jan 28, 2025
c501046
lint
seaona Jan 28, 2025
2adb681
reset envars again
seaona Jan 28, 2025
a65e2eb
env
seaona Jan 28, 2025
ba02294
smaller sleep
seaona Jan 28, 2025
28a43d2
review comment: update name
seaona Jan 28, 2025
d96ac91
review: address argument name and filter includes
seaona Jan 28, 2025
4702302
changedFilesPaths
seaona Jan 28, 2025
684366c
address more review comments
seaona Jan 29, 2025
3f58395
fix
seaona Jan 29, 2025
41427bd
back to js
seaona Jan 29, 2025
d49bd8b
Howard's fix .github/scripts/shared/circle-artifacts.ts
seaona Jan 30, 2025
9e15a13
Howard's suggestion getbranch
seaona Jan 30, 2025
0792a3b
add file status
seaona Feb 4, 2025
3bd0661
change workflow call for trigger
seaona Feb 4, 2025
0642693
test
seaona Feb 4, 2025
fdea90e
re-add tests as check has been successful
seaona Feb 4, 2025
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -379,6 +379,9 @@ jobs:
root: .
paths:
- changed-files
- store_artifacts:
path: changed-files
destination: changed-files
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


validate-locales-only:
executor: node-browsers-small
Expand Down
5 changes: 3 additions & 2 deletions .circleci/scripts/test-run-e2e-timeout-minutes.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
import { fetchManifestFlagsFromPRAndGit } from '../../development/lib/get-manifest-flag';
import { filterE2eChangedFiles } from '../../test/e2e/changedFilesUtil';
import { filterE2eChangedFiles, readChangedFiles } from '../../test/e2e/changedFilesUtil';

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

if (manifestFlags.circleci?.timeoutMinutes) {
timeout = manifestFlags.circleci?.timeoutMinutes;
} else {
const changedOrNewTests = filterE2eChangedFiles();
const changedFiles = readChangedFiles();
const changedOrNewTests = filterE2eChangedFiles(changedFiles);
Copy link
Contributor Author

@seaona seaona Jan 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// 20 minutes, plus 3 minutes for every changed file, up to a maximum of 30 minutes
timeout = Math.min(20 + changedOrNewTests.length * 3, 30);
Expand Down
120 changes: 120 additions & 0 deletions .github/scripts/utils/utils.ts
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
import { execSync } from 'child_process';
import fs from 'fs';

const owner = process.env.OWNER;
const repository = process.env.REPOSITORY;

/**
* Downloads an artifact from CircleCI.
* @param branch - The branch name.
* @param headCommitHash - The commit hash of the branch.
* @param artifactName - The name of the artifact to download.
* @param outputFilePath - The path to save the downloaded artifact.
* @param jobName - The name of the job that produced the artifact.
*/
export async function downloadCircleCiArtifact(branch: string, headCommitHash: string, artifactName: string, outputFilePath: string, jobName: string): Promise<void> {
console.log('Branch', branch);
console.log('Commit', headCommitHash);
console.log('Owner', owner);
console.log('Repository', repository);
console.log('url', `https://circleci.com/api/v2/project/gh/${owner}/${repository}/pipeline?branch=${branch}`);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// Get the pipeline ID for the current branch
const pipelineResponse = await fetch(
`https://circleci.com/api/v2/project/gh/${owner}/${repository}/pipeline?branch=${branch}`
);

if (!pipelineResponse.ok) {
throw new Error(`Failed to fetch pipeline: ${pipelineResponse.statusText}`);
}

const pipelineData = await pipelineResponse.json();
console.log('Pipeline data:', JSON.stringify(pipelineData, null, 2));

if (!pipelineData.items || pipelineData.items.length === 0) {
throw new Error('No pipeline items found');
}

const pipelineId = pipelineData.items.find((item: any) => item.vcs.revision === headCommitHash)?.id;
console.log('Pipeline ID', pipelineId);

if (!pipelineId) {
throw new Error('Pipeline ID not found');
}

// Get the workflow ID for the pipeline
const workflowResponse = await fetch(`https://circleci.com/api/v2/pipeline/${pipelineId}/workflow`);
const workflowData = await workflowResponse.json();
const workflowId = workflowData.items[0]?.id;
console.log('Workflow ID', workflowId);

if (!workflowId) {
throw new Error('Workflow ID not found');
}

// Get the job details for the specific job that produces artifacts
const jobResponse = await fetch(`https://circleci.com/api/v2/workflow/${workflowId}/job`);
const jobData = await jobResponse.json();
const jobDetails = jobData.items.find((item: any) => item.name === jobName);
console.log('Job Details', jobDetails);

if (!jobDetails) {
throw new Error('Job details not found');
}

const jobNumber = jobDetails.job_number;
console.log('Job ID', jobNumber);

// Get the artifact URL
const artifactResponse = await fetch(`https://circleci.com/api/v2/project/gh/${owner}/${repository}/${jobNumber}/artifacts`);
const artifactData = await artifactResponse.json();
const artifact = artifactData.items.find((item: any) => item.path.includes(artifactName));

if (!artifact) {
throw new Error(`Artifact ${artifactName} not found`);
}

const artifactUrl = artifact.url;
console.log('Artifact URL:', artifactUrl);

// Ensure the directory exists
const dir = require('path').dirname(outputFilePath);
if (!fs.existsSync(dir)) {
fs.mkdirSync(dir, { recursive: true });
}

// Download the artifact
const artifactDownloadResponse = await fetch(artifactUrl);
if (!artifactDownloadResponse.ok) {
throw new Error(`Failed to download artifact: ${artifactDownloadResponse.statusText}`);
}
const artifactArrayBuffer = await artifactDownloadResponse.arrayBuffer();
const artifactBuffer = Buffer.from(artifactArrayBuffer);
fs.writeFileSync(outputFilePath, artifactBuffer);

if (!fs.existsSync(outputFilePath)) {
throw new Error(`Failed to download artifact to ${outputFilePath}`);
}
}

/**
* Reads the content of a file.
* @param filePath - The path to the file.
* @returns The content of the file.
*/
export function readFileContent(filePath: string): string {
if (!fs.existsSync(filePath)) {
throw new Error(`File not found: ${filePath}`);
}

const content = fs.readFileSync(filePath, 'utf-8').trim();
return content;
}

/**
* Sleep function to pause execution for a specified number of seconds.
* @param seconds - The number of seconds to sleep.
*/
export function sleep(seconds: number) {
return new Promise(resolve => setTimeout(resolve, seconds * 1000));
}
91 changes: 91 additions & 0 deletions .github/scripts/validate-e2e-page-object-usage.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
import fs from 'fs';
import { execSync } from 'child_process';
import { filterE2eChangedFiles } from '../../test/e2e/changedFilesUtil';
import { downloadCircleCiArtifact, readFileContent, sleep } from '../scripts/utils/utils';

async function verifyE2ePageObjectsUsage() {
let e2eFiles: string[];

if (process.env.GITHUB_ACTIONS) {
// Running in Github Actions
const branch = process.env.BRANCH || '';
const headCommitHash = process.env.HEAD_COMMIT_HASH || '';
const artifactName = 'changed-files.txt';
const artifactPath = 'changed-files';
const jobName = 'get-changed-files-with-git-diff';

let attempts = 0;
const maxAttempts = 3;
let changedFilesContent = '';

// Small buffer to ensure the job id is accessible in circle ci
// once we have that job migrated into github actions, we can just add a dependency rule
await sleep(120);

while (attempts < maxAttempts) {
try {
console.log(`Downloading artifact: Attempt ${attempts + 1}/${maxAttempts}`);

const outputDir = `${artifactPath}/changed-files.txt`;
await downloadCircleCiArtifact(branch, headCommitHash, artifactName, outputDir, jobName);

changedFilesContent = readFileContent(outputDir);

if (changedFilesContent) {
console.log('Artifact downloaded and read successfully.');
break;
}
} catch (error) {
console.error(`Error fetching artifact: ${error.message}`);
}

attempts++;
if (attempts < maxAttempts) {
console.log(`Retrying in 10 seconds... (${attempts}/${maxAttempts})`);
await sleep(10);
}
}

if (!changedFilesContent) {
console.error('No artifacts found for changed files. Exiting with failure.');
process.exit(1);
}

e2eFiles = filterE2eChangedFiles(changedFilesContent.split('\n').filter(file => file.trim() !== ''));
console.log('e2e changed files', e2eFiles);
} else {
// Running locally
console.log('Running locally, performing git diff against main branch...');
const diffOutput = execSync('git diff --name-only main...HEAD').toString().trim();
const changedFiles = diffOutput.split('\n').filter(file => file.trim() !== '');
console.log('Changed files:', changedFiles);

e2eFiles = filterE2eChangedFiles(changedFiles);
console.log('Filtered E2E files:', e2eFiles);
}

if (e2eFiles.length === 0) {
console.log('No E2E files to validate. Exiting successfully.');
process.exit(0);
}

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

if (!usesPageObjectModel) {
console.error(`\x1b[31mFailure: You need to use Page Object Model in ${file}\x1b[0m`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙏🏻

process.exit(1);
}
}

console.log("\x1b[32mSuccess: All the new or modified E2E files use the Page Object Model.\x1b[0m");
}

// Run the verification
verifyE2ePageObjectsUsage().catch((error) => {
console.error('Not all the modified e2e use the Page Object Model', error);
process.exit(1);
});
4 changes: 4 additions & 0 deletions .github/workflows/main.yml
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,10 @@ jobs:
name: Wait for CircleCI workflow status
uses: ./.github/workflows/wait-for-circleci-workflow-status.yml

validate-page-object-usage:
name: Verify E2E Page Object Usage on modified files
uses: ./.github/workflows/validate-page-object-usage.yml

runway:
name: Runway
# For a `pull_request` event, the branch is `github.head_ref``.
Expand Down
23 changes: 23 additions & 0 deletions .github/workflows/validate-page-object-usage.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
name: Validate E2E Page Object usage on modified files

on:
workflow_call:

jobs:
run-e2e-pom-validation:
runs-on: ubuntu-latest
steps:
- name: Checkout repository
uses: actions/checkout@v4

- name: Setup environment
uses: metamask/github-tools/.github/actions/setup-environment@main

- name: Run E2E Page Object Usage Validation
env:
OWNER: ${{ github.repository_owner }}
REPOSITORY: ${{ github.event.repository.name }}
BRANCH: ${{ github.head_ref || github.ref_name }}
HEAD_COMMIT_HASH: ${{ github.event.pull_request.head.sha || github.sha }}
run: |
yarn validate-e2e-page-object-usage
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,8 @@
"attributions:check": "./development/attributions-check.sh",
"attributions:generate": "./development/generate-attributions.sh",
"ci-rerun-from-failed": "tsx .circleci/scripts/rerun-ci-workflow-from-failed.ts",
"git-diff-default-branch": "tsx .circleci/scripts/git-diff-default-branch.ts"
"git-diff-default-branch": "tsx .circleci/scripts/git-diff-default-branch.ts",
"validate-e2e-page-object-usage": "tsx .github/scripts/validate-e2e-page-object-usage.ts"
},
"resolutions": {
"chokidar": "^3.6.0",
Expand Down
10 changes: 5 additions & 5 deletions test/e2e/changedFilesUtil.js
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@seaona seaona Jan 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👋 @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 🙏

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ const CHANGED_FILES_PATH = path.join(
/**
* Reads the list of changed files from the git diff file.
*
* @returns {<string[]>} An array of changed file paths.
* @returns {string[]} An array of changed file paths.
*/
function readChangedFiles() {
try {
Expand All @@ -29,11 +29,11 @@ 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[]} changedFilesPaths - An array of changed file paths to filter.
* @returns {string[]} An array of filtered E2E test file paths.
*/
function filterE2eChangedFiles() {
const changedFiles = readChangedFiles();
const e2eChangedFiles = changedFiles
function filterE2eChangedFiles(changedFilesPaths) {
const e2eChangedFiles = changedFilesPaths
.filter(
(file) =>
file.startsWith('test/e2e/') &&
Expand Down
8 changes: 6 additions & 2 deletions test/e2e/run-all.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,10 @@ const { hideBin } = require('yargs/helpers');
const { runInShell } = require('../../development/lib/run-command');
const { exitWithError } = require('../../development/lib/exit-with-error');
const { loadBuildTypesConfig } = require('../../development/lib/build-type');
const { filterE2eChangedFiles } = require('./changedFilesUtil');
const {
filterE2eChangedFiles,
readChangedFiles,
} = require('./changedFilesUtil');

// These tests should only be run on Flask for now.
const FLASK_ONLY_TESTS = [];
Expand Down Expand Up @@ -64,7 +67,8 @@ function applyQualityGate(fullTestList, changedOrNewTests) {

// For running E2Es in parallel in CI
function runningOnCircleCI(testPaths) {
const changedOrNewTests = filterE2eChangedFiles();
const changedFilesPaths = readChangedFiles();
const changedOrNewTests = filterE2eChangedFiles(changedFilesPaths);
console.log('Changed or new test list:', changedOrNewTests);

const fullTestList = applyQualityGate(
Expand Down
Loading