Fix E2E locally

[mokagio]

Related to #37. This is a cherry pick of the work from #76 but without the CI part.

I'm not sure how to debug the CI failures yet, so I thought I'd get these fixes reviewed and landed in the meantime.

cc @Automattic/apps-quality-team. Has any of you ever worked with Playwright? It's pretty cool and similar to the kind of syntax sugar we tried to add on top of XCTest with XCUITestHelpers. It was also good to see these tests used the page object pattern, like what we tried to encourage on the mobile apps and with ScreenObject

Proposed Changes

Updates the E2E tests to work locally.

Testing Instructions

Run npm install && npm run e2e, it should pass:

Pre-merge Checklist

• Have you checked for TypeScript, React or other console errors? – N.A.

[jostnes]

cc @/Automattic/apps-quality-team. Has any of you ever worked with Playwright?

👋 @mokagio! Coincidentally, I am working on Playwright project right now 😄 Not an expert but have spent enough time on it to hopefully help if you have questions

I'm not sure how to debug the CI failures yet

I'm not sure if you have this set already, one way to debug in CI is to record the test execution in CI (or as Playwright calls it, a trace), return it as an artifact on the build and analyze it with trace viewer. It's very similar to the .xcresult file on a XCUITest. This feature can be updated in the playwright.config.ts file

[mokagio]

@jostnes legend!

I configured traces in this PR and made CI store them as artifacts but I didn't know how to open them 🤦‍♂️

---

Update for future reference, as the logs show, you can also open Playwright traces locally with npx playwright show-trace path-to-trace.zip

[mokagio]

🤔

The trace doesn't show the application window...

This is something I also noticed while using the UI mode (npm run e2e -- --ui). Maybe it has to do with Electron.

[jostnes]

Hmm I've not used it for testing an Electron app yet but there's this in the docs:

Known issues:
If you are not able to launch Electron and it will end up in timeouts during launch, try the following:

Ensure that nodeCliInspect ([FuseV1Options.EnableNodeCliInspectArguments](https://www.electronjs.org/docs/latest/tutorial/fuses#nodecliinspect)) fuse is not set to false.

the blank screen looks like the launch might have failed?

[mokagio]

Ping @Automattic/yolo .

From the lack of action in this PR I wonder how much E2E tests are currently used.

I am a strong testing advocate and would usually argue for more testing rather than less. But... E2E, UI-driven tests are notoriously temperamental and often flaky. If the team is not ready so maintain them, then I would suggest switching them off.

My preferred outcome would be for these to be looked at, for a way to fix them in CI to be identified, and for E2E to become a core part of the feedback loop. But if we're not at the stage in the project where we can focus on that, then I don't see the point in dragging this along.

Counter argument: The longer you wait, the harder it gets to set them up because of app complexity and non-test-friendly design inevitably sneaking in.

Counter-counter argument: If UI and UX are in flux, then one will spend a lot of time updating E2E page objects; sometimes is better to wait till UI/UX is established.

[wojtekn]

I am a strong testing advocate and would usually argue for more testing rather than less. But... E2E, UI-driven tests are notoriously temperamental and often flaky. If the team is not ready so maintain them, then I would suggest switching them off.

I think having at least an E2E testing framework with basic tests that run and succeed is beneficial. Thus, let's maintain what we have and add more tests for critical paths when time allows.

Regarding current diff, I ran tests on my local and the delete site failed:

  1) sites.test.ts:129:6 › Servers › delete site ───────────────────────────────────────────────────
    Error: Timed out 5000ms waiting for expect(locator).toBeVisible()
    Locator: getByRole('dialog', { name: 'Delete test' })
    Expected: visible
    Received: hidden

It may be related to the recent change, #27, where we removed the custom dialog for deleting site confirmation and used the system dialog instead.

[mokagio]

@wojtekn

think having at least an E2E testing framework with basic tests that run and succeed is beneficial. Thus, let's maintain what we have and add more tests for critical paths when time allows.

Sounds good!

Regarding current diff, I ran tests on my local and the delete site failed. [...] It may be related to the recent change, #27

I was aware of #27 and even rebased on top of a version of trunk that included it. Clearly I missed rebuilding my local package.

I can reproduce the failure with npm install && npm run package && npm run e2e.

This is a tangent, but I wonder if there's a way to check that the artifact on which we run the E2E tests is up to date?

Running package before every e2e run is time consuming. But I can see myself forgetting about the need to keep everything up to date, or assuming everything already is, in the future.

[wojtekn]

@mokagio I checked it more, and it seems that we can't currently use Playwright to test native dialogs opened by Electron (microsoft/playwright#21432).

I disabled this test for now, and the remaining E2E tests are passing. However, as we don't know when we could make it work, does it make more sense to delete that test instead?

[mokagio]

Thanks @wojtekn !

However, as we don't know when we could make it work, does it make more sense to delete that test instead?

Up to you.

I like that Playwright (or the underlying test runner) has a fixme option which lets us keep the test in the logs so it's harder to forget about it:

I would keep it there.

---

For reference, I had a go at implementing a check for process.env.E2E.

--- a/src/components/delete-site.tsx
+++ b/src/components/delete-site.tsx
@@ -29,6 +29,14 @@ const DeleteSite = () => {

                const trimmedSiteTitle = getTrimmedSiteTitle( selectedSite.name );

+               if ( process.env.E2E ) {
+                       try {
+                               await deleteSite(selectedSite.id, true);
+                       } catch (error) {
+                               await showErrorMessageBox(error, trimmedSiteTitle);
+                       }
+                       return;
+               }
+
                const { response, checkboxChecked } = await getIpcApi().showMessageBox( {
                        type: 'warning',
                        message: sprintf( __( 'Delete %s' ), trimmedSiteTitle ),

Unfortunately, I learned from it that process is not available in a .tsx file. There are ways around that as far as I can see between Google and ChatGPT via some kind of injection, but they are above my skill level with TypeScript and the time available to investigate.

Another idea I had, inspired by something I read in a comment in one Playwright-adjacent project which unfortunately I didn't track down at the time. would be to introduce a mock for the system dialog for the context of the E2E tests.

What I'm thinking is something we could call in place of or internally to await getIpcApi().showMessageBox( ... ) that bypasses the native dialog and returns something that E2E tests can easily interact with.

[wojtekn]

@mokagio thanks for trying that and for sharing some ideas.

I will merge this one to let us proceed with other E2E fixes. I also added https://github.com/Automattic/dotcom-forge/issues/7236 to tackle delete site test issue.