Rework the Dispatcher PR comment into the target-first layout - #25240
Conversation
❌ Dispatcher tests · failed
Caution Dispatcher tests failed. See the failures below.
Batches
❌ Failures
|
evalya-impact-summaryevalya impact analysis |
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: f5353af | Docs | View more details | Give us feedback! |
Rendered layoutsThe four comments below are the actual output of this branch's renderer, posted so the layout can be reviewed as GitHub renders it rather than as source. There were thirteen of these, and nine are gone. Most of them existed to show the comment changing shape: the compressed one-line form a group switched to at its fifth target, the nested Unlike the previous set, each preview has its own snapshot, chosen for the shape it demonstrates rather than derived from one shared run. They come from a committed module, so they are reproducible and cannot drift from the tested code: cd ddev
PYTHONPATH=. hatch run python -m tests.cli.ci.tests.preview_pr_comment /tmp/dispatcher-previewWhat to look for, in the order the design puts it:
The size fallbacks are not previewed here. Truncation only begins once the rows without any test names still exceed the byte budget, so the only faithful preview of it is a ~61 kB body, which is not worth a reviewer's scrollbar. The order it gives things up is the point, and it is pinned by tests: the test and step names go first, then rows are dropped with a notice saying exactly how many, and a target's job link is the last thing to go. A few notes so nothing here is mistaken for a real report:
|
Layout 1 — running, with failures already knownOne batch has finished and failed, one is still running, one is collecting its artifacts. The alert counts what is outstanding in batches, because a batch is the unit that actually finishes — a retrying batch has every job reported while the batch runs on, so a pending-jobs count alone can read as Worth noting in the 🔄 Dispatcher tests: in progress
Note Tests are still running. 2 of 3 batches have not finished yet. 1 of 9 jobs have not reported. This comment updates automatically.
✅ 5 passed · ❌ 3 failed · ⏳ 1 pending Batches · ❌ batch-01 4/4 · 🔄 batch-02 3/4 · 📥 batch-03 1/1 ❌
|
Layout 2 — finished, with an arbitrary target-to-test matrixThe layout that matters, and the one the redesign is really about: three groups whose targets failed in three different relationships to each other.
The group summaries count targets rather than tests. A test count over a group whose targets failed differently describes none of them, and it put a number nobody navigates by where the outcome belongs. ❌ Dispatcher tests: failed
Caution 3 integrations failed. 9 of 9 jobs failed.
❌ 9 failed Batches · ❌ batch-01 6/6 · ❌ batch-02 3/3 ❌
|
Disk usage changeCommit No integration or dependency changed size. |
Layout 3 — problems with no failing target behind themTwo bodies, because the two cases they cover are what the comment used to conflate into a single count called First: workflows that passed while their reports went missing. Neither failed nor a clean pass, so the heading is The two counts in the alert are in different units and are never added together: The three batches show the whole deduplication rule, which the header and the notes now apply identically. Second: a workflow that failed outside its integration test jobs. A setup step, an upload, the workflow itself — a real failure with no target to attribute it to, so it is reported against the batch and linked to its run. Every job in the run passed, and the totals say so without adding
|
Layout 4 — the terminal statesFour bodies: cancelled, stopped on a fatal error, timed out, and last a run that stopped before any batch reported at all. Each is one sentence. The previous versions explained that unfinished batches had been asked to stop and that anything below was what had been gathered by then — mechanics the reader cannot act on, in a place where what they need to know is that the run ended early. The report below the alert is already only what arrived before the stop, so saying so added nothing. Only the fatal error carries a reason, bounded to one line and rendered as literal text so an error string containing backticks cannot break out of it. A deadline explains itself, so the timeout does not repeat it; the reason stays in the logs. The last body is the one case with no snapshot behind it, so it has no totals and no batch strip by construction rather than by omission. 🚫 Dispatcher tests: cancelled
Caution The Dispatcher run and its unfinished batches were cancelled.
✅ 5 passed · ❌ 3 failed · ⏳ 1 pending Batches · ❌ batch-01 4/4 · 🔄 batch-02 3/4 · 📥 batch-03 1/1 ❌
|
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 726d57478f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| attempt = job.latest | ||
| if attempt is None or attempt.status is not Status.FAILURE: | ||
| if attempt is None or (attempt.status is not Status.FAILURE and attempt.error is None): | ||
| continue | ||
| entries.append(_failed_job_entry(job, attempt, detail=detail)) | ||
| # A batch whose workflow failed without any tracked job failing is a real failure with nothing | ||
| # to list; saying so beats an empty section or a silent omission. | ||
| entries += [ | ||
| f"<code>{html.escape(batch.batch_id)}</code> — the workflow failed with no tracked job failure" | ||
| for batch in progress.batches | ||
| if batch.status is Status.FAILURE | ||
| and all(job.complete for job in batch.jobs_progress) | ||
| and not any(_is_failed(job) for job in batch.jobs_progress) | ||
| ] | ||
| if not entries: | ||
| grouped.setdefault(job.job.target, []).append(FailedTarget(job, attempt, batch.batch_id)) |
There was a problem hiding this comment.
When a job completed successfully but has an error such as NO_ARTIFACTS, this condition still puts its integration in _failure_groups. If another job genuinely failed, _alert consequently counts the unknown-only group as an integration that failed; with more than GROUP_LIMIT unknown groups, the overflow pointer likewise says they failed even when the alert says nothing failed. Keep unavailable-only groups out of failure counts and failure-worded pointers while still rendering them as warnings.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The failed + warn groups are now sorted to show the failed first.
| targets = _compressed_targets(group) if len(group.targets) > TARGET_PREVIEW else _target_bullets(group) | ||
| paragraphs = ["\n".join(targets)] | ||
| if shared := _shared_tests(group): | ||
| paragraphs.append("\n".join(shared)) | ||
| if warnings := _group_warnings(group): | ||
| paragraphs.append("\n".join(warnings)) |
There was a problem hiding this comment.
When a target has two or more failed workflow steps, _target_qualifier returns no qualifier and this body builder only adds shared tests and warnings. The summary therefore reports, for example, 2 steps, but both step names are discarded even though the gatherer deliberately collects multiple failing steps when on-failure steps also fail. Add the distinct _group_steps details to the group body, analogous to the failed-test list.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: now listing them under the targets the way _shared_tests already lists failed tests
e74145a to
f53a288
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f53a2885d1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
|
|
||
| def _join_names(names: list[str]) -> str: | ||
| """Names in prose: ``a``, ``a and b``, ``a, b and c``.""" |
There was a problem hiding this comment.
This newly added docstring, along with several others in this change, uses reStructuredText double-backtick literals. Repository prose explicitly requires Markdown single-backtick literals in docstrings and comments, so convert the newly introduced occurrences to single backticks.
AGENTS.md reference: AGENTS.md:L296-L299
Useful? React with 👍 / 👎.
| tests = _group_tests(group) | ||
| if len(tests) < 2: | ||
| return [] |
There was a problem hiding this comment.
When an integration has more than four failed targets, _compressed_targets omits qualifiers unless every target has the same one. If those targets collectively report exactly one failed test but another target fails during setup or has no test detail, this early return also prevents _shared_tests from listing that test, so the group says 1 test without ever naming it. Render the sole test whenever the compressed target line could not carry it.
Useful? React with 👍 / 👎.
23fbe66 to
5572946
Compare
…layout Group failures into one collapsed disclosure per integration instead of one entry per failed job, and replace the batch table with a single batch strip. Every report uses the same section order, so a queued run and a failed one differ in what they say rather than in where they say it. A target's job link is the way into the run that failed, so every affected target keeps a row of its own carrying that link, whatever the counts. No cardinality threshold changes the shape of anything: the shape changes only when the body does not fit GitHub's limit, through one renderer with an explicit detail level. The names under each target go first and the links go last, and the tier that drops rows says exactly how many it omitted and leaves the failed batch links to reach them. A failed test stays under the target that failed it. The one list above that level is the tests that failed in every one of a group's targets, listed once with each target's remainder labelled as additional failures, which is lossless: a target's failures are the common list plus its own. It is claimed only where every target has reports to intersect, so a group holding a target that lost its artifacts keeps its tests where they can be believed. The standalone "Unavailable results" and "Retried jobs" sections go: a result Dispatcher could not collect joins its integration's group, on the target's own row, drawn as a warning rather than a failure. A job's status is the workflow's own conclusion while its error says whether the Dispatcher managed to collect its results afterwards, so the two are independent and a job can conclude `success` while carrying `NO_ARTIFACTS`. Only the integrations that really failed are counted in the alert; what could not be collected is counted apart from them, in targets and batches separately rather than summed into one number over both units. A batch-level problem is reported only where no target's row already accounts for it, so one underlying problem produces one user-facing explanation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ac007d0 to
e5984dd
Compare
The suite runs inside a workflow, so a running report's footer named whichever commit CI was testing and the whole-body goldens differed between a laptop and CI. The autouse fixture now unsets it; a test that wants a commit asks for one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Validation ReportAll 21 validations passed. Show details
|
2 participants
What does this PR do?
Reworks the Dispatcher PR comment layout. The new layout is in the comments below.
Motivation
The previous layout was job-first, and on a large run that made it unreadable: an HTML table of
batches followed by one entry per failed job, so a reader had to scan dozens of near-identical rows
to work out which integrations were actually broken.
Review checklist (to be filled by reviewers)
qa/requiredif this PR needs QA validation, orqa/skip-qaif it does not. Exactly one of the two is required.backport/<branch-name>label to the PR and it will automatically open a backport PR once this one is merged🤖 Generated with Claude Code