Add batch and job progress aggregate to the test gatherer - #24774
Conversation
The gatherer now publishes a DispatcherProgress -> BatchProgress -> JobProgress -> JobAttemptProgress snapshot on every UpdatePRComment, so the upcoming PR updater renders a complete immutable view of the run without touching GitHub API models or pipeline internals. The gatherer is initialized with the complete batch plan and emits revision 0 through build_initial_update(), and TestBatch/BatchFinished now carry an explicit batch_id instead of overloading BaseMessage.id as the logical batch identity. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 3fe2bbf | Docs | Datadog PR Page | Give us feedback! |
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Derive done from the aggregate instead of a received-batch counter, so a batch that reports no jobs no longer blocks completion, and key every registry and the duplicate guard on batch_id rather than the message id. Merge a finished batch into its registered plan instead of rebuilding it from the message: jobs are preserved and executions are appended to each job's history, which is what a failed-job rerun needs. Model conclusion as WorkflowJobConclusion and errors as a ProgressError enum, make the attempt status non-optional, drop the invented "timed out" step name, and share one batch_status rule between the flat view and the aggregate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The message shipped done and four counter properties that the progress snapshot it also shipped already answered. Keep revision, which is ordering metadata the snapshot deliberately does not carry, and progress, which is everything else. WorkflowStatus and JobResult stay: they are built from the same sources as the aggregate rather than from it, and remain the gatherer's local registry of what each batch reported. They are just no longer published. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Same content, fewer words: multi-paragraph docstrings collapse to one or two sentences and the three-line inline comments to one. No logic change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![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: 5f09922a80
ℹ️ 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".
| # Parse and organize artifacts outside the lock — these touch only this batch's own files. | ||
| results = [self._job_result(batch_job_result, message) for batch_job_result in message.batch_jobs] | ||
| # Outside the lock: these touch only this batch's own files. | ||
| gathered = [self._gather_job(batch_job_result, message) for batch_job_result in message.batch_jobs] |
There was a problem hiding this comment.
When a duplicate or unplanned BatchFinished carries artifact paths, this gathers each job and _organize_artifacts() copies coverage/JUnit files before the batch is rejected later under the lock. In that case an ignored message can still add or overwrite files in _output_base_path, so stale or rogue batch artifacts can leak into the results that are published from that directory; check the planned/finished state before parsing/copying, or defer copying until after the message is accepted.
Useful? React with 👍 / 👎.
AAraKKe
left a comment
AAraKKe
left a comment
There was a problem hiding this comment.
Thanks @HadhemiDD! Looks overall good and there are only a couple of comments. Take a look at the comment from Codex as well.
I keep thinking that there is a better way of abstracting this a bit so it is easier to maintain the status but for now this works. We can leave a refactor for later. I am thinking that it would help, once everything is in place and we see all the pieces together, to think about a progress registry where we abstract all the pieces together in a single responsiiblity place.
Anyways, this is for later, let see howe everything looks together and then we can update.
| across workflow attempts, and distinct from ``BaseMessage.id``, which identifies one message. | ||
| """ | ||
|
|
||
| batch_id: str |
There was a problem hiding this comment.
note: thanks for adding this, I will rewrite my PRs for the batching once we merge this because I am adding it as well. I needed it when deriving the batches.
| return sum(1 for _ in self._jobs) | ||
|
|
||
| @property | ||
| def _jobs(self) -> Iterator[JobProgress]: |
There was a problem hiding this comment.
suggestion: I am not 100% sure about this but it feels weird to call this _jobs as well as the jobs property because a batch also has jobs but these are the progress of jobs. Doing BatchProgress.jobs might seem like we are getting jobs of the given batch we are looking the progress for but that is not the case. Maybe we can call it job_progresses or jobs_progress or something like that? Not sure what the correct naming would be.
There was a problem hiding this comment.
jobs_progress it is
| It is constructed with the complete batch plan and, on each finished batch, emits an | ||
| ``UpdatePRComment`` carrying a monotonic ``revision`` and a ``DispatcherProgress`` snapshot of | ||
| every planned batch, including those still to run. ``done`` is derived from that snapshot, set | ||
| once no batch is left unfinished. Rendering the comment is a separate consumer's job. | ||
|
|
||
| ``WorkflowStatus``/``JobResult`` are the local registry of what each batch reported; they are not | ||
| published. Every registry is keyed by ``batch_id``, which stays stable across workflow attempts | ||
| while ``run_id`` and the message id do not. | ||
|
|
||
| This task makes no GitHub API calls — it works exclusively from the artifacts the runner | ||
| already downloaded to ``BatchFinished.artifacts_path``. | ||
| Makes no GitHub API calls: it works only from the artifacts the runner already downloaded. |
There was a problem hiding this comment.
request: I would tone down the comments. We have an almost unstructured 14 lines comment and some of them repeat the logic we hold for the entire dispatcher. These comments is normally AI reporting on what it is doing more than actual important information needed to understand the class.
| job_id=None if workflow_job is None else workflow_job.id, | ||
| status=status, | ||
| conclusion=None if workflow_job is None else workflow_job.conclusion, | ||
| failed_steps=tuple(failed_steps), | ||
| job_url=None if workflow_job is None else workflow_job.html_url, |
There was a problem hiding this comment.
suggestion: maybe we can bundle all these conditionals on workflow_job being none into a single if before the call.
job_id = conclusion = job_url = None
if workflow_job is not None:
job_id = workflow_job.id
conclusion = worfklow_job.conclusion
job_url = workflow_job.html_url| self._logger.warning( | ||
| "Gathered a job that is not in the batch plan", extra={"batch_id": message.batch_id, "job": name} | ||
| ) | ||
| jobs.append(JobProgress(job=reported_jobs[name], attempts=(attempt,))) |
There was a problem hiding this comment.
question: do we really want to have these unplanned jobs being tracked as part of the job counters? I think we shouldn't because, while this cannot happen, if there is an issue that somehow triggers this we will be mangling the totals. I leave the warning line and latr probably convert on a metric we can monitor more reliably but not include in the totals.
There was a problem hiding this comment.
Not really => Droping the second loop in _finished_batch_progress (:266-271) that appends unplanned reported jobs to jobs.
| workflow_url=message.workflow_url, | ||
| state=ExecutionState.FINISHED, | ||
| # Nothing reported means nothing to collapse, and a failed batch. | ||
| status=batch_status(statuses) if statuses else Status.FAILURE, |
There was a problem hiding this comment.
request: I think we cannot use this status aggreagation as the status of the whole batch. This only consider the jobs that we are tracking but there are other jobs in the workflow representing the batch that can make the batch fail. Setup, checkout, finalization... If the batch itself fails we want to know saying the batch failed right? Other wise a workflow for which all tests are green but the workflow itself failed (fails gathering results or some other closing action in the workflow) will be reported as successful.
I think the authoritative state of the workflow here is message.status which is already coming from the status of the workflow in the BatchFinished message. No need to rederive it here.
We still have the status information in each JobProgress. We could have the batch reported as failed and all JobProgress reporting as successful which likely mean that some untracked job in the workflow has failed.
There was a problem hiding this comment.
This also resolves an inventory finding from earlier in this PR: BatchFinished.status
had no reader anywhere in src. It becomes the source of truth.
- _finished_batch_progress sets status=message.status and drops the derived-status branch.
- batch_status (status.py:31) then has one caller left, WorkflowStatus.status
(messages.py:156), which I had rewritten to use it. Restore that property to the count-based
implementation it had before this PR and delete batch_status entirely — a shared rule with a
single caller is not worth the indirection, and this shrinks the diff against master. - _batch_error (:292) is unchanged: TIMED_OUT, else NO_JOB_RESULTS when nothing reported.
A workflow that concluded success while reporting no jobs keeps status=SUCCESS with
error=NO_JOB_RESULTS — the contradiction is data, and inventing a status here is exactly the
re-derivation he is asking to remove. - Test impact: test_progress_and_registry_agree must no longer assert
batch.status == workflow.status (they can now legitimately differ); it asserts the counts agree
and that the batch label is message.status. In test_dispatcher_scenario_three_batches, batch
b2 must be emitted with status="failure" — which is more faithful anyway, since a run with a
failing job concludes failure. Same for test_empty_batch_jobs_still_terminates_the_batch. - Add a test: every tracked job passes but the workflow concluded failure — the batch reads
FAILURE while every JobProgress reads SUCCESS, which is the untracked-job case he describes.
Powered by Claude
Take the batch status from the workflow instead of rolling it up from the tracked jobs: a workflow also runs setup and finalization steps that can fail while every job passes. That makes BatchFinished.status the source of truth and leaves batch_status without a caller, so it goes and WorkflowStatus.status returns to counting. Validate the batch before gathering, so an unplanned or already-gathered message cannot organize artifacts into the shared output tree, where the names carry no batch and could overwrite a planned job's coverage. Keep jobs the plan never mentioned out of the totals, log them instead. Rename jobs to jobs_progress so it cannot be read as a tuple of BatchJob, collapse the workflow_job conditionals, and cut the comments back. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Validation ReportAll 21 validations passed. Show details
|
2 participants
What does this PR do?
Adds the aggregate state layer the Dispatcher execution and PR reporting design specifies, inside the test gatherer:
ddev/src/ddev/cli/ci/tests/progress.pywith the frozenDispatcherProgress->BatchProgress->JobProgress->JobAttemptProgresshierarchy. Job/batch counters derive fromJobProgress.latestonly. No GitHub API model is exposed exceptconclusion, kept as itsWorkflowJobConclusionenum because it distinguishes outcomes (cancelled, timed out, action required) thatStatuscollapses. Errors are a closedProgressErrorenum, not prose the renderer would have to match on.TaskTestGathereris constructed with the complete batch plan and seeds every batch asPLANNED.UpdatePRCommentcarries exactly two things:revision, the ordering metadata the snapshot deliberately does not hold, andprogress, the complete snapshot covering batches that have not run yet, not only the ones already gathered. A finished batch is merged into its registered plan: it keeps every job it was planned with, and executions are appended to each job's history rather than replacing it.batch_id, anddoneis derived from the snapshot (no planned batch left unfinished) instead of a separate received-batch counter. Duplicate detection is the aggregate's own terminal state, so a re-delivered batch cannot inflate the revision even under a different message id.TestBatchandBatchFinishedcarry an explicitbatch_id, soBaseMessage.idstops doubling as the logical batch identity. The runner passes it through to the workflow input, the check-run name, and the emittedBatchFinished.batch_statushelper instatus.py, used by bothWorkflowStatus.statusand the aggregate.Retry execution is out of scope (
BatchAttemptFinished,rerun-failed-jobs, attempt polling,WorkflowJob.run_attempt). Where the aggregate can derive a retry-shaped value it does —attemptis the execution's position in the job's own history,current_attemptthe deepest history in the batch — so a failed-job rerun that reports only a subset of jobs already produces the right shape.max_attempts,retries_remainingandretrying_jobsare the only values still asserted rather than derived: they are fixed at their no-retry values so the PR updater's contract does not change when the retry work lands.Two deliberate notes for review:
JobResultandWorkflowStatusare kept and still populated as the gatherer's local registry of what each batch reported. They are not duplicates of the aggregate:_gather_jobbuilds both from the same inputs (BatchJobResult.workflow_job, the parsed JUnit reports,BatchFinished) in one pass, so the DTOs derive from those inputs rather than from these classes. They are simply no longer published, which is why the message lostdone,workflowsand its four counter properties.build_initial_update()returns revision0rather than submitting it, because a processor can only submit once the event bus has attached its queue. Its caller lands with the dispatcher entry point, which publishes it when it starts the bus.One known gap, unchanged from current behavior: a
BatchJobResultwith no correlated workflow job still raises instead of rendering as an unavailable result.Motivation
The PR updater must render one monotonic comment from a complete, immutable snapshot. The previous payload (
workflows: list[WorkflowStatus]) was a flat per-batch counter view built only from batches that had already finished, with no place for planned batches, no attempt history, and no separation between message identity and batch identity. This prepares the information the PR updater needs without it having to deal with GitHub API details or fields it does not use.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