-
Notifications
You must be signed in to change notification settings - Fork 22
tests: decouple Pagination from assertions #826
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
base: main
Are you sure you want to change the base?
tests: decouple Pagination from assertions #826
Conversation
Signed-off-by: Carlos Feria <[email protected]>
Reviewer's GuideThis PR decouples DOM selectors from assertion logic in the Pagination page object, migrates built-in validations to custom Playwright matchers, updates test specs to use the new matchers, and configures fixtures accordingly. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- Consider adding helper methods like nextPage(), previousPage(), firstPage(), and lastPage() on the Pagination object so tests can navigate without calling get…Button().click() directly.
- Remove the deprecated validatePagination method (or migrate its logic fully into the new matchers) in a follow-up to keep the page object focused only on selectors and actions.
- The validateItemsPerPage method still mixes pagination and table logic—extract it into a dedicated Table helper or a separate matcher to preserve single responsibility.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider adding helper methods like nextPage(), previousPage(), firstPage(), and lastPage() on the Pagination object so tests can navigate without calling get…Button().click() directly.
- Remove the deprecated validatePagination method (or migrate its logic fully into the new matchers) in a follow-up to keep the page object focused only on selectors and actions.
- The validateItemsPerPage method still mixes pagination and table logic—extract it into a dedicated Table helper or a separate matcher to preserve single responsibility.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Signed-off-by: Carlos Feria <[email protected]>
Currently the e2e/tests/ui/pages/Pagination.ts class mixes assertions (expressed with
expect) and selectors (to select elements of the page). This PR aims to evolve the code and make cleaner and more robust by decoupling both: assertions and DOM selections.Why decouple:
Even in playwright the assertions and selections are decoupled and it is a common pattern. E.g.
expectis the assertionpage.locator("button[data-action='previous']")is a selectorWhat we currently have in e2e/tests/ui/pages/Pagination.ts is code like the function
validatePaginationwhere both assertions and selections are mixed.Before:
validatePaginationverifies the first page is selected, then verifies the status of the "next page" buttons, moves to the next page, then verify "previous page".After:
Notice that the object
paginationis being passed toexpect. This way we could start enforcing:expect(pagination)should contain all test assertions and never write assertions in e2e/tests/ui/pages/Pagination.tsNote: the current CI for the e2e are failing and it is a know issue, we won't merge PRs until our e2e tests are back to be healthy, but I'd like to ask for review and first impressions.
Summary by Sourcery
Decouple pagination assertions from DOM selectors in the e2e tests by refactoring the Pagination page object to only provide locators, extracting validation logic into custom Playwright matchers, and updating tests and fixtures to use these matchers, while deprecating the old validatePagination method.
New Features:
Enhancements:
Tests: