Skip to content

fix(extract): derive unique names for GoogleTest macros to prevent same-file node collisions (#1266) - #1329

Merged
DeusData merged 1 commit into
DeusData:mainfrom
harshitaajoshi:fix/cpp-gtest-name-collision-1266
Jul 31, 2026
Merged

fix(extract): derive unique names for GoogleTest macros to prevent same-file node collisions (#1266)#1329
DeusData merged 1 commit into
DeusData:mainfrom
harshitaajoshi:fix/cpp-gtest-name-collision-1266

Conversation

@harshitaajoshi

@harshitaajoshi harshitaajoshi commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes #1266

Multiple GoogleTest TEST(...) cases defined in the same file collapse into a single graph node because cbm_resolve_func_name returns the bare macro name TEST with no argument-derived identity. Every test case in the file maps to the same qualified name (e.g. proj.file.TEST), so the graph upsert keeps only the last one and the call edges of all previous test cases are dropped.

This PR adds a GoogleTest macro detection step in extract_func_def that composes a unique name from the two macro arguments: TEST(WidgetSuite, DoublesSmallSize) becomes TEST_WidgetSuite_DoublesSmallSize. This gives each test case its own qualified name, preventing node collisions. The fix covers TEST, TEST_F, TEST_P, TYPED_TEST, and TYPED_TEST_P. It also sets is_test = true for these nodes so test exclusion queries work correctly.

Changes:

  • internal/cbm/extract_defs.c: Add is_cpp_test_macro() and resolve_cpp_test_macro_name() helpers; call them in extract_func_def before QN computation; set is_test flag
  • tests/test_extraction.c: Two regression tests using the exact fixture from the issue (TEST() three-case collision and TEST_F() two-case collision)

Checklist

  • Every commit is signed off (git commit -s) -- required, CI rejects unsigned commits (DCO, see CONTRIBUTING.md)
  • Tests pass locally (make -f Makefile.cbm test)
    ASan test-runner hangs on startup on macOS Tahoe (Apple Clang 17), a pre-existing environment issue unrelated to this change.
  • Lint passes (make -f Makefile.cbm lint-ci)
  • New behavior is covered by a test (reproduce-first for bug fixes)

Note: the local test checkbox is unchecked because the ASan test-runner hangs on startup for all suites locally (pre-existing environment issue unrelated to this change). The production build compiles cleanly with -Werror and clang-format passes on all changed files. CI will run the full test suite.

…me-file node collisions (DeusData#1266)

Signed-off-by: Harshita Joshi <j.harshitaa06@gmail.com>
@harshitaajoshi
harshitaajoshi requested a review from DeusData as a code owner July 29, 2026 04:29
@DeusData
DeusData merged commit 4573a51 into DeusData:main Jul 31, 2026
28 checks passed
@DeusData

Copy link
Copy Markdown
Owner

Merged — thank you, this closes #1266 properly.

The failure mode you fixed is a nasty one: TEST(Suite, Case) parses as a function_definition whose name is literally TEST, so every GoogleTest case in a file collided on the same qualified name and the upsert kept only the last one. The earlier cases did not merely get mislabelled — they disappeared, and their CALLS edges went with them. For a C++ codebase with a real test suite that is a large, silent hole in the graph.

What I liked about the implementation:

  • You compose the name from the two macro arguments (TEST_Suite_Case), which means the derived name is stable — it depends only on the source text, not on iteration order, a counter, or a byte offset. An unstable name would have churned the graph on every reindex and quietly broken incremental diffing, and that trap is easy to fall into here.
  • The result stays human-findable. TEST_WidgetSuite_DoublesSmallSize is something a person or an agent can actually search for, which matters a lot to us.
  • It is NULL-safe: unresolvable parameters fall back to the original name rather than producing something malformed.
  • Coverage across TEST / TEST_F / TEST_P / TYPED_TEST / TYPED_TEST_P, and is_test set consistently with the existing Rust convention rather than a new one.
  • Both polarities are asserted in the test — the new distinct node must exist and the old colliding TEST node must not.

One entirely optional follow-up, not a blocker and not something I want you to change now: a genuine C++ function literally named TEST/TEST_F with one or two typed parameters would also be renamed and flagged is_test. It is vanishingly rare and strictly bounded to those five names. If you ever want to close it, requiring the parameter shape to be bare identifiers (a parameter_declaration with a type but no declarator) before treating it as a macro would eliminate the surface entirely.

Thanks for the thorough work.

@harshitaajoshi

Copy link
Copy Markdown
Contributor Author

Thank you for the detailed review. The stability point about incremental diffing is something I will keep in mind for future contributions. I have noted the optional follow-up on tightening the parameter shape check and may pick that up separately.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

C++: same-named TEST cases in one file collapse into a single node, dropping their call edges

2 participants