Bundle per-job correlation into BatchFinished via BatchJobResult#24253
Conversation
Move the job spec / workflow-run / artifact-directory correlation out of the gatherer and into the TaskTestRunner producer, shipping it as a single enriched per-job BatchJobResult on BatchFinished. - BatchJob.artifact_name(): deterministic, collision-free, sanitized artifact name derived solely from the job's frozen fields. Replaces the gatherer's delimiter-bounded token match as the way a job's artifact directory is resolved. - BatchJobResult bundles the BatchJob spec, the matched WorkflowJob (or None), and the resolved artifact path (or None). - BatchFinished now carries batch_jobs; the runner joins each job to its workflow job by name and resolves its artifact directory by artifact name, tolerating missing API matches and missing artifacts. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 1c5693b | Docs | Datadog PR Page | Give us feedback! |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b99a8a45bc
ℹ️ 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".
AAraKKe
left a comment
There was a problem hiding this comment.
Thanks @HadhemiDD, I just took a look and I think there are some things we should update but overall looks good in intent.
| final_conclusion = raw or "neutral" | ||
|
|
||
| artifacts_path = await self._download_artifacts(run_id, log_extra) | ||
| artifacts_path, artifact_dirs = await self._download_artifacts(run_id, log_extra) |
There was a problem hiding this comment.
request: I don't think we need the artifact_path being returned right? Reading it I noticed that we are just returning self._options.artifacts_base_path which we have access here as well. We can return just the list of artifact_dirs
| self._logger.warning("Failed to list workflow jobs", extra=log_extra, exc_info=True) | ||
| return jobs | ||
|
|
||
| @staticmethod |
There was a problem hiding this comment.
suggestion: I am seeing this is static now, i.e. no need for this to be inside the TastTestRunner class. I think it would be better if we implement this is a static job in the BatchJobResult as a factory method where we implement BatchJobResult.correlate(...). This receives the list of jobs we have run, the output of the workflows that have run and their artifact and generates the BatchJobResult we can later consume.
It will be easier to test as well since we don't need an entire processor to test this feature.
There was a problem hiding this comment.
It doesn't look like this method will be used by any other part of the code except for the TaskTestRunner, but it is cleaner to move it to the BatchJobResult.
Done.
| def test_artifact_name_is_deterministic() -> None: | ||
| assert _job().artifact_name() == _job().artifact_name() |
There was a problem hiding this comment.
request: remove this test, this is not testing what we want to test which is that we get a given name given a given set of variables. This is just testing that each time we call the method the method returns the same, which would a much bigger issue if it it was not true than just knowing the artifact name before launching the workflow.
| def _job(**overrides: object) -> BatchJob: | ||
| base = { | ||
| "name": "job-1", | ||
| "target": "ntp", | ||
| "runner": "ubuntu-latest", | ||
| "environment": "py3.13", | ||
| "platform": "linux", | ||
| "unit_tests": True, | ||
| "e2e_tests": False, | ||
| } | ||
| base.update(overrides) | ||
| return BatchJob(**base) # type: ignore[arg-type] |
There was a problem hiding this comment.
request: this is a very convoluted way of building a default factory. We need to define all the defaults, have an object in the signature that tells nothing to the user about what parameters we can pass and, because of that, we need to do type: ignore.
Just define it with the proper set of arguments and return the object crated
def batch_job(
name="job-1",
target="ntp",
runner="ubuntu-latest",
environment="py3.13",
platform="linux",
unit_tests=True,
e2e_tests=True,
) -> BatchJob:
return BatchJob(
name=name,
target=target,
runner=runner,
environment=ennvironment,
platform=platform,
unit_tests=unit_tests,
e2e_tests=e2e_tests,
)| fake.mock_response("list_workflow_run_artifacts", _artifacts_page(artifacts)) | ||
|
|
||
|
|
||
| def _artifact_for(idx: int, job: BatchJob) -> Artifact: |
There was a problem hiding this comment.
nit: AI loves to prefix every single name that is not public with underscore in python but it is really not necessary and adds a lot of noise. Lets avoid that.
There was a problem hiding this comment.
request: this is not part of this PR but has been modified by it to add more logic. We cannot have a test that is almost 100 lines long. This points to some issue in the way tests are prepared, testing framework or a smell in the implementation.
Looking at it it seems we are testing too many things in a single test and any small change in implementation will break a test for unrelated reasons.
This is what plan we can use to split this
1. Extract the shared setup+act into one helper, call it once.
All the current giant test's boilerplate (fake client, mock artifacts, _make_runner, build a batch, await
runner.process_message(batch)) becomes a @pytest.fixture or a plain helper returning what downstream assertions need: the
fake client (to inspect calls) and the drained finished: BatchFinished. Every split-out test below calls it once instead of
re-running the pipeline five times — you get five tests instead of five reruns of the same mocked workflow.
2. Split into one test per concern, each named for what it actually verifies:
- test_dispatches_workflow_with_job_list_payload — only the create_workflow_dispatch kwargs assertion (lines 165-202).
- test_opens_check_run_with_head_sha_and_details_url — only the create_check_run kwargs assertion (lines 205-211).
- test_downloads_all_batch_artifacts — only the download_artifact call assertions (lines 214-223).
- test_emits_batch_finished_with_run_metadata — only the top-level BatchFinished field assertions (lines 226-234): id,
status, run_id, workflow_url, artifacts_path.
- test_closes_check_run_with_workflow_conclusion — only the update_check_run assertion (lines 248-252).
3. Move the correlation asserts (lines 236-245) out entirely — they don't belong in a "happy path" test.
They're the part of this test that's actually about this PR. Fold them into test_process_message_correlates_batch_jobs (which
already exists right below and is the real home for correlation behavior), or give them their own
test_batch_finished_records_unmatched_correlation_when_no_match since the case here (workflow_job is None and
artifact_name_path is None for both jobs) is a distinct scenario from the "correlates" test's happy-match case.
4. Rename what's left.
Once split, there's no single test called happy_path anymore — each new test name says exactly what it checks, so a failure
tells you which concern broke without reading the body.
… tests - Move correlation logic from TaskTestRunner._build_batch_jobs into a BatchJobResult.correlate static factory so it is testable without the processor. - _download_artifacts returns only the artifact-name -> path map; artifacts_path is taken from the runner options. - test_messages.py: explicit-args batch_job factory, drop the tautological determinism test, add direct correlate tests. - test_task_test_runner.py: drop underscore-prefixed helpers and split the monolithic happy-path test into one-concern-per-test cases via a shared helper. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Review from AAraKKe is dismissed. Related teams and files:
- agent-integrations
- ddev/changelog.d/24253.added
| @@ -0,0 +1 @@ | |||
| Bundle per-job correlation into the CI dispatcher's BatchFinished via BatchJobResult. | |||
There was a problem hiding this comment.
This change is just a small modification in an existing logic, not granting a minor bump. Consider this a fix.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Validation ReportAll 21 validations passed. Show details
|
Move the job spec / workflow-run / artifact-directory correlation out of the gatherer and into the TaskTestRunner producer, shipping it as a single enriched per-job BatchJobResult on BatchFinished.
What does this PR do?
Motivation
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