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

Implement playwright acceptance tests for vertical full page map #1155

Merged
merged 31 commits into from
Dec 4, 2023

Conversation

likimmy
Copy link
Contributor

@likimmy likimmy commented Nov 17, 2023

J=BACK-2714

@likimmy likimmy requested a review from cea2aj November 17, 2023 18:18
@likimmy likimmy changed the base branch from master to develop November 17, 2023 18:18
@coveralls
Copy link

coveralls commented Nov 17, 2023

Coverage Status

coverage: 8.737%. remained the same
when pulling 8466eca on playwright
into f84e49e on develop.

.gitignore Outdated Show resolved Hide resolved
await page.getByPlaceholder('Search for locations').fill('virginia');
await page.getByPlaceholder('Search for locations').press('Enter');
await page.getByRole('button', { name: 'Result number 5' }).click();
const locator = page.locator('#js-answersVerticalResults div').nth(134);
Copy link
Member

Choose a reason for hiding this comment

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

what is the 134 value here? is there another way to target it that is makes easier to understand what is being selected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

from what I was able to see there isn't a better way to target this component. js-answersVerticalResults has several children, each of which have nested children as well and this is the only component i could use to drill down into the specific div that has the pinFocused class

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it's possible to write a query that checks for the pinFocused class on any of the children of #js-answersVerticalResults div. That way we don't need to include the 134 number because that seems like it could break easily if the search results change

Comment on lines 65 to 67
const isScrolledUp = await page.evaluate(() => {
return window.scrollY === 0;
});
Copy link
Member

Choose a reason for hiding this comment

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

we should check the scrollTop of the .Answers-resultsWrapper. I don't believe the scrollY of the window itself changes on pagination

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 was struggling getting the correct component because Answers.resultsWrapper didn't seem to work/have a scrollTop member. Instead I just checked if the first element of the results was visible, but let me know if this is not robust enough for what we are testing here!

@@ -0,0 +1,437 @@
import { test, expect, type Page } from '@playwright/test';

test.beforeEach(async ({ page }) => {
Copy link
Member

@cea2aj cea2aj Nov 17, 2023

Choose a reason for hiding this comment

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

are these tests intended to be here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, these were auto-generated. deleted!

Copy link
Member

Choose a reason for hiding this comment

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

It looks like these were auto-generated again. I wonder if you need to disable something in order to prevent this from being created again

await page.getByPlaceholder('Search for locations').press('Enter');
await page.getByLabel('Go to the next page of results').click();
const secondPage = page.locator('#js-answersVerticalResultsCount');
await expect(secondPage).toHaveText(/21/);
Copy link
Member

Choose a reason for hiding this comment

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

I'm cautious about this check because this would break if an entity is added to the account because it would change the results count. Another idea is checking the query params of the web page to see if the offset changed. But if it's too much effort I think we can stick with this because the slapshot test account is pretty stable

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 used 21 because the offset is set to 20 for each page, so i think as long as the results size is more than 20, even if the total results count changes the check for 21 should work, but let me know if i am misunderstanding your comment!

e2e/vertical-full-page-map.spec.ts Outdated Show resolved Hide resolved
e2e/vertical-full-page-map.spec.ts Outdated Show resolved Hide resolved
e2e/vertical-full-page-map.spec.ts Outdated Show resolved Hide resolved
playwright.config.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@EmilyZhang777 EmilyZhang777 left a comment

Choose a reason for hiding this comment

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

Looks good! Just have a few comments.

.gitignore Outdated
/blob-report/
/playwright/.cache/
/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.

this is duplicated, delete?

test('clicking on a pin closes the filter view', async ({ page }) => {
await page.getByRole('button', { name: 'filter results' }).click();
await page.getByText('Cats (1)').click();
const filterView = page.getByLabel('Main location search').locator('div').filter({ hasText: 'Filters Services reset Cats (1) Dogs (1) Sleep (1)' }).first();
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this text always be exactly like this? I'm worried the order/number might change. Would you be able to select the element using something like classname/label or do a regex match of the text?

e2e/vertical-full-page-map.spec.ts Outdated Show resolved Hide resolved
e2e/vertical-full-page-map.spec.ts Show resolved Hide resolved
e2e/vertical-full-page-map.spec.ts Show resolved Hide resolved
e2e/vertical-full-page-map.spec.ts Outdated Show resolved Hide resolved
e2e/vertical-full-page-map.spec.ts Outdated Show resolved Hide resolved
e2e/vertical-full-page-map.spec.ts Outdated Show resolved Hide resolved
e2e/vertical-full-page-map.spec.ts Outdated Show resolved Hide resolved
e2e/vertical-full-page-map.spec.ts Outdated Show resolved Hide resolved
e2e/vertical-full-page-map.spec.ts Outdated Show resolved Hide resolved
e2e/vertical-full-page-map.spec.ts Show resolved Hide resolved
e2e/vertical-full-page-map.spec.ts Outdated Show resolved Hide resolved
e2e/vertical-full-page-map.spec.ts Outdated Show resolved Hide resolved
.github/workflows/playwright.yml Outdated Show resolved Hide resolved
e2e/vertical-full-page-map.spec.ts Outdated Show resolved Hide resolved
e2e/vertical-full-page-map.spec.ts Outdated Show resolved Hide resolved
e2e/vertical-full-page-map.spec.ts Outdated Show resolved Hide resolved
e2e/vertical-full-page-map.spec.ts Outdated Show resolved Hide resolved
e2e/vertical-full-page-map.spec.ts Outdated Show resolved Hide resolved
e2e/vertical-full-page-map.spec.ts Outdated Show resolved Hide resolved
e2e/vertical-full-page-map.spec.ts Outdated Show resolved Hide resolved
e2e/vertical-full-page-map.spec.ts Outdated Show resolved Hide resolved
e2e/vertical-full-page-map.spec.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@nmanu1 nmanu1 left a comment

Choose a reason for hiding this comment

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

🔥

@likimmy likimmy merged commit 1999838 into develop Dec 4, 2023
24 of 25 checks passed
@likimmy likimmy deleted the playwright branch December 4, 2023 14:50
nmanu1 pushed a commit that referenced this pull request Dec 8, 2023
* Implement playwright acceptance tests for vertical full page map

* Implement playwright acceptance tests for vertical full page map

* Implement playwright acceptance tests for vertical full page map

* Implement playwright acceptance tests for vertical full page map

* Implement playwright acceptance tests for vertical full page map

* Implement playwright acceptance tests for vertical full page map

* Implement playwright acceptance tests for vertical full page map

* Implement playwright acceptance tests for vertical full page map

* Implement playwright acceptance tests for vertical full page map

* Implement playwright acceptance tests for vertical full page map

* Implement playwright acceptance tests for vertical full page map

* Implement playwright acceptance tests for vertical full page map

* Implement playwright acceptance tests for vertical full page map

* Implement playwright acceptance tests for vertical full page map

* Implement playwright acceptance tests for vertical full page map

* Implement playwright acceptance tests for vertical full page map

* Implement playwright acceptance tests for vertical full page map

* Implement playwright acceptance tests for vertical full page map

* Implement playwright acceptance tests for vertical full page map

* Implement playwright acceptance tests for vertical full page map

* Implement playwright acceptance tests for vertical full page map

* Implement playwright acceptance tests for vertical full page map

* Implement playwright acceptance tests for vertical full page map

* Implement playwright acceptance tests for vertical full page map

* Implement playwright acceptance tests for vertical full page map

* Implement playwright acceptance tests for vertical full page map

* Implement playwright acceptance tests for vertical full page map

* Implement playwright acceptance tests for vertical full page map

* Implement playwright acceptance tests for vertical full page map

* Implement playwright acceptance tests for vertical full page map

* Implement playwright acceptance tests for vertical full page map
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

5 participants