Skip to content

ci: run the C++ unit tests in the release configuration #821

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rkennke merged 2 commits into from
Sep 25, 2026
+42 −5
Merged

ci: run the C++ unit tests in the release configuration #821

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
565c35c
ci: run the C++ unit tests in the release configuration
rkennke Sep 25, 2026
691178f
ci: gate artifact publication on the release gtests too
rkennke Sep 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
ci: run the C++ unit tests in the release configuration 
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]>
  • Loading branch information
@rkennke @claude
rkennke and claude committed Sep 25, 2026
commit 565c35c1e378e66ee91dcaa9f1113bc62f672d19
10 changes: 6 additions & 4 deletions .github/workflows/nightly.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,9 @@ jobs:
uses: ./.github/workflows/test_workflow.yml
with:
configuration: '["asan"]'
# C++ gtests (ASan + TSan) run on every PR via native-sanitizer-tests in ci.yml.
# Skip them here so the nightly focuses on Java functional tests under ASan.
# C++ gtests run on every branch push from .gitlab/sanitizer-tests, under
# ASan, TSan and release. Skip them here so the nightly focuses on Java
# functional tests under ASan.
skip_gtest: true
run-slow-test:
uses: ./.github/workflows/test_workflow.yml
Expand All @@ -28,8 +29,9 @@ jobs:
uses: ./.github/workflows/test_workflow.yml
with:
configuration: '["asan"]'
# C++ gtests (ASan + TSan) run on every PR via native-sanitizer-tests in ci.yml.
# Skip them here so the nightly focuses on Java functional tests under ASan.
# C++ gtests run on every branch push from .gitlab/sanitizer-tests, under
# ASan, TSan and release. Skip them here so the nightly focuses on Java
# functional tests under ASan.
skip_gtest: true
slow_tests: true
fuzz:
Expand Down
28 changes: 27 additions & 1 deletion .gitlab/sanitizer-tests/.gitlab-ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,13 @@
# C++ unit tests under ASan and TSan.
# C++ unit tests under ASan, TSan, and release optimisation.
#
# These run on every branch push (not MR pipelines — GitHub Actions handles those).
#
# The release configuration is here rather than left to the label-gated Java
# matrix because it is the only one that compiles the gtests with optimisation.
# Both sanitizer presets pass -fno-optimize-sibling-calls and no -O, so any
# defect that only appears once the optimiser runs -- a fixture frame folded
# away by a tail call, say -- is invisible to every other job in the pipeline.
#
# Strategy: use Gradle only for compile+link (buildGtest{Config}), then run
# each binary directly from the shell. This bypasses Gradle's daemon I/O
# which swallows child process output when fd 1/2 are not the terminal.
Expand Down Expand Up @@ -86,6 +92,26 @@ gtest-asan-arm64:
SANITIZER_CONFIG: Asan
SANITIZER_LC: asan

# Not a sanitizer, but it reuses .sanitizer_job unchanged: buildGtestRelease
# exists alongside the sanitizer variants and emits binaries under the same
# bin/gtest/<config>_<test>/ layout the runner loop already globs for. The
# SANITIZER_* variable names are a misnomer here and nothing more.
gtest-release-amd64:
extends: .sanitizer_job
Comment on lines +99 to +100

@chatgpt-codex-connector chatgpt-codex-connector Bot Sep 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Gate artifact publication on release gtests

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 👍 / 👎.

@rkennke rkennke Sep 25, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

tags: [ "arch:amd64" ]
image: $BUILD_IMAGE_X64
variables:
SANITIZER_CONFIG: Release
SANITIZER_LC: release

gtest-release-arm64:
extends: .sanitizer_job
tags: [ "arch:arm64" ]
image: $BUILD_IMAGE_ARM64
variables:
SANITIZER_CONFIG: Release
SANITIZER_LC: release

gtest-tsan-arm64:
extends: .sanitizer_job
# docker-in-docker:arm64 = EC2 VM. sysctl works directly.
Expand Down
Loading