Conversation
Implement Option A: skip C++-dependent tests when heidi_cpp is not installed. This prevents pytest collection failures on platforms like macOS by: 1. Creating tests/conftest.py with a `requires_heidi_cpp` marker and skip logic. 2. Removing module-level `import heidi_cpp` in dependent tests. 3. Using session-scoped fixtures for lazy loading of the heidi_cpp extension. 4. Adding explicit skip logic for daemon tests if the heidid binary is missing.
|
👋 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. |
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 addresses and resolves CI failures, particularly on macOS, which were occurring because tests attempted to import the optional 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
This pull request effectively resolves the CI failures on macOS related to the optional heidi_cpp extension by introducing a pytest marker and lazy-loading fixtures. The overall strategy is solid. However, there is an opportunity to improve maintainability by reducing code duplication. The heidi_cpp_mod fixture is defined identically in several test files. This fixture should be centralized in tests/conftest.py to be available to all tests, which will make the test suite cleaner and easier to maintain.
| @pytest.fixture(scope="session") | ||
| def heidi_cpp_mod(): | ||
| import heidi_cpp | ||
| return heidi_cpp |
There was a problem hiding this comment.
To improve maintainability and reduce code duplication, this heidi_cpp_mod fixture should be defined only once in tests/conftest.py. Pytest will automatically discover and provide it to the tests that need it. Please define the fixture in tests/conftest.py and remove the duplicated definitions from this file and other test files (tests/test_core_integration.py, tests/test_cpp_ext.py, tests/test_perf_baseline.py).
| @pytest.fixture(scope="session") | ||
| def heidi_cpp_mod(): | ||
| import heidi_cpp | ||
| return heidi_cpp |
| @pytest.fixture(scope="session") | ||
| def heidi_cpp_mod(): | ||
| import heidi_cpp | ||
| return heidi_cpp |
| def test_real_mode_blocked_in_core_integration(): | ||
| import heidi_cpp | ||
| core = heidi_cpp.Core() |
There was a problem hiding this comment.
For consistency with other tests, this test should use the shared heidi_cpp_mod fixture (from conftest.py) instead of importing heidi_cpp directly.
| def test_real_mode_blocked_in_core_integration(): | |
| import heidi_cpp | |
| core = heidi_cpp.Core() | |
| def test_real_mode_blocked_in_core_integration(heidi_cpp_mod): | |
| core = heidi_cpp_mod.Core() |
| @pytest.fixture(scope="session") | ||
| def heidi_cpp_mod(): | ||
| import heidi_cpp | ||
| return heidi_cpp |
| import shutil | ||
| import pytest | ||
|
|
||
| pytestmark = pytest.mark.requires_heidi_cpp |
This PR resolves multiple CI failures including: 1. Pytest collection errors due to module-level heidi_cpp imports. 2. NameError in scripts/02_validate_clean.py due to missing validate_schema. 3. SystemExit in tests due to strict Zero-Trust validation in general JSONL loading. 4. ImportError in tests/test_loop_runner.py when using the dummy CppLoopRunner placeholder. Changes: - Added `tests/conftest.py` with `requires_heidi_cpp` marker logic. - Refactored tests to use lazy-loading fixtures for `heidi_cpp`. - Implemented missing `validate_schema` in `scripts/02_validate_clean.py`. - Added `is_journal` parameter to `load_jsonl` in `heidi_engine/utils/io_jsonl.py` to allow graceful skipping of invalid lines for non-journal files. - Updated `tests/test_loop_runner.py` to exclude `CppLoopRunner` if extension is missing. - Updated journal tests to explicitly enable strict validation.
Implement Option A for CI stability and refactor JSONL utilities for security. This PR resolves CI collection failures and preserves Zero-Trust pipeline integrity by: 1. Creating tests/conftest.py with a `requires_heidi_cpp` marker and skip logic. 2. Refactoring tests to use lazy-loading fixtures for `heidi_cpp`. 3. Updating `CppLoopRunner` parametrization to avoid dummy placeholder instantiation. 4. Restoring `validate_schema` in `scripts/02_validate_clean.py` and adding regression tests. 5. Splitting `heidi_engine/utils/io_jsonl.py` into `load_jsonl_strict` (default) and `load_jsonl_best_effort`. 6. Updating non-critical scripts to use `load_jsonl_best_effort` while keeping journal paths strict.
This PR addresses the issue where CI fails during pytest collection on macOS because tests import the optional C++ extension
heidi_cppat module import time.Changes:
tests/conftest.pywithrequires_heidi_cppmarker logic.tests/test_budget_guardrails.py,tests/test_core_integration.py, andtests/test_perf_baseline.pyto use lazy-loading fixtures.tests/test_cpp_ext.py,tests/test_doctor.py, andtests/test_sec_redteam.pyto use the new marker system.tests/test_daemon.pyto skip tests if theheididbinary is not found.Verification:
heidi_cppis missing.heidi_cppis built and installed.PR created automatically by Jules for task 11963828567652270193 started by @heidi-dang