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

Replace Cypress with Playwright #9211

Merged
merged 82 commits into from
May 28, 2024
Merged

Replace Cypress with Playwright #9211

merged 82 commits into from
May 28, 2024

Conversation

oetherington
Copy link
Collaborator

@oetherington oetherington commented May 2, 2024

This PR replaces Cypress with Playwright, and translates all the old tests.

Setup and usage instructions copied from the updated README

There is some one-time setup before you can run the playwright tests:

  • Install docker (brew install --cask docker on OSX)
  • Download the Postgres docker image with yarn playwright-db (you can Ctrl+C to exit the container once it has successfully downloaded and started)
  • Install the browsers with yarn playwright install

You can then run the tests with yarn playwright test - you can run specific tests with the -g flag.

You can also open the Playwright UI with yarn playwright test --ui, but note that unlike in CLI mode this won't automatically start the database and server so you'll also need to run yarn playwright-db and yarn start-playwright in 2 separate terminals.

For information on how to create Playwright tests see their documentation and the existing tests in the playwright directory.

The server can be accessed at localhost:3456 for debugging.

You can open the tests in a normal browser (for instance, to access the DOM inspector or debugger) with PWDEBUG=console yarn playwright test. It's strongly recommended to use the -g to only run a single test, as each test will open in a separate browser window.

┆Issue is synchronized with this Asana task by Unito

@oetherington oetherington force-pushed the playwright branch 2 times, most recently from 27bb190 to a6622a8 Compare May 20, 2024 14:50
@@ -159,7 +160,7 @@ const PostsEditForm = ({ documentId, version, classes }: {
}}
showRemove={true}
collabEditorDialogue={!!document.collabEditorDialogue}
submitLabel={isDraft ? "Publish" : "Publish Changes"}
submitLabel={preferredHeadingCase(isDraft ? "Publish" : "Publish Changes")}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This change isn't related to playwright, but seems like it should be made anyway (I noticed it while editing a test).

@@ -280,6 +280,9 @@ const PostsNewForm = ({classes}: {
if (!currentUser) {
return (<LoginForm />);
}
if (!currentUserWithModerationGuidelines) {
return <Loading/>
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is an unrelated bugfix that we (Jim and I) found while debugging

@@ -774,7 +774,7 @@ const PostsPage = ({fullPost, postPreload, eagerPostComments, refetch, classes}:
/>}
{isAF && <AFUnreviewedCommentCount post={post}/>}
</AnalyticsContext>
{isFriendlyUI && post.commentCount < 1 &&
{isFriendlyUI && Math.max(post.commentCount, results?.length ?? 0) < 1 &&
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This fixes a bug where the "This post has no comments" message remains on screen after a user adds the first comment

strategy:
fail-fast: false
matrix:
project: [chromium, firefox]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For now we're only running the tests on chromium and firefox in CI. In the future it'd be nice to expand this to include safari and some mobile viewports as well, but the tests seem considerably more flaky on these platforms so more debugging would be needed to get them working reliably, and I don't think it's worth delaying this PR.

import { test, expect } from "@playwright/test";
import { loginNewUser } from "./playwrightUtils";

test("can send and receive messages", async ({browser}) => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This test is a really nice example of playwright's benefits over cypress - instead of creating an existing message as a fixture like we did before, we can now log two different users into two different browsers at the same time and send messages back and forth between them.

templateId?: string,
dropExisting?: boolean,
}

export const dropAndCreatePg = async ({seed, templateId, dropExisting}: DropAndCreatePgArgs) => {
export const dropAndCreatePg = async ({templateId, dropExisting}: DropAndCreatePgArgs) => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This file can probably be cleaned up further now it's only used by integration tests. We should probably also migrate the integration tests to use the new dockerized postgres DB which should speed them up considerably, but that can wait for a different PR.

@oetherington oetherington marked this pull request as ready for review May 20, 2024 15:19
@oetherington oetherington requested a review from a team as a code owner May 20, 2024 15:19
@oetherington oetherington requested review from Will-Howard and removed request for a team May 20, 2024 15:19
@Will-Howard
Copy link
Collaborator

I'm running into this problem with installing docker on my laptop. I'll try upgrading macos tonight and then start reviewing this

Copy link
Collaborator

@Will-Howard Will-Howard left a comment

Choose a reason for hiding this comment

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

LGTM, the playwright UI interface is so much nicer than the cypress one 😌. I just ran into a couple of problems setting it up and running locally:

Install docker (brew install --cask docker on OSX)

Perhaps obvious but you also have to open the docker application

Download the Postgres docker image with yarn playwright-db (you can Ctrl+C to exit the container once it has successfully downloaded and started)... You can then run the tests with yarn playwright test

I was able to get it to work consistently (wrt running the tests at all) by running yarn playwright-db (without exiting with Ctrl+C) and yarn start-playwright in separate terminals. Without this it only worked intermittently. I think the issues were:

  • When running without yarn start-playwright in a separate terminal, it would sometimes think that there already was a server running, and so timeout trying to connect to it. I became convinced that it thought there was a server running because if I set reuseExistingServer: false instead it would then say port 3456 was already in use, instead of timing out. It appeared to do this exactly every other time (i.e. it would run successfully once, then timeout on the next try, then run successfully after that)
  • With yarn playwright-db, pressing Ctrl+C once would shut down the db, and then pressing it again would exit the container but leave it running, so the test would then fail (because the db was shut down, but it wouldn't try running a new container because the existing one was still running). I think this is just a documentation bug, running it with -d or any other way of not having the database shut down worked
    • I also think I might have got a "port already in use" bug with the database like the one above at some point

There were also a couple of tests that were flaky, seemingly independent of whether I was running yarn playwright-db and yarn start-playwright in separate terminals (although, with both of these I thought maybe running against an existing server that had already been used once made them more likely):

1) [chromium] › posts.spec.ts:57:5 › voting on a post gives karma ────────────────────────────────

    Error: Timed out 5000ms waiting for expect(locator).toBeVisible()

    Locator: getByText('1 karma')
    Expected: visible
    Received: hidden
    Call log:
      - expect.toBeVisible with timeout 5000ms
      - waiting for getByText('1 karma')


      79 |   // The post author should now have the karma
      80 |   await page.goto(authorPage);
    > 81 |   await expect(page.getByText("1 karma")).toBeVisible();
         |                                           ^
      82 | });
      83 |
      84 | test("admins can move posts to draft", async ({page, context}) => {

        at /Users/wh/Documents/code/ForumMagnum/playwright/posts.spec.ts:81:43

and

1) [chromium] › posts.spec.ts:40:5 › can create 5 posts per day, but not 6 ───────────────────────

    Test timeout of 30000ms exceeded.

    Error: page.waitForURL: Test timeout of 30000ms exceeded.
    =========================== logs ===========================
    waiting for navigation to "/posts/**" until "load"
    ============================================================

      47 |     await page.getByLabel("Rich Text Editor, main").fill(`Test body ${i}`);
      48 |     await page.getByText("Submit").click();
    > 49 |     await page.waitForURL("/posts/**");
         |                ^
      50 |   }
      51 |
      52 |   // After creating five posts the post rate limit should be triggered

        at /Users/wh/Documents/code/ForumMagnum/playwright/posts.spec.ts:49:16

package.json Show resolved Hide resolved
Comment on lines +81 to +89
ClickAwayListenerProps={{
// Don't close flash messages on click
// This breaks some unit tests in Playwright, since a click that was
// supposed to go to a button instead gets eaten by the clickaway. And
// it's not actually a good UI interaction, since the message is going
// to close soon anyways and it's easy to dismiss by accident when you
// wanted to read it by clicking something unrelated.
mouseEvent: false
}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

packages/lesswrong/server/authentication/auth0.ts Outdated Show resolved Hide resolved
packages/lesswrong/server/authenticationMiddlewares.ts Outdated Show resolved Hide resolved
@@ -395,12 +396,28 @@ export const addAuthMiddlewares = (addConnectHandler: AddMiddlewareType) => {
})
}

// This is a mock method used in playwright tests - it gets overwritten below
// for actual servers
let profileFromAccessToken: ProfileFromAccessToken = async (token: string) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this would be better as const, and then explicitly checking for isE2E everywhere we use this, to avoid accidentally using it when hasAuth0 is false for some other reason

@oetherington
Copy link
Collaborator Author

@Will-Howard I'm going to go ahead and merge now - we can tweak in future PRs if we want (including to docs).

I'm don't think it's worth delaying because of the flaky tests for a couple of reasons:

  • They're not super flaky
  • It's easy to fix in future PRs
  • And most importantly the cypress tests have been flaky lately anyway (maybe since the react 18 upgrade?) so this seems like an upgrade anyway

@oetherington
Copy link
Collaborator Author

@Will-Howard Oh - I just realised you didn't approve. Is there any thing else that you think is blocking?

Copy link
Collaborator

@Will-Howard Will-Howard left a comment

Choose a reason for hiding this comment

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

@oetherington Happy for you to merge. I'm still getting the thing where it only works every other time without the server running in a separate terminal. Maybe for now you could just change the readme so it says you do have to run it?

@Will-Howard Will-Howard removed their assignment May 24, 2024
@oetherington oetherington merged commit 9762a68 into master May 28, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants