Conversation
* test: skip heidi_cpp-dependent tests when extension missing * chore: retrigger CI * fix: repair validate_clean smoke and relax JSON error assertion * Fix validate_schema error, update test regex, and resolve trusted path issue in 04_train_qlora.py. --------- Co-authored-by: Heidi <heidi@example.com>
Summary of ChangesHello @heidi-dang, 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 enhances the stability and predictability of the test suite, particularly in continuous integration environments where the C++ daemon might not be present. It ensures that tests dependent on the daemon are gracefully skipped if the necessary binary is missing, preventing unnecessary failures. Additionally, it refines error handling within core utility functions to align with standard Python exception practices, making the codebase more robust and easier to debug. 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
|
There was a problem hiding this comment.
Code Review
The pull request introduces logic to skip daemon-related tests when required C++ components are missing, which is a good improvement for CI environments. No security vulnerabilities were found. However, there are a few critical issues: a missing import in the daemon tests will cause them to fail, and a logic error in the validation script incorrectly unpacks a return value, effectively bypassing a validation step and introducing redundant code execution.
| import os | ||
| import shutil | ||
| import socket | ||
| import subprocess | ||
| import time | ||
| import urllib.request | ||
| from pathlib import Path | ||
| import pytest | ||
| import json |
There was a problem hiding this comment.
The urllib.request import was removed in this change, but the module is still used in test_daemon_status_json (line 69) and test_daemon_train_now_action (line 84). This will result in a NameError when these tests are executed.
| import os | |
| import shutil | |
| import socket | |
| import subprocess | |
| import time | |
| import urllib.request | |
| from pathlib import Path | |
| import pytest | |
| import json | |
| import os | |
| import shutil | |
| import socket | |
| import subprocess | |
| import time | |
| import urllib.request | |
| from pathlib import Path | |
| import pytest | |
| import json |
| # Step 1: Schema validation | ||
| valid, reason = validate_schema(sample) | ||
| valid, reason = validate_semantic(sample) | ||
| if not valid: | ||
| return None, f"schema: {reason}" |
There was a problem hiding this comment.
There are multiple issues with this block:
- Incorrect Unpacking:
validate_semanticreturns a nested tuple((bool, str), dict). Unpacking it intovalid, reasonsetsvalidto the tuple(bool, str), which will always evaluate toTruein a boolean context, effectively bypassing the validation check. - Redundancy:
validate_semanticis called again on line 392, making this call redundant. - Safety: This call is not guarded by
HAS_SEMANTIC_VALIDATOR, which will cause aNameErrorif the validator failed to import.
Since semantic validation is handled later in the function (lines 391-399), this block should be removed.
| return parser.parse_args() | ||
|
|
||
|
|
||
| return True, "ok" |
CI runners do not build the C++ daemon. Skip daemon tests at module import when heidid is unavailable to keep matrix deterministic.