Repository navigation
PROF-16533: Merged test coverage across CI-tested Linux platforms and JVMs - #948
zhengyu123 wants to merge 7 commits into
Conversation
Run :ddprof-lib:coverageReport for each CI-tested libc/arch/JVM cell in a container and merge the results into one C++ and one Java report. - run-containers-tests.sh: --coverage-out=DIR runs the coverage report for one cell and exports reports, JaCoCo data, classes and cell.properties; clone mode works from a git worktree - coverageReport also writes a gcovr JSON tracefile - merge-coverage.sh merges cells built from one commit - hermetic tests for both scripts Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Coverage test tasks do not fail on an empty -Ptests match, so a filter that matches only slow or only regular tests still reaches the reports - run-containers-tests.sh labels each JDK image with its base image ID and rebuilds it when the base image changed - test-coverage skill: memory preflight caps parallel cells at one per 8 GB, checks base images for gcovr, and counts a killed Gradle daemon as blocked instead of failed Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- --coverage-out replaces the previous export, records the -Ptests filter, exits 70 when Gradle passed but the export failed, rejects an empty directory and creates it only for an accepted run - --rebuild-base again forces the JDK image rebuild - merge-coverage.sh replaces --out, rejects cells without a commit, marks per cell whether it is in the C++ and Java reports, flags filtered runs and checks the downloaded JaCoCo CLI against a pinned SHA-256 - tests run the container command with a stub Gradle and cover the new merge and image-reuse branches - test-coverage skill clears a cell before each attempt, checks RUN against colima's $HOME share, and reports cells missing from a report Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- --coverage-out clears the previous export on the host before the run, so a run that dies before the export leaves no stale data, and rejects --tests patterns that would break the spliced container command - merge-coverage.sh refuses an --out that is not empty or an earlier merge, and marks a cell in the Java column only when it contributed .exec files to a report that was written - test-coverage skill: the $HOME check covers podman machine too, and the merge snippet sets the worktree it uses Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- run-containers-tests.sh accepts only letters, digits and _ . * # , - in --tests, in every mode: the pattern is spliced unquoted into the host-side eval and the container shell, so other characters could run commands on the host or inject cell.properties keys - the host-side clearing of a previous export is best effort; with a rootful runtime the in-container export clears root-owned files - merge-coverage.sh marks its output directory, so a merge that failed midway can be rerun into the same --out, and links a report in summary.md only when it was written Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Scan-Build Report
Bug Summary
Reports
|
|||||||||||||||||||||||||||||||||||||||||||||||||
![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: 9a545ac893
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // -Ptests filters both coverage tasks, and a pattern usually matches tests of only one | ||
| // (slow or not); an empty match must not stop the build before the reports. | ||
| if (project.hasProperty("tests")) { | ||
| filter.isFailOnNoMatchingTests = false |
There was a problem hiding this comment.
When a -Ptests typo or stale pattern matches neither testCoverage nor testSlowCoverage, both tasks now succeed with zero tests, so the coverage run can exit successfully without Java test coverage. Gradle's filter normally fails when no tests match (TestFilter documentation); keep allowing an empty match in one task for fast/slow-only patterns, but fail if neither task matched.
Useful? React with 👍 / 👎.
| source_commit=$(git -C "$SOURCE_ROOT" rev-parse HEAD 2>/dev/null || true) | ||
| if [[ "$source_commit" != "$COMMIT" ]]; then |
There was a problem hiding this comment.
If --source-root has edits to the C++ or Java sources, HEAD still matches the commit recorded by the cells, so this check passes and reports are generated against modified sources rather than the snapshot the cells built. That can misattribute coverage when source lines or class contents changed; require the relevant source tree to be clean or report against the recorded commit snapshot.
Useful? React with 👍 / 👎.
![datadog-official[bot]](https://avatars.githubusercontent.com/in/1358509?s=60&v=4){kind=small}
There was a problem hiding this comment.
Two of the most critical issues are a Git mount/clone path overlap that causes every worktree coverage cell to fail before testing, and an unescaped eval of the coverage output path that allows shell injection and silently redirects exports to the wrong location. A separate gcovr version mismatch between the preflight-checked host and the unpinned container image can abort coverage merging after the full matrix completes.
🤖 Bits Code Review · Commit 9a545ac
| GIT_COMMON_DIR="" | ||
| if [[ -f "$PROJECT_ROOT/.git" ]]; then | ||
| GIT_COMMON_DIR=$(git -C "$PROJECT_ROOT" rev-parse --path-format=absolute --git-common-dir) | ||
| CONTAINER_CMD="$CONTAINER_CMD -v \"$GIT_COMMON_DIR\":\"$GIT_COMMON_DIR\"${CONTAINER_VOLUME_RO_OPTIONS}" |
There was a problem hiding this comment.
When the main checkout is under /workspace, such as /workspace/repo, this mount creates /workspace/repo/.git inside the container before cloning. Git then rejects /workspace because it is nonempty, so every worktree coverage cell fails before testing. The mounted metadata and clone destination must not overlap; relocating either requires updating the corresponding Git resolution or build/export paths.
React: 👍 useful · 👎 wrong · 😕 unclear · 👀 right, but not acting on it
What each reaction means
- 👍 Useful: correct, and you're fixing it or will.
- 👎 Wrong: the finding is incorrect, for example the bug can't happen or the code was misread.
- 😕 Unclear: you can't tell what it means or why it matters.
- 👀 Right, not acting: correct, but intended, out of scope for this PR, a known risk, or too minor.
🤖 Bits Code Review · Open Bits AI session
| After all four agents send their final reports, merge every cell that has | ||
| `cell.properties`. A `failed` cell still holds the Java coverage of the tests that ran, | ||
| but a failed gtest stops the build before the C++ report, so such a cell has no C++ | ||
| tracefile. `summary.md` marks, per cell, whether it is in each report. |
There was a problem hiding this comment.
If a base image is missing or rebuilt, preflight does not check its eventual gcovr version, while the runner installs unpinned gcovr. A host that passes preflight can therefore be incompatible with newly generated tracefiles—for example, host 8.6 versus image 8.8—causing merging to abort after the matrix finishes. Recheck the resulting images and update the isolated host environment before merging.
| tracefile. `summary.md` marks, per cell, whether it is in each report. | |
| tracefile. `summary.md` marks, per cell, whether it is in each report. | |
| Before merging, repeat step 1's gcovr version check on all resulting base images, | |
| including images created or rebuilt during step 3. Compare the newest version with | |
| `gcovr --version` on the host. If the host is older or missing, create or update | |
| `$RUN/tools/venv` with `gcovr==<newest base image version>` and put | |
| `$RUN/tools/venv/bin` first on `PATH` for the merge. |
React: 👍 useful · 👎 wrong · 😕 unclear · 👀 right, but not acting on it
What each reaction means
- 👍 Useful: correct, and you're fixing it or will.
- 👎 Wrong: the finding is incorrect, for example the bug can't happen or the code was misread.
- 😕 Unclear: you can't tell what it means or why it matters.
- 👀 Right, not acting: correct, but intended, out of scope for this PR, a known risk, or too minor.
🤖 Bits Code Review · Open Bits AI session
| CONTAINER_CMD="$CONTAINER_CMD -e TEST_CONFIGURATION=$LIBC/${JDK_VERSION}-$CONFIG-$ARCH" | ||
| CONTAINER_CMD="$CONTAINER_CMD -e GRADLE_USER_HOME=/gradle-cache" | ||
| if [[ -n "$COVERAGE_OUT" ]]; then | ||
| CONTAINER_CMD="$CONTAINER_CMD -v \"$COVERAGE_OUT\":/coverage-out${CONTAINER_VOLUME_RW_OPTIONS}" |
There was a problem hiding this comment.
A literal --coverage-out='/tmp/coverage-$(printf EXECUTED >&2)' creates the requested directory, but the final host-side eval executes the substitution and mounts /tmp/coverage- instead. Double quotes in directory names can also break parsing. Shell-escape the complete mount argument so valid directory names remain literal and exports reach the requested location.
| CONTAINER_CMD="$CONTAINER_CMD -v \"$COVERAGE_OUT\":/coverage-out${CONTAINER_VOLUME_RW_OPTIONS}" | |
| printf -v CONTAINER_CMD '%s -v %q' "$CONTAINER_CMD" "${COVERAGE_OUT}:/coverage-out${CONTAINER_VOLUME_RW_OPTIONS}" |
React: 👍 useful · 👎 wrong · 😕 unclear · 👀 right, but not acting on it
What each reaction means
- 👍 Useful: correct, and you're fixing it or will.
- 👎 Wrong: the finding is incorrect, for example the bug can't happen or the code was misread.
- 😕 Unclear: you can't tell what it means or why it matters.
- 👀 Right, not acting: correct, but intended, out of scope for this PR, a known risk, or too minor.
🤖 Bits Code Review · Open Bits AI session
CI Test ResultsRun: #42289533870 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-11-06 20:56:43 UTC |
What does this PR do?:
Adds a merged C++ and Java coverage report across the Linux platforms and JVMs that CI tests.
utils/run-containers-tests.sh --coverage-out=DIRruns:ddprof-lib:coverageReport -PenableCoveragefor one container cell. It exports the reports, the JaCoCo.execfiles, the classes they were recorded against and acell.propertiesfile (libc, arch, jdk, tests filter, commit, Gradle exit code).coverageReportalso writes a gcovr JSON tracefile,reports/coverage/cpp/coverage.json.utils/merge-coverage.shmerges cells built from one commit into one gcovr report and one JaCoCo report, plussummary.md.test-coverageagent skill runs the CI matrix (test_workflow.yml): one worktree and one subagent per libc×arch, capped by container memory, then the merge.Other changes to
run-containers-tests.sh:.gitnames a gitdir in the main repository; the script mounts that git dir read-onlyjava-profiler.base-idlabel and are rebuilt when the base image changes--rebuild-baseon another JDK--testsaccepts only letters, digits and_ . * # , -, in every modeeval; a quote in it could run commands on the hostWith
-Ptests, the coverage test tasks no longer fail on an empty match (ddprof-test/build.gradle.kts). A pattern usually matches tests of only one oftestCoverageandtestSlowCoverage.Motivation:
One
coverageReportrun covers one libc, one architecture and one test JVM. Code that only runs elsewhere (aarch64 stack walking, musl paths, OpenJ9 and GraalVM support) shows as not covered. A merged report shows what the CI-tested matrix actually executes.Additional Notes:
--testsnow rejects patterns with other characters, e.g. spaces,$or;. Such patterns could not work reliably before.base-idlabel, so each is rebuilt once on first use.8-orcland8-ibm(no container image) and macOS. The C++ report covers the-O0 -DDEBUGbuild only (seedoc/building/CodeCoverage.md)./code-reviewthen re-reviewed each fix commit; the last pass was clean. Declined findings:safe.directory "*"in worktree mode"*"applies only inside the throwaway container--rebuild-baseshould not force a JDK image rebuildcoverage.jsonand the empty-match filterHow to test the change?:
bash utils/tests/test_coverage_scripts.shhas 103 tests and runs in CI withutils/tests/*.sh. Stubpodman,gcovr,javaandcurlprograms record their arguments. The container command itself runs on the host against a stub Gradle and checks exit codes,cell.propertiesand the export.tests=JavaProfilerTest: 9 glibc (HotSpot 17/21/25, OpenJ9 8/11/17, GraalVM 17/21/25) and 5 musl (8–25). All passed and merged into one report.0825b2dcb; the review fixes after it are covered by the hermetic tests only.For Datadog employees:
credentials of any kind, I've requested a security review (run the
dd:platform-security-reviewskill, or file a request via the PSEC review form).
bewairealso runs automatically on every PR.🤖 Generated with Claude Code