Add the new e2e tests that work with new_header flag on
#1918
Conversation
|
Storybook and Tailwind configuration previews: Ready Storybook: https://wordpress.github.io/openverse-frontend/_preview/1918 Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again. You can check the GitHub pages deployment action list to see the current status of the deployments. |
openverse-bot
added
the
🚦 status: awaiting triage
|
Size Change: -3.12 kB (0%) Total Size: 812 kB
ℹ️ View Unchanged
|
obulat
added
🟧 priority: high
obulat
force-pushed
the
update_pw_helpers
branch
from
16c3db0 to
b73ace1
Compare
obulat
force-pushed
the
add/new_header_e2e_tests
branch
from
6773c69 to
4aaa86a
Compare
obulat
force-pushed
the
add/new_header_e2e_tests
branch
from
4aaa86a to
5c24cc4
Compare
|
Based on the high urgency of this PR, the following reviewers are being gently reminded to review this PR: @zackkrida Excluding weekend1 days, this PR was updated 2 day(s) ago. PRs labelled with high urgency are expected to be reviewed within 2 weekday(s)2. @obulat, if this PR is not ready for a review, please draft it to prevent reviewers from getting further unnecessary pings. Footnotes |
There was a problem hiding this comment.
Sorry for the delay. The tests look good but why is there duplication of the breakpoints utilities. I noted it in two places but it seems to be consistent. Do the breakpoint utilities need to be updated to accommodate something different about the new header?
test/playwright/e2e/filters.spec.ts
Outdated
| for (const breakpoint of testScreens) { | ||
| test.describe(`filters on ${breakpoint}`, () => { | ||
| const width = SCREEN_SIZES.get(breakpoint as TestScreen) as number | ||
|
|
||
| test.use({ viewport: { width, height: 700 } }) | ||
|
|
||
| test.beforeEach(async ({ context, page }) => { | ||
| await mockProviderApis(context) | ||
| await enableNewHeader(page) | ||
| }) |
There was a problem hiding this comment.
What is the reason not to use the describeBreakpoints utility here?
There was a problem hiding this comment.
The main reason was I didn't understand how describeBreakpoints utility works 🤦♀️ I thought it was only for the snapshots, because that's how it's used in the VR tests, and after this comment I realized that I don't have to use the expectSnapshot that it provides :) I'm going to re-factor the tests to use it.
I think for the functionality tests, we can simply test one mobile and one desktop breakpoint. This will make sure that the functionality is tested, but will not increase the testing time too much (as if we tested all breakpoints).
There was a problem hiding this comment.
Right on! If you use describeEachBreakpoint(testScreens) you'd get the behaviour you're looking for (only sm and xl tested). Or we could add a new alias for those called something like describeEndToEndTestScreens or something (names! 🥴). Agreed that for non-snapshot tests we can skip all the intermediary sizes and save some time 🏎️
There was a problem hiding this comment.
I've added a new alias describeMobileAndDesktop, and added documentation comments stating that it's to be used with e2e tests. It's easier for me to understand than endToEndTests that it has 2 variants from this name, even if that can be ambivalent since the describeEvery also basically tests mobile and desktop breakpoints.
There was a problem hiding this comment.
Nice! Despite the ambiguity I think it's a great name 😁
| const mockUaString = | ||
| 'Mozilla/5.0 (Android 7.0; Mobile; rv:54.0) Gecko/54.0 Firefox/54.0' | ||
| const mobileFixture = { | ||
| viewport: { width: 640, height: 700 }, | ||
| userAgent: mockUaString, | ||
| } |
There was a problem hiding this comment.
This duplicates part of the breakpoints utilities which already configure the user agent string and viewport. What is the reason to duplicate that here? Could we import the UA string from breakpoints at least to deduplicate there if the rest is not possible?
obulat
force-pushed
the
add/new_header_e2e_tests
branch
from
5c24cc4 to
040f75e
Compare
obulat
force-pushed
the
add/new_header_e2e_tests
branch
from
287cf59 to
ef7a818
Compare
| : props.isFilterVisible | ||
| ? 'l' | ||
| : 'm' | ||
| const audioTrackSize = ref( |
There was a problem hiding this comment.
I think we'll need to revisit the audioTrackSize selection with @panchovm after the new_header is added. Currently, I feel like the l/m change on the md screen makes it look worse instead of better. And the code is fairly complicated.
src/pages/search.vue
Outdated
| @@ -156,9 +153,9 @@ export default defineComponent({ | |||
| } | |||
| focusIn(document.getElementById('__layout'), Focus.First) | |||
| }, | |||
| fetchMedia(...args: unknown[]) { | |||
| fetchMedia({ shouldPersistMedia } = { shouldPersistMedia: false }) { | |||
There was a problem hiding this comment.
Is shouldPersistMedia always false here or should the type be boolean?
| fetchMedia({ shouldPersistMedia } = { shouldPersistMedia: false }) { | |
| fetchMedia({ shouldPersistMedia } = { shouldPersistMedia: boolean }) { |
There was a problem hiding this comment.
Oh dear, I totally misread this code! I thought it was the type but it was the default value 🤦 Sorry for the bad suggestion 😞
Co-authored-by: sarayourfriend <24264157+sarayourfriend@users.noreply.github.com>
|
Playwright Failure Test Results It looks like some of the Playwright tests failed. You can download the Playwright trace https://github.com/WordPress/openverse-frontend/actions/runs/3342725568 Read more about how to use this artifact here: https://github.com/WordPress/openverse-frontend/blob/main/test/playwright/README.md#debugging |
Fixes
Related to #1843 by @dhruvkb
Description
This PR is based on the work from #1914. It adds the new e2e tests that test the functionality when the
new_headerflag is on.Testing Instructions
The CI should pass.
Checklist
Update index.md).main) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin