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

SFINT-5832: Sort E2E tests migrate from Cypress to Playwright #4777

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

lvu285
Copy link
Contributor

@lvu285 lvu285 commented Dec 6, 2024

https://coveord.atlassian.net/browse/SFINT-5832

IN THIS PR:

  • Added Playwright E2E tests for the quantic-sort component

UNIT TESTS:

  • No need, as UTs already there

E2E PLAYWRIGHT TESTS:

  • Playwright for Sort component

@lvu285 lvu285 requested a review from a team as a code owner December 6, 2024 21:51
Copy link

github-actions bot commented Dec 6, 2024

Pull Request Report

PR Title

❌ Title should follow the conventional commit spec:
<type>(optional scope): <description>

Example: feat(headless): add result-list controller

Live demo links

Bundle Size

File Old (kb) New (kb) Change (%)
case-assist 242.9 242.9 0
commerce 354 354 0
search 414 414 0
insight 405.2 405.2 0
recommendation 255.1 255.1 0
ssr 407.4 407.4 0
ssr-commerce 371.3 371.3 0

SSR Progress

Use case SSR (#) CSR (#) Progress (%)
search 39 44 89
recommendation 0 4 0
case-assist 0 6 0
insight 0 27 0
commerce 0 15 0
Detailed logs search : buildInteractiveResult
search : buildInteractiveInstantResult
search : buildInteractiveRecentResult
search : buildInteractiveCitation
search : buildGeneratedAnswer
recommendation : missing SSR support
case-assist : missing SSR support
insight : missing SSR support
commerce : missing SSR support

@lvu285 lvu285 changed the title SFINT-5832: Sorts e2e migrate from Cypress to Playwright SFINT-5832: Sort E2E tests migrate from Cypress to Playwright Dec 6, 2024
@@ -0,0 +1,112 @@
import {SortObject} from './sortObject';
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import {SortObject} from './sortObject';
import {SortObject} from './pageObject';

in line with my other comment

});

test.describe('when testing accessibility', () => {
test('should be accessible to keyboard', async ({sort}) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

should we test selecting with SPACE key here?
what about navigating with TAB or left/right arrow keys?

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 added test using Space to open Dropdown
Tab is not necessary as it wont trigger anything

await this.sortButton(buttonName).click();
}

async selectSortButtonKeyboard(): Promise<void> {
Copy link
Contributor

Choose a reason for hiding this comment

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

In line with my suggestion to test with space, you could probably modify this to take the key you want to use as argument (ENTER || SPACE)

return this.page.getByRole('combobox', {name: 'Sort by'});
}

get sortPreviewHeader(): Locator {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
get sortPreviewHeader(): Locator {
get sortPreviewButton(): Locator {

@mmitiche
Copy link
Contributor

@erocheleau I will integrate the content from this PR: #4782 in this PR as well, to have more unit tests to test the component rather going E2E only.

urlHash: string;
};

type QuanticSortE2EInsightFixtures = QuanticSortE2ESearchFixtures & {
Copy link
Contributor

Choose a reason for hiding this comment

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

The Insight Fixture should contain QuanticSortE2EFixtures + insightSteup and not QuanticSortE2ESearchFixtures + insightSetup

You can follow the following example:

type QuanticTabE2EFixtures = {
tab: TabObject;
search: SearchObject;
options: Partial<TabOptions>;
};
type QuanticTabE2ESearchFixtures = QuanticTabE2EFixtures & {
urlHash: string;
};
type QuanticTabE2EInsightFixtures = QuanticTabE2EFixtures & {
insightSetup: InsightSetupObject;
};
.

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 will update this. Do you want I update pager too? as I followed your example on Pager component?

Copy link
Contributor

Choose a reason for hiding this comment

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

Need to be cleaned out according to the new unit tests that test the invalid sort options and custom sort sort options, no need for example for the invalidMessage selector.

@lvu285 lvu285 requested a review from mmitiche December 27, 2024 22:16
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.

4 participants