fix: Rust integration tests never detected, and merge script fails on Python 3.9 - #623
Open
micahldixon wants to merge 2 commits into
Open
fix: Rust integration tests never detected, and merge script fails on Python 3.9#623micahldixon wants to merge 2 commits into
micahldixon wants to merge 2 commits into
Conversation
The module carries PEP 604 annotations (dict[str, Any] | None) which are
evaluated at runtime on function definition, so on Python 3.9 the script dies
at import with:
TypeError: unsupported operand type(s) for |: 'type' and 'NoneType'
That reads as a bug in the script rather than a version problem, and the
shebang is a bare #!/usr/bin/env python3 with no floor declared. macOS still
ships 3.9.6 as the system python3, so a Mac without a newer interpreter first
on PATH hits this on the first run.
Deferring annotation evaluation is the smallest fix and keeps the annotations
readable. The full suite now passes on 3.9.6 as well as 3.13.
Cargo compiles every .rs file directly inside a crate's tests/ directory as an integration-test binary, whatever it is named. Rust unit tests live inline behind #[cfg(test)] rather than in their own files, so there is no basename convention for is_test_path() to match on, and .rs was absent from _TEST_NAME_PATTERNS entirely. The effect was that no Rust file was ever classified as a test, so every tested_by edge on a Rust project was dropped during the merge and reported as an orphan or prod-prod pair. On a 141-file Tauri repo this silently discarded 10 real production-to-test links, producing a graph that claimed the backend had no test coverage. Matches on directory position for .rs only. Files under a nested directory such as tests/helpers/util.rs stay classified as production, consistent with the existing treatment of __tests__/helpers.ts.
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.
Ran
/understandon a Tauri v2 project, 141 files, mostly Rust with a React frontend. Great tool, the graph and the guided tour both came out genuinely useful. Hit two bugs inmerge-batch-graphs.pyalong the way, so here are fixes for both with tests.1. Rust test coverage gets dropped silently
This is the one that actually cost me something.
_TEST_NAME_PATTERNShas no.rsentry, sois_test_path()falls through toreturn Falseand no Rust file is ever seen as a test. On my run the merge discarded 12tested_byedges, 10 unique pairs, and reported them as "orphan endpoint or test<->test / prod<->prod pair". Both endpoints were present in the merged node set, so that reason was wrong in every case.In other words, Rust has no test-name convention to pattern match on. Cargo compiles every
.rsfile directly inside a crate'stests/directory as an integration-test binary, whatever it's called. Mine weresmoke_risk.rs,smoke_hooks.rs,sonar_shellout.rs. None of those carry a prefix or suffix marker, and they never will, because Rust unit tests live inline behind#[cfg(test)]instead of in their own files. Directory position is the only signal there is.Repro on current main:
Why it's worth fixing: my run happened to have a review pass that caught it and restored the edges, so my graph is fine. Without that pass it ships a graph claiming the Rust backend has zero test coverage. That's confidently wrong rather than merely missing, which is the worse failure mode for something an agent queries later.
The fix matches on directory position for
.rsonly. Files under a nested directory liketests/helpers/util.rsstay classified as production, which keeps it consistent with how__tests__/helpers.tsis already handled.2. The script won't run on Python 3.9
Four PEP 604 annotations (
dict[str, Any] | Noneand friends) get evaluated at runtime on function definition, so on 3.9 the module dies at import:Honestly that reads like a bug in the script rather than a version problem, and it took me a minute to work out what was going on. The shebang is a bare
#!/usr/bin/env python3with no floor declared anywhere. Worth noting macOS still ships 3.9.6 as the systempython3, so anyone on a Mac without a newer interpreter first on PATH hits this on their first run.from __future__ import annotationsis the smallest fix and keeps the annotations readable. If you'd rather declare a 3.10 floor and fail with a clear message instead, definitely happy to redo it that way, just say the word.Testing
python -m unittest tests.skill.understand.test_merge_batch_graphspasses, 82 tests, on both 3.9.6 and 3.13.5. On current main the same command cannot even import the module under 3.9.test_merge_subdomain_graphs,test_parse_knowledge_base) still pass, 15 tests, no regressions.build.rsand the nested-helper case.I have not run
pnpm test(the vitest side), since these changes are Python only and don't touch anything the JS suite covers.Two commits, split so each one stands alone.
3. Separate suggestion, no code attached
One file-analyzer subagent cleared its own stale output with
rm -f .understand-anything/intermediate/batch-1*.json. That glob also matchesbatch-10.json,batch-11.jsonandbatch-12.json, so it destroyed a finished batch's work and I had to re-run it.The agent improvised that command, it isn't in your instructions, but
agents/file-analyzer.mddoesn't mention cleanup either way so there's nothing steering it away from a glob. A line in the agent prompt saying to delete exact filenames only would close it, since all batches share one output directory. I left that one alone because it's your call how to word it. Happy to open it as a separate issue if you'd prefer to track it that way.P.S. the layer assignment on a mixed Rust/TypeScript repo came out better than I expected, it put both halves of the IPC contract in the same layer without being told to. 🙂