Migrate from Jest to Playwright #62

Gudahtt · 2023-02-16T19:09:07Z

The Jest tests have been completely replaced by e2e tests written using Playwright.

Jest is not well suited for testing the behavior of this website. Too much of the code-under-test was related to DOM/browser behavior that is not adequately mocked by Jest's jsdom environment, and there was no way to fully reset in between each test to prevent them from interfering with each other. As a result, most of the functionality was not being tested.

We should now have nearly complete coverage. The tests have been split into modules, one for each use-case.

The dependency http-server was added to serve the site locally during development, and in CI.

The README has been updated with some basic instructions, but there is still much room for improvement there.

The Playwright config was auto-generated by Playwright's setup command, with minimal changes made. The change from default were:

Set a baseURL
Uncomment Edge and Google Chrome
Set the webServer config

socket-security · 2023-02-16T19:09:46Z

Socket Security Pull Request Report

👍 No new dependency issues detected in pull request

Pull request report summary

Issue	Status
Install scripts	✅ 0 issues
Native code	✅ 0 issues
Bin script confusion	✅ 0 issues
Bin script shell injection	✅ 0 issues
Unresolved require	✅ 0 issues
Invalid package.json	✅ 0 issues
HTTP dependency	✅ 0 issues
Git dependency	✅ 0 issues
Potential typo squat	✅ 0 issues
Known Malware	✅ 0 issues
Telemetry	✅ 0 issues
Protestware/Troll package	✅ 0 issues

Bot Commands

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore foo@1.0.0 bar@2.4.2

Ignoring: playwright@1.30.0, @playwright/test@1.30.0, playwright-core@1.30.0

Powered by socket.dev

Gudahtt · 2023-02-16T20:22:29Z

@SocketSecurity ignore playwright@1.30.0
@SocketSecurity ignore @playwright/test@1.30.0
@SocketSecurity ignore playwright-core@1.30.0

The install script is legitimate, and we have it disabled anyway.

The bin confusion is a similar situation to Jest; multiple packages pointing to the same place

The Jest tests have been completely replaced by e2e tests written using Playwright. Jest is not well suited for testing the behavior of this website. Too much of the code-under-test was related to DOM/browser behavior that is not adequately mocked by Jest's `jsdom` environment, and there was no way to fully reset in between each test to prevent them from interfering with each other. As a result, most of the functionality was not being tested. We should now have nearly complete coverage. The tests have been split into modules, one for each use-case. The dependency `http-server` was added to serve the site locally during development, and in CI. The README has been updated with some basic instructions, but there is still much room for improvement there. The Playwright config was auto-generated by Playwright's setup command, with minimal changes made. The change from default were: * Set a `baseURL` * Uncomment Edge and Google Chrome * Set the `webServer` config

mcmire

Wow — was Playwright always this good?! Either way, these tests are excellent. I like how simple and readable they are. I had some questions, but I didn't see anything that is absolutely necessary to change here.

mcmire · 2023-02-17T17:09:51Z

README.md

+Before running tests using Playright, you will need to install the required browsers using this command:
+
+```
+yarn playwright install --with-deps chrome chromium firefox msedge


I kind of wish that we'd kept the yarn setup convention for this and other similar use cases, but I guess this makes sense as a one-off case. Might be something that people gloss over though. Is it worth it to figure out a way to bake it into yarn install? Or maybe we start employing postinstall again?

I kinda see this being a separate step as a feature rather than a bug. This is an arduous install process; huge download sizes and long install times, and all totally unnecessary unless you're working with e2e tests.

I've just improved these instructions: 160154a

Oh, okay. If you're working on this repo, wouldn't you want to run all tests locally, including the e2e tests? But, I see your point. Someone would probably have to read the README to understand how the e2e tests work, anyway, so that's fair to have a separate step.

Personally I've only been testing with firefox and chromium. There was an issue with installing Google Chrome that I hadn't bothered look into. I could understand contributors maybe only testing with one or two browsers, or not bothering test at all if their contribution isn't changing any test expectations.

mcmire · 2023-02-17T17:18:14Z

tests/ambiguous-block.spec.ts

+  await page.goto(`/#${querystring}`);
+
+  const popupPromise = page.waitForEvent('popup');
+  await page.getByRole('link', { name: 'report a detection problem' }).click();


This looks eerily similar to Testing Library. What great inspiration.

Yep, they directly credited testing-library in the release notes for this feature: https://github.com/microsoft/playwright/releases/tag/v1.27.0

mcmire · 2023-02-17T17:18:42Z

tests/ambiguous-block.spec.ts

+  });
+  await page.goto(`/#${querystring}`);
+
+  const popupPromise = page.waitForEvent('popup');


You wait for a popup to appear by listening to the "popup" event... how clever. Makes perfect sense to me!

mcmire · 2023-02-17T17:19:53Z

tests/bypass.spec.ts

+  });
+  await page.goto(`/#${querystring}`);
+
+  await page.locator('css=#unsafe-continue').click();


Interesting. Wonder why they accept a prefixed string rather than an object in this case. Ah, well, it's readable just the same.

mcmire · 2023-02-17T17:25:28Z

tests/helpers/stream-initialization.ts

+            targetOrigin,
+          );
+        } else if (message.data !== 'ACK') {
+          window.logCall(message.data, targetOrigin);


It looks like logCall does not exist and is not referenced anywhere else but here, so I'm curious, why define it globally? Is it possible to say logs.push({ message: message.data, targetOrigin }) here?

The function passed to addInitScript is serialized and executed in the window-under-test itself, so it wouldn't have access to the logs variable.

logCall is defined using page.exposeFunction a few lines up from here. exposeFunction basically lets you execute a function in the context of the test from the page. window.logCall ends up messaging the test-runner with the serialized parameters, and calling the callback passed to exposeFunction.

This solution was developed from an example in their documentation: https://playwright.dev/docs/mock-browser-apis#verifying-api-calls

Some comments added here to help explain this: 2355649

The function passed to addInitScript is serialized and executed in the window-under-test itself, so it wouldn't have access to the logs variable.

Got it. That was the key thing here. Thanks! I also appreciate the comments as well.

mcmire · 2023-02-17T17:27:13Z

tests/bypass.spec.ts

+
+  await expect(page).toHaveURL('https://test.com');
+  await expect(postMessageLogs.length).toBe(1);
+  await expect([postMessageLogs[0].message]).toStrictEqual([


Nit: Why wrap this in an array?

Oops! This was a mistake

Fixed here: c065dfa

mcmire · 2023-02-17T17:27:38Z

tests/helpers/default-mocks.ts

+ */
+export async function setupDefaultMocks(context) {
+  await context.route('https://github.com/**/*', (route) =>
+    route.fulfill({


What a great API. Love these names.

mcmire · 2023-02-17T17:31:14Z

tests/defaults.spec.ts

+  // on `about:blank`.
+  // We need a 1-length history so that the browser allows `window.close()` to work.
+  const baseURL = config.use?.baseURL;
+  await page.evaluate((url) => window.location.replace(url), `${baseURL}/`);


Nit: Does a URL need to be passed into this function? Wondering if it would be more clear to build it inside.

Suggested change

await page.evaluate((url) => window.location.replace(url), `${baseURL}/`);

await page.evaluate(() => window.location.replace(`${baseURL}/`));

We need to pass in the URL because this function is serialized and executed on the page itself, so it doesn't have access to locally-scoped variables like baseURL.

Ah. Same story as addInitScript from above, I guess.

mcmire · 2023-02-17T17:34:20Z

tests/extension-startup.spec.ts

+  });
+  const expectedMessage = 'Service worker registered!';
+
+  await page.goto('/#extensionStartup');


Interesting. When is #extensionStartup usually accessed? Is that something that the browser does automatically?

The extension loads this during startup, so that the phishing-warning page service worker is registered before the user hits their first warning page. This ensures it can work offline (e.g. if GitHub pages is down), so we're not as reliant on hosting for this feature.

Cool! Thanks for the explanation.

mcmire · 2023-02-17T17:42:55Z

tests/iframe.spec.ts

+  await setupDefaultMocks(context);
+});
+
+test('does not render any buttons or links', async ({ page }) => {


Curious — what code is this testing? What hides user interactions?

We have an "antiClickjack" style block that ensures the main contents of the warning page are hidden by default, including the buttons at links. This is to ensure a malicious site cannot load the phishing warning page in an iframe and trick the user into bypassing the warning page for their other malicious site.

There is a little script embedded directly in the HTML document that checks to see if this is the top frame, and if so it modifies that style block to no longer hide the page.

I'm just noticing that this script references a now-removed HTML element (#content__framed-body). That was an oversight. But it still effectively works in that it un-hides the page.

Ah, I see, it's this bit, it looks like. Makes sense!

Gudahtt force-pushed the playwright branch 3 times, most recently from 9d32ea3 to 2312b49 Compare February 16, 2023 20:15

Gudahtt force-pushed the playwright branch 5 times, most recently from dfc705c to 651afa9 Compare February 16, 2023 20:56

Base automatically changed from fix-detection-repo-initialization to main February 16, 2023 21:08

Gudahtt force-pushed the playwright branch from 651afa9 to 574d9b1 Compare February 16, 2023 21:21

Gudahtt changed the base branch from main to update-github-actions February 16, 2023 21:21

Gudahtt force-pushed the playwright branch 3 times, most recently from a5693f0 to 5616e34 Compare February 16, 2023 22:03

This comment was marked as resolved.

Sign in to view

Gudahtt mentioned this pull request Feb 16, 2023

Dynamic lookup of block source (with Playwright tests) #64

Closed

Gudahtt force-pushed the update-github-actions branch from 8b48149 to ca3a5ac Compare February 16, 2023 23:14

Base automatically changed from update-github-actions to main February 17, 2023 14:26

Gudahtt force-pushed the playwright branch 2 times, most recently from c2e6460 to 9d277c2 Compare February 17, 2023 16:44

Gudahtt marked this pull request as ready for review February 17, 2023 16:44

Gudahtt requested a review from a team as a code owner February 17, 2023 16:44

Gudahtt force-pushed the playwright branch from 9d277c2 to 998304e Compare February 17, 2023 17:50

mcmire previously approved these changes Feb 17, 2023

View reviewed changes

Gudahtt added 3 commits February 17, 2023 17:23

Add comments to setupStreamInitialization

2355649

Remove leftover console.log

aea1b1e

Remove unnecessary array

c065dfa

Gudahtt dismissed mcmire’s stale review via c065dfa February 17, 2023 20:54

Improve README instructions

160154a

Gudahtt force-pushed the playwright branch from fe64ee7 to 160154a Compare February 17, 2023 21:02

mcmire approved these changes Feb 17, 2023

View reviewed changes

Gudahtt merged commit 3d48389 into main Feb 17, 2023

Gudahtt deleted the playwright branch February 17, 2023 21:43

Gudahtt mentioned this pull request Jul 10, 2023

Implement EIP-6963 MetaMask/providers#263

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate from Jest to Playwright #62

Migrate from Jest to Playwright #62

Gudahtt commented Feb 16, 2023 •

edited

Loading

socket-security bot commented Feb 16, 2023 •

edited

Loading

Gudahtt commented Feb 16, 2023

This comment was marked as resolved.

mcmire left a comment

mcmire Feb 17, 2023

Gudahtt Feb 17, 2023 •

edited

Loading

Gudahtt Feb 17, 2023

mcmire Feb 17, 2023

Gudahtt Feb 17, 2023

mcmire Feb 17, 2023

Gudahtt Feb 17, 2023

mcmire Feb 17, 2023

mcmire Feb 17, 2023

mcmire Feb 17, 2023

Gudahtt Feb 17, 2023

Gudahtt Feb 17, 2023

mcmire Feb 17, 2023 •

edited

Loading

mcmire Feb 17, 2023

Gudahtt Feb 17, 2023

Gudahtt Feb 17, 2023

mcmire Feb 17, 2023

mcmire Feb 17, 2023

Gudahtt Feb 17, 2023

mcmire Feb 17, 2023

mcmire Feb 17, 2023

Gudahtt Feb 17, 2023

mcmire Feb 17, 2023

mcmire Feb 17, 2023

Gudahtt Feb 17, 2023

Gudahtt Feb 17, 2023

mcmire Feb 17, 2023

	await page.evaluate((url) => window.location.replace(url), `${baseURL}/`);
	await page.evaluate(() => window.location.replace(`${baseURL}/`));

Migrate from Jest to Playwright #62

Migrate from Jest to Playwright #62

Conversation

Gudahtt commented Feb 16, 2023 • edited Loading

socket-security bot commented Feb 16, 2023 • edited Loading

Gudahtt commented Feb 16, 2023

This comment was marked as resolved.

mcmire left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gudahtt Feb 17, 2023 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcmire Feb 17, 2023 • 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

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

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

Choose a reason for hiding this comment

Gudahtt commented Feb 16, 2023 •

edited

Loading

socket-security bot commented Feb 16, 2023 •

edited

Loading

Gudahtt Feb 17, 2023 •

edited

Loading

mcmire Feb 17, 2023 •

edited

Loading