Skip to content

Fixes 30654: wait for the newly triggered pipeline run in Playwright incident helper - #30655

Open
ShaileshParmar11 wants to merge 2 commits into
mainfrom
ShaileshParmar11/flaky-test-rca
Open

Fixes 30654: wait for the newly triggered pipeline run in Playwright incident helper#30655
ShaileshParmar11 wants to merge 2 commits into
mainfrom
ShaileshParmar11/flaky-test-rca

Conversation

@ShaileshParmar11

@ShaileshParmar11 ShaileshParmar11 commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Describe your changes:

Fixes #30654

triggerTestSuitePipelineAndWaitForSuccess polled /pipelineStatus?limit=1 and accepted whatever the latest status record was, with no notion of which run it had triggered. Airflow queues a DAG asynchronously, so ~2s after the trigger the previous run's success record is still the latest one — the helper returned immediately and callers went on to assert against stale results.

In run 30439518657 that surfaced as Incident Manager › Resolving incident & re-run pipeline reading an incident status of Resolved where it expected New: the poll read run 178b5ca8 (success, recorded 2m17s before the trigger) while the new DAG was still queued, so no new failure or incident existed yet. Serial-mode describe took 5 further tests down as skipped.

What changed:

  1. Snapshot the run recorded before triggering, then poll until a record with a different runId or a later timestamp reaches success:
    const previousRun = await fetchBaselineRun();
    // Either signal alone is enough: `runId` is optional in the schema, so a
    // conjunction deadlocks the poll when neither record carries one.
    const isNewRun = (latestRun: PipelineStatus) =>
      latestRun.runId !== previousRun?.runId ||
      (latestRun.timestamp ?? 0) > (previousRun?.timestamp ?? 0);
  2. Extracted fetchLatestPipelineStatus() (primary endpoint + entity fallback), previously inlined in the poll. It reports fetched separately from run so a failed read is distinguishable from no run yetfetchBaselineRun() retries and then throws rather than silently treating the stale previous run as new.
  3. Folded the two fixed waitForTimeout(5000) "deployment settling time" sleeps into a bounded retry of the trigger request itself — the orchestrator rejects a trigger until the workflow is registered, so we retry until it is accepted rather than paying 5s on every call. The two waitForTimeout calls that remain are both backoffs inside retry loops (the trigger retry and the baseline-status retry) — neither runs unless a request actually failed.
  4. Typed against generated PipelineStatus / PipelineState instead of string literals.

Net effect on runtime: the target test went 20.2s → 14.3s, whole spec file 2.8m → 2.4m.

Type of change:

  • Bug fix

High-level design:

N/A — single-file test-infrastructure fix.

Tests:

Use cases covered

  • Re-triggering a TestSuite pipeline mid-spec now blocks until that run completes, so incident assertions observe post-re-run state instead of pre-trigger state.
  • A pipeline that has never run before (the beforeAll path) still resolves — previousRun is undefined and the first recorded run counts as new.
  • A trigger rejected because the workflow is not yet registered is retried, and only falls back to re-deploy once the retry budget is spent.

Unit tests

Not applicable — this changes a Playwright helper. Jest is scoped to src (roots: ['<rootDir>/src']) and playwright.config.ts uses testDir: './playwright/e2e', so a test file for this helper has no home that CI would execute. The regression surface is the existing IncidentManager.spec.ts.

Backend integration tests

  • Not applicable (no backend changes).

Ingestion integration tests

  • Not applicable (no ingestion changes).

Playwright (UI) tests

  • Not applicable (no UI changes) — modifies an existing Playwright helper; no new spec added.
  • File updated: openmetadata-ui/src/main/resources/ui/playwright/utils/incidentManager.ts

Manual testing performed

  1. Live E2EIncidentManager.spec.ts against a running stack on localhost:8585:

    npx playwright test playwright/e2e/Features/IncidentManager.spec.ts --project=chromium --workers=1 --retries=0
    → 10 passed (2.4m)
    

    Covers all three re-run call sites (lines 869, 950, 1178) plus the never-run-before path in beforeAll.

  2. Replay harness — a throwaway spec driving the real exported helper against a stub APIRequestContext replaying the exact responses recorded in run 30439518657 (previous run 178b5ca8 / success / ts 1785320083346, then a new runId), counting pipelineStatus reads:

    scenario before fix after fix
    waits for a genuinely new run resolved after 1 pipelineStatus call(s) — Expected 3, Received 1
    never-run pipeline (beforeAll path)
    pipelineStatus down → entity fallback ✘ Expected 3, Received 1
    retries a rejected trigger instead of redeploying
    redeploys once the retry budget is spent
    detects a new run by timestamp when runId is absent on both records ✘ hangs to the poll timeout
    6 failed 6 passed (10.9s)

    Received: 1 is the bug reproduced exactly — the old helper returns on its first poll, off the stale record. The harness was deleted after verifying (see "Unit tests" above for why it has no home in CI).

  3. Staticyarn tsc:playwright: 163 errors before and after (pre-existing baseline for the playwright project), 0 in the changed file. ESLint + Prettier + organize-imports on the changed file: clean, no residual diff.

Caveat, stated plainly: the live E2E passes with and without this change. The local stack is Argo-backed and does not reproduce the window; the trace shows Airflow still queued 2s after the trigger. So the live run is a non-regression check, and the replay harness is what evidences the cure. CI's Airflow-backed Ingestion lane is the real confirmation.

UI screen recording / screenshots:

Not applicable — no UI change.

Checklist:

  • I have read the CONTRIBUTING document.
  • My PR title is Fixes <issue-number>: <short explanation>
  • My PR is linked to a GitHub issue via Fixes #<issue-number> above.
  • I have commented on my code, particularly in hard-to-understand areas.
  • For JSON Schema changes: N/A — no schema changes.
  • For UI changes: N/A — no UI changes.
  • I have added tests (unit / integration / Playwright as applicable) and listed them above.
  • I have added a test that covers the exact scenario we are fixing — the replay harness above reproduces run 30439518657's responses and fails without the fix. It is not committed because neither the Jest nor the Playwright project collects files for this helper; see the "Unit tests" section.

Follow-up: the same fix is needed on 2.0, where the failing run occurred. The file is identical there, so it cherry-picks cleanly.

Review follow-up

isNewRun as originally pushed used && with >=, not the ||/> this description claimed — the description was written from a revision that differed from the committed one. gitar-bot and greptile both caught it. runId is optional in the generated PipelineStatus schema, so with a conjunction, two records that both omit it give undefined !== undefinedfalse forever, and the poll spins to the 300s timeout. Reproduced: that scenario hangs on the conjunction and passes in 2.0s on the disjunction. Fixed in 380cc2f, and the harness now covers it.

🤖 Generated with Claude Code

Greptile Summary

The Playwright incident helper now waits for the pipeline run initiated by the current trigger.

  • Captures the latest status before triggering and identifies a subsequent run by a changed run ID or later timestamp.
  • Retries baseline reads and rejected deployment or trigger requests with bounded backoff.
  • Consolidates pipeline-status retrieval with an entity-endpoint fallback and generated pipeline types.

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/playwright/utils/incidentManager.ts Adds baseline-aware run polling, status-fetch fallback handling, bounded request retries, and generated pipeline-state types; the previously reported conjunctive new-run check is corrected.

Sequence Diagram

sequenceDiagram
  participant Test as Playwright helper
  participant API as OpenMetadata API
  participant Orchestrator as Pipeline orchestrator
  Test->>API: Read latest pipeline status baseline
  Test->>API: Trigger pipeline (bounded retries)
  API->>Orchestrator: Queue new run
  loop Until new run succeeds
    Test->>API: Read latest pipeline status
    API-->>Test: Latest run ID, timestamp, and state
  end
  Test-->>Test: Continue incident assertions
Loading

Reviews (2): Last reviewed commit: "fix(playwright): treat runId or timestam..." | Re-trigger Greptile

…revious one

triggerTestSuitePipelineAndWaitForSuccess polled /pipelineStatus?limit=1 and
accepted whatever the latest record was, with no notion of which run it had
triggered. Airflow queues a DAG asynchronously, so ~2s after the trigger the
previous run's `success` record is still the latest one: the helper returned
immediately and callers went on to assert against stale results.

In CI run 30439518657 that surfaced as "Resolving incident & re-run pipeline"
reading an incident status of "Resolved" where it expected "New" — the poll
had read run 178b5ca8 (success, recorded 2m17s before the trigger) while the
new DAG was still queued, so no new failure or incident existed yet.

Snapshot the run recorded before triggering, then poll until a record with a
different runId (or a later timestamp) reaches success.

Also fold the two fixed 5s "deployment settling time" sleeps into a bounded
retry of the trigger request itself: the orchestrator rejects a trigger until
the workflow is registered, so retry until it is accepted rather than paying
5s on every call.

Co-Authored-By: Claude <noreply@anthropic.com>
@ShaileshParmar11
ShaileshParmar11 requested a review from a team as a code owner July 29, 2026 14:03
Copilot AI review requested due to automatic review settings July 29, 2026 14:03

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions

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 safe to test Add this label to run secure Github workflows on PRs UI UI specific issues labels Jul 29, 2026
Comment thread openmetadata-ui/src/main/resources/ui/playwright/utils/incidentManager.ts Outdated
Comment thread openmetadata-ui/src/main/resources/ui/playwright/utils/incidentManager.ts Outdated
… new run

isNewRun required BOTH a different runId AND a later-or-equal timestamp.
`runId` is optional in the generated PipelineStatus schema, so when neither
the baseline nor the polled record carries one, `undefined !== undefined` is
always false and the conjunction can never become true: the poll spins on
Queued until the 300s timeout and the test fails — the opposite of the
reliability this helper is meant to provide.

Either signal alone is sufficient, and both still reject the stale record
(same runId, same timestamp), so use a disjunction with a strict timestamp
comparison.

Reported by gitar-bot and greptile on #30655.

Co-Authored-By: Claude <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 29, 2026 14:40

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@gitar-bot

gitar-bot Bot commented Jul 29, 2026

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

Updates the Playwright incident manager helper to wait for the newly triggered pipeline run rather than accepting stale status records. The isNewRun predicate logic has been addressed.

✅ 1 resolved
Bug: isNewRun uses && where design/robustness needs ||

📄 openmetadata-ui/src/main/resources/ui/playwright/utils/incidentManager.ts:355-357
isNewRun is implemented as runId !== previousRun?.runId && (timestamp ?? 0) >= (previousRun?.timestamp ?? 0), but the PR's stated design is "a record with a different runId or a later timestamp" (|| with >). runId is optional in the generated PipelineStatus type, so if both the baseline and the polled record lack a runId (both undefined), the first clause undefined !== undefined is always false and, with &&, the whole predicate can never become true — the poll spins on PipelineState.Queued until the 300s timeout and the test fails, the opposite of the reliability this PR intends. The ||/> form is strictly more robust: it still detects a new run via the timestamp when runId is unavailable, and still rejects the stale latest record (same runId, equal timestamp). Change && to || and >= to > to match the documented intent.

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 👍 / 👎 | Gitar | Powered by Gitar — free for open source

@github-actions

Copy link
Copy Markdown
Contributor

✅ Playwright Results — workflow succeeded

Validated commit 380cc2fef6a2365e1f223890ed66459a46527899 in Playwright run 30462034502, attempt 1.

✅ 536 passed · ❌ 0 failed · 🟡 1 flaky · ⏭️ 5 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) 52m 25s

⏱️ Max setup 3m 24s · max shard execution 14m 39s · max shard-job elapsed before upload 18m 25s · reporting 4s

🌐 203.00 requests/attempt · 2.86 app boots/UI scenario · 10.13% common-shard skew

Optimization targets still in progress:

  • Browser traffic was 203 requests per attempt (convergence target: fewer than 200).
  • Application boot ratio was 2.86 per UI scenario (1599 boots / 559 scenarios; convergence target: at most 1).
Shard Passed Failed Flaky Skipped Lifecycle failed Lifecycle flaky
✅ Shard chromium-01 83 0 0 0 0 0
✅ Shard chromium-02 109 0 0 0 0 0
✅ Shard chromium-03 103 0 0 0 0 0
🟡 Shard chromium-04 103 0 1 3 0 0
✅ Shard data-asset-rules-01 61 0 0 0 0 0
✅ Shard domain-isolation-01 14 0 0 0 0 0
✅ Shard global-state-01 23 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 10 0 0 0 0 0
✅ Shard search-rbac-01 27 0 0 2 0 0
🟡 1 flaky test(s) (passed on retry)
  • Pages/Entity.spec.tsUser as Owner with unsorted list (shard chromium-04, 1 retry)

📦 Download artifacts

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

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.

Flaky Playwright spec: Incident Manager "Resolving incident & re-run pipeline"

2 participants