ci: run the C++ unit tests in the release configuration - #821
Conversation
No job compiled the gtests with optimisation. Both sanitizer presets pass -fno-optimize-sibling-calls and no -O, and the release gtests the Java matrix could reach are behind a test:release label, so a defect that only appears once the optimiser runs had nothing to catch it on an ordinary pull request. That is not hypothetical: a fixture whose frame -O3 folded away via a tail call merged green and stayed broken on main until someone happened to build release locally. The two new jobs extend .sanitizer_job unchanged -- buildGtestRelease already exists next to the sanitizer variants and emits binaries under the same bin/gtest/<config>_<test>/ layout the runner loop globs for, so only the two variables differ. The nightly comment claiming gtests run per-PR from a sanitizer job in ci.yml described a job that does not exist; it is the same misreading that left the gap open, so both copies now say where they actually run. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
![datadog-prod-us1-6[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
![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: 565c35c1e3
ℹ️ 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".
| gtest-release-amd64: | ||
| extends: .sanitizer_job |
There was a problem hiding this comment.
When either new release job fails after the existing sanitizer dependencies complete, build-artifact can still run because its DAG needs list names only the four ASan/TSan jobs; deploy-artifact then needs only build-artifact and the native builds. Thus an O3-only failure—the precise case these jobs are meant to catch—can still publish the affected artifact before the pipeline turns red. Add both release jobs to the publication gate.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 691178f.
Checked it against the config rather than taking it on trust: build-artifact's needs block named exactly the four sanitizer jobs, and because that is an explicit DAG rather than a stage dependency, it starts as soon as those pass. The release jobs would have run alongside publication instead of ahead of it — so an -O3-only failure would still have turned the pipeline red, but only after the affected artifact was built, and deploy-artifact needs build-artifact. That would have left these jobs advisory, which defeats the point of adding them.
Both are now in the gate:
gtest-asan-amd64 optional=false
gtest-tsan-amd64 optional=true
gtest-asan-arm64 optional=false
gtest-tsan-arm64 optional=true
gtest-release-amd64 optional=false <- added
gtest-release-arm64 optional=false <- added
Two notes on the shape of the fix:
Non-optional, matching the ASan entries rather than the TSan ones. All six jobs come from .sanitizer_job and share its rules, so whenever build-artifact exists they exist too. optional: true would tolerate a future rules change, but it would also let the coverage disappear silently — the same invisibility this PR is closing — so a loud failure is the better mode here.
Only build-artifact needed changing; deploy-artifact names it in its own needs, so it is gated transitively. Verified rather than assumed.
build-artifact declares an explicit needs DAG rather than waiting on the sanitizer stage, so it starts the moment the four jobs it names have passed. The two release jobs were not among them, which left them running alongside publication instead of ahead of it: an -O3-only failure would eventually turn the pipeline red, but only after the artifact it affects had been built, and deploy-artifact needs build-artifact, so publication followed. That is the exact failure these jobs were added to catch, so leaving them outside the gate would have made them advisory. Non-optional, matching the ASan entries: all six come from .sanitizer_job and share its rules, so whenever build-artifact exists they exist too. Reported by Codex review on #821. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
CI Test ResultsRun: #36114778247 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-09-25 09:05:42 UTC |
1 participant
Follow-up to #819, which fixed a test fixture that only broke under
-O3. This closes the gap that let it merge green and sit onmainundetected.The gap
No CI job compiles the gtests with optimisation.
gtest-asan-{amd64,arm64}-fno-optimize-sibling-calls, no-Ogtest-tsan-{amd64,arm64}-fno-optimize-sibling-calls, no-Otest-matrixrelease cellstest:releaselabel; does not build gtestsSo any defect that appears only once the optimiser runs has nothing to catch it on an ordinary pull request. #819 was exactly that: a trampoline whose frame
-O3folded away via a tail call. Its own CI was green, because#786carried notest:releaselabel and every cell read(<jdk>, debug, regular).The change
Two jobs, extending
.sanitizer_jobwith nothing else altered:That works because
buildGtestReleasealready exists alongside the sanitizer variants, and emits binaries under the samebin/gtest/<config>_<test>/layout the runner loop already globs for — so the template needs no modification at all.I left the template named
.sanitizer_jobrather than renaming it to something config-neutral: the rename touches all four existing jobs and is unrelated to closing the gap. There is a comment at the new jobs noting theSANITIZER_*variable names are a misnomer for them. Say the word if you would rather have the rename.Also fixed
nightly.ymlclaimed C++ gtests "run on every PR via native-sanitizer-tests in ci.yml". No such job exists —ci.ymlhas no sanitizer job at all; they run from.gitlab/sanitizer-testson branch pushes. That is the same misreading that made this gap invisible, so it is worth correcting rather than leaving. The comment appears twice in that file, not once; both are updated.Verification, and its limits
yq).asan/asan,tsan/tsan(×2 arches),release/release(×2 arches).buildGtestReleaseexists, and it producesbin/gtest/release_<test>/, which thegrep "/release_"in the runner matches.-O3-only breakage across tests that have never run optimised in CI. Nothing surfaced.What I have not verified: this against GitLab's schema, or the jobs actually executing. The local probe was macOS/arm64, so Linux-gated fixtures were skipped there — including, ironically, the one from #819. The first pipeline run on this PR is the real test, and it is the cheapest place to find out.
Expected cost: two additional jobs per branch push, comparable in runtime to the existing sanitizer jobs (30m timeout, local full-suite build+run was ~90s).
🤖 Generated with Claude Code