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

Add Playwright for Web E2E Testing #7922

Merged
merged 41 commits into from
Mar 25, 2024
Merged

Add Playwright for Web E2E Testing #7922

merged 41 commits into from
Mar 25, 2024

Conversation

rickyrombo
Copy link
Contributor

Overview

Adds Playwright, a replacement to our existing E2E browser tests using Cypress currently.

Benefits:

  • Faster execution time, faster dev cycles, better tooling
  • No OOM problems
  • Better trace capturing in CI (console logs, network requests, etc)
  • Sharding/Parallelism (tests are 4x faster on CI than web-test-staging w/ Cypress - shards currently set to 4)
  • (Personal taste) more ergonomic

Converts the following tests:

  • Smoke tests
  • Play/pause test
  • Track page socials test
  • Upload tests

Coming soon from @DejayJD:

  • Sign In
  • Sign Up

Page load timeouts

Adds a helper for initial page navigation, as timeouts continue to be a problem in CI. Overrides page.goto when using the test fixtures, so that it has all the existing ergonomics of page.goto but with a built-in default wait which waits for headers.

Accessibility Improvements

Improves accessibility of "Stems & Downloads" modal's stem list, and the Collection Edit Page of upload's track list for easier querying via playwright.

Test flakiness reduced

  • Through the use of the navigation helper, smoke tests will be less flaky as they wait longer
  • Toggling favorites/reposts for the social actions test now works regardless of the initial state of the buttons, and tests that it persists through refresh
  • Play/pause test waits for skeletons to disappear before attempting to click on a list item, and clicks on track artwork specifically to ensure the play button appears. (Note: We should revisit this if we're afraid staging might have a top trending track that's gated probably)

More maintainable

Adds Page Object Models (POMs) for the upload flow, which will allow us to write tests that are more maintainable in the wake of redesigns of the product, as we will only have to update selectors (Locators in playwright's lingo) in one place, rather than in every test that goes through upload.

@@ -0,0 +1,74 @@
import { Locator, Page, expect } from '@playwright/test'

export class SocialActions {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, I expect this to come in handy 💯

Copy link
Contributor

@dylanjeffers dylanjeffers left a comment

Choose a reason for hiding this comment

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

everything is so clean! i think im on board with the object models as well cause it makes the testing so much more declarative.

import { Locator, Page } from '@playwright/test'
import path from 'path'

export class StemsAndDownloadsModal {
Copy link
Contributor

Choose a reason for hiding this comment

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

i do like this pattern a lot! How do we feel about the implementation details of a modal that is specific to certain experiences be in this generalized location? i suppose it's alright because there may be multiple tests that would reference it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I'm down to reorganize if needed! don't feel strongly here

@@ -0,0 +1,74 @@
import { Locator, Page, expect } from '@playwright/test'

export class SocialActions {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe "track-social-actions" since this seems specific to that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah been thinking on this - maybe TrackPage and only have the social actions?

await this.repostButton.page().reload()

// Check that it persists after reload
await expect(this.unrepostButton).toBeVisible()
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a case where the initial load of the page shows unrepost, then user data comes, and it's reposted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, great callout... hmm

Copy link
Contributor

Choose a reason for hiding this comment

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

We can potentially address this with an aria-busy or some a11y loading thing and wait for that to end, then check the status. I don't think you need to spend a bunch of time solving this, but heads up for us

await expect(description).toBeVisible()

// Assert track list
const trackTable = page.getByRole('table')
Copy link
Contributor

Choose a reason for hiding this comment

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

great tests

await accessAndSaleModal.save()

await editPage.openAttributionSettings()
const attributionModal = new AttributionModal(page)
Copy link
Contributor

Choose a reason for hiding this comment

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

so clean

await expect(page.getByText('$' + price)).toBeVisible()
})

test('should upload a track with free stems', async ({ page }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

awesome job covering these differtent cases

@@ -106,13 +106,15 @@ export const StemFilesView = ({
</Box>
</Flex>
<Flex
role='list'
Copy link
Contributor

Choose a reason for hiding this comment

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

niceee

}

private async setTrackMood(index: number, mood: string) {
const trackItem = this.trackList.getByRole('listitem').nth(index)
Copy link
Contributor

Choose a reason for hiding this comment

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

love the nth use

Comment on lines +265 to +292
await this.assertProgress(1)
await this.assertProgress(10)
await this.assertProgress(20)
await this.assertProgress(30)
await this.assertProgress(40)
await this.assertProgress(50)
await this.assertProgress(60)
await this.assertProgress(70)
await this.assertProgress(80)
await this.assertProgress(90)
await this.assertProgress(99)

// Assert finalizing
await expect(this.finalizing).toBeVisible({ timeout: 60 * 1000 })

// Assert success
await expect(this.successHeading).toBeVisible({ timeout: 60 * 1000 })
}

private async assertProgress(progress: number) {
await expect
.poll(
async () =>
Number(await this.progressBar.getAttribute('aria-valuenow')),
{ timeout: 60 * 1000 }
)
.toBeGreaterThanOrEqual(progress)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

this progress polling is super dope, love the way you set this up


const socialActions = new SocialActions(page)
await expect(
socialActions.favoriteButton.or(socialActions.unfavoriteButton)
Copy link
Contributor

Choose a reason for hiding this comment

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

love the use of .or here

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh yeah this is clean! Nicer than what I did to achieve this

packages/web/e2e/test.ts Outdated Show resolved Hide resolved

/* Test against mobile viewports. */
// {
// name: 'Mobile Chrome',
Copy link
Contributor

Choose a reason for hiding this comment

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

no action for now since we should def limit ourselves to getting everything 100% on chromium; but I do wonder if/when we should enable more browsers. These mobile ones in particular seem like they might be handy

},

/* Total timeout for individual tests */
timeout: 5 * 60 * 1000,
Copy link
Contributor

@DejayJD DejayJD Mar 25, 2024

Choose a reason for hiding this comment

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

noop: I understand why this is here but this 5min timeout still makes me sus
IIRC you put this in place to account for the upload suite right?
I guess its not a huge deal tho since most of the time expect timeouts will be the ones to handle timeouts
Also I guess at the end of the day if the whole CI job is under 10 mins its still plenty fast 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fwiw: 5min is the default, this just made it explicit

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure? The default shown here is 30s
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my brain saw 5 zeros 💀

baseURL: 'http://localhost:3001',

/* Collect trace when retrying the failed test. See https://playwright.dev/docs/trace-viewer */
trace: 'on-first-retry',
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure the implications for CI storage bills, but if possible it would be nice to store traces on every run so that we can debug flaky tests that pass on retry

/* Run tests in files in parallel */
fullyParallel: true,
/* Fail the build on CI if you accidentally left test.only in the source code. */
forbidOnly: !!process.env.CI,
Copy link
Contributor

Choose a reason for hiding this comment

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

this is dope ❤️

page.getByRole('heading', { name: /complete your playlist/i })
).toBeVisible()

const editPage = new EditPlaylistPage(page)
Copy link
Contributor

Choose a reason for hiding this comment

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

the POMs make this code so nice 💯

Comment on lines +76 to +77
const tag1 = page.getByRole('link', { name: tags[0] })
const tag2 = page.getByRole('link', { name: tags[1] })
Copy link
Contributor

Choose a reason for hiding this comment

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

super nit: I think I would prefer

Suggested change
const tag1 = page.getByRole('link', { name: tags[0] })
const tag2 = page.getByRole('link', { name: tags[1] })
const track1tag1 = page.getByRole('link', { name: tags[0] })
const track1tag2 = page.getByRole('link', { name: tags[1] })

and down below

  const track2tag1 = page.getByRole('link', { name: tags[0] })
  const track2tag2 = page.getByRole('link', { name: tags[1] })

instead of 1-4

@audius-infra
Copy link
Collaborator

Preview this change https://demo.audius.co/mjp-playwright

Comment on lines +45 to +48
await editPage.openRemixSettings()
const remixSettingsModal = new RemixSettingsModal(page)
await remixSettingsModal.setAsRemixOf(remixUrl, remixName)
await remixSettingsModal.save()
Copy link
Contributor

@DejayJD DejayJD Mar 25, 2024

Choose a reason for hiding this comment

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

idea: what if .openRemixSettings() could return the modal POM? Since its an "output" of the openRemixSettings action anyways

const remixSettingsModal = await editPage.openRemixSettings()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about this and initially wrote it this way, but I think that's coupling a bit too much and I felt like keeping the POMs fairly "dumb" was a better play

Copy link
Contributor

@DejayJD DejayJD left a comment

Choose a reason for hiding this comment

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

Super awesome work! Really appreciate the effort and hours you've put in here. Don't really have much to say beyond some nits.
I think in general this sets us up with a comfortable foundation to iterate on.
(Also already seeming better than our cypress suite 😜)

I think the POMs you have here are a great start and I'm interested to see how they play out over time; i.e. when we find them the most useful later on, how easy they are to maintain, when we choose to make them vs not. I do really like how declarative they make the test actions.

Copy link
Contributor

@sliptype sliptype left a comment

Choose a reason for hiding this comment

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

Killer PR

@@ -60,3 +60,7 @@ yarn-error.log*
.idea
.yalc
yalc.lock
/test-results/
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can put these in the root .gitignore?


const socialActions = new SocialActions(page)
await expect(
socialActions.favoriteButton.or(socialActions.unfavoriteButton)
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh yeah this is clean! Nicer than what I did to achieve this

} from './page-object-models/modals'
import { test } from './test'

test('should upload a remix, hidden, AI-attributed track', async ({ page }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I like how many cases you covered! Are we going to have "essential" and "comprehensive" suites like @DejayJD set up in #7859

Would love to discuss which tests belong in which suite

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we definitely can! Maybe we should define a target runtime for tests - personally I think if the tests take < 10 min there's no need to split them up but I'm curious what others think is tolerable

Copy link
Contributor

Choose a reason for hiding this comment

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

That does seem reasonable! I'm more so concerned with the maintainability and size of our core test suite. I've been in situations where the test suite is big and always broken and I want to avoid that

IMO core test suite should only be the super critical paths:

  • Sign up / sign in
  • Stream track
  • Upload track

And everything else could be in the "comprehensive" suite which is less blocking and we have more time to fix. BUT who knows, if all of our tests are written as well as these maybe we don't have much of a maintenance concern :)

Copy link
Contributor

Choose a reason for hiding this comment

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

yea I would love to revisit that concept at some point in the future. Grep filtering is already built into playwright so we can quickly port over that PR. Although I do want to let our pw stuff run for a bit and see how fast/flaky things are out in the wild

@rickyrombo rickyrombo merged commit ec6c7a6 into main Mar 25, 2024
41 of 45 checks passed
@rickyrombo rickyrombo deleted the mjp-playwright branch March 25, 2024 20:35
schottra added a commit that referenced this pull request Mar 26, 2024
* origin/main: (40 commits)
  Add playwright report task (#7946)
  [PROTO-1717] Clean up DDEX e2e with table tests (#7935)
  Fix sign up toasts not going away (#7944)
  Bump version to 0.6.67
  Remove Probers project and Cypress (#7923)
  [C-4017] Small fixes for ssr profile page rendering (#7937)
  [C-4065, C-4067] Update sdk instances in ssr files to avoid http provider errors (#7930)
  [C-4070] Improve sign-up page for various screen sizes (#7938)
  [ONC-58] Add next param to opensea requests (#7936)
  [INF-594] Add e2e tests for sdk example app (#7917)
  [C-4019] Fix web track menu options (#7934)
  [C-4066] Fix edit-collectibles modal spacing (#7925)
  Add Playwright for Web E2E Testing (#7922)
  add child logger to relay (#7927)
  Update mobile app versions for release .73 (#7931)
  Fix prepare transfer copy (#7929)
  Expire cached Solana transactions (#7926)
  v1.5.73
  Change internal sdk deps back to wildcard version (#7928)
  [C-4026, C-4059] Update profile page to render basic info and pictures on the server side (#7915)
  ...
audius-infra pushed a commit that referenced this pull request Mar 30, 2024
[d0f02bd] [PAY-2648] Improve state of collectibles and their collections (#7979) Saliou Diallo
[712f589] [PAY-2595] Access checker handles albums (#7973) Reed
[e45a0ef] [C-3996] Add Privacy manifest with privacy api reasons (#7966) Sebastian Klingler
[5978ffe] [C-4079] Add rewards cooldown to web (#7967) Isaac Solo
[592e664] Bump version to 0.6.70 audius-infra
[3600a6a] Remove spammy log from transaction handler (#7978) Reed
[76fcd36] [C-4069, C-4072] Render basic collection page info and image on the server side (#7974) Kyle Shanks
[a7f45be] [C-4211] Add non SSR paths to makePageRoute function (#7976) Kyle Shanks
[e246619] Make secondary buy $AUDIO button full-width (#7977) Reed
[cbcecb5] Disable sign up referral e2e test till it can be refactored (#7975) JD Francis
[361d87e] [ONC-65] Fix search auto suggest (#7972) Sebastian Klingler
[f79e98b] [PAY-2625, PAY-2618] Show dog ear on collection page (#7968) Andrew Mendelsohn
[55b0e8e] Bump version to 0.6.69 audius-infra
[de02017] [PROTO-1742] Crawl ddex folders and improve polling+schema (#7960) Theo Ilie
[d60a2be] Disable buy button on track screen on ios (#7971) Reed
[8ccc5e1] [C-4008] Fix video collectible card (#7940) Dylan Jeffers
[ae77ce8] [PAY-2576] Solana relay retries if initial send fails (#7910) Reed
[6e71ce2] Fix challenge info decimal (#7965) Isaac Solo
[20fea34] [PAY-2554, PAY-2555] Purchase album via purchase modal (#7941) Andrew Mendelsohn
[197c0a0] Playwright auth tests (#7948) JD Francis
[5633bf0] Fix pause button pt 2 (#7964) Sebastian Klingler
[46cb255] [C-4071] Set up the collection page for SSR (#7963) Kyle Shanks
[370d127] ONC-54: indexer concurrency (#7933) alecsavvy
[722f122] Whitespace lint fix for identity (#7962) Andrew Mendelsohn
[7dcd1ff] Bump version to 0.6.68 audius-infra
[3889398] Update Tooltip component to render children on server-side (#7945) Kyle Shanks
[08969e6] [PAY-2581] Add support for Helius DAS API for Solana NFTs (#7932) Saliou Diallo
[fe77d63] [ONC-62] Fix pause on track screen (#7959) Sebastian Klingler
[09756a5] [C-4076] Fix search results profile images (#7958) Dylan Jeffers
[4382ab9] Remove priority fee from track listens (#7957) Marcus Pasell
[b2a70a1] [C-4015] Fix hotkeys with modifiers (#7952) Dylan Jeffers
[ea11599] Fix upload body text (#7956) Dylan Jeffers
[85c6efd] Add junit web-test results to CI (#7954) Marcus Pasell
[e33d709] [C-4074] Add Dancehall genre (#7947) Dylan Jeffers
[417516f] [PAY-2566] Check playlists_previously_containing_track in access checker (#7953) Reed
[e751480] [PAY-2565] playlists_previously_containing_track column in tracks table (#7853) Reed
[3e8ebc9] Disable failing web-tests (#7949) Marcus Pasell
[fb22032] [C-4075] Harmony qa round 2 (#7951) Dylan Jeffers
[93b06c7] [ONC-60] Don't delete tracks when deleting album (#7950) Sebastian Klingler
[a38544c] [C-4062] Fix supporter tiles gradient (#7939) Dylan Jeffers
[d7a59b4] [C-2938] Fix robots.txt (#7943) Sebastian Klingler
[147a4f7] [PAY-2564] Remove old track download code (#7942) Reed
[0ae0170] Add playwright report task (#7946) Marcus Pasell
[6aa730b] [PROTO-1717] Clean up DDEX e2e with table tests (#7935) Theo Ilie
[2222c9d] Fix sign up toasts not going away (#7944) JD Francis
[8ef5ec1] Bump version to 0.6.67 audius-infra
[d03df66] Remove Probers project and Cypress (#7923) Marcus Pasell
[b8f638f] [C-4017] Small fixes for ssr profile page rendering (#7937) Kyle Shanks
[1598562] [C-4065, C-4067] Update sdk instances in ssr files to avoid http provider errors (#7930) Kyle Shanks
[cc18be2] [C-4070] Improve sign-up page for various screen sizes (#7938) Dylan Jeffers
[b313b4f]  [ONC-58] Add next param to opensea requests (#7936) Sebastian Klingler
[7ab8ed3] [INF-594] Add e2e tests for sdk example app (#7917) Sebastian Klingler
[5d93bf7] [C-4019] Fix web track menu options (#7934) Dylan Jeffers
[35e4091] [C-4066] Fix edit-collectibles modal spacing (#7925) Dylan Jeffers
[ec6c7a6] Add Playwright for Web E2E Testing (#7922) Marcus Pasell
[f340cd5] add child logger to relay (#7927) alecsavvy
[17ccd74] Update mobile app versions for release .73 (#7931) Dylan Jeffers
[d7a3e81] Fix prepare transfer copy (#7929) Isaac Solo
[b497af7] Expire cached Solana transactions (#7926) Marcus Pasell
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants