Conversation
harshach
requested review from
a team,
ShaileshParmar11,
akash-jain-10,
chirag-madlani,
karanh37 and
tutte
as code owners
September 9, 2026 18:43
Contributor
✅ PR checks passedThe linked issue has a description and all required Shipping project fields set. Thanks! |
github-actions
Bot
added
backend
safe to test
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
There are verified issues in the new CI artifact download recovery logic and an API-contract mismatch in test pipeline creation that should be corrected before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR targets merge-queue reliability by hardening GitHub Actions artifact transport/recovery and by removing common Playwright/E2E races (selection, search readiness, polling, and deterministic cleanup), while also isolating flaky shared dependencies (AutoPilot MySQL source, Airflow DAG lifecycle, and retry-queue worker interference).
Changes:
- Add bounded artifact upload/download recovery + execution/commit integrity normalization, and tighten merge-group gating to require verifiable execution evidence.
- Stabilize Playwright helpers/specs (focus-first selection, response predicates, execution-aware polling for contracts/AutoPilot, deterministic API context disposal/cleanup).
- Improve supporting infrastructure/tests: dedicated AutoPilot MySQL fixture, Airflow DAG deploy/delete behavior + integration tests, retry-queue IT isolation, and richer server-latency/performance reporting.
File summaries
| File | Description |
|---|---|
| openmetadata-ui/src/main/resources/ui/playwright/utils/serviceFormUtils.ts | Stabilize select interactions via focus and post-selection assertions. |
| openmetadata-ui/src/main/resources/ui/playwright/utils/search.ts | Use page-scoped API context for domain asset-count gating; fail fast on HTTP errors. |
| openmetadata-ui/src/main/resources/ui/playwright/utils/dataContracts.ts | Refactor contract validation/polling with stricter response matching and execution-id-based polling. |
| openmetadata-ui/src/main/resources/ui/playwright/utils/contractExecution.ts | New API polling helper for contract execution results with strict validation. |
| openmetadata-ui/src/main/resources/ui/playwright/utils/contractExecution.test.ts | New helper tests for contract execution polling behaviors. |
| openmetadata-ui/src/main/resources/ui/playwright/utils/autopilotExecution.ts | New API polling helper for AutoPilot workflow instance completion and error surfacing. |
| openmetadata-ui/src/main/resources/ui/playwright/utils/autopilotExecution.test.ts | New helper tests for AutoPilot polling scenarios. |
| openmetadata-ui/src/main/resources/ui/playwright/utils/AutoPilot.ts | Switch AutoPilot status checks to execution-aware polling via new helper. |
| openmetadata-ui/src/main/resources/ui/playwright/support/fixtures/serverLoad.ts | Clarify routing/cache implications when intercepting requests under server-load fixture. |
| openmetadata-ui/src/main/resources/ui/playwright/support/entity/TableClass.ts | Allow configurable test-suite pipeline schedule interval for redeploy scenarios. |
| openmetadata-ui/src/main/resources/ui/playwright/support/entity/ingestion/ServiceBaseClass.ts | Remove unnecessary routing and stabilize runner selection assertions. |
| openmetadata-ui/src/main/resources/ui/playwright/support/entity/ingestion/MySqlIngestionClass.ts | Allow per-test injected MySQL connection settings (supports dedicated AutoPilot source). |
| openmetadata-ui/src/main/resources/ui/playwright/helper-tests.config.ts | New Playwright config to run helper/unit-like tests in CI. |
| openmetadata-ui/src/main/resources/ui/playwright/eslint-rules/tests/corpus.test.mjs | Update baseline counts after locator-rule improvements. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/DataContracts.spec.ts | Simplify contract validation visibility assertion based on new deterministic polling. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/PageObject/Explore/OverviewPageObject.ts | Replace forced click with retryable open-and-assert flow for owner picker. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Flow/MetricListSearch.spec.ts | Derive restored count from clear response; add response shape checks. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Flow/IngestionBot.spec.ts | Ensure API context is always disposed and gate asset picker on indexing. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/TestSuitePipelineRedeploy.spec.ts | Make fixture creation deterministic, assert per-pipeline deploy responses, add cleanup. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/GlobalPageSize.spec.ts | Stabilize page-size changes with response predicates and retryable menu interactions. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/DataQuality/TestLibrary.spec.ts | Use retryable select option helper; assert visible trigger text for selection. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/ContextCenterMemories.spec.ts | Tighten search response waiting and scope options to popover to avoid locator collisions. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/AutoPilot.spec.ts | Add dedicated AutoPilot MySQL support, execution-aware polling, and stronger cleanup. |
| openmetadata-ui/src/main/resources/ui/eslint-suppressions.json | Remove suppressions made unnecessary by locator/test fixes. |
| openmetadata-ui-core-components/src/main/resources/ui/src/components/base/select/select.test.tsx | New regression test for Select behavior in modal/unfocused trigger scenarios. |
| openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/SearchIndexRetryQueueIT.java | Isolate retry-queue IT from app worker; add stability test asserting no worker interference. |
| openmetadata-integration-tests/src/test/java/org/openmetadata/it/bootstrap/TestSuiteBootstrap.java | Add accessor to retrieve the search-index retry worker managed object. |
| openmetadata-airflow-apis/tests/unit/test_operation_diagnostics.py | Adjust unit test stubs to align with new Airflow deletion path. |
| openmetadata-airflow-apis/tests/integration/operations/test_dag_lifecycle.py | New Airflow 3.x integration tests for versioned DAG deletion, invalid DAG deploy, concurrency. |
| openmetadata-airflow-apis/openmetadata_managed_apis/operations/deploy.py | Ensure deploy fails when DAG isn’t actually loaded; improve exception logging. |
| openmetadata-airflow-apis/openmetadata_managed_apis/operations/delete.py | Delegate DAG deletion to Airflow’s delete_dag to respect FK ordering and running-task rejection. |
| docs/index.md | Index new merge-queue recovery documentation. |
| docker/development/playwright-autopilot-mysql.sql | New SQL fixture for isolated AutoPilot MySQL source. |
| .github/workflows/populate-playwright-caches.yml | Adjust cache-warmer concurrency behavior (no cancel-in-progress). |
| .github/workflows/playwright-postgresql-e2e.yml | Use retry-capable artifact downloads; add normalization + native-evidence coverage requirement. |
| .github/workflows/playwright-e2e-reusable.yml | Use retryable artifact transport; run helper polling tests; extend diagnostics and execution status stamping. |
| .github/workflows/merge-queue-daily-report.yml | Grant actions: read for richer reporting. |
| .github/workflows/integration-tests-postgres-opensearch.yml | Rename retry artifact to avoid ghost-name collision; use retryable downloads. |
| .github/workflows/integration-tests-mysql-elasticsearch.yml | Rename retry artifact to avoid ghost-name collision; use retryable downloads. |
| .github/scripts/verify_playwright_coverage.py | Add native-evidence verification option and quarantine inventory accounting. |
| .github/scripts/tests/test_playwright_server_latency.py | New unit tests for bounded latency histogram behavior and merge semantics. |
| .github/scripts/tests/test_playwright_native_coverage.py | New unit tests for native-evidence coverage verification. |
| .github/scripts/tests/test_playwright_ci_planning.py | Update merge-group summary expectations to require execution evidence + integrity; add conflict test. |
| .github/scripts/tests/test_playwright_artifact_recovery.py | New unit tests for normalization/integrity rules around transport retries and reruns. |
| .github/scripts/tests/test_merge_queue_recovery_metrics.py | New unit tests for paginated checks and report aggregation behavior. |
| .github/scripts/test_classify_playwright_outcome.py | Add regression for raw first-attempt accounting (retries/skips). |
| .github/scripts/summarize_playwright_requests.py | Add bounded latency histograms per route + merge support. |
| .github/scripts/stop_playwright_fast_environment.sh | Ensure AutoPilot MySQL container is removed during teardown. |
| .github/scripts/start_playwright_fast_environment.sh | Start isolated AutoPilot MySQL fixture and export related env vars. |
| .github/scripts/start_playwright_autopilot_mysql.sh | New script to boot and health-check isolated MySQL fixture container. |
| .github/scripts/render_playwright_summary.cjs | Normalize artifacts, treat optional reporting failures as warnings, and emit server-traffic/latency summary. |
| .github/scripts/playwright_cache_fingerprint.py | Include new AutoPilot MySQL fixture inputs in cache fingerprint. |
| .github/scripts/normalize_playwright_artifacts.cjs | New execution artifact normalizer validating shard/commit/run/attempt identity and reconciling retries. |
| .github/scripts/merge_queue_metrics.py | Add paginated REST helpers, richer failure evidence, and report artifact reading (via gh api). |
| .github/scripts/merge_queue_daily_report.py | Expand daily report with failure details + Playwright report aggregation and formatting updates. |
| .github/scripts/evaluate_playwright_performance.py | Merge latency histograms across shards and emit richer performance metrics. |
| .github/scripts/classify_playwright_outcome.py | Add measurement fields (first-attempt vs retries), and surface coverage/integrity verification into output. |
| .github/playwright/impact-map.generated.json | Update generated impact map for changed/added Playwright coverage. |
| .github/actions/upload-artifact-with-retry/action.yml | New composite action for bounded upload recovery under a fresh name. |
| .github/actions/download-artifact-with-retry/action.yml | New composite action for bounded download recovery, including -retry fallback. |
Review details
- Files reviewed: 61/61 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
chromium-01 failed waiting 15s for "Task resolved successfully". The approval had worked -- the failure snapshot shows the article already "Approved" -- so only the notification was missed. The toast auto-dismisses, and `approveTaskFromDetails` awaits the task-action response and then a possible confirmation modal before returning. By the time an assertion placed after it starts looking, the toast can have come and gone. Starting the wait first means it is already polling while all of that happens. Reuses `toastNotification` rather than building a second locator: its `.first()` is deliberate (toasts stack, and other workers' sockets push unrelated ones into the page), and a new positional locator would have grown eslint-suppressions.json, which may only shrink. The promise is caught and rethrown after the approve so a failure there surfaces as itself rather than as an unhandled rejection. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
main's #32252 unified Owner into a single core-components component, which changed the DOM these specs reach for: the selectable-list option is now `<button data-testid="owner-option">` inside an `<li>`, carrying the `active` class itself, where it used to be a wrapper with a `title` and a listitem role. Every content conflict is that change meeting this branch's determinism work. Resolutions: - DataProductAndSubdomains, DomainUIInteractions, entity.ts — main's `owner-option` locator, since the listitem role no longer exists. Not main's five-attempt retry loop: this branch replaced that with a blocking waitForSearchIndexed plus an exact response matcher, which is strictly more deterministic. `maxRetries` went with the loop, so it is dropped rather than left unused. - entity.ts additionally keeps this branch's `usersPanel` scope instead of main's page-wide locator — the Teams tab renders its own owner-option list that a page-wide one can reach into. - OverviewPageObject — main's locator, and its placement after the search response and loader wait, because the list is only final then. The early getByTitle declaration is gone: the title attribute it matched no longer exists, and `active` moved onto the button it now targets. - teams.png — main's baseline. Ours came from an earlier merge of main; main's was regenerated by #32252, the commit that changed how the page renders. Users.spec.ts auto-merged but did not pass: main added a bare `adminPage.reload()`, which this branch's om-playwright/ no-implicit-navigation-load rule rejects. Given an explicit `waitUntil: 'domcontentloaded'`; the assertion on the destination state was already on the next line. impact-map.generated.json regenerated for the incoming specs. Verified: lint:playwright 0 errors, src eslint clean on the merged SelectableList, i18n in sync, no conflict markers, nothing unmerged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Twice on this branch a shard died on a timeout that named the wrong
thing. chromium-04 spent 167s waiting for a lineage tab that was never
going to render, and chromium-08 15s on a sidebar; in both cases the app
had already been replaced by its error boundary -- once by a chunk that
failed to link ("does not provide an export named 't'"), once by a lazy
element that resolved to undefined (React #306). Working that out took a
trace download and, for the first, pulling the built bundle apart.
The context fixture now watches every page in the context for those
signatures and attaches what it saw to the test. The listener is
deliberately narrow: only errors that mean the route is gone, not console
noise, 404s or React warnings. Checked against both real messages plus a
benign 404, a key-prop warning, and `#3060` -- which the word boundary
correctly leaves alone.
Recorded rather than thrown. Failing a test that otherwise passed would
change behaviour across every spec at once, and the cost here was never
detection -- it was diagnosis. This puts the cause next to the timeout it
produced.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UserTeamSelectableList.test.tsx stubs Popover out entirely --
`({children}) => <div>{children}</div>` -- and then asserts only that
`isOpen: true` reaches it. That assertion holds whether or not the popover
can render, which is how the bulk-edit owner picker regressed with the
unit tests green.
Two tests that can actually fail:
- a jsdom case rendering the real Popover, for the shape the bulk-edit
grid uses (`popoverProps={{ open: true }}`, so open on first render),
plus its negative;
- a browser case, since jsdom has no layout and react-aria's positioning
is geometry-dependent. It builds a react-data-grid cell editor around
the real core-components Popover with the same mounted-flag gate,
triggerRef and an onOpenChange wired to close the editor.
Both pass today, and that is the point of recording them: this shape is
NOT what breaks the picker, so the next person does not spend the time I
did eliminating it. If either goes red the regression really is in the
Popover.
The browser bundle aliases react/react-dom to one copy. ui-core-components
is a yarn `link:` and otherwise resolves its own, which leaves every hook
reading a null dispatcher and nothing mounts at all.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#32252 replaced the app's OwnerAvatarStack with the core one. The app's version wrapped every stacked avatar in a Link carrying data-testid "owner-link" and an aria-label, with the owner-name test id nested inside; the core AvatarGroup renders a bare span with only the name test id. So an entity with two or more owners lost the ability to navigate to an owner at all -- the avatars are not clickable -- and both shapes the suite uses to find an owner stopped resolving: addMultiOwner's `[data-testid="owner-link"]` -> name chain, and `getByRole('link', { name: owner })`. One owner was unaffected, since that path renders an OwnerChip, which still has the wrapper. owner-chip.tsx already documents this contract for its own non-compact branch ("owner-link -> name chains both resolve"); this restores it for the stack. Verified by reverting the change and watching ApiCollection "Owner Propagation" fail on the link assertion.
#32252 (862e2d9) removed clickActiveGridCell but left the call in openOwnerPickerEditor. Playwright transpiles specs without type checking, so instead of failing the build it threw a ReferenceError on every pass of the retry loop -- straight into that loop's empty catch, which then pressed Escape and tried again. The helper therefore never clicked the cell and never sent Enter or F2, and all six attempts ended with the picker still closed: playwright/utils/importUtils.ts(431,13): error TS2552: Cannot find name 'clickActiveGridCell'. Did you mean 'doubleClickActiveGridCell'? The product was never broken. A DOM observer over the failing run shows the picker opening the moment the cell is actually clicked (editor=true tabs=true), then closing on the second press of a double click, which lands on body and reads as a click outside the popover. That is why the restored helper clicks once rather than reusing doubleClickActiveGridCell. Verified against the production bundle the container serves, which is what CI runs -- the dev server hides this behind a separate StrictMode-only fault.
The guard's own comment claimed only that the Popover was not at fault. The cause is now known -- a deleted helper left a dangling call -- so say so.
The multiple-containers allow case read `.all()` on the container locators and called `.isVisible()` on the result. `.all()` resolves against the DOM as it is at that instant -- it has none of the auto-waiting `expect(locator)` gives you -- so when the containers had not mounted yet the list came back empty, `.some()` was false, and the assertion failed with no retry. That is chromium-14's "Topic allow common operations permissions": the buttons are in the failure snapshot, they just were not there yet when the check ran. The test passes locally on the same build, which is the tell. Retry the whole read via toPass so the assertion measures the settled page. Leaves the deny case alone deliberately: `.every(v => !v)` is vacuously true on the same empty list, so the race makes it pass early rather than fail. That is a weak assertion rather than a flaky one, and tightening it needs a separate look at whether the containers render at all under deny.
owners is stripped from the storage json for every entity -- EntityRepository.FIELDS_STORED_AS_RELATIONSHIPS carries it alongside href, children, tags, domains, dataProducts, dataContract, followers, experts, reviewers and certification -- so it is always rebuilt from the OWNS relationship. editors, which KnowledgePageRepository derives the same way from EDITED_BY, had no such protection: it is absent from that list and this repository overrode nothing. Nothing persists it today, but only because editors is missing from KNOWLEDGE_PATCH_FIELDS, so the update path leaves it null at store time. That is a property of a field list rather than a guarantee -- adding editors there would silently start writing a snapshot of the relationship into the json, and setFields hands the stored value back to any caller that does not request the field. A sweep of every jsonb column in a populated database backs this up: no entity table stores owners except nine table_entity rows holding an empty array, and entity_extension, which is version history and keeps the whole entity on purpose. The test fails without the override and covers owners too, so the base list cannot quietly lose it either.
…e changing Both header counts are system-wide totals. The test captured one before clicking a folder and compared a later read against it, so any document another worker deleted in between showed up as a failure of this test: chromium-24 read 77 before the click and 52 after, having lost 25 documents it never created. The property being tested is that the folder-view header is not scoped by the sidebar selection. Assert that directly -- it still counts the document outside the folder as well as the one inside, both of which this test created and owns -- instead of re-checking a number it does not control. The earlier comparison of the two headers to each other has the same defect, two lines up, so re-read both counts on each attempt rather than measuring one against a value captured a moment earlier.
chromium-09 spent its whole 60s timeout retrying a click that Playwright had already resolved to the right button: the form's Save sits bottom-right, the toast region is fixed bottom-center, and an error toast was parked on top of it intercepting every attempt. The toast was not this test's. It read Entity not found: glossary "PW%'0ac272da.Noblec831c3bf" which the Glossary landing page raises while restoring a glossary a parallel worker had already deleted. waitForToastStackToClear would not have helped -- that toast never leaves on its own, which is why the click burned the full timeout instead of recovering. dismissToasts closes what is on screen rather than waiting for it to go, and lives next to the other toast helpers since any bottom-anchored button can be covered this way.
TaskDetailPanel renders a comment row's edit/delete icons behind `isHovered` (line 435), so they exist in the DOM only while the pointer is on the card. The test hovered once and then clicked, which leaves a window: anything that re-renders the comment list -- the refetch after the second comment lands, an activity-feed push -- resets that state, unmounts the icon, and the click waits on a locator that will never resolve. chromium-19 spent its full 60s timeout there, and its failure screenshot shows the panel having re-rendered down to a single comment card. Hover and click as one retried unit so a re-render costs a retry instead of the test. The click only runs after a fresh hover, and a successful click ends the loop, so it cannot fire twice.
The multiple-containers deny case read `.all()` and asserted `.every(visible => !visible)`. That is vacuously true on an empty list, so the check passed whenever the containers had not mounted -- and measuring it across fourteen entity types showed the list resolving to zero elements every single time. The assertion has never once separated a denied page from an unrendered one; it has been passing for free since it was written. Anchor it on the containers, which render regardless of permission (GlossaryTermsSection emits glossary-container from both of its branches), and then assert the button is absent. Absence is the right test rather than invisibility: TagsContainerV2 renders add-tag behind `permission && isEmpty(tags)`, so under deny it is never in the DOM at all, which the same measurement confirms. Both assertions auto-wait, so this also drops the hand-rolled `.all()` read that made the allow case need an explicit retry. Verified two ways: the seventeen deny tests pass against the new assertion, and pointing the button selector at the containers instead makes it fail with "Expected: 0, Received: 16" -- so it can now actually fail.
…nothing Three checks in this file could be satisfied by a page that had not rendered, which is indistinguishable from the permission behaviour they claim to verify. Measured across all fourteen entity types, under both allow and deny: deny / direct target present in DOM: 0 everywhere deny / with-manage-button manage-button present: 2 of 14 entities allow / with-manage-button manage-button present: 14 of 14 entities owner-label and Tier present on every entity in both modes `not.toBeVisible()` is also satisfied by an element that is simply absent, so the deny/direct check could never tell denial from a page still loading. It now asserts absence outright, which the measurement says is the real behaviour -- the affordance is not rendered rather than hidden. The with-manage-button checks guarded on `isVisible()`, a point-in-time read, and did nothing at all when it was false. Under deny that was twelve of the fourteen entities: a menu is genuinely not offered there, which is a legitimate denial, so its absence must not fail -- but the action must still be shown to be absent, and now it is. Under allow the menu is always offered, so that side requires it instead of skipping. Both branches reach the assertion through openManageMenu, because the trigger is a react-aria Dropdown and the two configs run back to back: the second arrived with the menu already open, so a bare click would have closed it and the assertion would have read a menu that was not on screen. testCommonOperations now waits for the header these affordances live in before running any of them, so "absent" means denied rather than not-yet-rendered. Verified: the fourteen allow and fourteen deny tests pass, and pointing either rewritten assertion at an element that does exist makes it fail with "Expected: 0, Received: 1" -- so both can now actually fail. Removing the `.first()` from deny/direct drops one recorded positional-locator violation, so the suppressions baseline shrinks from 9 to 8.
Last of the deny branches that could pass by doing nothing.
`getByText('Usage').first()` with `not.toBeVisible()` is also satisfied by text
that has not rendered yet, so it could not tell a denied page from a loading
one. All four call sites -- table, database, dashboard and mlmodel -- pass this
config, and two of them reach it with no precondition beyond navigating to the
entity.
Measured on all four, in both modes:
allow Usage matches: 1, visible: 1
deny Usage matches: 0, visible: 0
entity-header-title present in every case, both modes
So denial removes the label rather than hiding it, which makes absence the right
assertion, and the header is a sound point to take it from. Settle the page
first, since a table reaches this after switching to the profiler tab.
Verified: the entity-specific allow and deny tests pass, and pointing the
assertion at the header instead makes it fail with "Expected: 0, Received: 1",
so it can now actually fail.
Dropping the `.first()` clears another recorded positional-locator violation;
the suppressions baseline shrinks from 8 to 7.
The UI Checkstyle playwright step reported zero ESLint errors and failed on a guardrail instead: corpus.test.mjs pins the suppressions totals by exact equality, and fixing two positional-locator violations left the recorded number stale. om-playwright/no-positional-locator: 1109 -> 1107 Those two are the `.first()` calls dropped from the deny branches of checkElementVisibility while making those assertions able to fail; the pruned eslint-suppressions.json went with them in 1c85a29 and 6ee6801. Lowering the number here is what that test's own comment asks for -- a count going up would mean a new violation was suppressed instead of fixed. Verified with the command CI runs: 133 tests, 133 pass. Prettier reports no diff across the changed playwright and core-components files, so nothing else in the job was dirty.
…g nothing Measured every remaining site first, settling the page before reading -- the first pass sampled too early and reported absences that were really not-yet-rendered: Tier / certification controls present and visible on all 14, both modes their pickers, after a click 0 cards on all 14 under deny custom_properties tab present and visible on all 14, both modes custom-properties-card present in both modes its edit icons hundreds under allow, 0 under deny .react-flow on the Lineage tab present under allow and under deny edit-lineage 1 under allow, 0 under deny Three `isVisible()` guards wrapped these checks, and unlike manage-button -- legitimately absent on 12 of 14 entities -- every one of these controls renders in both modes. The guards could therefore only ever skip on a page that had not finished rendering, which is indistinguishable from passing. They are now required, and each check asserts absence, which is what denial actually produces at all of these sites. The Lineage checks anchor on the canvas, and the two permission-error helpers settle before judging: both take the absence branch under allow, which an unrendered panel satisfies just as well as a permitted one. `type` was `string`, so a mistyped check fell through to `default` -- asserting `not.toBeVisible()` on a test id nothing renders, a guaranteed silent pass. It is a union now, and both defaults throw. The narrowing immediately caught that testIdsConfigs was still inferred as `string`. Verified: all 49 tests in the spec pass, and each rewritten assertion fails when pointed at something that does exist -- tier picker, custom-property card and edit-lineage all report "Expected: 0, Received: 1". Dropping four `.first()` chains from the custom-property assertions shrinks the positional-locator baseline 1107 -> 1103, with corpus.test.mjs lowered to match.
The plan step has been failing since 26ccb5f, so no Playwright shard has run at all on the last two pushes -- the board was not red, it was empty: Atomic Playwright units exceed their lane's execution budget: chromium|Features/ContextCenterArticles.spec.ts|Context Center Articles (20.2m) Forty-four tests in one top-level describe, which the planner sums into a single atomic unit and weighs against COMMON_SHARD_BUDGET_MS (19m). The recent full-suite timing history puts them at 23.3m together, so the unit cannot be placed on any shard and the planner refuses the whole plan. This is the case AUDITED_PARALLEL_SUITES exists for, and BulkImport was added to it for exactly the same failure at 25.8m. Splitting is safe here: the chromium project inherits fullyParallel, so these tests already run across workers today, and every fixture the describe shares is a module-scoped binding assigned in beforeAll from a uuid(), so a parallel unit builds its own entities rather than racing for shared ones. It is not free: the units land on 28 shards and beforeAll re-runs on each, creating roughly fifteen entities every time. That is worth saying out loud because the alternative is not a cheaper plan, it is no plan. Verified by reproducing the failure locally against the real inputs -- the timing-history artifact CI used plus a generated test list. Without the entry the planner exits 1 with "Context Center Articles (23.3m)"; with it the planner exits 0 and lays out 47 shards. The planner's own 125 tests still pass. Noted while here: .github/playwright/timing-baseline.json still has this suite under budget, so the committed fallback disagrees with measured reality.
The scoped-query test failed with
strict mode violation: locator('.ontology-g6-container') resolved to 2 elements
because query mode mounts OntologyGraphG6 twice -- OntologyExplorer draws the
studio graph and OntologyQueryResults draws the SPARQL results -- and both carry
that class. readGraphEdges selected it bare, so it matched both.
Two RDF runs on this same commit disagreed, one passing and one failing, which
is the tell: whether both graphs are mounted at the moment of the read is a
race. The passing direction is not obviously better. The helper's wait uses
`document.querySelector`, which silently takes whichever element comes first, so
a run that got away with it may have read the explorer graph and found the edge
there -- the relation is projected into both -- and reported success for a graph
the test was not asking about.
Scope the read to the results container instead, which pins it to the graph
under test in both directions. The selector is a named export because the same
hazard applies to any caller reading a graph while the studio's own is on
screen; the other four callers run with a single graph mounted and keep the
default.
Not verified locally: the ontology lane needs an RDF backend and this stack has
none -- /system/config/rdf reports rdfEnabled false -- so this rests on the DOM
the failure printed plus the markup in OntologyQueryResults, and CI is the
check.
I broke this in 4e30000. Requiring certification-value and the custom-properties tab was justified by measuring fourteen data-asset entity types -- but testCommonOperations has a second caller I never measured, ServiceEntityPermissions, and service pages carry neither control. Sixteen tests failed, every service type in both modes: expect(locator).toBeVisible() failed Locator: locator('[data-testid="custom_properties"]') entityPermissionUtils.ts:347 Locator: getByTestId('certification-value') entityPermissionUtils.ts:333 Measured across all eight service types, both modes: certification-value 0, custom_properties 0, while owner-label and Tier are present -- so the header anchor was fine and only these two assertions were wrong. Branch on whether the surface offers the control, which is what the original code did. The difference from the original is the anchor: it guarded on `isVisible()` with nothing proving the page had rendered, so skipping and passing were indistinguishable. The anchor now runs first, so a count of zero is a real absence. Tier stays required -- present on all fourteen entities and all eight services. Verified both callers: ServiceEntityPermissions 47 passed, EntityPermissions 49 passed. The lesson is narrower than "measure first": measure every caller, which is the same mistake manage-button nearly caused and this time I walked into.
The test scrolled the left hierarchy to the parent, expanded it, then asserted the child node was visible without scrolling to the child. The hierarchy is an infinite-scroll list, so expanding a node says nothing about whether its child landed inside the rendered window. That was survivable while the Context Center held few articles. It stopped being survivable in the same run that first split this suite across 28 shards, each re-running a beforeAll that creates its own articles: the list got long enough that the child fell outside the window and the assertion found nothing. scrollHierarchyToNode already walks an infinite-scroll list to a named node, and the test already uses it for the parent. Use it for the child too.
… per spec def99b5 made this suite schedulable by splitting it per spec. That worked -- 44 units, plan exits 0 -- but the units landed on 28 shards, and this describe's beforeAll creates roughly fifteen entities every time it runs. Twenty-eight copies of that setup is a lot of churn to add to a shared server, and the same run produced a hierarchy-pagination failure caused by exactly that: the Context Center held enough articles that a child node fell outside the rendered window. The suite's natural seams are already the right size. Measured from the same timing history: the twenty top-level tests weigh 12.25m and the four nested describes 3.7m, 2.5m, 2.5m and 2.3m -- every one under the 19m budget. AUDITED_CHILD_SUITE_PARTITIONS cuts along exactly those seams. Against the real CI inputs, same test list and same timing artifact: per spec 44 units on 28 shards, 47 shards total by child suite 5 units on 4 shards, 47 shards total Same plan size, same schedulability, one seventh of the setup. The planner's own 125 tests still pass.
The PR had gone CONFLICTING, and a conflicting PR gets no pull_request runs at all -- GitHub cannot build the merge commit those workflows test. Every run on the branch was the pull_request_target gate variant with zero shards, so the green Playwright check was a no-op and the last three fixes had never been exercised. Four conflicts: build_playwright_shards.py -- main hit the same Context Center Articles budget failure and fixed it the same way I first did, by adding the suite to AUDITED_PARALLEL_SUITES. Kept this branch's planner instead: main's is behind it here, with COMMON_MAX_SHARDS still at 28 where this branch needs 36, older imports, and no AUDITED_CHILD_SUITE_PARTITIONS. Taking main's file made the planner abort with "Lane chromium needs more than 28 shards". This branch already covers the same suite through the child-suite partition, which costs 5 units on 4 shards rather than 44 on 28. lineage.ts -- kept this branch's delete-edge verification and took main's wait for the scene refetch. The two are complementary: main's keeps the next action from racing the re-render, this branch's proves the DELETE named the edge the test asked for. BulkImport.spec.ts -- kept previewBulkImportChanges, which waits on the validation response rather than a vendor loader class. Main's inline validateSuccessfulImportStatus is redundant; the spec already calls it two lines later with a row count. ServiceForm.spec.ts -- neither side compiled: the auto-merge kept main's caCertificateField declaration, so HEAD's fileInput1 and main's testCertPath were both undeclared. Used main's locator with this branch's in-memory buffer, which keeps the asserted file name under the test's control and needs no temp file to write or clean up. Verified: planner exits 0 at 47 shards with its 125 tests passing, playwright guardrails 133 pass, lint 0 errors. The 100 tsc errors under playwright/ are the pre-existing backlog and predate this merge on both sides.
Seven conflicts. schemaChanges.sql (2.1.0 postgres) -- append-only file, both sides appended. Kept both: this branch's audit_log_event.entity_fqn varchar->text widening and main's LOWER(email)/LOWER(name) functional indexes. They are independent DDL. The MySQL counterpart auto-merged correctly; main's functional indexes are Postgres-only by design. impact-map.generated.json -- generated, so regenerated rather than hand-merged, and regenerated a second time after the lint fixes below changed the sources it derives from. `generate_playwright_impact_map.py --check` now passes. ServiceListing.spec.ts -- main extracted toggleServiceTypeFilter, this branch tightened the response matcher to assert which service types the query_filter actually carries. Took both: main's helper drives the UI, this branch's matcher waits on the response. Main's helper is the better interaction -- its own comment records that getByLabel, which this branch was using, is unreliable under CI. Dropped the now-unused waitForAntdPopupToSettle import. lineage/DataAssetLineage.spec.ts -- kept main's openImpactAnalysisTab import, dropped performZoomOut: this branch replaced that call with fitToScreen, so it has no remaining use and would have been an unused import. polling.ts, CustomPropertiesPanel.spec.ts, TeamDetailsV1.test.tsx -- import collisions where both sides added different symbols. Unioned after checking each symbol is actually referenced. Main's new specs violate six of this branch's playwright guardrails, which main does not have. Suppressions may only shrink and corpus.test.mjs pins the totals, so these are fixed rather than suppressed: five page.goto calls gained an explicit waitUntil (each already asserts destination state), and SuggestionsSliderPermissions moved off a status-filtered waitForResponse onto waitForResponseWithStatus, which preserves the first HTTP failure. Verified: playwright lint 0 errors, guardrails 133 pass, impact map drift-free, TeamDetailsV1 15 tests pass, CI script tests 235 pass. Playwright tsc sits at 99 pre-existing errors, one fewer than before the merge. The two remaining test_playwright_cache_assets failures are a local macOS artifact -- the script runs fine standalone and this merge touches neither it nor its tests.
|
One conflict, and both sides had independently made the same fix. OntologyQueryRdf.spec.ts -- scoping readGraphEdges to the SPARQL results graph. Query mode mounts OntologyGraphG6 twice, once for the explorer and once for the results, and both carry .ontology-g6-container, so an unscoped read either trips strict mode or silently takes whichever element comes first. Main inlined the selector at the call site; this branch passes QUERY_RESULT_GRAPH_CONTAINER, exported from ontologyStudio.ts next to the explanation of why it exists. The two resolve to the same string, so this is presentation only -- kept the named constant, since any future caller reading a graph while the studio's own is on screen needs the same selector. Verified: playwright lint 0 errors, guardrails 133 pass, impact map reports up to date, CI script tests 235 pass, playwright tsc unchanged at its 99 pre-existing errors. The two test_playwright_cache_assets failures are the same local macOS artifact as before -- untouched by this merge.
UI Checkstyle failed the src lint step on one error: LineageMap.component.tsx 815:63 Use <Tooltip> from @openmetadata/ui-core-components instead of raw title="" attributes openmetadata-ui-patterns/no-raw-title-attribute The design-system Tooltip cannot be used at this call site. Breadcrumbs renders a crumb's label inside an AriaLink whenever onAction is set and the crumb is not the current one, and TooltipTrigger wraps any child that is not natively focusable -- a span is not -- in an AriaButton. That is a button inside a link: invalid HTML, and the breadcrumb would stop responding to presses. Breadcrumbs truncates the label itself through maxItemWidth and offers no overflow tooltip of its own, so deleting the title would drop the only way to read a clipped crumb. So this takes the narrow disable, matching the one exception the rule already carries in TableAliases, where a native title is likewise the idiomatic overflow affordance and react-aria's Tooltip does not fit the surrounding markup. Also fixes the licence-header step, which was failing as a consequence rather than on its own: it inserted headers into 0 files, then failed its `git status --porcelain` check on the working tree the src step had left dirty. Verified with CI's own sequence on the branch's 59 changed src files -- organize-imports, eslint --fix, prettier --write -- which now reports 198 problems, 0 errors and leaves the tree clean. i18n and app-docs regenerate with no diff; playwright lint 0 errors, guardrails 133 pass, core-components lint clean.
Code Review 👍 Approved with suggestions 5 closed / 6 findingsComprehensive Playwright coverage and CI race fixes addressing shared-helper defects and backend races. Results-JSON upload, DAG registration, retry toggles, stale-request handling, and pagination error recovery have been resolved. Consider scoping the 💡 Quality: selectOwnersOnTab owner-option locator not scoped to active tab panel📄 openmetadata-ui/src/main/resources/ui/playwright/utils/importUtils.ts:464-467 📄 openmetadata-ui/src/main/resources/ui/playwright/utils/entity.ts:308-314 In importUtils.ts ✅ 5 closed✅ Edge Case: Results-JSON upload can now hard-fail PR shards on transient 403
✅ Edge Case: DAG registration wait relies on cross-process clock agreement
✅ Edge Case: Retry on expand toggles collapse when child fetch is slow
✅ Bug: Stale-request guards drop own-search resolver, hang promise
✅ Edge Case: Pagination error is unrecoverable until hook remounts
🤖 Prompt for agentsOptionsDisplay: 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 |
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe your changes:
Fixes #33087
PRs could reach the merge queue with selected Playwright coverage, where full validation exposed shared-helper defects and backend races. Run the full suite on authorized PR pushes, repair those defects, and retain first-attempt failures without test retries or quarantine.
Type of change:
High-level design:
RUNNER_TEMP; source stays read-only and builds/browser checks run without network access. No alert suppression.pull_request_targetexecutes the workflow frommain; then synchronize this PR with main for a fresh validation.Tests run:
Unit tests
Backend integration tests
Ingestion integration tests
c3305dea56: all 41 tests passed in run 34487109427; the Sonar analysis discarded their coverage due to the base-workflow path rewritePlaywright (UI) tests
Fresh-checkout mount regression: reproduced Docker exit 125 with no core dependency/build directories; corrected preflight passes dependency installation, library build, and 15 browser-helper tests with zero retries against a Git-exported checkout
After the main merge: 124 planning tests, 117 lint-rule tests, Playwright lint, and discovery of 4,574 instances pass. The revised container preflight passes a clean dependency install, library build, and 15 browser-helper tests; runtime isolation probes pass
The 50-repetition results and full Maven build below were recorded at
56a4fcc4da, before the latest main mergeLinux Playwright 1.57: 15 browser-helper scenarios × 50 repetitions = 750 passes in 1.6 minutes, three workers, zero retries
All 15 visual scenarios × 50 repetitions = 750 passes in 8.7 minutes against the current-source local Docker stack, three workers, zero retries; only two affected baselines refreshed, with unchanged thresholds
Full Maven distribution build passed in 5:05; isolated component-library preflight build passed in 7 seconds
Local Select/ComboBox and slider reproductions
Full discovery: 4,571 configured test instances, no collection errors
Java/UI formatting and license checks pass. No new Playwright type diagnostic signatures: 165 current versus 166 on main
The full GitHub run on the updated commit is pending. The merge of main includes #33079 and its previously missing
DataInsightsExtension; a fresh Collate compatibility run is pending. The stored-procedure read-after-delete failure observed in #32919 still needs deterministic cache-race reproduction.Operational acceptance remains 50 affected full-scenario repetitions on GitHub under normal concurrency, cold and warm, with zero retries; then 20 complete consecutive merge-group validations and monitoring of the next 100. The local visual repetitions reuse one seeded environment; they do not establish complete-suite or cold/warm GitHub acceptance. No 100% first-pass or throughput improvement is claimed yet.
UI screen recording / screenshots:
No visual design change. Browser reproductions and CI trace evidence are documented in the audit.
Checklist:
Greptile Summary
This PR substantially expands full Playwright validation on authorized pull requests and merge groups while repairing CI races, test-fixture isolation, backend readiness, search encoding, data-contract result lookup, and UI interaction stability.
Confidence Score: 5/5
No new actionable issue was introduced since the previous review, and the PR appears safe to merge based on the reviewed changes.
The only post-review change adds explanatory comments around existing lineage breadcrumb markup. The merge-group cache adjustment is self-contained because every consuming job can rebuild its environment on a miss, and no outstanding previous findings were supplied.
Important Files Changed
Sequence Diagram
sequenceDiagram participant PR as Pull request / merge group participant Plan as Playwright planning participant Build as Distribution and fixture build participant E2E as Native Playwright shards participant Artifacts as Evidence normalization participant Summary as Required summary PR->>Plan: Determine authorized full coverage par Planning and build Plan->>Plan: Discover and validate test inventory Build->>Build: Build distribution and isolated UI dependencies end Plan->>E2E: PostgreSQL, RDF, visual, and data-access plans Build->>E2E: Server and stable fixtures E2E->>Artifacts: Raw first-attempt results and execution evidence Artifacts->>Summary: Validate completeness and attribution Summary-->>PR: Pass only with valid native execution evidenceReviews (3): Last reviewed commit: "fix(ui): justify the lineage breadcrumb'..." | Re-trigger Greptile