Conversation
- Use pytest.importorskip('heidi_cpp') in tests to avoid collection errors on macOS/Windows.
- Add a preflight CI job to detect C++ availability.
- Gate C++ build/test jobs behind runner OS and preflight output.
- Add an always-run 'ci-complete' job to every workflow for reliable branch protection status.
- Ensure integration tests check for heidid binary existence before running.
- Fix tests/test_jsonl_utils.py and tests/test_loop_runner.py to work with current engine logic while safely skipping C++ components.
|
👋 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 enhances the cross-platform compatibility and stability of the CI pipeline by making optional C++ components non-fatal on platforms where they are not supported or built. The changes primarily involve modifying test files to conditionally skip tests that rely on these components, rather than failing due to missing imports or binaries. This ensures that CI runs smoothly across different operating systems while maintaining full test coverage on Linux. Highlights
Changelog
Ignored Files
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 effectively addresses CI breakages on non-Linux platforms by making optional C++ components non-fatal. The use of pytest.importorskip and existence checks for binaries are appropriate solutions. The test data in test_jsonl_utils.py has been updated to align with stricter validation, which is a great improvement. I've identified a critical issue in tests/test_doctor.py where a missing import will cause tests to fail. Additionally, I've suggested a couple of improvements in tests/test_loop_runner.py to make the type checks more robust and less reliant on string comparisons. Overall, the changes are solid and move in the right direction for cross-platform compatibility.
|
|
||
| def test_real_mode_blocked_in_core_integration(): | ||
| import heidi_cpp | ||
| heidi_cpp = pytest.importorskip("heidi_cpp") |
There was a problem hiding this comment.
The pytest module is used here but it does not appear to be imported in this file. This will cause a NameError when the test runs. Please add import pytest at the top of the file to resolve this.
tests/test_loop_runner.py
Outdated
| @pytest.fixture(params=get_runner_classes()) | ||
| def runner_class(request, monkeypatch): | ||
| if CppLoopRunner and request.param == CppLoopRunner: | ||
| if "CppLoopRunner" in str(request.param): |
There was a problem hiding this comment.
Using str() for type checking is brittle and can lead to bugs if class names change or if another class name contains this substring. It's better to check the class name directly using __name__ for robustness.
| if "CppLoopRunner" in str(request.param): | |
| if request.param.__name__ == "CppLoopRunner": |
tests/test_loop_runner.py
Outdated
|
|
||
| # Check that scripts were "called" if using Python | ||
| if runner_class == PythonLoopRunner: | ||
| if "PythonLoopRunner" in str(runner_class): |
There was a problem hiding this comment.
Using str() for type checking is brittle. Since PythonLoopRunner is imported in this module, you can use a direct identity check with is. This is more robust and Pythonic. This feedback applies to similar checks on lines 94, 108, and 136 in this file.
| if "PythonLoopRunner" in str(runner_class): | |
| if runner_class is PythonLoopRunner: |
- Re-implemented pytest gating using standard importorskip and subclass checks. - Added os.X_OK check for heidid binary in integration tests. - Hardened CI preflight job to check for both build scripts and source files. - Ensured all tests skip gracefully on non-Linux platforms without collection errors. - Verified ci-complete aggregator logic across all workflows.
- Use pytest.importorskip('heidi_cpp') in all relevant tests.
- Gate C++ builds in CI using a robust preflight check and runner.os filter.
- Add always-run ci-complete aggregator jobs to all workflows.
- Fix load_jsonl to support optional telemetry validation (defaulting to True).
- Update scripts/02_validate_clean.py and scripts/03_unit_test_gate.py to correctly opt-out of telemetry validation.
- Restore validate_schema in scripts/02_validate_clean.py to fix NameError.
- Add os.X_OK check for heidid binary in integration tests.
1 participant
This PR addresses cross-platform CI breakages where macOS and Windows runners fail during test collection because they try to import or build optional C++ components (heidi_cpp, heidid) that are only supported/present on Linux.
Key changes:
heidi_cppto usepytest.importorskip("heidi_cpp"). This ensures that if the extension is not built, the tests are skipped instead of causing a collection error.heididbinary intests/test_daemon.py.preflightjob to.github/workflows/ci.ymlto detect ifsetup_cpp.pyexists.if: runner.os == 'Linux'.ci-completejob to all workflows that always runs and aggregates status. This is critical for branch protection when some jobs are skipped.tests/test_jsonl_utils.pyto provide valid telemetry data, satisfying the "Zero-Trust" validation inload_jsonlwithout changing the core engine logic. Refactoredtests/test_loop_runner.pyto safely handle the absence ofheidi_cpp.These changes ensure that CI remains green on all platforms while still running the full suite on Linux. No product behavior was changed.
PR created automatically by Jules for task 16828375979512091972 started by @heidi-dang