Skip to content

test(ui): select tables by shared attributes, not AntD class names - #31960

Merged
harsh-vador merged 3 commits into
feat/core-table-compact-radiofrom
feat/tablev2-testid-contract
Aug 25, 2026
Merged

test(ui): select tables by shared attributes, not AntD class names#31960
harsh-vador merged 3 commits into
feat/core-table-compact-radiofrom
feat/tablev2-testid-contract

Conversation

@harsh-vador

@harsh-vador harsh-vador commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Stacked on #31954. Base retargets to main once that merges.

Prep for the call-site sweeps, plus the guide those sweeps get reviewed against.

The selector problem

A test that selects .ant-table-row breaks the moment its page moves to TableV2 — which would drop an unrelated red suite into the middle of every sweep PR.

AntD's rc-table emits data-row-key natively, and TableV2 emits it too. So a selector written against it passes on both wrappers and survives the migration. Same for the expander: both render data-testid="expand-icon".

Three suites rewritten (SampleDataTable ×2, and ChildTermsTab in the Collate repo). All pass against the legacy wrapper, which is the point — nothing has migrated yet.

Adds filter-trigger / filter-dropdown testids to TableV2 so the filter UI has a hook that doesn't depend on AntD's DOM, and lists the stable hooks in the TableV2 header comment.

The parity suite keeps its .ant-table selectors — they live in the legacy adapter, which exists precisely to drive the AntD DOM.

docs/antd-migration/table.md

The review contract for every sweep PR: what's supported (with a spec behind each claim), what's blocked and why, the selector table, the per-page checklist, and the gotchas that cost time during the parity work — including that ts-jest does not fail on a missing export here, so a green jest run is not a typecheck.

Playwright is deliberately untouched

13 specs reference .ant-table. They need a running stack to verify, and rewriting e2e selectors blind is how a suite goes quietly broken. Each moves in the sweep PR for its page, where it can actually be run. The mapping for the three cases with no shared hook (tree depth, filter trigger, filter dropdown) is in the guide.

Scope correction

An earlier estimate of "39 jest files" was wrong — that grep caught source files using AntD classes for styling, which is Phase 6 work, not selectors. The real count is 3.

Verification

  • SampleDataTable — 37 tests, ChildTermsTab — 10 tests, common/Table — 144 tests
  • eslint 0 errors

🤖 Generated with Claude Code

Greptile Summary

The PR replaces AntD-specific selectors in SampleDataTable tests with shared row attributes and adds stable TableV2 filter/header hooks. Major changes:

  • Documents TableV2 migration contracts, supported props, selectors, and migration checks.
  • Updates duplicate- and reserved-column tests to select rows through data-row-key.
  • Adds filter trigger, filter dropdown, and header-content test hooks to TableV2.
  • Extends filter-title alignment styling to TableV2 headers.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
docs/antd-migration/table.md Adds the Table-to-TableV2 migration guide, including supported contracts, stable selectors, and validation guidance.
openmetadata-ui/src/main/resources/ui/src/components/Database/SampleDataTable/SampleDataTable.duplicateColumns.test.tsx Replaces AntD row-class selectors with the shared data-row-key attribute in duplicate-column assertions.
openmetadata-ui/src/main/resources/ui/src/components/Database/SampleDataTable/SampleDataTable.reservedColumns.test.tsx Uses the shared row-key attribute for row-count and uniqueness assertions.
openmetadata-ui/src/main/resources/ui/src/components/common/Table/TableV2.tsx Adds stable header and filter test hooks without changing the filter interaction flow.
openmetadata-ui/src/main/resources/ui/src/styles/components/table.less Extends filter-icon alignment styling to TableV2 header-content containers.

Reviews (3): Last reviewed commit: "Merge branch 'feat/tablev2-parity-and-ga..." | Re-trigger Greptile

Tests that select on `.ant-table-row` break the moment a page moves to TableV2,
which would put an unrelated red suite in the middle of every sweep PR. AntD's
rc-table emits `data-row-key` natively and TableV2 emits it too, so a selector
written against it passes on both wrappers and survives the migration.

Rewrites the three suites that reached for AntD class names, and adds
`filter-trigger` / `filter-dropdown` testids to TableV2 so the filter UI has a
hook that does not depend on AntD's DOM. The stable hooks are listed in the
TableV2 header comment.

The parity suite keeps its `.ant-table` selectors: they live in the legacy
adapter, which exists precisely to drive the AntD DOM.

Adds docs/antd-migration/table.md — the review contract for the call-site
sweeps: what is supported (with a spec behind each claim), what is blocked and
why, the selector table, and the per-page checklist.

Playwright specs are deliberately not touched. They need a running stack to
verify, and rewriting e2e selectors blind is how a suite goes quietly broken —
each one moves in the sweep PR for its page, where it can actually be run.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

❌ PR checklist incomplete

This PR cannot be merged until the following are addressed on its linked issue:

  • No GitHub issue is linked. Link an issue in the Development section of the PR (or add Fixes #12345 to the description). For a same-org cross-repo issue, add Fixes open-metadata/<repo>#123 to the description.

The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically.

Maintainers can bypass this check by adding the skip-pr-checks label.

@github-actions github-actions Bot added the UI UI specific issues label Aug 24, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Hi there 👋 Thanks for your contribution!

The OpenMetadata team will review the PR shortly! Once it has been labeled as safe to test, the CI workflows
will start executing and we'll be able to make sure everything is working as expected.

Let us know if you need any help!

@harsh-vador harsh-vador added the safe to test Add this label to run secure Github workflows on PRs label Aug 24, 2026
@harsh-vador harsh-vador self-assigned this Aug 24, 2026
@github-actions

github-actions Bot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

✅ Playwright Results — workflow succeeded

Validated commit 7ef2d447d592d02cd4fff6b8a709ce9625b20a1c in Playwright run 32730266985, attempt 1.

✅ 553 passed · ❌ 0 failed · 🟡 0 flaky · ⏭️ 0 skipped · 🧰 0 lifecycle flaky

Performance

Blocking targets: ✅ met · Optimization targets: 🟡 in progress

Shard-job maxima below are not the full workflow wall time; the linked run includes build, fixture, planning, and reporting.

🕒 Full workflow signal wall (to summary) 1h 2m 12s

⏱️ Max setup 4m 34s · max shard execution 16m 6s · max shard-job elapsed before upload 19m 6s · reporting 4s

🌐 215.50 requests/attempt · 2.83 app boots/UI scenario · 20.70% common-shard skew

Optimization targets still in progress:

  • Common shard skew was 20.7% (convergence target: at most 15%).
  • Browser traffic was 215.5 requests per attempt (convergence target: fewer than 200).
  • Application boot ratio was 2.83 per UI scenario (1623 boots / 574 scenarios; convergence target: at most 1).
Shard Passed Failed Flaky Skipped Lifecycle failed Lifecycle flaky
✅ Shard chromium-01 138 0 0 0 0 0
✅ Shard chromium-02 128 0 0 0 0 0
✅ Shard chromium-03 133 0 0 0 0 0
✅ Shard data-asset-rules-01 61 0 0 0 0 0
✅ Shard domain-isolation-01 16 0 0 0 0 0
✅ Shard global-state-01 34 0 0 0 0 0
✅ Shard ingestion-01 1 0 0 0 0 0
✅ Shard reindex-01 2 0 0 0 0 0
✅ Shard search-01 11 0 0 0 0 0
✅ Shard search-rbac-01 29 0 0 0 0 0

📦 Download artifacts

How to debug locally
# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip    # view trace

The rule reversed the filter icon and title by targeting
.ant-table-filter-column, which TableV2 does not render — so the icon would
silently move back to the right on every filtered table the sweep touches
(5 call sites across both repos).

TableV2's column header now carries data-testid=column-header-content, and the
rule matches both, so the class keeps working either side of a migration.
@github-actions

github-actions Bot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Jest test Coverage

UI tests summary

Lines Statements Branches Functions
Coverage: 67%
67.2% (80845/120299) 51.69% (49508/95773) 52.69% (14836/28152)

@harsh-vador
harsh-vador force-pushed the feat/tablev2-testid-contract branch from 0b3930b to 7ef2d44 Compare August 24, 2026 13:01
@gitar-bot

gitar-bot Bot commented Aug 24, 2026

Copy link
Copy Markdown
Code Review 👍 Approved with suggestions 0 resolved / 1 findings

Refactors UI component tests to use shared data-row-key attributes instead of AntD class names and adds stable TableV2 test hooks. Consider mirroring dedup IDs in sort and onChange column lookups.

💡 Edge Case: Dedup ids not mirrored in sort/onChange column lookups

📄 openmetadata-ui/src/main/resources/ui/src/components/common/Table/TableV2.tsx:319-329 📄 openmetadata-ui/src/main/resources/ui/src/components/common/Table/TableV2.tsx:1005 📄 openmetadata-ui/src/main/resources/ui/src/components/common/Table/TableV2.tsx:1145

getColumnIds deduplicates the header id/cell key (e.g. second duplicate becomes foo-1), but handleSortChange, sortedDataSource, and the onChange match logic still resolve columns via the raw formula String(c.key ?? dataIndex ?? idx), which yields foo for both duplicates. So when a user sorts/filters the second of two same-key columns, React Aria emits foo-1 and the lookup finds no matching column (or the wrong one), silently doing nothing. This only affects the duplicate-column edge case this change targets and sorting duplicates is already ambiguous, so impact is low; consider routing those lookups through the same columnIds array for full consistency.

🤖 Prompt for agents
Code Review: Refactors UI component tests to use shared data-row-key attributes instead of AntD class names and adds stable TableV2 test hooks. Consider mirroring dedup IDs in sort and onChange column lookups.

1. 💡 Edge Case: Dedup ids not mirrored in sort/onChange column lookups
   Files: openmetadata-ui/src/main/resources/ui/src/components/common/Table/TableV2.tsx:319-329, openmetadata-ui/src/main/resources/ui/src/components/common/Table/TableV2.tsx:1005, openmetadata-ui/src/main/resources/ui/src/components/common/Table/TableV2.tsx:1145

   `getColumnIds` deduplicates the header `id`/cell `key` (e.g. second duplicate becomes `foo-1`), but `handleSortChange`, `sortedDataSource`, and the `onChange` match logic still resolve columns via the raw formula `String(c.key ?? dataIndex ?? idx)`, which yields `foo` for both duplicates. So when a user sorts/filters the second of two same-key columns, React Aria emits `foo-1` and the lookup finds no matching column (or the wrong one), silently doing nothing. This only affects the duplicate-column edge case this change targets and sorting duplicates is already ambiguous, so impact is low; consider routing those lookups through the same `columnIds` array for full consistency.

Options

Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Compact
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source

@github-actions

Copy link
Copy Markdown
Contributor

❌ UI Checkstyle Failed

❌ ESLint + Prettier + Organise Imports (src)

One or more source files have linting or formatting issues.

Affected files
  • openmetadata-ui/src/main/resources/ui/src/components/common/Table/TableV2.tsx

🔍 ESLint findings in this PR's files — 0 error(s), 20 warning(s)

Errors block the build. Warnings do not yet — they are rules whose backlog is still
being worked down, listed so this PR does not add to it. See docs/ui-code-quality-gate.md.

0 error(s), 20 warning(s) across 1 changed file(s).

Count Rule
7 sonarjs/no-nested-functions
4 react-hooks/exhaustive-deps
4 sonarjs/no-nested-conditional
3 sonarjs/cyclomatic-complexity
1 @typescript-eslint/no-non-null-assertion
1 sonarjs/expression-complexity
All findings
Location Rule Message
🟡 src/components/common/Table/TableV2.tsx:344:3 sonarjs/cyclomatic-complexity {"message":"Function has a complexity of 18 which is greater than 10 authorized.","cost":8,"secondaryLocations":[{"line":344,"column":2,"endLine":344,"endColumn
🟡 src/components/common/Table/TableV2.tsx:464:20 sonarjs/no-nested-functions Refactor this code to not nest functions more than 4 levels deep.
🟡 src/components/common/Table/TableV2.tsx:469:37 sonarjs/no-nested-functions Refactor this code to not nest functions more than 4 levels deep.
🟡 src/components/common/Table/TableV2.tsx:470:15 @typescript-eslint/no-non-null-assertion Forbidden non-null assertion.
🟡 src/components/common/Table/TableV2.tsx:510:6 react-hooks/exhaustive-deps React Hook useMemo has a missing dependency: 'scroll'. Either include it or remove the dependency array.
🟡 src/components/common/Table/TableV2.tsx:537:5 react-hooks/exhaustive-deps React Hook useCallback has a missing dependency: 'setPreference'. Either include it or remove the dependency array.
🟡 src/components/common/Table/TableV2.tsx:559:6 react-hooks/exhaustive-deps React Hook useCallback has a missing dependency: 'setPreference'. Either include it or remove the dependency array.
🟡 src/components/common/Table/TableV2.tsx:713:17 sonarjs/no-nested-conditional Extract this nested ternary operation into an independent statement.
🟡 src/components/common/Table/TableV2.tsx:723:5 react-hooks/exhaustive-deps React Hook useCallback has missing dependencies: 'filteredDataSource.length' and 'rest'. Either include them or remove the dependency array.
🟡 src/components/common/Table/TableV2.tsx:1000:49 sonarjs/cyclomatic-complexity {"message":"Function has a complexity of 13 which is greater than 10 authorized.","cost":3,"secondaryLocations":[{"line":1000,"column":48,"endLine":1000,"endCol
🟡 src/components/common/Table/TableV2.tsx:1066:65 sonarjs/no-nested-functions Refactor this code to not nest functions more than 4 levels deep.
🟡 src/components/common/Table/TableV2.tsx:1073:65 sonarjs/no-nested-functions Refactor this code to not nest functions more than 4 levels deep.
🟡 src/components/common/Table/TableV2.tsx:1143:55 sonarjs/cyclomatic-complexity {"message":"Function has a complexity of 17 which is greater than 10 authorized.","cost":7,"secondaryLocations":[{"line":1143,"column":54,"endLine":1143,"endCol
🟡 src/components/common/Table/TableV2.tsx:1203:37 sonarjs/no-nested-conditional Extract this nested ternary operation into an independent statement.
🟡 src/components/common/Table/TableV2.tsx:1209:60 sonarjs/no-nested-functions Refactor this code to not nest functions more than 4 levels deep.
🟡 src/components/common/Table/TableV2.tsx:1219:54 sonarjs/no-nested-functions Refactor this code to not nest functions more than 4 levels deep.
🟡 src/components/common/Table/TableV2.tsx:1230:39 sonarjs/no-nested-conditional Extract this nested ternary operation into an independent statement.
🟡 src/components/common/Table/TableV2.tsx:1236:60 sonarjs/no-nested-functions Refactor this code to not nest functions more than 4 levels deep.
🟡 src/components/common/Table/TableV2.tsx:1284:8 sonarjs/expression-complexity Reduce the number of conditional operators (5) used in the expression (maximum allowed 3).
🟡 src/components/common/Table/TableV2.tsx:1288:11 sonarjs/no-nested-conditional Extract this nested ternary operation into an independent statement.

Fix locally (fast - only checks files changed in this branch):

make ui-checkstyle-changed

@sonarqubecloud

Copy link
Copy Markdown

Base automatically changed from feat/tablev2-parity-and-gaps to feat/core-table-compact-radio August 25, 2026 05:20
@harsh-vador
harsh-vador merged commit df03820 into feat/core-table-compact-radio Aug 25, 2026
82 of 86 checks passed
@harsh-vador
harsh-vador deleted the feat/tablev2-testid-contract branch August 25, 2026 05:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

safe to test Add this label to run secure Github workflows on PRs UI UI specific issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant