Repository navigation
Treat every symbol in a test file as test code - #1023
Merged
Merged
Conversation
The PR review comment this project posts on its own pull requests was asking authors to write tests for their tests. On PR #1014 it listed tests/test_upgrade_path.py::_check, _base_env, _venv_python and _venv_script as test gaps. Two causes, fixed at their own level. The parser only ever passed is_test when it built a Function or Test node, so every Class in a test file was stored with is_test = 0, and so was every private helper and fixture whose name did not match a test pattern. parse_bytes now marks each node it returns from a test file, whatever its kind, in one pass after extraction. Running it after extraction leaves TESTED_BY generation, which keys off the test functions alone, unchanged. Test-file detection itself was three regexes in three modules, each with a different idea of what a test file is: flows.py knew nothing about Go, refactor.py knew nothing about tests/ directories, and the parser's directory patterns matched anywhere in the string, so src/latest/ read as test/ and contests/ read as tests/. There is now one definition in parser.is_test_file, anchored on path separators and extended to the conventions it was missing: Ruby spec/ and *_spec.rb, C# *Tests.cs, conftest.py, Java and Kotlin *Tests, and the e2e and test-utils directories refactor.py already knew about. flows.py and refactor.py use it. changes.py and tools/review.py exclude a node whose file is a test file regardless of what is_test says on the row, so a graph built before this change stops producing the bad report without a rebuild. Measured on real diffs from this repository, rebuilding the graph each time: - PR #1014's diff: 20 test gaps, all 20 test-file artefacts, to 0. - detect-changes --base HEAD~6 at that commit, a test-heavy 46-file diff: 55 gaps of which 53 were test-file artefacts, down to 2. The two production gaps, MissingOptionalDependencyError and main, are the same before and after. - PR #997's production files (graph.py, parser.py, tools/query.py): 107 gaps to 98. The 9 dropped are all test-file artefacts; the 98 production gaps are identical sets before and after, including parser.py::_collect_python_receiver_evidence. The eval suite runs all 42 benchmarks over 6 repositories before and after. impact_accuracy, search_quality, agent_baseline and multi_hop_retrieval are unchanged cell for cell. token_efficiency ratios are unchanged. flow_completeness keeps its entry-point recall but reports fewer flows (fastapi 401 to 374, gin 186 to 161) because test-file entry points are no longer traced as production flows, which is what include_tests=False asks for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
code-review-graph reviewOverall risk: 0.65 (MEDIUM) — 108 changed function(s)/class(es), 28 affected flow(s), 42 test gap(s) Risk-scored changes
Affected execution flows
Test gaps
Token savings: this graph-backed report used ~299,464 fewer tokens (~84%) than reading every changed file in full (estimated, chars/4 approximation). Powered by code-review-graph — local-first analysis; no code leaves the CI runner. |
is_test_file was applied to the absolute path the graph stores, so its directory rules matched parent directories above the checkout. A repository cloned into a directory named test, tests, spec or specs had every one of its production symbols classified as test code. Measured on this repository checked out under a directory named "test": 399 test gaps became 0, 210 entry points became 0, 76 dead-code candidates became 0, all 6589 nodes were marked is_test, and the review-guidance coverage warning disappeared. A CI workspace or job directory named "test" was enough to make the tool report a clean bill of health for an entire repository. Test-ness is now read from the path relative to the repository root. Directory rules need that root; filename rules do not, so they are separated and only the directory rules require it. When the root is unknown, or the path lies outside it, the directory rules are dropped rather than guessed: that under-reports test files but never hides a production gap. Every call site passes a root - CodeParser through _is_test_path and _is_test_function, changes/flows/refactor/review through the root recorded at build time, and jedi_resolver through the root it already had. Builds record repo_root in metadata; a graph built before that falls back to the default database location. Four narrower false negatives, each one a production file read as test code: - specs?/ matched by directory name alone. "spec/" and "specs/" hold design documents and generated API specs as often as RSpec files, so a Ruby source file under the directory is now required as evidence. - Test.php was unanchored on the stem, so src/ABTest.php, an A/B testing feature, was test code. The character before "Test" must now be lowercase or a digit, which makes it a class-name boundary. Same fix for the C#, Java, Kotlin, Scala and Groovy rules, which had the same defect. - test-utils/ and test_utils/ were new to the parser and so to changes.py and tools/review.py. A shared library published from such a directory is production code. The rule is now opt-in and only dead-code detection asks for it, where a false "dead" report costs more than a missed one. - A library that ships a test package to its users, the django/test/client.py shape, was excluded from gaps. A singular test/ directory that carries an __init__.py and sits inside another package is now production code. find_dead_code had the same absolute-path defect in its package-alias heuristic: directory segments were taken from the absolute path, so a workspace directory named "test" matched import specifiers mentioning "test" and suppressed candidates that the same tree elsewhere reported. It now reads segments from the repository-relative path. Measured on this repository, same tree, before and after: production test-gap sets are identical (402), production entry points identical (210), production dead-code candidates identical (76), eval accuracy identical, and a checkout under test/, spec/ or prod/ now produces byte-identical reports. The HCL test that filtered REFERENCES targets on "path" matched the absolute file half of the target, so any checkout path containing "path" failed it. It now inspects the symbol half. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Sep 17, 2026
gowrishacv
pushed a commit
to gowrishacv/code-review-graph
that referenced
this pull request
Nov 6, 2026
dead-code reports two families of method whose caller is a contract or a test runner rather than a call site in the repository: - @OverRide in production code. An override implements a supertype contract, so its caller is that contract. No hierarchy walk can rescue these, because the base is often the JDK and outside the graph. - JUnit/TestNG annotations outside a test-named file. A shared harness deliberately lives in main so several test modules can depend on it, so the test-file exclusion from tirth8205#1023 does not cover it. Both annotations are already stored on the node in extra['decorators'], and _is_entry_point already consults them through _has_framework_decorator; the patterns list simply had no entry for Override, nor any for JUnit or TestNG. Both new patterns are anchored so they cannot match unrelated names such as override_settings, which has its own pattern further down the list. Measured on a 125-file multi-module Java library: symbols reported dead 174 -> 120, the removal set being exactly the 54 @OverRide methods, with nothing added. Seven tests across the two modules that share the predicate. Five fail on staging; the remaining two are guards that hold in both directions — the anchoring of ^Override$, and the requirement that a genuinely unreferenced method still be reported. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The PR review comment this project posts on its own pull requests asks authors to write tests for their tests. On PR #1014 it listed
tests/test_upgrade_path.py::_check,_base_env,_venv_pythonand_venv_scriptas test gaps.Root cause
Two, fixed at their own level.
The parser never set
is_teston anything but functions.is_testwas passed at the Function/Test construction sites only, so everyClassin a test file was stored withis_test = 0, and so was every private helper and fixture whose name did not match a test pattern.changes.pypullsClassinto the changed-symbol set and filtered only onnode.is_test, so those rows all survived intotest_gaps.Test-file detection was three regexes in three modules that disagreed.
flows.pyknew nothing about Go,refactor.pyknew nothing abouttests/directories, and the parser's directory patterns were unanchored, sosrc/latest/read astest/andcontests/read astests/.Fix
parser.parse_bytesmarks every node it returns from a test file, whatever its kind, in one pass after extraction. Running it after extraction leaves TESTED_BY generation, which keys off the test functions alone, unchanged.parser.is_test_file, anchored on path separators and extended to the conventions it was missing: Rubyspec/and*_spec.rb, C#*Tests.cs,conftest.py, Java and Kotlin*Tests, and thee2e/test-utilsdirectoriesrefactor.pyalready knew about.flows.pyandrefactor.pyuse it.changes.pyandtools/review.pyexclude a node whose file is a test file regardless of whatis_testsays on the row, so a graph built before this change stops producing the bad report without a rebuild.is_testnow means "this symbol lives in test code".kindstill carries what the name/annotation detector decided, so@Test-annotation and name-prefix coverage is unchanged; the tests that pinned it now assert onkind.Measured on real diffs from this repository
Graph rebuilt from scratch at each measured commit.
--base HEAD~6, 46 files--base HEAD~6, 46 files--base HEAD~6, 46 filesNo production gap is lost: the 98 names are identical sets before and after, including
code_review_graph/parser.py::_collect_python_receiver_evidence, which has no TESTED_BY edge of any kind. The two production gaps on theHEAD~6diff,MissingOptionalDependencyErrorandmain, are the same before and after.Eval suite
All 42 benchmarks over all 6 repositories, before and after.
impact_accuracy,search_quality,agent_baseline,multi_hop_retrieval: unchanged cell for cell.token_efficiency: ratios unchanged,graph_tokensmoves by under 0.2%.build_performance: timing columns only.flow_completeness: entry-point recall unchanged (fastapi 1.0, gin 1.0);detected_flowsdrops (fastapi 401 to 374, gin 186 to 161, crg 92 to 65) because test-file entry points are no longer traced as production flows, which is whatinclude_tests=Falseasks for. Average flow depth rises on crg (2.5 to 3.0): the flows that went away were shallow test-helper chains.Still wrong, not fixed here
changes.pylooks tests up withget_edges_by_source(node.qualified_name)and has no bare-name fallback. 10769 of the 15291 TESTED_BY edges in a full build of this repository are stored with a bare source, so a function whose edge landed that way is called untested.detect_changes_funchas three TESTED_BY edges, all bare-sourced, and is reported untested. 10 of the 98 production gaps above are this, not real gaps.@pytest.mark.skiptest and an assertion-free test both emit CALLS, so both produce TESTED_BY and clear the gap.Gates
pytest tests/ -q: 3428 passed, 10 skipped, 2 xpassed (was 3383 passed, 10 skipped, 2 xpassed).ruff check code_review_graph/: clean.mypy code_review_graph/ --ignore-missing-imports --no-strict-optional: clean, 72 source files.🤖 Generated with Claude Code