fix(launch_manager): enable ASSERT_DBG in unit tests - #579
Conversation
License Check Results🚀 The license check job ran with the Bazel command: bazel run --lockfile_mode=error //:license-checkStatus: Click to expand output |
lm_cc_test never defined SCORE_LANGUAGE_FUTURECPP_ASSERT_LEVEL_DEBUG, so SCORE_LANGUAGE_FUTURECPP_ASSERT_DBG(_MESSAGE) checks compiled out to a no-op dummy in every unit test binary (see score/language/futurecpp's assert.hpp) — several existing call sites in launch_manager rely on this macro (graph.cpp, process_info_node.cpp, dependency_graph.hpp, alive_interface_path.hpp, lm_control_impl.hpp) but none of them were actually being exercised. Define it in the shared lm_cc_test wrapper so it applies uniformly across all launch_manager unit tests, rather than one target at a time.
a500d7a to
e1ffc82
Compare
danth
left a comment
There was a problem hiding this comment.
Is it possible to also enable this for integration tests?
Good question — yes, it's possible, but it needs a bit more than the one-line macro change this PR makes for unit tests. lm_cc_test only affects the cc_test binary it wraps, but the integration tests here link the actual production launch_manager daemon binary (//score/launch_manager) directly as the binary under test, so enabling the define there would mean enabling it on the same target that ships in production — so I prototyped this as a separate opt-in build variant instead:
Verified locally in host mode: 19 of 21 integration tests pass unchanged with the debug-assert binary; the other 2 failures reproduce identically on main with no changes, so they're pre-existing local environment issues, not caused by this. Happy to open this as a follow-up PR on top of this one if that's useful — didn't want to bundle it into #579 since it touches integration.bzl and 21 test BUILD files. Let me know if you'd rather I fold it in here instead. |
Summary
Follow-up to #542 (comment: #542 (comment)).
lm_cc_test(the shared cc_test wrapper used by launch_manager's unit tests,tests/utils/bazel/unit_test.bzl) never definedSCORE_LANGUAGE_FUTURECPP_ASSERT_LEVEL_DEBUG. Without it,SCORE_LANGUAGE_FUTURECPP_ASSERT_DBG/_DBG_MESSAGEchecks compile down to a no-op dummy (seescore/language/futurecpp'sassert.hpp), so every debug-level assertion in launch_manager was silently unchecked in CI — build/link succeeds either way, so this was easy to miss.Several existing call sites rely on this macro and were never actually being exercised:
score/launch_manager/src/daemon/src/process_group_manager/details/graph.cppscore/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.cppscore/launch_manager/src/daemon/src/process_group_manager/details/dependency_graph.hppscore/launch_manager/src/daemon/src/common/alive_interface_path.hppscore/launch_manager/src/lm_control/src/details/lm_control_impl.hppWires the define into
lm_cc_testitself so it applies uniformly to all launch_manager unit tests, rather than adding it target-by-target.Test plan
bazel test --lockfile_mode=error --config=x86_64-linux //score/launch_manager/...— all previously-passing tests still pass with the define enabled (verified specificallygraph_UTandprocess_group_manager_UT, which exercise the affected call sites)buildifier --mode=checkon the changed.bzlfileFixes the follow-up raised in #542.