🧪 [testing improvement] Add tests for decryptor parser and sanitizer#229
🧪 [testing improvement] Add tests for decryptor parser and sanitizer#229
Conversation
Introduces comprehensive unit tests for `parse_ctrtool_output`, `parse_twl_ctrtool_output`, and `sanitize_filename` in `cia_3ds_decryptor.py`. Fixes a bug in the parser logic where `title_version` would not be updated if it was already initialized to its default value '0'. The updated logic ensures that any version information found in the tool output is correctly captured. 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 introduces a new, dedicated test suite to improve the reliability and robustness of the 3DS decryption script's parsing and sanitization functionalities. It addresses a critical bug in the cia_3ds_decryptor.py module, ensuring that title version information is accurately extracted. These changes collectively enhance the script's ability to correctly process and interpret ctrtool output, leading to more dependable decryption operations. 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
|
![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 tests for the decryptor parsing logic and fixes a bug in version parsing. The changes are well-implemented. I've added a couple of suggestions to further improve the test suite's robustness and maintainability. Specifically, I recommend adding a test case that explicitly verifies the bug fix for multiple version entries and refactoring the duplicated module import logic into a shared test utility.
| # Dynamically import cia_3ds_decryptor.py | ||
| file_path = Path(__file__).parent / "cia_3ds_decryptor.py" | ||
| spec = importlib.util.spec_from_file_location("cia_3ds_decryptor", str(file_path)) | ||
| if spec is None: | ||
| 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) |
There was a problem hiding this comment.
This dynamic import logic is duplicated in test_decryptor_counters.py. To improve maintainability and reduce code duplication, consider refactoring this logic into a shared test helper function. For example, you could create a test_helper.py with a function that handles the module loading, which both test files can then import and use.
| # Test that it preserves case (if TRANSLATE_TABLE is correct) | ||
| self.assertEqual(decryptor.sanitize_filename("UPPERCASE.CIA"), "UPPERCASE.CIA") | ||
| # If all characters are invalid, it returns the original string | ||
| self.assertEqual(decryptor.sanitize_filename("!!!"), "!!!") |
There was a problem hiding this comment.
To ensure the bug fix for title_version parsing is robustly tested, it's a good practice to add a test case that explicitly covers the fixed behavior. The current tests don't cover the scenario where multiple TitleVersion lines are present in the input. Adding a test for this will verify that the parser correctly captures the last value, as intended by the fix.
| self.assertEqual(decryptor.sanitize_filename("!!!"), "!!!") | |
| self.assertEqual(decryptor.sanitize_filename("!!!"), "!!!") | |
| def test_version_parsing_uses_last_value(self): | |
| text_with_multiple_versions = """ | |
| Title id: 0004000000000100 | |
| TitleVersion: 1 | |
| TitleVersion: 10 | |
| Crypto Key: Secure | |
| """ | |
| # Test standard parser | |
| info = decryptor.parse_ctrtool_output(text_with_multiple_versions) | |
| self.assertEqual(info.title_version, "10") | |
| # Test TWL parser | |
| info_twl = decryptor.parse_twl_ctrtool_output(text_with_multiple_versions) | |
| self.assertEqual(info_twl.title_version, "10") |
|
❌ Lint/Format Check Failed Please run |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge OverviewThis PR fixes a bug in
AnalysisThe original code had: if not info.title_version and (m := TITLE_VERSION_RE.search(line)):Since Files Reviewed (2 files)
This is a valid bug fix with appropriate test coverage. Reviewed by minimax-m2.5 · 316,980 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 covering ctrtool output parsing and filename sanitization for the 3DS CIA decryptor, and fixes a parsing bug where title_version could remain at its default value even when present in the input.
Changes:
- Add
unittestsuite forparse_ctrtool_output,parse_twl_ctrtool_output, andsanitize_filename. - Fix
title_versionparsing so it updates when aTitleVersion:line is found (CTR + TWL parsers).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Cachyos/Scripts/WIP/emu/test_decryptor_parser.py | New unit tests for parser and sanitizer behavior. |
| Cachyos/Scripts/WIP/emu/cia_3ds_decryptor.py | Fixes TitleVersion extraction by removing the incorrect “only if unset” guard. |
You can also share your feedback on Copilot code review. Take the survey.
| def test_parse_ctrtool_output_full(self): | ||
| text = """ | ||
| Title id: 0004000000000100 | ||
| TitleVersion: 10 | ||
| Crypto Key: Secure | ||
| """ | ||
| info = decryptor.parse_ctrtool_output(text) | ||
| self.assertEqual(info.title_id, "0004000000000100") | ||
| self.assertEqual(info.title_version, "10") | ||
| self.assertEqual(info.crypto_key, "Crypto Key: Secure") | ||
|
|
||
| def test_parse_ctrtool_output_partial(self): | ||
| text = "Title id: 0004000000000100" | ||
| info = decryptor.parse_ctrtool_output(text) | ||
| self.assertEqual(info.title_id, "0004000000000100") | ||
| self.assertEqual(info.title_version, "0") | ||
| self.assertEqual(info.crypto_key, "") | ||
|
|
||
| def test_parse_ctrtool_output_empty(self): | ||
| info = decryptor.parse_ctrtool_output("") | ||
| self.assertEqual(info.title_id, "") | ||
| self.assertEqual(info.title_version, "0") | ||
| self.assertEqual(info.crypto_key, "") | ||
|
|
||
| def test_parse_twl_ctrtool_output_full(self): | ||
| text = """ | ||
| TitleId: 0004800000000100 | ||
| TitleVersion: 5 | ||
| Encrypted: YES | ||
| """ | ||
| info = decryptor.parse_twl_ctrtool_output(text) | ||
| self.assertEqual(info.title_id, "0004800000000100") | ||
| self.assertEqual(info.title_version, "5") | ||
| self.assertEqual(info.crypto_key, "YES") | ||
|
|
||
| def test_sanitize_filename(self): | ||
| self.assertEqual(decryptor.sanitize_filename("Game Name (USA)!?.cia"), "Game Name USA.cia") | ||
| self.assertEqual(decryptor.sanitize_filename("valid_name-123.3ds"), "valid_name-123.3ds") | ||
| # Test that it preserves case (if TRANSLATE_TABLE is correct) | ||
| self.assertEqual(decryptor.sanitize_filename("UPPERCASE.CIA"), "UPPERCASE.CIA") | ||
| # If all characters are invalid, it returns the original string | ||
| self.assertEqual(decryptor.sanitize_filename("!!!"), "!!!") | ||
|
|
||
| if __name__ == '__main__': | ||
| unittest.main() |
| info = decryptor.parse_ctrtool_output(text) | ||
| self.assertEqual(info.title_id, "0004000000000100") | ||
| self.assertEqual(info.title_version, "10") | ||
| self.assertEqual(info.crypto_key, "Crypto Key: Secure") |
This PR addresses the testing gap for the
parse_ctrtool_outputfunction incia_3ds_decryptor.py.🎯 What:
Cachyos/Scripts/WIP/emu/test_decryptor_parser.py.cia_3ds_decryptor.pywheretitle_versionwas not being updated correctly during parsing.📊 Coverage:
parse_ctrtool_output: Full, partial, and empty output scenarios.parse_twl_ctrtool_output: TWL-specific output parsing.sanitize_filename: Filename sanitization with various character sets and case preservation.✨ Result:
PR created automatically by Jules for task 10260828444860348659 started by @Ven0m0