Move the test case configuration into the right rail card - #33110
Move the test case configuration into the right rail card#33110ShaileshParmar11 wants to merge 2 commits into
Conversation
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
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 |
github-actions
Bot
added
safe to test
🔄 Playwright impact map auto-refreshedThis PR touched specs or UI source that changed the source→spec routing map. I regenerated What is this file? It is the auto-generated half of Playwright's PR planner. It routes "if source X changes, run specs Y" by walking spec imports and cross-referencing What if I want to regenerate locally instead? Run this before pushing your next change to skip the bot commit: python3 .github/scripts/generate_playwright_impact_map.py
git add .github/playwright/impact-map.generated.json
git commit --amend --no-edit # or a separate commit |
Jest test CoverageUI tests summary
|
✅ Playwright Results — workflow succeededValidated commit ✅ 810 passed · ❌ 0 failed · 🟡 0 flaky · ⏭️ 0 skipped · 🧰 0 lifecycle flaky PerformanceBlocking 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) 50m 43s ⏱️ Max setup 4m 40s · max shard execution 17m 28s · max shard-job elapsed before upload 21m 21s · reporting 4s 🌐 226.05 requests/attempt · 2.18 app boots/UI scenario · 23.57% common-shard skew Optimization targets still in progress:
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
TCD-10a. The parameters and the assertion SQL were the last two blocks the redesign wants in the rail and TCD-0 left in the main column. The existing layout could not fit the rail: `.parameter-label` sets `min-width: 200px` and rows are chunked two-up inside a 12-column sub-grid, so each half-cell is ~140px and the *label* overflows before any value does. That is why the ticket says redesign rather than relocate. The card follows the prototype: one row per parameter, label left and value right, the value truncating with a tooltip so a long `tableDiff` `table2` degrades instead of forcing width. The prototype's four shapes are independent `sc-if` blocks, not a switch — a custom-SQL test shows parameters *and* SQL — so `getConfigurationShapes` returns one flag per section with `isEmpty` as the "nothing at all" fallback. The version page keeps its parameter diff and suppresses the assertion SQL, which has no diff rendering. The SQL block is rendered directly rather than through `SchemaEditor`: the prototype highlights keyword-vs-plain only, a keyword set rather than a parser, so the bespoke block is less code than CodeMirror and keeps a lazy editor chunk out of a rail rendering three static lines. `toSqlLines` keeps the separators and each token's column offset, so text round-trips byte-for-byte and the offset doubles as a stable render key. The rail moves from `col-span-3` to `col-span-4`. Measured at a 1280px viewport it was 205px, not the ~315px assumed in the ticket discussion — that figure only holds near 1920px — against the prototype's fixed 320px. That is where the wrapping and the clipped SQL came from. At `col-span-4` the rail is 276px and nothing wraps. The per-SQL-param pencil merges into the single header pencil the prototype shows, and `parameterItems` goes away — it baked the dynamic-assertion badge into a parameter row, which the card now renders as its own callout. Colours map to semantic tokens, so the card gets dark mode. The collapse toggle is kept per the epic's Sep 8 decision, so collapsing the rail now also hides the configuration; that trade is asserted in a test and flagged for product on #6116. Also fixes the mock store's `isTabExpanded` default, which was `false` while `useTestCase.store` defaults to `true` — several tests only passed because the rail was absent. Replanted onto main after #32966 was squash-merged: this is the branch's net change over the squashed base, byte-identical to the previously verified tip (ec115e3) for every file it touches. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Regenerated for the configuration card's specs on the replanted branch, so the drift workflow has nothing to push after the force-push. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ec115e3 to
9893abb
Compare
Code Review ✅ ApprovedMoves test case parameters and assertion SQL from the main column into a Configuration card in the right rail, completing the details-page reflow. The card uses a single-row-per-parameter layout with values that truncate with tooltips, renders SQL with keyword highlighting and line numbers, and includes comprehensive unit and Playwright test coverage. No issues found. OptionsDisplay: compact → Counting what did not apply, without listing it. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
❌ UI Checkstyle Failed❌ Licence HeaderOne or more files are missing or have an outdated Apache 2.0 licence header. Affected files
🔍 ESLint findings in this PR's files — 0 error(s), 3 warning(s)Errors block the build. Warnings do not yet — they are rules whose backlog is still 0 error(s), 3 warning(s) across 3 changed file(s).
All findings
Fix locally (fast - only checks files changed in this branch): make ui-checkstyle-changed |
|
Describe your changes:
Fixes open-metadata/openmetadata-collate#6116
TCD-10a, part of epic open-metadata/openmetadata-collate#6074.
I moved the parameters and the assertion SQL out of the main column into a Configuration card at the top of the right rail, which is the last piece of the details-page reflow the redesign asks for, and widened the rail so the card has the width the design assumes.
TCD-0 deliberately left them in the main column, and the reason turned out to be sharper than "the rail is narrow". The existing parameter layout cannot fit the rail at all:
.parameter-labelsetsmin-width: 200pxand rows are chunked two-up inside a 12-column sub-grid, so each half-cell is ~140px and the label overflows before any value does. That is why the ticket says redesign rather than relocate.Type of change:
High-level design:
One new component (
TestCaseConfigurationCard/) plus the result-tab composition root, which loses two blocks and gains one rail child.Card layout — one row per parameter, label left and value right (
justify-between), replacing the two-up 200px-label grid. Values truncate with a tooltip, so a longtableDifftable2value degrades instead of forcing width.The prototype's four shapes are independent, not a switch.
hasParams,isDynamic,hasSqlandnoConfigare separatesc-ifblocks in the mock — its owncustomSqlcase shows parameters and SQL. SogetConfigurationShapesreturns one flag per section withisEmptyas the "nothing at all" fallback, rather than an exclusive branch. Getting this wrong would have hidden the parameters on every custom-SQL test.The SQL block is rendered directly, not through
SchemaEditor. I reached forSchemaEditorfirst to avoid hand-rolling a tokenizer, and that was the wrong call: the prototype highlights keyword-vs-plain only, which is a keyword set rather than a parser, so the bespoke block is less code than CodeMirror, matches the design, and avoids pulling a lazy editor chunk into a rail that renders three static read-only lines.toSqlLineskeeps the split separators and each token's column offset, so the text round-trips byte-for-byte (author indentation survivespre-wrap) and the offset doubles as a stable render key.The rail moves from
col-span-3tocol-span-4. Measured on a live page at a 1280px viewport, the rail was 205px, not the ~315px the ticket discussion assumed:col-span-3of an 849px grid with 10px gaps is(849 − 110) / 12 × 3 + 20. The 315px figure only holds near a 1920px viewport, and the prototype's rail is a fixedwidth: 320px— so the card was running at roughly two thirds of the width it was drawn for. That is where the wrapping came from; the banner and grid measure flush either way, so it was never a padding or gap problem.col-span-4puts the rail at 276px and the main column at 563px:Nothing wraps or clips after the change: the definition name fits on one line,
Compute Row Countstops breaking in two, andWHERE email IS NULLis fully visible rather than cut at the card edge. The chart keeps 529px. Bothcol-spanvalues pre-date this branch — they are onmain— so this is a deliberate widening, not a revert.A pre-existing spacing bug surfaced by the reflow, fixed on #32966. The last-run banner's wrapper carried
tw:pt-4and nothing below it, so the banner's bottom edge and the grid's top edge measured at the same y — 0px where every other vertical gap on that grid is 10px.mainhastw:px-4 tw:pt-4, so the bottom gap never existed; removing the horizontal inset and putting the result-history card directly beneath simply made the two read as touching.tw:pb-2.5puts the banner in the same rhythm as the cards (banner bottom 408, grid top 418).Two consolidations fall out. The per-SQL-param pencil merges into the single header pencil the prototype shows.
parameterItemsis deleted — it baked the dynamic-assertion badge into a parameter row, which the card now renders as its own callout; keeping it would have left two parameter-shaping paths with one unused.Deliberate deviations from the mock, so it fits our system rather than the static design:
#7A5AF8→utility-purple-600,#C0C4CC→quaternary,#1570EF→utility-blue-600). The card gets dark mode, which the mock has no answer for.shadow-xs; the mock has none, but the rail's other cards do, and an odd card out reads worse than a shadow difference.Open product question — please weigh in. The collapse toggle is kept, per the epic's Sep 8 decision that its removal was cancelled. The consequence is that collapsing the rail now also hides the parameters and SQL, which used to sit in the always-visible main column. That trade is asserted in a test rather than left implicit, but it is the decision the ticket flagged and it is still open.
Also fixes a test-fidelity bug: the mock store defaulted
isTabExpandedtofalsewhileuseTestCase.storedefaults it totrue. Several tests were passing only because the rail was absent — including "should not show edit icon without permission", which was passing for the wrong reason.Tests:
Use cases covered
Every shape was exercised against a live stack, one test case per row:
tableRowCountToBeBetweenminValue/maxValuerows underTable testcolumnValueLengthsToBeBetweenColumn test · <column>category linetableRowCountToBeBetween+useDynamicAssertiontableCustomSQLQuerycolumnValuesToBeUniquePlus: editing parameters still opens the editor and the affordance is hidden without permission; the version page renders the parameter diff with the assertion SQL suppressed; Collate's
getAdditionalComponentsandgetAlertBannerstill mount.One finding worth a reviewer's eye. A test type with no parameters does not reach the mock's empty state —
columnValuesToBeUniquestill renders aCompute Row Count: falserow, becauseshowComputeRowCountkeys off the definition'ssupportsRowLevelPassedFailedrather than whether it has parameters. Both zero-parameter definitions in a stock instance support it, so "No configurable parameters for this test type." may be effectively unreachable in practice. Not a regression — that row rendered before this PR too — but the mock's fourth shape is rarer than it implies, and it is covered by unit test rather than by anything a user is likely to see.Unit tests
TestCaseConfigurationCard.test.tsx— one case per configuration shape, the category line for both test kinds, permission-gated edit, the version diff, SQL line numbering and keyword highlighting.TestCaseConfigurationCard.utils.test.ts— the shape matrix (including that parameters and SQL co-render, and that the version page suppresses SQL), definition-name fallback, and the SQL tokenizer round-trip.TestCaseResultTab.test.tsx— updated for the new layout: configuration lives in the rail, the rail leads with it, and collapsing the rail hides it.Backend integration tests
Ingestion integration tests
Playwright (UI) tests
TestCaseDetailsPage.spec.ts— asserts the Configuration card renders the real seeded parameter rows (minValue12 /maxValue34) and theTable testcategory fortableRowCountToBeBetween.TestCaseSoftDeleteRestore.spec.ts— repointed at the card; it asserted the oldparameter-containertestid and would otherwise have broken.Manual testing performed
/observability/test-case/<fqn>/test-case-results— screenshots below391→1240in both)yarn jest src/components/DataQuality src/pages/IncidentManager src/components/observability/TestCaseDetail— 1037 tests / 77 suites pass on the replanted branchyarn tsc:checkandyarn tsc:playwright— no errors in the changed filesyarn lint:playwright0 errorsyarn check-i18npasses, with genuine translations in all 20 localesNot verified: Collate's
AiModelLearningAlert(viagetAlertBanner) andDataDiffResults(viagetAdditionalComponents) cannot be seen from the OSS repo — the base class returnsnullhere and only the Collate build supplies them. Both seams are covered by unit test, and the epic already lists them as post-submodule-bump follow-ups.UI screen recording / screenshots:
Full page — AI mode, rail leading with the Configuration card:
Configuration card, all five shapes at the widened 276px rail:
Column test · <column>)Checklist:
Fixesabove.