Skip to content

Test suite cleanup: separate benchmarks from tests, fix silently-passing cases #80

Description

@Nikos-d

Scanned all 19 registered test targets (plus the disabled test_gjk_epa.cpp) for tests that silently pass, benchmarks living inside TEST-registered CTest binaries, and other test-hygiene issues. Found during work on #nikos/tight_validation and split out for a dedicated cleanup PR.

1. Silently-passing test cases (zero assertions)

Catch2 reports a TEST_CASE with no CHECK/REQUIRE as passed by default (verified against this project's Catch2 v2.13.10 build) — these currently give no signal at all:

  • world/test_broadphase.cpp:1208"Optimization benchmark": only std::couts results, no CHECK
  • world/test_broadphase.cpp:1364"Test": same, prints stats only. Also a terrible name regardless.
  • world/test_primitives.cpp:121"Collision method benchmarks": pure BENCHMARK(), no CHECK
  • world/test_gjk_epa.cpp:190"Benchmark - GJK computation time vs point cloud size": same (file isn't even built today, see Andre/ur5 #3)

2. Benchmarks living inside TEST-registered CTest binaries

Should move to tests/benchmarks/ as proper nanobench BENCH targets (see tests/benchmarks/bench_core.cpp for the existing pattern), so they never run under ctest or a default test invocation:

  • world/test_broadphase.cpp — 3 of its 4 TEST_CASEs are benchmarks: the two above, plus "Benchmark - Broadphase vs Blast..." at line 626. That one's a hybrid: 3 BENCHMARK() blocks + one real CHECK(is_close(...)) at the end comparing naive vs. BVH distance — the check needs to stay behind as a TEST when the benchmarks move out.
  • world/test_primitives.cpp:121"Collision method benchmarks", pure BENCHMARK().
  • world/test_broadphase.cpp:1119"Constraints calculation" isn't a benchmark by name, but it's gated by a hardcoded #define BENCHMARKING_ON 0 (line 7) that makes it loop num_tests_per_world = 1e3 per world instead of using BENCHMARK(). This is why it hangs past 150s when run directly. It has real CHECK(is_close(...)) assertions cross-checking with_segments/broadphase/double_broadphase, so it should stay a TEST, but the iteration count needs fixing independent of the benchmark move — 1000 iterations × 8 worlds × 3 solve methods is not a reasonable default ctest cost.

3. Dead/disabled test infrastructure

  • world/test_gjk_epa.cpp — entire file commented out of tests/CMakeLists.txt (line 105), so it doesn't build or run at all today. Has 7 commented-out BENCHMARK() blocks inside too (lines 159-244). Needs a decision: revive it cleanly (split test/bench like everything else) or delete it — right now it's dead weight nobody notices.
  • world/test_primitives.cpp:144,156 — a commented-out BENCHMARK() and an entire commented-out TEST_CASE("Collision method comparison exhaustive (Box-capsules)").
  • Dead #define CATCH_CONFIG_ENABLE_BENCHMARKING (enables the macro but the file never calls BENCHMARK()) in: optimization/test_task.cpp, trajectory/test_bsplines.cpp, optimization/test_optimization_task_violation.cpp, world/test_gjk_epa.cpp. Harmless but noise — cheap one-line deletes.

4. Non-determinism / flakiness risk

  • world/test_broadphase.cpp:629-632"Benchmark - Broadphase vs Blast..." picks its world via unseeded std::random_device/std::mt19937 every run. Moot if it moves to BENCH, but if the trailing CHECK(is_close(...)) stays behind as a TEST, it should get a fixed seed so failures are reproducible.
  • world/test_gjk_epa.cpp"Test - GJK Box vs Capsule (random generation)" is randomized by its own name; same concern, moot while the file's disabled.

5. CTest/Catch2 config gap

  • tests/CMakeLists.txt:63add_test(NAME ${target}_test COMMAND ${target}) runs each binary with no arguments, so -w NoAssertions is never passed. That's why item Andre/benchmark #1 can happen silently — worth adding at the CMake level so this class of bug becomes self-reporting instead of needing another manual scan like this one.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions