Skip to content

[CRAFTING] Add Page Object Model for datagrid-web's E2E specs#2308

Open
leonardomendix wants to merge 5 commits into
mainfrom
test/datagrid-pom
Open

[CRAFTING] Add Page Object Model for datagrid-web's E2E specs#2308
leonardomendix wants to merge 5 commits into
mainfrom
test/datagrid-pom

Conversation

@leonardomendix

Copy link
Copy Markdown
Collaborator

Pull request type

Test related change (New E2E test, test automation, etc.)


Description

This PR introduced a Page Object Model for datagrid-web's E2E specs, rewrote all six specs on it, and documented POM conventions in the e2e guidelines.

@leonardomendix leonardomendix requested a review from a team as a code owner July 6, 2026 14:02
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Comment on lines +22 to +25
/** Navigate to a page; the fixture auto-waits for Mendix readiness. */
async open(path = "/") {
await this.page.goto(path);
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I beleive this is overkill, I would rather wrap a datagrid, then we can use for example two datagrids on a page.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done! ✅

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

AI Code Review

⚠️ Approved with suggestions — low-severity items only, safe to merge


What was reviewed

File Change
docs/requirements/e2e-test-guidelines.md Added Page Object Model section with conventions and template
packages/pluggableWidgets/datagrid-web/e2e/DataGrid.spec.js Refactored to use DataGridPage POM; removed waitForMendixApp calls
packages/pluggableWidgets/datagrid-web/e2e/DataGridSelection.spec.js Refactored to use DataGridPage POM; changed window.getComputedStyle to globalThis.getComputedStyle
packages/pluggableWidgets/datagrid-web/e2e/filtering/DataGridFilteringEmptyString.spec.js Refactored to use DataGridPage POM with shared beforeEach
packages/pluggableWidgets/datagrid-web/e2e/filtering/DataGridFilteringIntegration.spec.js Refactored to use DataGridPage POM; removed leftover commented-out code
packages/pluggableWidgets/datagrid-web/e2e/filtering/DataGridFilteringMulti.spec.js Refactored to use DataGridPage POM with shared beforeEach
packages/pluggableWidgets/datagrid-web/e2e/filtering/DataGridFilteringSingle.spec.js Refactored to use DataGridPage POM with shared beforeEach
packages/pluggableWidgets/datagrid-web/e2e/pages/DataGridPage.js New POM file

Skipped (out of scope): dist/, pnpm-lock.yaml


Findings

⚠️ Low — DataGridFilteringMulti.spec.js: second and third tests don't share the beforeEach grid setup

File: packages/pluggableWidgets/datagrid-web/e2e/filtering/DataGridFilteringMulti.spec.js lines 34–65

Note: The beforeEach sets up grid = new DataGridPage(page, "dataGrid21") and navigates to /p/filtering-multi, but the second test (filter rows where ReferenceSet…) and third test (filter rows where Reference equal…) rely on the beforeEach navigation without any inline page.goto, which is good. However these two tests also skip the await expect(grid.rows).toHaveCount(11) initial guard that the first test has. That's fine for the third test (it adds its own row-count assertion) but the second test jumps straight into interactions without any wait for the grid to load. Consider adding an initial await expect(grid.rows).toHaveCount(11) (or any web-first assertion) at the start of the second test to ensure the grid is rendered before interacting — this follows the same pattern as the other tests in the suite.


⚠️ Low — DataGridPage.js: columnCells uses CSS :nth-child which is global, not scoped to the grid root

File: packages/pluggableWidgets/datagrid-web/e2e/pages/DataGridPage.js line 925

Note: The locator this.root.locator('[role="gridcell"]:nth-child(${n})') scopes by role="gridcell" AND :nth-child(n) within the root — the :nth-child selector applies relative to the element's parent within the CSS tree, not the Playwright root scope. This behaves as expected today (gridcells are direct children of rows), but it's worth noting this is a subtle CSS semantics trap. A more robust alternative would be this.root.locator('[role="row"] [role="gridcell"]:nth-child(${n})') or using nth() on a column-based locator. Low risk since existing tests appear to use it correctly, but document the 1-based convention in the JSDoc comment (the comment says "1-based, matches :nth-child" which is good — no action needed beyond awareness).


⚠️ Low — DataGridFilteringMulti.spec.js: drop_downFilter selector uses [role=combobox] without quotes around attribute value

File: packages/pluggableWidgets/datagrid-web/e2e/filtering/DataGridFilteringMulti.spec.js line 581

Note: .mx-name-drop_downFilter1[role=combobox] — the unquoted attribute value is technically valid CSS but inconsistent with the rest of the codebase which uses [role="combobox"] with quotes. Suggest normalizing to '.mx-name-drop_downFilter1[role="combobox"]' for consistency.


Positives

  • DataGridPage correctly stores no reference to page — the constructor uses it only to build root, preventing cross-instance contamination. This exactly matches the convention documented in the new guidelines section.
  • All waitForMendixApp calls are cleanly removed from every spec; the custom fixture handles the wait automatically, which is the correct pattern per the guidelines.
  • Page-level selectors for sibling filter widgets (drop_downFilter*) are correctly kept in the spec rather than added to the POM, with inline comments explaining why. This is good for maintainability and matches the documented "Page-level selectors stay in the spec" rule.
  • The new POM documentation in e2e-test-guidelines.md is thorough: it covers file placement, constructor conventions, the no-this.page rule, locator-vs-assertion separation, and a complete code example — easy for future contributors to follow.
  • globalThis.getComputedStyle over window.getComputedStyle in the evaluate callback is a meaningful correctness improvement for non-browser contexts.
  • The DataGridFilteringMulti and DataGridFilteringSingle specs consolidate beforeEach navigation correctly — each test now gets a fresh grid instance scoped to the right widget name, eliminating per-test boilerplate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants