🧪 [testing improvement] Add unit tests for sanitize_filename#230
🧪 [testing improvement] Add unit tests for sanitize_filename#230
Conversation
This commit adds a new test file `Cachyos/Scripts/WIP/emu/test_sanitize_filename.py` which provides unit tests for the `sanitize_filename` function in `cia_3ds_decryptor.py`. The tests cover: - Preservation of valid characters (a-z, A-Z, 0-9, -, _, ., and spaces) - Removal of invalid characters - Fallback behavior when all characters are invalid - Mixed case preservation - Empty string handling Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the robustness of the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
❌ Lint/Format Check Failed Please run |
![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 adds valuable unit tests for the sanitize_filename function, significantly improving test coverage for this utility. The tests cover the main use cases, including valid and invalid characters, and edge cases like empty strings. My review includes a suggestion to refactor the tests into a single data-driven test method. This improves maintainability and also allows for the inclusion of new test cases that expose a potential bug in the sanitize_filename function's handling of Unicode characters outside the Latin-1 range.
| def test_preservation_of_valid_characters(self): | ||
| # a-z, A-Z, 0-9, -, _, ., and spaces | ||
| input_name = "test-FILE_123.3ds" | ||
| expected = "test-FILE_123.3ds" | ||
| self.assertEqual(decryptor.sanitize_filename(input_name), expected) | ||
|
|
||
| def test_removal_of_invalid_characters(self): | ||
| # !@#$%^&*()+={}[]|\:;"'<>,/? should be removed | ||
| input_name = "test!@#$ %^&*()_+= file.cia" | ||
| # VALID_CHARS = frozenset("-_abcdefghijklmnopqrstuvwxyz1234567890. ") | ||
| # "test", " ", "_", " ", "file.cia" are valid | ||
| expected = "test _ file.cia" | ||
| self.assertEqual(decryptor.sanitize_filename(input_name), expected) | ||
|
|
||
| def test_fallback_behavior(self): | ||
| # If all characters are removed, the original name is returned. | ||
| input_name = "!!!@@@###" | ||
| # All are invalid, so 'out' would be empty, returns original | ||
| expected = "!!!@@@###" | ||
| self.assertEqual(decryptor.sanitize_filename(input_name), expected) | ||
|
|
||
| def test_mixed_case_preservation(self): | ||
| input_name = "MixedCaseFILENAME.3ds" | ||
| expected = "MixedCaseFILENAME.3ds" | ||
| self.assertEqual(decryptor.sanitize_filename(input_name), expected) | ||
|
|
||
| def test_empty_string(self): | ||
| self.assertEqual(decryptor.sanitize_filename(""), "") |
There was a problem hiding this comment.
The tests can be consolidated into a single, data-driven method using subTest for improved maintainability and readability. This refactoring also allows for easily adding more test cases.
I've included new test cases for Unicode and emoji characters. The current sanitize_filename implementation appears to have a bug with characters outside the Latin-1 range (e.g., emoji) because it only considers range(256). The new test case for emoji will fail, highlighting this gap in functionality and test coverage.
| def test_preservation_of_valid_characters(self): | |
| # a-z, A-Z, 0-9, -, _, ., and spaces | |
| input_name = "test-FILE_123.3ds" | |
| expected = "test-FILE_123.3ds" | |
| self.assertEqual(decryptor.sanitize_filename(input_name), expected) | |
| def test_removal_of_invalid_characters(self): | |
| # !@#$%^&*()+={}[]|\:;"'<>,/? should be removed | |
| input_name = "test!@#$ %^&*()_+= file.cia" | |
| # VALID_CHARS = frozenset("-_abcdefghijklmnopqrstuvwxyz1234567890. ") | |
| # "test", " ", "_", " ", "file.cia" are valid | |
| expected = "test _ file.cia" | |
| self.assertEqual(decryptor.sanitize_filename(input_name), expected) | |
| def test_fallback_behavior(self): | |
| # If all characters are removed, the original name is returned. | |
| input_name = "!!!@@@###" | |
| # All are invalid, so 'out' would be empty, returns original | |
| expected = "!!!@@@###" | |
| self.assertEqual(decryptor.sanitize_filename(input_name), expected) | |
| def test_mixed_case_preservation(self): | |
| input_name = "MixedCaseFILENAME.3ds" | |
| expected = "MixedCaseFILENAME.3ds" | |
| self.assertEqual(decryptor.sanitize_filename(input_name), expected) | |
| def test_empty_string(self): | |
| self.assertEqual(decryptor.sanitize_filename(""), "") | |
| def test_all_scenarios(self): | |
| scenarios = { | |
| "preservation_of_valid_characters": ("test-FILE_123.3ds", "test-FILE_123.3ds"), | |
| "removal_of_invalid_characters": ("test!@#$ %^&*()_+= file.cia", "test _ file.cia"), | |
| "fallback_behavior": ("!!!@@@###", "!!!@@@###"), | |
| "mixed_case_preservation": ("MixedCaseFILENAME.3ds", "MixedCaseFILENAME.3ds"), | |
| "empty_string": ("", ""), | |
| "unicode_chars_in_latin1": ("file-éà.txt", "file-.txt"), | |
| "unicode_chars_outside_latin1": ("test👍.txt", "test.txt"), | |
| } | |
| for name, (input_name, expected) in scenarios.items(): | |
| with self.subTest(name): | |
| self.assertEqual(decryptor.sanitize_filename(input_name), expected) |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| # VALID_CHARS = frozenset("-_abcdefghijklmnopqrstuvwxyz1234567890. ") | ||
| # "test", " ", "_", " ", "file.cia" are valid | ||
| expected = "test _ file.cia" | ||
| self.assertEqual(decryptor.sanitize_filename(input_name), expected) |
There was a problem hiding this comment.
WARNING: Incorrect expected value - the output should have double spaces where special characters were removed.
The characters !, @, #, $, %, ^, &, *, (, ), +, = are all removed, leaving two consecutive spaces between "test" and "" and between "" and "file.cia".
Current expected: "test _ file.cia"
Correct expected: "test _ file.cia"
## Code Review Summary**Status:** 1 Issue Found | **Recommendation:** Address before merge### Overview| Severity | Count ||----------|-------|| CRITICAL | 0 || WARNING | 1 || SUGGESTION | 0 |Issue Details (click to expand)#### WARNING| File | Line | Issue ||------|------|-------|| `Cachyos/Scripts/WIP/emu/test_sanitize_filename.py` | 30 | Incorrect expected value - should have double spaces where special characters were removed |Other Observations (not in diff)None - the underlying `sanitize_filename` function in `cia_3ds_decryptor.py` appears to work correctly.Files Reviewed (1 file)- `Cachyos/Scripts/WIP/emu/test_sanitize_filename.py` - 1 issueReviewed by minimax-m2.5-20260211 · 333,992 tokens |
Current Aviator status
This pull request is currently open (not queued). How to mergeTo merge this PR, comment
See the real-time status of this PR on the
Aviator webapp.
Use the Aviator Chrome Extension
to see the status of your PR within GitHub.
|
There was a problem hiding this comment.
Pull request overview
Adds unit tests for sanitize_filename in the 3DS/CIA decryptor script under Cachyos/Scripts/WIP/emu, aiming to close an existing testing gap.
Changes:
- Introduces a new
unittestmodule that dynamically importscia_3ds_decryptor.py. - Adds test cases covering valid character preservation, invalid character removal, fallback behavior, mixed-case handling, and empty input.
You can also share your feedback on Copilot code review. Take the survey.
| #!/usr/bin/env python3 | ||
| import importlib.util | ||
| import unittest | ||
| import sys | ||
| from pathlib import Path |
| raise ImportError(f"Could not load {file_path}") | ||
| decryptor = importlib.util.module_from_spec(spec) | ||
| sys.modules["cia_3ds_decryptor"] = decryptor | ||
| spec.loader.exec_module(decryptor) | ||
|
|
||
|
|
||
| class TestSanitizeFilename(unittest.TestCase): | ||
| def test_preservation_of_valid_characters(self): | ||
| # a-z, A-Z, 0-9, -, _, ., and spaces | ||
| input_name = "test-FILE_123.3ds" | ||
| expected = "test-FILE_123.3ds" | ||
| self.assertEqual(decryptor.sanitize_filename(input_name), expected) | ||
|
|
||
| def test_removal_of_invalid_characters(self): | ||
| # !@#$%^&*()+={}[]|\:;"'<>,/? should be removed | ||
| input_name = "test!@#$ %^&*()_+= file.cia" | ||
| # VALID_CHARS = frozenset("-_abcdefghijklmnopqrstuvwxyz1234567890. ") | ||
| # "test", " ", "_", " ", "file.cia" are valid | ||
| expected = "test _ file.cia" | ||
| self.assertEqual(decryptor.sanitize_filename(input_name), expected) | ||
|
|
||
| def test_fallback_behavior(self): | ||
| # If all characters are removed, the original name is returned. | ||
| input_name = "!!!@@@###" | ||
| # All are invalid, so 'out' would be empty, returns original | ||
| expected = "!!!@@@###" | ||
| self.assertEqual(decryptor.sanitize_filename(input_name), expected) | ||
|
|
||
| def test_mixed_case_preservation(self): | ||
| input_name = "MixedCaseFILENAME.3ds" | ||
| expected = "MixedCaseFILENAME.3ds" | ||
| self.assertEqual(decryptor.sanitize_filename(input_name), expected) | ||
|
|
||
| def test_empty_string(self): | ||
| self.assertEqual(decryptor.sanitize_filename(""), "") | ||
|
|
||
| if __name__ == '__main__': | ||
| unittest.main() |
🎯 What: The testing gap addressed
The
sanitize_filenamefunction inCachyos/Scripts/WIP/emu/cia_3ds_decryptor.pywas previously untested. This PR adds a comprehensive suite of unit tests for this function.📊 Coverage: What scenarios are now tested
✨ Result: The improvement in test coverage
The
sanitize_filenamefunction is now fully covered by automated unit tests, ensuring its logic remains correct during future refactoring or enhancements. These tests useimportlib.utilfor dynamic loading, consistent with existing tests in the directory.PR created automatically by Jules for task 4989178813754679629 started by @Ven0m0