feat: add matrix tests to containers#637
Conversation
CI Test ResultsRun: #29108122307 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Failed TestsSummary: Total: 32 | Passed: 31 | Failed: 1 Updated: 2026-07-10 16:57:31 UTC |
4fb3c21 to
d10ab7f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53fb055c24
ℹ️ 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".
|
Reliability & Chaos Results❌ 1 failure(s) detected Pipeline: https://gitlab.ddbuild.io/DataDog/java-profiler/-/pipelines/124061214 ❌ chaos: profiler tcmalloc amd64 21 0 3 temXchaos |
rkennke
left a comment
There was a problem hiding this comment.
Review
Scope: 3 files, +739/-25 — README.md, utils/README.md, utils/run-containers-tests.sh. No CI workflow files touched; this only extends the local dev/test script that mirrors CI's container test matrix (glibc/musl × JDK × arch × config), adding --matrix, --run, --fail-fast, and multi-value (all/comma-list) dimension filters, plus a status table and markdown/JSON summary reports.
Overview
The script gains dimension expansion (expand_dimension, expand_jdk_dimension), a skip_reason() table that mirrors known CI exclusions (musl+j9/graal, graal+asan, glibc/aarch64 8-11, musl+sanitizers), a matrix preview/confirmation flow, sequential matrix execution with --fail-fast, and report writers. Docs in both READMEs are updated consistently with the new flags.
Bug found (verified by running the script)
utils/run-containers-tests.sh:907:
if (( MATRIX_TOTAL > 1 || MATRIX_RUNNABLE > 1 || MATRIX_MODE )); thenMATRIX_MODE holds the string "true"/"false", not 1/0. In a bash arithmetic context, a variable holding a non-numeric string is itself re-evaluated as a variable name — since there's no shell variable named true, it silently resolves to 0. So MATRIX_MODE never actually contributes to this condition.
Confirmed with:
./run-containers-tests.sh --matrix --libc=glibc --jdk=21 --arch=x64 --config=debug --container=docker
# → "Error: container runtime 'docker' not found" — i.e. it fell through to the single-cell
# execution path instead of showing the matrix preview/confirmation, even with --run.
Impact is narrow — bare --matrix or partial filters expand to >1 cell and work correctly (verified separately). But it's a real correctness bug in exactly the "preview a specific single-cell matrix explicitly" path the flag is meant to support, and "boolean flag in arithmetic context" is a bug class worth fixing before it recurs elsewhere. Fix:
if (( MATRIX_TOTAL > 1 || MATRIX_RUNNABLE > 1 )) || $MATRIX_MODE; thenMinor / non-blocking
matrix_command()(run-containers-tests.sh:336) is defined but never called anywhere in the script — dead code, presumably superseded by the inline command-building inrun_matrix(). Worth deleting.- Heavy use of
evalfor indirect array access (append_unique_line,read_lines_into_array,read_string_into_array) works but is easy to get wrong later; a bash-4+local -nnameref would be safer if the minimum bash version allows it — just flagging the tradeoff (macOS ships bash 3.2 by default, where namerefs aren't available, soevalmay be the pragmatic choice here). - No automated test coverage for the new flag-parsing/expansion logic (understandable for a bash dev-utility script, but a quick self-check invocation, e.g. a
--matrixdry run, in CI would catch regressions like the one above).
Verdict
Nothing here is a security or CI-breaking risk — it's an opt-in local dev tool with no production/CI wiring changed. The MATRIX_MODE arithmetic bug is real and demonstrable but narrow in scope. Flagging as should-fix-before-merge rather than a hard blocker — it's a quick one-line fix, and the documented usage patterns all work correctly as tested.
a5905e8 to
6462618
Compare
@rkennke thanks for the review; I've applied your suggestions in 6462618. |
Benchmark Results (commit afa13dd)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/124061231 Commit: ✅ Within expected boundariesNo significant runtime deltas (all within run-to-run noise) and no internal-counter outliers. Runtime details (per benchmark × JDK)
Internal counter details (ddprof)ddprof internal counters, latest / dev (✅ = 0, · = unavailable):
|
What does this PR do?:
This PR allows to run all the CI tests matrix locally in containers.
Motivation:
Run the full CI tests matrix locally. It comes out to be easier to run full testsuite with agents and is also useful in case of infrastructure flakiness.
Additional Notes:
How to test the change?:
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.Unsure? Have a question? Request a review!