Skip to content

fix(extract): converge Function/Method is_test with the tests/ path filter - #1308

Merged
DeusData merged 1 commit into
DeusData:mainfrom
Yyunozor:fix/issue-1294-is-test-tests-dir
Aug 17, 2026
Merged

DeusData merged 1 commit into
DeusData:mainfrom
Yyunozor:fix/issue-1294-is-test-tests-dir

Conversation

@Yyunozor

Copy link
Copy Markdown
Contributor

Closes #1294.

Summary

  • Function/Method.is_test only ever became true via the Rust #[test] attribute check; the per-file test-file flag (cbm_is_test_file) was already computed and stored on the Module node, but never propagated to the definitions inside it. A C function in tests/helpers/fixtures.c stayed is_test=false and leaked past store.c's is_test != 1 filters, even though trace_path's own independent path check already treated the file as a test.
  • extract_func_def and push_method_def now OR ctx->result->is_test_file into def.is_test, additive to the existing Rust attribute check (#[test] functions embedded in an otherwise regular .rs file are unaffected).
  • cbm_is_test_file (extraction-time) gained the same tests//test//spec//__tests__/ directory-prefix convention the project's own cbm_is_test_path (pass_tests.c) already uses, so a file under tests/ with no test_/_test naming is caught too — applying an existing convention where it was missing, not inventing a new one.
  • Secondary bug: trace_call_path's own include_tests filter (mcp.c's separate, query-time is_test_file()) matched a nested .../tests/... path but not a project-root-relative one, so tests/repro/rpd_index_and_fingerprint leaked into results under the default include_tests=false. Added the same missing directory-prefix check there.

Verification

  • make -f Makefile.cbm test -j12: parent 6786 passed / 1 failed / 4 skipped; fix branch 6789 passed / 1 failed / 4 skipped. Delta is exactly the 3 new tests; the 1 pre-existing failure (test_cli.c:8526, Copilot CLI/VS Code detection) and 4 skips (Windows-only) are identical on both sides.
  • New tests: extract_c_test_dir_marks_is_test_issue1294, extract_python_method_test_dir_marks_is_test_issue1294 (positive/negative for both extraction call sites) and tool_trace_totals_respect_test_filter_tests_root_subtree_issue1294 (end-to-end trace_call_path include_tests toggle).
  • clang-format/clang-tidy/cppcheck are not available in my local environment (clang-format errors on a libz3 version mismatch in my Homebrew LLVM), so CI is the authority for lint, as in fix(extract-rust): mark #[test]/#[tokio::test] fns is_test so they are filtered #857. The build itself uses -Wall -Wextra -Werror and compiled clean on both branches.

Known limitations

  • Path matching (here and in the pre-existing cbm_is_test_path) is forward-slash-only and case-sensitive; unchanged, to keep this diff minimal.
  • store.c's cbm_is_test_file_path() (a third, broader strstr(fp,"test") implementation feeding search_graph/dead-code) is untouched — outside this issue's reported scope.
  • A natural follow-up: unify the four divergent is-test-path implementations (pass_tests.c, helpers.c, mcp.c, store.c) behind one shared function instead of a third copy of the same directory list — left for a separate PR.

@Yyunozor
Yyunozor requested a review from DeusData as a code owner July 27, 2026 22:09
@Yyunozor
Yyunozor force-pushed the fix/issue-1294-is-test-tests-dir branch from 35b8826 to 45c162c Compare July 28, 2026 16:33
@DeusData DeusData added bug Something isn't working parsing/quality Graph extraction bugs, false positives, missing edges priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker. labels Jul 28, 2026
@DeusData DeusData added this to the 0.9.1-rc milestone Jul 28, 2026
@DeusData

DeusData commented Jul 28, 2026 •

Copy link
Copy Markdown
Owner

Thank you for the contribution and for tracing the inconsistent test classification through extraction and MCP filtering. This is now triaged as a high-priority parsing-quality bug for 0.9.1-rc. Our community PR queue is currently quite full, so it may take a little time before we can complete the review and, if approved, merge it. We are doing our best to support community contributions and will return with code-grounded feedback as capacity opens.

@DeusData

Copy link
Copy Markdown
Owner

A quick note so this does not sit here looking like your problem: your red CI is not caused by your change.

The failure is FAIL: invalid Windows PATH smoke seam fell back to the live registry — a main-side smoke seam issue, fixed on main by #1346 after your run. Your diff is not implicated.

One thing worth knowing, because it will save you a wasted click: "Re-run failed jobs" will not clear it. A re-run re-tests the same recorded merge commit and fails identically. Only a fresh push refreshes the merge ref — so a rebase on current main and a push is what picks up the fix.

Apologies for the delay. We are working through a large review backlog oldest-first; the queue is real rather than a judgement on this PR.

…ilter

Function/Method nodes only got is_test=true via a Rust #[test] attribute
check; the per-file test-file flag (cbm_is_test_file) was set on the Module
node but never propagated to the definitions inside it, so e.g. a C function
in tests/helpers/fixtures.c stayed is_test=false — invisible to store.c's
is_test!=1 filters — even though trace_path's own independent path check
already treated the file as a test. Propagate is_test_file into
extract_func_def and push_method_def, and teach cbm_is_test_file the same
tests/ (and test/, spec/, __tests__/) directory convention cbm_is_test_path
already uses, so a non-test_-named file anywhere under tests/ is caught too.

Separately, trace_call_path's own include_tests filter (mcp.c is_test_file)
matched a nested ".../tests/..." path but not a project-root-relative one,
so tests/repro/foo.c leaked into results with the default
include_tests=false; add the missing tests/ (and test/, spec/, __tests__/)
prefix check.

Closes DeusData#1294.

Signed-off-by: Yyunozor <yyunozor@icloud.com>
@Yyunozor
Yyunozor force-pushed the fix/issue-1294-is-test-tests-dir branch from 45c162c to cc7b4a8 Compare July 31, 2026 14:49
@DeusData
DeusData merged commit 33a3f03 into DeusData:main Aug 17, 2026
28 checks passed
@DeusData

Copy link
Copy Markdown
Owner

Merged (33a3f03) — thank you! Mirroring cbm_is_test_path instead of inventing a new convention, and shipping both positive and negative reproduce-first tests, made this a textbook review. The 4-way is-test-path duplication you flagged is a real follow-up; noted. Heads-up in the notes: graphs reindexed on the next release will intentionally reclassify defs under tests/ as tests.

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

Labels

bug Something isn't working parsing/quality Graph extraction bugs, false positives, missing edges priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Function.is_test is false for functions under tests/ — query_graph cannot exclude tests

2 participants