fix(dq): stale incident status on the Test Case page after reopening a resolved incident (1.13)#30183
fix(dq): stale incident status on the Test Case page after reopening a resolved incident (1.13)#30183manerow wants to merge 3 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 |
|
The Python checkstyle failed. Please run You can install the pre-commit hooks with |
🔴 Playwright Results — 1 failure(s), 9 flaky✅ 3968 passed · ❌ 1 failed · 🟡 9 flaky · ⏭️ 31 skipped
Genuine Failures (failed on all attempts)❌
|
|
The Python checkstyle failed. Please run You can install the pre-commit hooks with |
Code Review ✅ Approved 1 resolved / 1 findingsUpdates TestCase incident derivation to query the incident timeline directly instead of using frozen result stamps, ensuring the UI accurately reflects reopened incidents. Added a deterministic tie-breaker for batch queries to resolve the incident selection ambiguity. ✅ 1 resolved✅ Edge Case: Latest-incident selection has no tie-breaker on equal timestamps
OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
|
The Python checkstyle failed. Please run You can install the pre-commit hooks with |
|



Fixes #30168
1.13 backport. The main PR is #30182; this branch carries the read-derivation fix only (the task-first reopen behavior is main-only).
The mental model you need first
When a Data Quality test fails, the system opens an incident: a tracked problem someone has to work. An incident has a lifecycle (New -> Acknowledged -> Assigned -> Resolved) and is identified by a
stateId; every status change appends a row to the incident timeline.Two separate things get written as a test lives its life:
result.incidentId). This stamp is written once, at the moment the result is ingested, and never touched again.stateId.The Test Case page needs to answer one question: "Does this test case have an open incident right now, and what's its status?" It reads a field called
TestCase.incidentIdto do that. That field is the whole story.The problem (what the customer saw)
Someone resolved an incident by mistake, then set it back to Acknowledged. The incident itself showed Acknowledged correctly, but the Test Case page kept showing Resolved, until the test failed again.
Why it happened (how it worked before)
TestCase.incidentIdwas computed from the wrong source: the read paths derived it from the latest test result's stamp (result.incidentId), which is frozen at ingestion time. That answers "which incident did the last run belong to," which is not the same question as "what is the current incident."Walk the reopen scenario:
incidentId = S1.Now
TestCase.incidentId(from the frozen stamp) still says S1, whose latest status is Resolved. So the page shows Resolved even though the live incident is S2/Ack. It only self-corrects on the next failed run, because that writes a new result stamped with the current incident.What the PR changes
This is a read-derivation fix only (the task-first reopen behavior lands on main in #30182).
TestCase.incidentIdnow means one thing on both read paths: thestateIdof the latest unresolved incident record, or null if there is no open incident.TestCaseRepository.getIncidentId(single read) reads the incident timeline (latest record, unresolved only) instead of the result stamp.fetchAndSetTestCaseResultsAndIncidents(bulk/list read) reads the same thing via a new batch query over the incident timeline.Incident lifecycle is unchanged on this branch: reopening a resolved incident still starts a new incident, but the pointer now follows it, so the page shows the current status.
How it works now (same scenario)
TestCase.incidentIdbecomes null -> page shows No Incident (correct: nothing is open).TestCase.incidentId= S2 -> page shows Ack.Verified on a live 1.13.1 deployment: after
Resolvedthe entity returns null, and after the reopen it returns the new incident and the page shows Ack.Why this is the right way
Two intended, correct side effects fall out of this: after a resolve the page shows "No Incident" instead of a stale Resolved chip, and a currently-passing test that still has an unresolved incident now correctly shows it (because the incident is genuinely still open until someone resolves it).
Testing
TestCaseResourceIT#test_incidentIdDerivation_followsLatestUnresolvedTcrs: derivation matrix over the single GET and the bulk list, with the 1.13 expectations (no incident -> null, New/Ack -> stateId, Resolved -> null, reopen -> new stateId the pointer follows, passing run while open -> stateId).TestCaseResourceITpass.Resolved -> Ackthe test case entity returns the new incident and the page shows Ack; afterResolvedit returns null.Greptile Summary
This PR derives the current test-case incident from the incident timeline instead of the latest test result. The main changes are:
Confidence Score: 5/5
No additional blocking issue qualifies for this follow-up review.
Important Files Changed
Reviews (3): Last reviewed commit: "fix(dq): deterministic tie-breaker for l..." | Re-trigger Greptile
Context used: