[TRTLLM-12090][infra] add static tests validation hook#13423
[TRTLLM-12090][infra] add static tests validation hook#13423xinhe-nv merged 9 commits intoNVIDIA:mainfrom
Conversation
|
/bot run --skip-test |
📝 WalkthroughWalkthroughThis pull request introduces a test validation and cleanup infrastructure system. A new pre-commit hook invokes test list validation, which uses AST analysis to verify test list entries exist in source files. A new cleanup script automates removal of unused test and helper functions from test directories. Changes
Sequence Diagram(s)sequenceDiagram
participant PC as Pre-commit Hook
participant VS as Validation Script
participant TLP as Test List Parser
participant AST as AST Indexer
participant VL as Validation Logic
PC->>VS: invoke check_test_list.py --validate
VS->>TLP: collect_entries(test_lists_dir)
TLP->>TLP: parse .txt/.yml/waives.txt files
TLP-->>VS: test entry list
VS->>AST: build_ast_index(source_files)
AST->>AST: extract functions/methods/parameters
AST-->>VS: indexed symbols
VS->>VL: validate_test_lists()
VL->>VL: check malformed entries
VL->>VL: verify files/classes/methods exist
VL->>VL: validate waive parameter IDs
VL-->>VS: validation errors (if any)
VS-->>PC: pass/fail result
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/check_test_list.py`:
- Around line 733-744: The --test-lists-dir and --test-base-dir parser args (and
their defaults _DEFAULT_TEST_LISTS_DIR/_DEFAULT_TEST_BASE_DIR) must be resolved
as absolute paths relative to the repository root (llm_src) before invoking the
validator; update the main/validate invocation to call
os.path.abspath(os.path.join(llm_src, value)) (or equivalent) for those args so
passing "tests/..." works regardless of CWD, and then pass the normalized paths
into the validator call.
- Around line 402-404: The comprehension's hanging indent around the sorted()
call trips flake8 E125/E131; reformat the for loop so the key= lambda expression
is not split with a misaligned hanging indent: move the entire key=lambda x:
(...) expression onto the same line as sorted( or place the tuple (x[0], x[1] or
"", x[2], x[3] or "") on the next indented line aligned under the opening
parenthesis, keeping the variable names waive_only_keys and the lambda x
reference unchanged (i.e., preserve the sorted(waive_only_keys, key=lambda x:
(x[0], x[1] or "", x[2], x[3] or "")) structure).
- Around line 381-399: collecting malformed entries from both active_malformed
and waive_malformed currently double-reports the same bad lines because
collect_entries(include_waives=True) rescans non-waive files; fix by
deduplicating before adding to errors—e.g., build an ordered unique list (or
append only those in waive_malformed that are not in active_malformed) and then
extend errors with that deduplicated set; reference the variables
active_malformed, waive_malformed and errors (and collect_entries) when making
the change.
In `@scripts/clean_tests.py`:
- Around line 158-185: The function _collect_top_level_defs is using ast.walk
which also visits nested functions and class methods; restrict collection to
actual module-level definitions by iterating only over tree.body and selecting
instances of ast.FunctionDef or ast.AsyncFunctionDef there (check decorator_list
and determine is_fixture as before), so update _collect_top_level_defs to
inspect top-level nodes from tree.body (keeping the existing decorated/fixture
logic and keys in defs) instead of using ast.walk to avoid capturing nested defs
and methods.
- Around line 69-106: The current _parse_test_name function only returns the
bare function token, causing collisions across modules; update it (or replace
it) to return a canonical test identifier including relative file path, optional
class name, and function name (e.g., "path.py::Class::test_name" or
"path.py::test_name") rather than just the final token; adapt get_used_tests to
add those full identifiers to the used set (use the same parsing semantics as
scripts/check_test_list.py's parser for .txt and .yml entries to preserve class
and param stripping) and ensure YAML "- " prefixes and inline TIMEOUT(...)
tokens are ignored the same way so duplicate test names in different
files/classes are treated distinctly.
- Around line 1-21: This new source file is missing the required NVIDIA
copyright header; add the repository-mandated NVIDIA copyright header (with the
year of latest meaningful modification) to the top of the file before any
imports or code. Update the header in scripts/clean_tests.py so it appears above
the first symbol (e.g., before DEFAULT_TEST_DIR and CONFTEST) and ensure the
wording and year match other files in the repo's header template.
- Around line 35-37: The run_cmd function uses subprocess.run(..., shell=True)
and returns only stdout, allowing shell injection when test_dir is interpolated
and hiding tool failures; change run_cmd to call subprocess.run with a list of
arguments (no shell=True) and include test_dir as a separate arg to avoid shell
interpretation, capture stderr as well, check result.returncode and on non-zero
either raise subprocess.CalledProcessError or log/exit with both stdout and
stderr so missing/broken tools (e.g., vulture) aren't silently treated as
success; update all call sites that pass a single combined string to run_cmd to
pass argument lists (reference: run_cmd and the call that builds the vulture
command).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b7c91681-a031-4ceb-9aa1-01e42b13dcd9
📒 Files selected for processing (3)
.pre-commit-config.yamlscripts/check_test_list.pyscripts/clean_tests.py
|
PR_Github #45378 [ run ] triggered by Bot. Commit: |
|
PR_Github #45378 [ run ] completed with state
|
|
/bot run --skip-test |
|
PR_Github #45631 [ run ] triggered by Bot. Commit: |
|
PR_Github #45631 [ run ] completed with state |
|
/bot run --skip-test |
|
/bot run |
|
PR_Github #45658 [ run ] triggered by Bot. Commit: |
|
PR_Github #45660 [ run ] triggered by Bot. Commit: |
|
PR_Github #45658 [ run ] completed with state |
Signed-off-by: Xin He (SW-GPU) <200704525+xinhe-nv@users.noreply.github.com>
728527c to
997c037
Compare
|
PR_Github #45660 [ run ] completed with state
|
|
/bot run |
|
PR_Github #45672 [ run ] triggered by Bot. Commit: |
|
/bot run --skip-test |
|
PR_Github #45687 [ run ] triggered by Bot. Commit: |
|
PR_Github #45687 [ run ] completed with state |
Signed-off-by: xinhe-nv <200704525+xinhe-nv@users.noreply.github.com>
Signed-off-by: xinhe-nv <200704525+xinhe-nv@users.noreply.github.com>
Signed-off-by: xinhe-nv <200704525+xinhe-nv@users.noreply.github.com>
|
/bot reuse-pipeline |
|
PR_Github #45897 [ reuse-pipeline ] triggered by Bot. Commit: |
|
PR_Github #45897 [ reuse-pipeline ] completed with state |
Signed-off-by: Xin He (SW-GPU) <200704525+xinhe-nv@users.noreply.github.com> Signed-off-by: xinhe-nv <200704525+xinhe-nv@users.noreply.github.com>
@coderabbitai summary
Description
local test result.
Test Coverage
PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.