Fix E2E tests on Windows #76

mokagio · 2024-05-02T01:42:12Z

Update: Rebased on top of #99. The diff is noisy because it includes the changes from #17, too.
Update2: Merged trunk to the branch and solved conflicts. Shouldn't be noisy anymore.
Update3: Merged trunk to the branch and scope of this PR changes to fixing tests for Windows

Works on my machine™

Let's see what CI does...

Proposed Changes

The PR adds E2E tests to Buildkite pipeline.

Testing Instructions

Confirm E2E tests run successfully in Buildkite

Pre-merge Checklist

Have you checked for TypeScript, React or other console errors?

mokagio · 2024-05-02T01:43:06Z

src/components/onboarding.tsx

+		<div
+			className="flex flex-row flex-grow"
+			data-testid="onboarding"
+		>


We could actually remote this, given the current code doesn't use it because I couldn't get it to work.

mokagio · 2024-05-02T01:43:25Z

e2e/sites.test.ts

 		const frontendUrl = await siteContent.frontendButton.textContent();
-		expect( frontendUrl ).toBeTruthy();
+		expect( frontendUrl ).not.toBeNull();


It might be better to make this "to not be empty"

e2e/sites.test.ts

mokagio · 2024-05-02T01:45:56Z

e2e/page-objects/onboarding.ts

+	get siteNameInput() {
+		return this.locator.getByLabel( 'Site name' );
+	}
+
+	get sitePathInput() {
+		return this.locator.getByLabel( 'Local path' );
+	}
+
+	get continueButton() {
+		return this.locator.getByRole('button', { name: 'Continue' });
+	}
+
+	private get localPathButton() {
+		return this.locator.getByTestId( 'select-path-button' );
+	}
+
+	// This usually opens an OS folder dialog, except we can't interact with it in playwrite.
+	// In tests the dialog returns the value of the E2E_OPEN_FOLDER_DIALOG environment variable.
+	async clickLocalPathButtonAndSelectFromEnv() {
+		await this.localPathButton.click();
+	}


Part of this logic is duplicated from add-site-modal.ts. I shall extract it in a site-form.ts object, but maybe in a followup PR. Very keen to get E2E back to stable in CI.

Currently failing, see #37

In #17 , 6619e75 , we duplicated the logic to run unit tests and linters on Buildkite. After running them side by side for a few days, no issues have been reported. It's now time to remove the GitHub Actions step. See #99 and #76 for the E2E counterpart.

wojtekn · 2024-06-05T07:37:54Z

Thanks @mokagio. I merged the other branch, merged trunk to this one, and changed PR title to account for its reduced scope.

fluiddot · 2024-06-05T07:43:24Z

@wojtekn @fluiddot, if that's okay with you, I'd like to merge the macOS work in the meantime. See #207.

Sounds good. Thanks @wojtekn for reviewing and merging it 🙇 .

fluiddot · 2024-06-05T09:37:58Z

I managed to reproduce the issue by downloading the binary generated in CI. However, it only fails when running E2E tests. If I open the executable and create a site, it copies all WordPress files 🙃.

After adding more logs, I realized that the problem is related to not waiting for the server files to be copied before using the app. In general, this is not a problem as by the time you interact with the app the copy operation is finished. This log entry (reference) should have happened before creating the site.

In e2e tests actions are executed as soon as the window is ready, so the combination of copy operation being a bit slower on Windows and creating a site right after the app is opened is resulting in a race condition problem. I'll solve this by moving the setupWPServerFiles call to be invoked when the app is ready but before the main window is created. This way we'll ensure that all server files are copied.

fluiddot · 2024-06-05T14:52:35Z

I managed to address the issue about the site folder but the test case delete site is still failing. As far as I've checked, there seems to be an error on that specific action. I'll keep investigating...

UPDATE: As expected, the delete action is failing with the error Error: Failed to perform delete operation.

mokagio · 2024-06-06T01:20:01Z

Great job @fluiddot !

I managed to address the issue about the site folder but the test case delete site is still failing. As far as I've checked, there seems to be an error on that specific action. I'll keep investigating...

At least it's progress 😄

mokagio · 2024-06-06T01:23:08Z

@fluiddot

I realized that the problem is related to not waiting for the server files to be copied before using the app. In general, this is not a problem as by the time you interact with the app the copy operation is finished. This log entry (reference) should have happened before creating the site.

In e2e tests actions are executed as soon as the window is ready, so the combination of copy operation being a bit slower on Windows and creating a site right after the app is opened is resulting in a race condition problem. I'll solve this by moving the setupWPServerFiles call to be invoked when the app is ready but before the main window is created. This way we'll ensure that all server files are copied.

Just for my understanding, the fix you implemented is at the app runtime level, not in the E2E tests, right?

If I understand correctly, the unusual setup in the tests brought to light a design flaw that was present but hard to run into when interacting with the app as a human. If that's the case, I'll add this to my collection of examples of how testing helps discover edge cases 😄

(If instead I have misunderstood, then that's good too as it's an occasion for me to learn more)

fluiddot · 2024-06-06T09:33:18Z

Just for my understanding, the fix you implemented is at the app runtime level, not in the E2E tests, right?

If I understand correctly, the unusual setup in the tests brought to light a design flaw that was present but hard to run into when interacting with the app as a human. If that's the case, I'll add this to my collection of examples of how testing helps discover edge cases 😄

(If instead I have misunderstood, then that's good too as it's an occasion for me to learn more)

Exactly, here's the fix I applied. The issue is more prone to happen on E2E due to the fact that UI interaction happens quickly. Still, I presume it could be reproducible if you open the app and click on the "Add Site" button right away. Additionally, if disk operations are slow in the machine, this would be also easier to experience.

fluiddot · 2024-06-06T16:49:07Z

I tried different approaches to solve the error when deleting a site but none have proven to work (reference). I tested locally and all E2E tests pass, so I'm thinking of disabling that test case temporarily on Windows. WDYT?

wojtekn · 2024-06-10T07:16:32Z

I tested locally and all E2E tests pass, so I'm thinking of disabling that test case temporarily on Windows. WDYT?

@fluiddot makes sense, let's do it. We can have a proper comment there and come back to that in the future, and still benefit from running other tests on Windows.

This test case fails when running in CI but not locally. Until we find a workaround, we'll temporarily disable it.

This is mostly needed for Windows because some operations take longer, like creating a site.

fluiddot · 2024-06-12T10:44:35Z

I continued exploring the needed workarounds to unblock this PR and managed the E2E tests to pass in a separate branch with the following changes:

Fix race condition when setting up server files: As mentioned here, the fact that you can create a site before the operation of copying the WordPress server files finishes might result in an exception. This should be uncommon as the copy operation should be fast. However, I noticed in the Windows CI machine that copying the bundled copy of WordPress can take around 30 seconds. Probably, this is potentially related to Microsoft Defender slowing down the process.
Increase E2E timeouts: Some of the actions like creating a site take longer on Windows, hence we need to increase the timeouts to avoid the failure.
Skip delete site test on Windows: As shared here, I couldn't find a solution for the delete site test case. Until we find a workaround, we'll temporarily disable it.

Since this PR is related to E2E, I'm also going to apply the following changes:

Re-enable E2E tests on Windows (seems they were disabled after updating with trunk).
Disable unit tests temporarily on Windows until we find a solution to the failures.

cc @mokagio

E2E tests are now run in Buildkite.

fluiddot · 2024-06-12T12:26:24Z

@mokagio I took the liberty to set the PR ready for review as all CI jobs are passing (816fbb9) 🎊 . Let me know if you have any thoughts/concerns about this, thanks 🙇 !

mokagio · 2024-06-17T10:22:00Z

Excellent! Thanks @fluiddot

mokagio commented May 2, 2024

View reviewed changes

e2e/sites.test.ts Outdated Show resolved Hide resolved

mokagio commented May 2, 2024

View reviewed changes

mokagio mentioned this pull request May 8, 2024

Fix E2E locally #99

Merged

1 task

mokagio force-pushed the mokagio/e2e-fix-attempt branch from 47a027b to 034f575 Compare May 9, 2024 02:46

mokagio changed the base branch from mokagio/macos-e2e-tests to mokagio/e2e-fix-locally May 9, 2024 02:47

mokagio force-pushed the mokagio/e2e-fix-attempt branch from 034f575 to 1a39796 Compare May 9, 2024 02:50

mokagio and others added 20 commits May 13, 2024 09:49

WIP

c40c4c0

Makes onboarding work!

67e982a

Make create site work

f7a476d

Make delete site work

f64fad1

All E2E tests work 🎉

e458a73

Fix E2E tearDown cleanup condition

d2c950b

Cleanup

b079a59

Add a check for consistency

1abe0e9

Only run onboarding when needed

f3595e6

Improve debugging

d3af31a

Remove useless comment

500e64d

Address lint issues

7a138c1

Run lint, unit, and E2E tests on Buildkite (issue with caching)

f82846d

Remove E2E step – It's failing and don't want to slow the unit tests

5675a1f

Add step to run E2E tests in Buildkite

eba39d5

Currently failing, see #37

Add build matrix with Windows and Mac queues for E2E tests

8953376

Call E2E with bash explicitly

e810fab

Add OS name in E2E tests label

624e32e

Add traces to E2E runs

c6c6ed2

Fix E2E matrix combinations

2e6fd2d

mokagio force-pushed the mokagio/e2e-fix-attempt branch from 1a39796 to 2e6fd2d Compare May 12, 2024 23:50

Merge branch 'trunk' into mokagio/e2e-fix-attempt

df51874

wojtekn changed the title ~~Fix E2E tests~~ Fix E2E tests on Windows Jun 5, 2024

fluiddot mentioned this pull request Jun 5, 2024

E2E tests are failing locally and in CI #37

Open

fluiddot mentioned this pull request Jun 11, 2024

Fix race condition when setting up server files #229

Merged

1 task

fluiddot added 4 commits June 12, 2024 11:57

Skip delete site test on Windows

09818dc

This test case fails when running in CI but not locally. Until we find a workaround, we'll temporarily disable it.

Increase E2E timeouts

2382be9

This is mostly needed for Windows because some operations take longer, like creating a site.

Merge branch 'trunk' into mokagio/e2e-fix-attempt

2743bbd

Re-enable E2E tests on Windows

094f67e

fluiddot added 3 commits June 12, 2024 12:46

Disable unit tests on Windows

0123348

Update skip message of Windows unit test job

816fbb9

Remove Windows E2E tests GitHub workflow

bc2f30e

E2E tests are now run in Buildkite.

fluiddot marked this pull request as ready for review June 12, 2024 12:24

fluiddot self-assigned this Jun 12, 2024

fluiddot requested review from a team June 12, 2024 12:25

fluiddot approved these changes Jun 12, 2024

View reviewed changes

mokagio merged commit c34d313 into trunk Jun 17, 2024
12 checks passed

mokagio deleted the mokagio/e2e-fix-attempt branch June 17, 2024 10:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix E2E tests on Windows #76

Fix E2E tests on Windows #76

mokagio commented May 2, 2024 •

edited by fluiddot

Loading

mokagio May 2, 2024

mokagio May 2, 2024

mokagio May 2, 2024

wojtekn commented Jun 5, 2024

fluiddot commented Jun 5, 2024

fluiddot commented Jun 5, 2024

fluiddot commented Jun 5, 2024 •

edited

Loading

mokagio commented Jun 6, 2024

mokagio commented Jun 6, 2024

fluiddot commented Jun 6, 2024

fluiddot commented Jun 6, 2024

wojtekn commented Jun 10, 2024

fluiddot commented Jun 12, 2024

fluiddot commented Jun 12, 2024

mokagio commented Jun 17, 2024

Fix E2E tests on Windows #76

Fix E2E tests on Windows #76

Conversation

mokagio commented May 2, 2024 • edited by fluiddot Loading

Proposed Changes

Testing Instructions

Pre-merge Checklist

mokagio May 2, 2024

Choose a reason for hiding this comment

mokagio May 2, 2024

Choose a reason for hiding this comment

mokagio May 2, 2024

Choose a reason for hiding this comment

wojtekn commented Jun 5, 2024

fluiddot commented Jun 5, 2024

fluiddot commented Jun 5, 2024

fluiddot commented Jun 5, 2024 • edited Loading

mokagio commented Jun 6, 2024

mokagio commented Jun 6, 2024

fluiddot commented Jun 6, 2024

fluiddot commented Jun 6, 2024

wojtekn commented Jun 10, 2024

fluiddot commented Jun 12, 2024

fluiddot commented Jun 12, 2024

mokagio commented Jun 17, 2024

mokagio commented May 2, 2024 •

edited by fluiddot

Loading

fluiddot commented Jun 5, 2024 •

edited

Loading