Skip to content

Conversation

@aryansri05
Copy link

@aryansri05 aryansri05 commented Dec 28, 2025

[None][test] Add unit tests for filesystem path safety
Description
This PR introduces a new unit test file, tests/unittest/utils/test_path_safety.py, to enhance the robustness of file path handling within the utility modules.

The new tests focus on "sanity checks" for filesystem operations without requiring GPU resources (CPU-only), ensuring that the system gracefully handles edge cases such as:

Directory Traversal: Verifying that paths containing .. (dot-dot) do not cause crashes or unsafe resolution behaviors.

Empty Paths: Ensuring that empty string inputs for path checks are correctly handled as non-existent rather than raising unhandled exceptions.

Test Coverage
New Test File: tests/unittest/utils/test_path_safety.py

test_directory_traversal_protection: Validates pathlib.Path.resolve() behavior on unsafe inputs.

test_empty_path_handling: Validates os.path.exists() behavior on empty strings.

Verification: Verified locally using pytest on a CPU environment (MacBook Air), ensuring cross-platform compatibility for utility logic.

image

Summary by CodeRabbit

  • Tests
    • Added comprehensive test coverage for path safety validation, including directory traversal protection and empty path handling scenarios.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 28, 2025

📝 Walkthrough

Walkthrough

A new test file is added to the codebase containing a test class with two unittest cases. The tests cover path safety scenarios: directory traversal protection using ".." and empty path handling. Tests use only the Python standard library.

Changes

Cohort / File(s) Change Summary
Test: Path Safety
tests/unittest/utils/test_path_safety.py
New file with TestPathSafety class containing two test methods: test_directory_traversal_protection (validates path resolution with ".." does not crash and existence is handled deterministically) and test_empty_path_handling (verifies empty paths do not exist).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: adding unit tests for path safety and edge cases, which aligns with the actual changeset.
Description check ✅ Passed The PR description includes all key required sections: clear explanation of changes, test coverage details, and verification steps. It follows the template structure appropriately.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 270be80 and 06aa882.

📒 Files selected for processing (1)
  • tests/unittest/utils/test_path_safety.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py

📄 CodeRabbit inference engine (CODING_GUIDELINES.md)

**/*.py: Code developed for TensorRT-LLM should conform to Python 3.8+
Indent Python code with 4 spaces. Do not use tabs
Always maintain the namespace when importing in Python, even if only one class or function from a module is used
Python files should use snake_case naming: some_file.py
Python classes should use PascalCase naming: class SomeClass
Python functions and methods should use snake_case naming: def my_awesome_function():
Python local variables should use snake_case naming: my_variable = ...
Python variable names that start with a number should be prefixed with 'k': k_99th_percentile = ...
Python global variables should use upper snake_case with prefix 'G': G_MY_GLOBAL = ...
Python constants should use upper snake_case naming: MY_CONSTANT = ...
Avoid shadowing variables declared in an outer scope in Python
Initialize all externally visible members of a Python class in the constructor
For Python interfaces that may be used outside a file, prefer docstrings over comments
Python comments should be reserved for code within a function, or interfaces that are local to a file
Use Google style docstrings in Python for classes and functions, which can be parsed by Sphinx
Python attributes and variables can be documented inline with type and description
Avoid using reflection in Python when functionality can be easily achieved without reflection
When using try-except blocks in Python, limit the except to the smallest set of errors possible
When using try-except blocks in Python to handle multiple possible variable types (duck-typing), keep the body of the try as small as possible, using the else block for logic

Files:

  • tests/unittest/utils/test_path_safety.py
**/*.{cpp,h,cu,cuh,py}

📄 CodeRabbit inference engine (CODING_GUIDELINES.md)

All TensorRT-LLM Open Source Software code should contain an NVIDIA copyright header that includes the year of its latest meaningful modification

Files:

  • tests/unittest/utils/test_path_safety.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: achartier
Repo: NVIDIA/TensorRT-LLM PR: 6763
File: tests/integration/defs/triton_server/conftest.py:16-22
Timestamp: 2025-08-11T20:09:24.389Z
Learning: In the TensorRT-LLM test infrastructure, the team prefers simple, direct solutions (like hard-coding directory traversal counts) over more complex but robust approaches when dealing with stable directory structures. They accept the maintenance cost of updating tests if the layout changes.
🔇 Additional comments (2)
tests/unittest/utils/test_path_safety.py (2)

1-5: Add required NVIDIA copyright header.

Per coding guidelines, all TensorRT-LLM code must include an NVIDIA copyright header with the year of latest modification.

📝 Suggested copyright header
+# SPDX-FileCopyrightText: Copyright (c) 2025 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
+# SPDX-License-Identifier: Apache-2.0
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
 import unittest
 import os
 import tempfile
 from pathlib import Path

As per coding guidelines.

⛔ Skipped due to learnings
Learnt from: galagam
Repo: NVIDIA/TensorRT-LLM PR: 6487
File: tests/unittest/_torch/auto_deploy/unit/singlegpu/test_ad_trtllm_bench.py:1-12
Timestamp: 2025-08-06T13:58:07.506Z
Learning: In TensorRT-LLM, test files (files under tests/ directories) do not require NVIDIA copyright headers, unlike production source code files. Test files typically start directly with imports, docstrings, or code.

23-28: Add missing copyright header and reconsider whether tests should validate project utilities.

The test file is missing the required NVIDIA copyright header. All TensorRT-LLM code must include a copyright notice per coding guidelines.

Additionally, the tests exercise only Python standard library behavior (os.path.exists(), pathlib.Path.resolve()) rather than project-specific utility functions. If these tests are meant to validate actual path-handling utilities used within the project, they should import and test those utilities directly.

⛔ Skipped due to learnings
Learnt from: galagam
Repo: NVIDIA/TensorRT-LLM PR: 6487
File: tests/unittest/_torch/auto_deploy/unit/singlegpu/test_ad_trtllm_bench.py:1-12
Timestamp: 2025-08-06T13:58:07.506Z
Learning: In TensorRT-LLM, test files (files under tests/ directories) do not require NVIDIA copyright headers, unlike production source code files. Test files typically start directly with imports, docstrings, or code.
Learnt from: xinhe-nv
Repo: NVIDIA/TensorRT-LLM PR: 8534
File: scripts/format_test_list.py:1-6
Timestamp: 2025-10-22T06:53:47.017Z
Learning: The file `scripts/format_test_list.py` in the TensorRT-LLM repository does not require the NVIDIA Apache-2.0 copyright header.

@aryansri05 aryansri05 force-pushed the tests/add-path-safety branch from 06aa882 to 3e4a21c Compare December 28, 2025 11:26
Signed-off-by: Aryan Srivastava <your_github_email@example.com>
@aryansri05 aryansri05 force-pushed the tests/add-path-safety branch from 3e4a21c to ddfc782 Compare December 28, 2025 11:30
@svc-trtllm-gh-bot svc-trtllm-gh-bot added the Community want to contribute PRs initiated from Community label Dec 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Community want to contribute PRs initiated from Community

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants