Skip to content

fix: retry tests_for with bare name when qualified lookup misses - #929

Open
yzxcj797 wants to merge 2 commits into
tirth8205:stagingfrom
yzxcj797:fix/903-tests-for-bare-name
Open

yzxcj797 wants to merge 2 commits into
tirth8205:stagingfrom
yzxcj797:fix/903-tests-for-bare-name

Conversation

@yzxcj797

Copy link
Copy Markdown
Contributor

Summary

TESTED_BY edges can store bare source names while the tests_for query uses path-qualified names (/abs/path/file.py::ClassName.method). When the qualified-name lookup returns nothing, the query silently reports covered code as uncovered (#903).

The fix adds a fallback: when get_transitive_tests(qualified_name) returns no results and the node has a bare name different from the qualified name, retry with the bare name. This catches the mismatch without changing any edge storage.

Testing

New tests/test_tests_for_bare_name.py — source-level guard verifying the fallback exists:

pytest tests/test_tests_for_bare_name.py -q
→ 1 passed

ruff check code_review_graph/tools/query.py
→ All checks passed!

Fixes #903

TESTED_BY edges can store bare source names while the query uses path-qualified names, so tests_for returned 0 for symbols that are covered (tirth8205#903). A fallback retry with the bare name catches this mismatch.
@github-actions

github-actions Bot commented Aug 27, 2026

Copy link
Copy Markdown

code-review-graph review

Overall risk: 0.75 (HIGH) — 3 changed function(s)/class(es), 1 affected flow(s), 1 test gap(s)

Risk-scored changes

Risk Level Symbol Location Tested
0.75 high code_review_graph/tools/query.py::query_graph code_review_graph/tools/query.py:433 yes
0.50 medium tests/test_tests_for_bare_name.py::TestBareNameFallback.test_query_source_contains_fallback tests/test_tests_for_bare_name.py:8 (test)
0.30 low tests/test_tests_for_bare_name.py::TestBareNameFallback tests/test_tests_for_bare_name.py:7 no

Affected execution flows

  • run — criticality 0.64, 28 node(s) across 3 file(s)

Test gaps

  • tests/test_tests_for_bare_name.py::TestBareNameFallback (tests/test_tests_for_bare_name.py:7)

Token savings: this graph-backed report used ~12,878 fewer tokens (~81%) 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.

@tirth8205

Copy link
Copy Markdown
Owner

Changes required: the extra bare-name retry bypasses the evidence safeguards already used by get_transitive_tests. In the repository's default GraphStore, create two work nodes in a.py and b.py using absolute file paths plus a bare TESTED_BY edge from work to a test in a third file, with no import or same-file evidence; query_graph('tests_for', f'{repo_root}/a.py::work', str(repo_root)) now attributes that test, while main leaves it unassigned. Repair the missing import evidence instead of treating a shared name as proof of coverage.

@tirth8205
tirth8205 changed the base branch from main to staging September 15, 2026 13:11
@tirth8205

tirth8205 commented Sep 15, 2026

Copy link
Copy Markdown
Owner

Holding this one: the problem is the change, not the tests.

Reject: the change is wrong on staging. (1) Issue #903's root cause (file paths compared against dotted module names in the evidence gate) was already fixed by PR #960 (commit 3d6e303 on staging) with proper tests; the probe shows the PR changes nothing for the #903 scenario. (2) The added retry calls store.get_transitive_tests(node.name) with the BARE name, whose direct lookup SELECT ... WHERE source_qualified = ? AND kind='TESTED_BY' matches every bare-source edge with that name with no import evidence, bypassing _select_evidence_backed_candidate.

What would change our mind:

  • Any behavioral test of query_graph(pattern='tests_for') through the new bare-name retry (the shipped test is a string-in-source assertion)
  • A query-level test that an unrelated same-named function does NOT receive the test (the PR fails this: verified by probe)
  • A query-level test that an ambiguous bare source with two imported candidates is NOT attributed (the PR fails this: verified by probe)
  • Either an inferred_by/confidence marker on results produced by the bare retry, or (preferably) removal of the retry, since the evidence-gated fallback already inside get_transitive_tests handles the legitimate case

Checks pass, so this only showed up when we probed the behaviour directly. Push back here if you think that is wrong.

PRs now target staging, not main. Yours was retargeted already, so nothing to do there.

@tirth8205 tirth8205 added the changes-requested Approach needs a change before merge label Sep 15, 2026
@tirth8205

Copy link
Copy Markdown
Owner

The bare-name retry invents test attributions. I built graphs on both branches over psf/requests and scrapy/scrapy and swept tests_for over every non-test function, method and class. On scrapy, total attributions go from 19806 to 24187. 4908 new symbol-to-test claims appear across 305 symbols, and 731 of those tests are claimed by the same bare name in more than one file, so at least 2491 of the new claims, 51 percent, are false by pigeonhole. from_crawler alone resolves to 47 different production symbols, and all 47 now claim the same test. On requests, 1191 becomes 1351, with 162 new claims across 18 symbols.

One I checked by hand:

tests/test_commands.py::TestHelpDeprecation.test_calling_help_is_deprecated

That test instantiates a subclass of ScrapyCommand and calls long_desc(). The PR also attributes it to Command.long_desc in edit.py, fetch.py, shell.py, view.py, runspider.py and parse.py. The test imports none of those six files.

The unevidenced rows also consume the max_results cap and push real evidence-backed attributions out, 548 lost across 33 symbols on scrapy. 249 results lose their inferred_by: naming_convention marker, because the new branch fills seen before the naming-convention block runs. A labelled guess comes back as an unlabelled fact.

The stored graph is untouched. Both branches produce identical node and edge tables on both repos, 8027 nodes and 53764 edges on scrapy, with zero node or edge identities differing. Existing databases are safe. Gates are green on the head, 4236 passed, ruff clean, mypy clean across 77 files.

Nothing has changed since the last review. The only commit added is a staging merge with no code change, so all four requested changes are still outstanding:

  • Remove the retry, or route it through the same evidence gate and mark its results inferred_by: "bare_name", the way inheritors_of already marks ambiguous bare-name matches.
  • Replace tests/test_tests_for_bare_name.py with a test that actually calls query_graph. The current one asserts a string appears in inspect.getsource(query), so it would still pass if the branch read if False and node.name != qn.
  • Add the two negative tests: an unrelated same-named function gets no attribution, and an ambiguous bare source with two imported candidates gets none.

This branch has not been deployed

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

Labels

changes-requested Approach needs a change before merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: tests_for returns 0 for Python — bare-name fallback compares file paths against dotted module names

2 participants