Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 4 additions & 4 deletions .github/workflows/e2e-chrome.yml
Original file line number Diff line number Diff line change
Expand Up @@ -77,14 +77,14 @@ jobs:
matrix-index: ${{ matrix.index }}
matrix-total: ${{ strategy.job-total }}

test-e2e-chrome-vault-decryption:
test-e2e-chrome-dist:
# This if statement is equivalent to IS_FORK
if: ${{ !(github.event.pull_request.head.repo.fork || github.event.repository.fork) }}
uses: ./.github/workflows/run-e2e.yml
with:
test-suite-name: test-e2e-chrome-vault-decryption
test-suite-name: test-e2e-chrome-dist
build-artifact: build-dist-browserify
test-command: yarn test:e2e:single test/e2e/vault-decryption-chrome.spec.ts --browser chrome
test-command: yarn test:e2e:chrome:dist

test-e2e-chrome-api-specs:
uses: ./.github/workflows/run-e2e.yml
Expand Down Expand Up @@ -175,7 +175,7 @@ jobs:
- test-e2e-chrome-multiple-providers
- test-e2e-chrome-rpc
- test-e2e-chrome-flask
- test-e2e-chrome-vault-decryption
- test-e2e-chrome-dist
- test-e2e-chrome-api-specs
- test-e2e-chrome-api-specs-multichain
runs-on: ubuntu-latest
Expand Down
1 change: 1 addition & 0 deletions .github/workflows/main.yml
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,7 @@ jobs:
e2e-firefox:
needs:
- needs-e2e
- build-dist-mv2-browserify
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding this happens to be a little unfortunate, because you're adding the longest build process as a needs before the longest test suite. So this definitely adds time to the workflow as a whole. Possible solutions include:

  1. Wait for webpack to completely replace browserify
  2. Add more shards to test-e2e-firefox-browserify and test-e2e-firefox-flask
  3. Take test-e2e-firefox-dist out of e2e-firefox.yml and rework how the YML files call each other and are organized, but I think that's not worth it

Edit: hmmmm and then I noticed that test-e2e-firefox-dist is running zero tests....

Copy link
Member Author

Choose a reason for hiding this comment

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

Is Firefox substantially slower than Chrome to build and/or run tests? I had thought the difference was fairly small

Copy link
Contributor

Choose a reason for hiding this comment

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

For the build step, it's the dist build that's slower. build-dist-browserify and build-dist-mv2-browserify take the same amount of time. But this needs did not have a dist build in the list before.

For the E2E tests, the test-e2e-firefox-browserify shards take 12-14 minutes, but the test-e2e-firefox-browserify take 9-11 minutes. I have not done any additional investigation as to why it's slower. If we can't make it faster, we could also increase the number of Firefox shards.

- build-test-mv2-browserify
- build-test-webpack
- build-test-flask-mv2-browserify
Expand Down
2 changes: 1 addition & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ app/.DS_Store
storybook-build/
coverage/
jest-coverage/
dist
/dist
builds*/
builds.zip
development/ts-migration-dashboard/build
Expand Down
2 changes: 2 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
"test:integration": "yarn webpack-cli build --config ./development/webpack/webpack.integration.tests.config.ts && jest --config jest.integration.config.js",
"test:integration:coverage": "yarn test:integration --coverage",
"test:e2e:chrome": "SELENIUM_BROWSER=chrome tsx test/e2e/run-all.ts",
"test:e2e:chrome:dist": "SELENIUM_BROWSER=chrome tsx test/e2e/run-all.ts --dist",
Copy link
Contributor

Choose a reason for hiding this comment

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

I generally argue for "we already have too many scripts in package.json, and if you're adding another command that really isn't intended for general use, but will just be run in one place in GitHub Actions, let's keep it out of package.json"

Copy link
Member Author

Choose a reason for hiding this comment

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

It can be run locally as well, e.g. when debugging an E2E test failure for this job. It's important to have a simple command that can replicate CI tests locally.

"test:e2e:chrome:flask": "SELENIUM_BROWSER=chrome tsx test/e2e/run-all.ts --build-type flask",
"test:api-specs": "SELENIUM_BROWSER=chrome tsx test/e2e/run-openrpc-api-test-coverage.ts",
"test:api-specs-multichain": "SELENIUM_BROWSER=chrome ts-node test/e2e/run-api-specs-multichain.ts",
Expand All @@ -62,6 +63,7 @@
"test:e2e:chrome:rpc": "SELENIUM_BROWSER=chrome tsx test/e2e/run-all.ts --rpc",
"test:e2e:chrome:multi-provider": "MULTIPROVIDER=true SELENIUM_BROWSER=chrome tsx test/e2e/run-all.ts --multi-provider",
"test:e2e:firefox": "SELENIUM_BROWSER=firefox tsx test/e2e/run-all.ts",
"test:e2e:firefox:dist": "SELENIUM_BROWSER=firefox tsx test/e2e/run-all.ts --dist",
"test:e2e:firefox:flask": "SELENIUM_BROWSER=firefox tsx test/e2e/run-all.ts --build-type flask",
"test:e2e:single": "node test/e2e/run-e2e-test.js",
"ganache:start": "./development/run-ganache.sh",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,14 @@ import os from 'os';
import path from 'path';
import fs from 'fs-extra';
import level from 'level';
import { Driver } from './webdriver/driver';
import { WALLET_PASSWORD, WINDOW_TITLES, withFixtures } from './helpers';
import HeaderNavbar from './page-objects/pages/header-navbar';
import HomePage from './page-objects/pages/home/homepage';
import PrivacySettings from './page-objects/pages/settings/privacy-settings';
import SettingsPage from './page-objects/pages/settings/settings-page';
import VaultDecryptorPage from './page-objects/pages/vault-decryptor-page';
import { completeCreateNewWalletOnboardingFlowWithCustomSettings } from './page-objects/flows/onboarding.flow';
import { Driver } from '../webdriver/driver';
import { WALLET_PASSWORD, WINDOW_TITLES, withFixtures } from '../helpers';
import HeaderNavbar from '../page-objects/pages/header-navbar';
import HomePage from '../page-objects/pages/home/homepage';
import PrivacySettings from '../page-objects/pages/settings/privacy-settings';
import SettingsPage from '../page-objects/pages/settings/settings-page';
import VaultDecryptorPage from '../page-objects/pages/vault-decryptor-page';
import { completeCreateNewWalletOnboardingFlowWithCustomSettings } from '../page-objects/flows/onboarding.flow';

const VAULT_DECRYPTOR_PAGE = 'https://metamask.github.io/vault-decryptor';

Expand Down Expand Up @@ -165,6 +165,10 @@ async function closePopoverIfPresent(driver: Driver) {

describe('Vault Decryptor Page', function () {
it('is able to decrypt the vault uploading the log file in the vault-decryptor webapp', async function () {
if (process.env.SELENIUM_BROWSER !== 'chrome') {
// TODO: Get this working on Firefox
this.skip();
}
Comment on lines +168 to +171
Copy link
Contributor

Choose a reason for hiding this comment

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

We're going to skip ALL of the tests in the dist folder on Firefox? Are more things coming here soon? Because as it is now, a VM will spin up to run test-e2e-firefox-dist, and then it will run zero tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

These changes were split from this PR, which has a test that would run on Firefox: #31435

I can certainly omit the Firefox workflow until that PR though, so it doesn't spin up a VM that does nothing. Good point.

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's coming very soon, it matters a little less.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed here: 3e45984

I'll re-add it in the PR that adds the first test

await withFixtures(
{
disableServerMochaToBackground: true,
Expand Down Expand Up @@ -223,6 +227,10 @@ describe('Vault Decryptor Page', function () {
});

it('is able to decrypt the vault pasting the text in the vault-decryptor webapp', async function () {
if (process.env.SELENIUM_BROWSER !== 'chrome') {
// TODO: Get this working on Firefox
this.skip();
}
await withFixtures(
{
disableServerMochaToBackground: true,
Expand Down
9 changes: 9 additions & 0 deletions test/e2e/run-all.ts
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,10 @@ async function main(): Promise<void> {
description: `run json-rpc specific e2e tests`,
type: 'boolean',
})
.option('dist', {
description: `run e2e tests for production-like builds`,
type: 'boolean',
})
.option('multi-provider', {
description: `run multi injected provider e2e tests`,
type: 'boolean',
Expand Down Expand Up @@ -128,6 +132,7 @@ async function main(): Promise<void> {
const {
browser,
debug,
dist,
retries,
rpc,
buildType,
Expand All @@ -137,6 +142,7 @@ async function main(): Promise<void> {
} = argv as {
browser?: 'chrome' | 'firefox';
debug?: boolean;
dist?: boolean;
retries?: number;
rpc?: boolean;
buildType?: string;
Expand All @@ -161,6 +167,9 @@ async function main(): Promise<void> {
...(await getTestPathsForTestDir(path.join(__dirname, 'flask'))),
...featureTestsOnMain,
];
} else if (dist) {
const testDir = path.join(__dirname, 'dist');
testPaths = await getTestPathsForTestDir(testDir);
} else if (rpc) {
const testDir = path.join(__dirname, 'json-rpc');
testPaths = await getTestPathsForTestDir(testDir);
Expand Down
Loading