cpp-gtest-timing-benchmark-flake

Answer

Two independent defects combined to produce the flake: (1) a statistics bug in the assertion itself, and (2) a CI exclusion that was a no-op because it lived in a step that was never reached. Fix both.

1. benchmark_ts.cc:302 — compare medians with 1.5x tolerance, not raw means

At 100 rows, append is only ~10–23% faster than vector insert, so the two distributions nearly overlap. A shared-runner stall that overlaps one side of the measurement loop inflates that side's mean arbitrarily (a single +3000 ns sample added to 10k samples adds 0.3 ns, but a stall covering hundreds of iterations adds tens of ns to the mean), which flips EXPECT_LE(mean_append, mean_insert). The median of 10k samples is unchanged unless more than half of the iterations are delayed, and the 1.5x tolerance absorbs the remaining margin — it only trips on a genuine >1.5× regression.

// benchmark_ts.cc — replaces the raw mean-vs-mean EXPECT_LE at line 302.

#include <algorithm>
#include <vector>

namespace {

// Median of per-iteration timings. Unlike the mean, the median is immune to
// asymmetric CI load spikes unless >half of the 10k samples are delayed, so a
// shared-runner stall overlapping one side of the benchmark cannot flip the
// comparison. 10k iterations -> even count, so average the two middle samples.
double MedianNs(std::vector<double> samples) {
  if (samples.empty()) return 0.0;
  std::sort(samples.begin(), samples.end());
  const size_t n = samples.size();
  if (n % 2 == 1) return samples[n / 2];
  return 0.5 * (samples[n / 2 - 1] + samples[n / 2]);
}

}  // namespace

TEST(BenchmarkTimeSeries, CatalogAppendVsSortedVectorInsert) {
  // ...existing setup: collect ~10k per-iteration timings per side
  //    (append_ns, insert_ns) exactly as before...

  // OLD (flake, benchmark_ts.cc:302):
  //   EXPECT_LE(mean(append_ns), mean(insert_ns));
  // Append is only ~10-23% faster than insert at 100 rows; a one-sided load
  // spike inflates one mean and flips the check (3 fails / 1 pass same day).

  // NEW: medians with 1.5x tolerance.
  constexpr double kAppendFasterTolerance = 1.5;  // fail only if append is >1.5x slower
  const double append_median_ns = MedianNs(append_ns);
  const double insert_median_ns = MedianNs(insert_ns);
  EXPECT_LE(append_median_ns, insert_median_ns * kAppendFasterTolerance)
      << "CatalogAppend median " << append_median_ns
      << " ns must be within " << kAppendFasterTolerance
      << "x of SortedVectorInsert median " << insert_median_ns
      << " ns (means: " << mean(append_ns) << " vs " << mean(insert_ns)
      << " ns; medians are robust to asymmetric load spikes)";
}

Rationale for the margin: the healthy ratio is append/insert ≈ 0.87–0.91, so the check passes with 1.3–1.5x of headroom. It fails only when append is genuinely >1.5× slower than insert (a real regression), or when a spike delays >50% of append's iterations and by enough to exceed the margin — the documented, astronomically unlikely corner.

2. CI — the :-*Benchmark* exclusion was a no-op; put the filter where the full suite actually runs

Root cause of the 4 failing runs: the exclusion was applied to a later, runtime invocation (--gtest_filter=*:-*Benchmark*), but an earlier step ran make unit, which executes the full 820-test suite through the configure-time UNIT_TEST_FILTER=*. That full-suite step hit the flake and aborted under set -e before the filtered invocation ever executed. An exclusion attached to a step that is never reached is dead code.

Fix: make the exclusion a configure-time single source of truth, so every consumer (make unit, make check, ctest) inherits it:

# CMakeLists.txt — one definition, baked in at configure time.
set(UNIT_TEST_FILTER "*:-*Benchmark*" CACHE STRING
    "Default gtest filter for all unit-test targets. Timing benchmarks are
     excluded by default (they must not block normal CI on shared runners);
     dedicated perf jobs opt back in with '*Benchmark*'.")

# .github/workflows/ci.yml (excerpt)
jobs:
  unit:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
      - name: Configure
        # Was: -DUNIT_TEST_FILTER='*'  <- this is what made the later
        # invocation-level exclusion a no-op. Now the exclusion is baked in.
        run: cmake -S . -B build -DUNIT_TEST_FILTER='*:-*Benchmark*'
      - name: Build
        run: cmake --build build
      - name: Unit tests (full default suite, 819 tests)
        # Cannot abort on the benchmark flake anymore: the flaky test never
        # runs here. The old `set -e` abort path is removed at the source.
        run: |
          set -euo pipefail
          make -C build unit

  benchmark:
    runs-on: ubuntu-latest
    needs: unit
    if: always()          # informational; must not block merges
    steps:
      - name: Benchmark smoke test (explicit opt-in)
        run: |
          set -euo pipefail
          ./build/unit_tests --gtest_filter='*Benchmark*'
        continue-on-error: true

The primary fix is the median comparison — tests should be correct regardless of filtering. The configure-time filter is defense-in-depth: it guarantees the flake can never abort make unit on shared runners, and the explicit benchmark job still exercises the benchmark binary (now stable under the median fix) without blocking the merge path.