[update_lib] show dependent tests in deps subcommand
#6828
Merged
+564
−135
Conversation
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
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThe changes introduce a comprehensive caching and analysis system for Python import graphs in the Lib and test directories. A shelve-based cross-process cache stores precomputed import graphs, supplemented by regex-based parsers for fast extraction of test and library imports. New utilities locate tests importing specific modules, consolidate paths, and build dependency trees, now integrated into the show_deps output. Changes
Sequence Diagram(s)sequenceDiagram
participant File as Source Files
participant Parser as Regex Parsers
participant Cache as Shelve Cache
participant Builder as Graph Builder
participant Query as Query API
participant User as Caller
User->>Builder: _build_lib_import_graph()
Builder->>Cache: Check cache
Cache-->>Builder: Cache miss
Builder->>File: Read Lib modules
File-->>Builder: File content
Builder->>Parser: parse_lib_imports()
Parser-->>Builder: Import set
Builder->>Builder: Build adjacency graph
Builder->>Cache: Store graph
Cache-->>Builder: Cached
Builder-->>User: Return import graph
User->>Query: find_tests_importing_module("os")
Query->>Builder: _build_lib_import_graph()
Builder->>Cache: Check cache (hit)
Cache-->>Builder: Return cached graph
Query->>Builder: _build_test_import_graph()
Builder->>File: Read test modules
File-->>Builder: Test file content
Builder->>Parser: parse_test_imports()
Parser-->>Builder: Import set
Builder->>Builder: Build test graph
Builder-->>Query: Return test graph
Query->>Query: Find tests importing "os"
Query-->>User: Frozenset of test paths
sequenceDiagram
participant Caller as Caller
participant Tree as find_dependent_tests_tree()
participant Graph as Import Graphs
participant Consolidate as consolidate_submodules()
participant Output as Result Tree
Caller->>Tree: find_dependent_tests_tree("json")
Tree->>Graph: Get lib and test import graphs
Graph-->>Tree: Graphs (cached)
Tree->>Tree: Locate "json" in lib graph
Tree->>Tree: Find tests importing "json"
Tree->>Consolidate: Group dependent modules
Consolidate-->>Tree: Consolidated modules map
Tree->>Tree: Recursively build nested tree
Tree->>Output: Nested dict with tests and dependencies
Output-->>Caller: Dependent tests tree
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The diff introduces substantial new functionality across multiple areas: a sophisticated caching system with shelve integration, multiple regex-based parsers, recursive graph-building algorithms, and a nested tree consolidation system. The scope is broad (3 files modified, 20+ new functions), and the logic density is moderate-to-high with interrelated components. While patterns are mostly consistent, the heterogeneity of concerns (caching, parsing, graph theory, path consolidation) demands separate reasoning for each major subsystem. Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Member
|
I suggest to include it as default output, Then this is copyable to Output like current way when |
Contributor
|
Code has been automatically formatted The code in this PR has been formatted using:
git pull origin deps-impact-command |
moreal
changed the title
--impact option for deps subcommanddeps subcommand
Contributor
Author
|
I've made some more modifications. However, as I noted in the PR description, the outcome isn't significantly different from a complete overhaul, so I'm questioning its effectiveness. It might be better to just close it. |
youknowone
force-pushed
the
deps-impact-command
branch
from
fac5e1d to
dc860d9
Compare
Member
|
I like this idea a lot. fix a little to show the data more useful way |
youknowone
force-pushed
the
deps-impact-command
branch
from
9e4efd8 to
4b0026d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@scripts/update_lib/deps.py`:
- Around line 10-52: Remove the unused "from collections import deque" import
and simplify clear_import_graph_caches by removing the unnecessary global
statement; instead check for the module-level cache names
(_test_import_graph_cache and _lib_import_graph_cache) via "if '<name>' in
globals()" and call .clear() on them if present, avoiding declaring them as
globals inside the function.
- Around line 261-342: The truncation in _extract_top_level_code causes
parse_lib_imports to miss imports that occur after the first def/class (breaking
stdlib dependency detection); fix by removing the call to
_extract_top_level_code inside parse_lib_imports so it runs its regexes against
the full file content (i.e., delete or skip "content =
_extract_top_level_code(content)" in parse_lib_imports and leave
_extract_top_level_code available for parse_test_imports if you want faster test
parsing).
Extend find_tests_importing_module() and _build_test_import_graph() to recursively search Lib/test/test_*/ directories using **/*.py glob pattern instead of just *.py. This fixes incomplete dependency analysis that was missing test files inside module directories like test_json/, test_importlib/, etc. Changes: - Add _parse_test_submodule_imports() to handle "from test.X import Y" - Update _build_test_import_graph() with recursive glob and submodule import handling - Update find_tests_importing_module() to use relative paths and handle __init__.py files correctly - Update show_deps.py to display relative paths (e.g., test_json/test_decode.py) - Add TestFindTestsInModuleDirectories test class with 3 tests Co-Authored-By: Claude <[email protected]>
Add consolidate_test_paths() to group test_*/ directory contents into single entries (e.g., test_sqlite3/test_dbapi.py → test_sqlite3). Co-Authored-By: Claude Opus 4.5 <[email protected]>
Outputs space-separated test names for direct use with python3 -m test: python3 -m test $(python3 scripts/update_lib deps sqlite3 --impact-only) Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Remove --impact flag, dependent tests are now shown by default - Rename --impact-only to --dependent-tests-only - Change output label from "[+] impact:" to "[+] dependent tests:" Co-Authored-By: Claude Opus 4.5 <[email protected]>
youknowone
force-pushed
the
deps-impact-command
branch
from
7de1568 to
fba17e1
Compare
youknowone
force-pushed
the
deps-impact-command
branch
from
aa7db93 to
26319e8
Compare
youknowone
force-pushed
the
deps-impact-command
branch
from
fb4fad5 to
6c44b6c
Compare
youknowone
force-pushed
the
deps-impact-command
branch
from
a114b3d to
184abf1
Compare
Contributor
Author
|
Thank you! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Note
I'm trying to fixing the following issue.
This pull request makes the deps subcommand show tests that depend on a given module. This is to speed up the feedback loop, as while CI does run the tests, waiting for it takes quite a long time.
Running it like
python3 scripts/update_lib deps json --dependent-tests-onlywill output just the list of tests without any verbose logs, which can then be passed directly as arguments after-m test.However, when you check with the command
ls Lib/test | grep "test_" | wc -l, there are about 342 tests in total, and even with this feature, the number only drops to around 330, so it doesn't feel very useful.With
--dependent-tests-onlyoption:Summary by CodeRabbit
New Features
Bug Fixes
Performance
Chores
✏️ Tip: You can customize this high-level summary in your review settings.