Skip to content

Improvement: lint against ambiguous search/aggregate waits in Playwright - #31873

Open
harsh-vador wants to merge 2 commits into
mainfrom
chore/lint-aggregation-wait-helper
Open

Improvement: lint against ambiguous search/aggregate waits in Playwright#31873
harsh-vador wants to merge 2 commits into
mainfrom
chore/lint-aggregation-wait-helper

Conversation

@harsh-vador

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

Copy link
Copy Markdown
Contributor

Describe your changes:

Fixes #31859

Follow-up to #31860, which fixed the stale-response race in the Explore filter dropdowns. This one goes after the test-side half of that bug so it cannot be written again.

The mistake worth linting. A facet dropdown fires two aggregations against the same field: one when it opens (value=.*) and one per typed search (value=.*text.*). A wait like waitForResponse('…/aggregate?…&field=ownerDisplayName*deleted=true*') matches both. In #31859 it resolved off the dropdown-open request, so the test ran ahead of the search it had queued, and the orphaned response landed in a dropdown opened later — behind a passing assertion.

  • playwright/utils/searchAggregation.tswaitForAggregation(page, { field, value, deleted }). Matches on parsed URL params rather than a glob, which removes the encoding footguns entirely. value is required and takes null for "the dropdown-open request", so intent is stated at every call site instead of being inferred. Backslashes are stripped from both sides of the comparison because getAggregateFieldOptions runs the search text through escapeESReservedCharacters before putting it in value — without that, any value containing - or : would never match and would time out.
  • eslint-rules/openmetadata-playwright.mjs — new rule require-aggregation-wait-helper, flagging raw waits on search/aggregate in string, template-literal and URL-predicate form. The helper module itself is exempt.
  • Severity: warn. 36 call sites exist today; this PR migrates 8, leaving 27. That matches the convention already in eslint.config.mjserror for rules with a zero backlog, warn for ones being worked down. Promote to error once the rest are migrated.
  • Migrated the sites where the ambiguity is a live latent bug: ExploreDiscovery.spec.ts (4 waits, replacing the value-globs from Fixes 31859: discard an Explore filter search that resolves after its dropdown closed #31860 with the helper), utils/explore.ts (2), utils/glossary.ts, DataProductCertificationFilter.spec.ts. The explore-tree drill-down is a POST aggregate with no field/value pair to discriminate on, so it carries a narrow suppression with the reason inline.

Two of the 36 sites are legitimate dropdown-open waits (utils/advancedSearch.ts, AdvancedSearchSuggestions.spec.ts) — the helper expresses those as value: null, which is why the option exists rather than the rule demanding a value.

Stacked on fix/explore-quick-filter-stale-options (#31860) because both touch ExploreDiscovery.spec.ts; GitHub will retarget this to main when that merges.

Type of change:

  • Improvement

High-level design:

N/A — small change.

Tests:

Use cases covered

  • A wait armed for a typed search no longer resolves on the dropdown's open request.
  • A deliberate wait for the open request is expressible (value: null) and not flagged.
  • Search values containing ES reserved characters still match.
  • Raw waits on the aggregate endpoint are reported in all three forms (string, template literal, URL predicate).

Unit tests

  • eslint-rules/openmetadata-playwright.test.mjs — 4 valid / 3 invalid cases via RuleTester, plus the plugin-export assertion, in the same style as the existing rule tests.
  • yarn test:eslint-rules → 93 tests, 93 pass.
  • npx eslint playwright eslint-rules → 0 errors (299 warnings: 272 pre-existing + the 27 remaining sites this rule now names).
  • npx tsc --noEmit → 0 errors under playwright/.

Backend integration tests

  • Not applicable (no backend changes).

Ingestion integration tests

  • Not applicable (no ingestion changes).

Playwright (UI) tests

  • Migrated 8 existing waits; no new specs.
  • The E2E suite has not been run locally. The migrated waits are stricter than the globs they replace, so the risk to flag in review is a wait that never matches and times out. Mitigated by matching on the documented shape of getAggregateFieldOptions (field, value, deleted on every GET aggregate) and by the backslash normalisation above, but the AUT run is the real check.

Manual testing performed

Lint and rule-unit level only, as above.

UI screen recording / screenshots:

Not applicable — test tooling only, no product code.

Checklist:

  • I have read the CONTRIBUTING document.
  • My PR is linked to a GitHub issue.
  • I have commented on my code, particularly in hard-to-understand areas.
  • I have added tests and listed them above.

🤖 Generated with Claude Code

Greptile Summary

This PR introduces a parsed-URL Playwright helper for aggregation waits and a repository lint rule discouraging ambiguous raw waits.

  • Migrates selected Explore, glossary, and certification-filter waits to the new helper.
  • Adds ESLint rule coverage for direct, predicate, local-variable, outer-scope, reassigned, and concatenated matchers.
  • Enables the rule as a warning while existing call sites are migrated.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
openmetadata-ui/src/main/resources/ui/eslint-rules/openmetadata-playwright.mjs Adds the Playwright-specific aggregation-wait lint rule and identifier lookup support.
openmetadata-ui/src/main/resources/ui/eslint-rules/openmetadata-playwright.test.mjs Adds RuleTester coverage for supported raw-wait matcher forms and plugin export registration.
openmetadata-ui/src/main/resources/ui/playwright/utils/searchAggregation.ts Adds parsed-URL matching for typed and dropdown-open aggregation requests.
openmetadata-ui/src/main/resources/ui/eslint.config.mjs Registers the new Playwright plugin and enables its aggregation-wait rule as a warning.

Reviews (7): Last reviewed commit: "Address review: widen the rule's matcher..." | Re-trigger Greptile

@github-actions

github-actions Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

❌ PR checklist incomplete

This 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 skip-pr-checks label.

@github-actions github-actions Bot added the UI UI specific issues label Aug 21, 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!

@github-actions

github-actions Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

✅ Playwright Results — workflow succeeded

Validated commit 2cc0b7f804531440bd030e7c9f51796f6922e94f in Playwright run 32477173313, attempt 1.

✅ 564 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) 48m 43s

⏱️ Max setup 4m 24s · max shard execution 15m 37s · max shard-job elapsed before upload 18m 41s · reporting 4s

🌐 214.33 requests/attempt · 2.81 app boots/UI scenario · 6.08% common-shard skew

Optimization targets still in progress:

  • Browser traffic was 214.33 requests per attempt (convergence target: fewer than 200).
  • Application boot ratio was 2.81 per UI scenario (1642 boots / 585 scenarios; convergence target: at most 1).
Shard Passed Failed Flaky Skipped Lifecycle failed Lifecycle flaky
✅ Shard chromium-01 103 0 0 0 0 0
✅ Shard chromium-02 108 0 0 0 0 0
✅ Shard chromium-03 95 0 0 0 0 0
✅ Shard chromium-04 104 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

Comment thread openmetadata-ui/src/main/resources/ui/playwright/utils/searchAggregation.ts Outdated
@harsh-vador
harsh-vador changed the base branch from fix/explore-quick-filter-stale-options to main August 21, 2026 09:06
Comment thread openmetadata-ui/src/main/resources/ui/eslint-rules/openmetadata-playwright.mjs Outdated
Comment thread openmetadata-ui/src/main/resources/ui/playwright/utils/searchAggregation.ts Outdated
@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!

2 similar comments
@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!

@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 21, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Jest test Coverage

UI tests summary

Lines Statements Branches Functions
Coverage: 66%
67.03% (80390/119920) 51.47% (49240/95666) 52.44% (14701/28029)

@sonarqubecloud

Copy link
Copy Markdown

A facet dropdown aggregates the same field twice — once when it opens
(`value=.*`) and once per typed search. A wait naming only the endpoint or the
field matches both, so it can resolve on the open request while the typed
search is still in flight; the test then runs ahead of the request it queued.
That is how #31859 stayed hidden behind a passing assertion.

- playwright/utils/searchAggregation.ts: `waitForAggregation(page, { field,
  value, deleted })` matches on parsed URL params instead of a glob. `value` is
  required and takes `null` (or `''`) for the request fired on open, so the
  intent is stated at the call site. The search text is compared exactly after
  unwrapping the API's `.*text.*`, so a wait for `service` cannot resolve on an
  in-flight response for `service-name`; backslashes are normalised away
  because the API escapes ES reserved characters.
- eslint-rules/openmetadata-playwright.mjs: `require-aggregation-wait-helper`
  flags raw waits on the endpoint — string, template literal or URL predicate —
  and follows an identifier to its declaration so hoisting the URL to a local
  const does not silence it. Registered at `warn` while the remaining 27 call
  sites are migrated, matching the convention used for the other aspirational
  Playwright rules.
- Migrated the sites where the ambiguity is a live latent bug: ExploreDiscovery
  (4), utils/explore.ts (2), utils/glossary.ts, DataProductCertificationFilter.
  The explore-tree POST aggregate carries no field/value to discriminate on and
  is suppressed with a reason.

Verified against a local stack: 45/46 of the affected specs pass; the one
failure (EntitySummaryPanel display-name modal) contains no aggregation wait
and passes in isolation.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@harsh-vador
harsh-vador force-pushed the chore/lint-aggregation-wait-helper branch from 8b940ed to c3a2872 Compare August 21, 2026 10:23
Comment thread openmetadata-ui/src/main/resources/ui/eslint-rules/openmetadata-playwright.mjs Outdated
Comment thread openmetadata-ui/src/main/resources/ui/eslint-rules/openmetadata-playwright.mjs Outdated
Comment thread openmetadata-ui/src/main/resources/ui/playwright/utils/searchAggregation.ts Outdated
…than strip

- resolveMatcherText now walks the scope chain via the same findVariable helper
  openmetadata-performance.mjs uses, and reads later assignments as well as
  declarations, so a matcher declared at module scope or assigned to a `let` no
  longer escapes the rule. Quotes and `+` are dropped before the endpoint check
  so a path split across concatenated literals still matches.
- searchAggregation compares against an unescaped URL value instead of
  stripping backslashes from both sides, which kept `foo\bar` and `foobar`
  distinct rather than collapsing them onto each other.
- Restores the rule's own doc block, which an earlier comment trim had left
  sitting above resolveMatcherText.

Rule tests cover the module-scope, reassigned-let and concatenated forms: 99
pass. The affected specs still pass against a local stack (27/27).

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

gitar-bot Bot commented Aug 21, 2026

Copy link
Copy Markdown
Code Review ✅ Approved 2 resolved / 2 findings

Adds the waitForAggregation Playwright helper and a new ESLint rule to prevent ambiguous search and aggregate waits, resolving the empty-string value and lint rule coverage findings.

✅ 2 resolved
Edge Case: Empty-string value reintroduces the open-request ambiguity

📄 openmetadata-ui/src/main/resources/ui/playwright/utils/searchAggregation.ts:71-75 📄 openmetadata-ui/src/main/resources/ui/playwright/utils/explore.ts:47-50
matches only special-cases value === null for the dropdown-open request; a non-null empty string falls into the typed-search branch where normalize(value).includes(normalize('')) is always true, so it matches the open request (value=.*) too — exactly the ambiguity the helper exists to prevent. In searchAndClickOnOption, value: filter.value ?? null only maps undefined/null to null, so a caller passing filter.value === '' would silently get open-request matching. Consider treating empty string as null (or asserting non-empty) so intent stays explicit.

Quality: Lint rule misses concatenated/reassigned aggregate URLs

📄 openmetadata-ui/src/main/resources/ui/eslint-rules/openmetadata-playwright.mjs:28-42 📄 openmetadata-ui/src/main/resources/ui/eslint-rules/openmetadata-playwright.mjs:78-80
resolveMatcherText only reads text directly or follows an Identifier to a const ... = init declaration, then flags when the source text contains the contiguous substring search/aggregate. A raw wait built by string concatenation (e.g. page.waitForResponse('/api/v1/search/' + 'aggregate?*')) or via a reassigned let/parameter (where def.node.init is undefined) yields text without the contiguous substring, so it silently escapes the rule. This is an acknowledged, low-impact gap (warn severity, test-only), but the sibling rule openmetadata-performance.mjs already has a findReferencedVariable/findVariable helper that walks scope.upper more robustly; reusing that pattern would narrow the gap further.

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

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.

Explore filter dropdown shows another filter's options when a queued search resolves after it closes

1 participant