Phase 3: Add comprehensive tests for gitignore functionality - #295
Phase 3: Add comprehensive tests for gitignore functionality#295GargiGupta-io wants to merge 3 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a git-based file packaging mechanism to kinetic/utils/packager.py to package only tracked and non-ignored files when zipping the working directory, falling back to os.walk if git is not available. It also adds corresponding unit tests. The review feedback highlights several robustness improvements: explicitly excluding __pycache__ and .git directories in the git-based path to match the fallback behavior, verifying that a directory is a git repository before running git ls-files recursively, and gracefully skipping the new git-dependent tests if the git command is not available in the test environment.
| for relative_path in git_files: | ||
| file_path = os.path.join(base_dir, relative_path) |
There was a problem hiding this comment.
The zip_working_dir function is documented to always exclude __pycache__ and .git directories. However, when zipping via the git-based path (_write_git_files), untracked __pycache__ files or .git files that are not ignored by .gitignore will be included in the ZIP archive. We should explicitly filter them out to ensure consistent behavior with the fallback os.walk path and prevent bloated/stale Python bytecode from being packaged.
for relative_path in git_files:
if ".git" in relative_path.split("/") or "__pycache__" in relative_path.split("/"):
continue
file_path = os.path.join(base_dir, relative_path)References
- Demand Robustness: Do not accept fragile code. If the proposed code is not robust enough or lacks proper error handling, explicitly tell the author why the current approach is brittle and what must be done to reinforce it. (link)
| if os.path.isdir(file_path) and not os.path.islink(file_path): | ||
| nested_files = _list_git_files(file_path) |
There was a problem hiding this comment.
To prevent running git ls-files on normal directories (which can happen due to type changes or other edge cases, causing git to find the parent repository and duplicate files in the archive), we should explicitly verify that the directory is a git repository or submodule by checking for the presence of a .git file or directory before recursing.
| if os.path.isdir(file_path) and not os.path.islink(file_path): | |
| nested_files = _list_git_files(file_path) | |
| if os.path.isdir(file_path) and not os.path.islink(file_path) and os.path.exists(os.path.join(file_path, ".git")): | |
| nested_files = _list_git_files(file_path) |
References
- Demand Robustness: Do not accept fragile code. If the proposed code is not robust enough or lacks proper error handling, explicitly tell the author why the current approach is brittle and what must be done to reinforce it. (link)
| def test_list_git_files_respects_gitignore(self): | ||
| """Test that _list_git_files respects .gitignore.""" | ||
| tmp_path = _make_temp_path(self) |
There was a problem hiding this comment.
If git is not installed or available in the test environment (e.g., in a minimal CI container), running git init and git add via subprocess.run will raise a FileNotFoundError and fail the test suite. We should gracefully skip these tests if the git command is not available.
| def test_list_git_files_respects_gitignore(self): | |
| """Test that _list_git_files respects .gitignore.""" | |
| tmp_path = _make_temp_path(self) | |
| def test_list_git_files_respects_gitignore(self): | |
| """Test that _list_git_files respects .gitignore.""" | |
| import shutil | |
| if not shutil.which("git"): | |
| self.skipTest("git command not available") | |
| tmp_path = _make_temp_path(self) |
References
- Demand Robustness: Do not accept fragile code. If the proposed code is not robust enough or lacks proper error handling, explicitly tell the author why the current approach is brittle and what must be done to reinforce it. (link)
| def test_write_git_files_excludes_paths(self): | ||
| """Test that _write_git_files respects exclude_paths.""" | ||
| tmp_path = _make_temp_path(self) |
There was a problem hiding this comment.
If git is not installed or available in the test environment, this test will fail with a FileNotFoundError. We should gracefully skip it if the git command is not available.
| def test_write_git_files_excludes_paths(self): | |
| """Test that _write_git_files respects exclude_paths.""" | |
| tmp_path = _make_temp_path(self) | |
| def test_write_git_files_excludes_paths(self): | |
| """Test that _write_git_files respects exclude_paths.""" | |
| import shutil | |
| if not shutil.which("git"): | |
| self.skipTest("git command not available") | |
| tmp_path = _make_temp_path(self) |
References
- Demand Robustness: Do not accept fragile code. If the proposed code is not robust enough or lacks proper error handling, explicitly tell the author why the current approach is brittle and what must be done to reinforce it. (link)
Adds helper functions to support git-aware packaging: - _list_git_files(): Lists tracked and non-ignored untracked files - _path_is_excluded(): Checks if a path should be excluded - _write_git_files(): Recursively writes files to ZIP respecting exclusions These functions enable respecting .gitignore when zipping working directories.
- Uses git ls-files when in a git repository to respect .gitignore - Falls back to directory traversal with os.walk when not in a git repo - Maintains all existing features: empty directory preservation, secret detection, path exclusion - Updated docstring to document git integration
Adds tests for: - _list_git_files in a git repository - _list_git_files when not in a git repository (returns None) - _path_is_excluded functionality Tests ensure git integration works correctly and falls back gracefully.
92c48be to
6dcb782
Compare
1 participant
Adds tests for git ls-files integration:
Part of #288 (Phase 3 of 4). Merge after #294.