test(resilience): add CircuitBreaker tests and fix HALF_OPEN bug#77
test(resilience): add CircuitBreaker tests and fix HALF_OPEN bug#77JuanCS-Dev wants to merge 2 commits intomainfrom
Conversation
…HALF_OPEN bug - Added `tests/unit/core/resilience/test_circuit_breaker.py` to cover state transitions and error handling. - Fixed a bug in `packages/vertice-core/src/vertice_core/resilience/circuit_breaker.py` where the circuit would deadlock in `HALF_OPEN` state if `success_threshold` was greater than 1. Now `_half_open_pending` is reset on success if the threshold is not yet reached, allowing subsequent probes. Co-authored-by: JuanCS-Dev <227056558+JuanCS-Dev@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. |
Summary of ChangesHello @JuanCS-Dev, 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 a critical bug within 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
|
![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 introduces a crucial fix for a potential deadlock in the HALF_OPEN state of the CircuitBreaker and adds a comprehensive suite of unit tests. The fix correctly allows for multiple probe requests when the success threshold is greater than one. The new tests are well-structured and cover all state transitions, significantly improving the robustness of the resilience module. My feedback focuses on enhancing the new tests by making assertions more specific and avoiding direct manipulation of internal state, which will improve their clarity and long-term maintainability.
| assert circuit_breaker.state == CircuitState.HALF_OPEN or circuit_breaker.state == CircuitState.CLOSED | ||
| # Note: If execution succeeds immediately, it might transition to CLOSED depending on success_threshold. | ||
| # In our config, success_threshold=2, so one success keeps it HALF_OPEN or moves towards CLOSED. | ||
| # Wait, execute calls _record_success. If success_threshold is 2, one success means 1/2. | ||
| # So state should still be HALF_OPEN *before* the second success? | ||
| # Actually, `execute` checks state *before* running. | ||
| # If it was OPEN, and time passed, `_check_state` transitions to HALF_OPEN. | ||
| # Then it runs. Then `_record_success` runs. | ||
|
|
||
| # Let's check logic: | ||
| # 1. execute -> _check_state -> (OPEN & time passed) -> _transition_to(HALF_OPEN) | ||
| # 2. execute runs func -> success | ||
| # 3. execute -> _record_success -> consecutive_successes=1. | ||
| # If 1 < 2, remains HALF_OPEN. | ||
|
|
||
| # However, since we mock datetime, let's verify exact behavior step by step if needed. | ||
| # But here we just want to verify it entered HALF_OPEN and executed. |
There was a problem hiding this comment.
This test contains a lot of commented-out developer notes that should be removed to improve readability. Additionally, the assertion assert circuit_breaker.state == CircuitState.HALF_OPEN or circuit_breaker.state == CircuitState.CLOSED is too broad. Given the test's configuration (success_threshold=2), after a single successful call, the state should definitively be HALF_OPEN. A more precise assertion would make the test stronger. It's also good practice to assert that the mocked function was called to ensure the probe request was executed.
| assert circuit_breaker.state == CircuitState.HALF_OPEN or circuit_breaker.state == CircuitState.CLOSED | |
| # Note: If execution succeeds immediately, it might transition to CLOSED depending on success_threshold. | |
| # In our config, success_threshold=2, so one success keeps it HALF_OPEN or moves towards CLOSED. | |
| # Wait, execute calls _record_success. If success_threshold is 2, one success means 1/2. | |
| # So state should still be HALF_OPEN *before* the second success? | |
| # Actually, `execute` checks state *before* running. | |
| # If it was OPEN, and time passed, `_check_state` transitions to HALF_OPEN. | |
| # Then it runs. Then `_record_success` runs. | |
| # Let's check logic: | |
| # 1. execute -> _check_state -> (OPEN & time passed) -> _transition_to(HALF_OPEN) | |
| # 2. execute runs func -> success | |
| # 3. execute -> _record_success -> consecutive_successes=1. | |
| # If 1 < 2, remains HALF_OPEN. | |
| # However, since we mock datetime, let's verify exact behavior step by step if needed. | |
| # But here we just want to verify it entered HALF_OPEN and executed. | |
| assert circuit_breaker.state == CircuitState.HALF_OPEN | |
| mock_func.assert_called_once() |
| circuit_breaker._state = CircuitState.HALF_OPEN | ||
| circuit_breaker._stats.state_changed_at = datetime.utcnow() |
There was a problem hiding this comment.
Directly manipulating internal state like _state and _stats makes tests brittle and overly dependent on the implementation details of the CircuitBreaker class. If these internal attributes are renamed or their logic changes, these tests will break. It's better to use the public API to bring the circuit breaker into the desired state for testing. For example, you can trigger failures to open the circuit, then mock time to transition it to HALF_OPEN. This makes tests more robust and also verifies the state transitions correctly. This approach of modifying internal state is also used in test_half_open_failure_reopens and test_half_open_concurrent_calls and should be refactored there as well.
- **CI/CD:**
- Created `requirements-dev.txt` for development dependencies (fixing `Code Quality & Security` failure).
- Updated `.github/workflows/quality.yml` to use `radon cc --min D` instead of invalid `--fail`.
- Updated `.github/workflows/vertice-mcp-cicd.yaml` to point to correct paths for basic validation.
- Updated `.github/workflows/tui_e2e.yaml` imports implicitly via `measure_quality.py`.
- **Linting & Code Quality:**
- Fixed massive `F821` (undefined name) errors in `shell_main.py` and `cli/repl_masterpiece/repl.py` by adding missing tool and UI component imports.
- Fixed `E721` (type comparison) in `adk/tools.py`.
- Fixed `F601` (duplicate key) in `agents/planner/coordination.py`.
- Fixed `F401` (unused imports) in `agents/planner/coordination.py`.
- Fixed `E741` (ambiguous variable name `l`) in `autonomy/uncertainty.py`.
- Fixed `F821` (forward reference) in `tui/core/response/view_compactor.py`.
- **Python Compatibility:**
- Fixed `NotRequired` import in `core/types.py` for Python 3.10 compatibility using `typing_extensions`.
- **TUI E2E:**
- Fixed `ImportError` in `scripts/e2e/measure_quality.py` by importing from `vertice_core.tui` instead of `vertice_tui`.
Co-authored-by: JuanCS-Dev <227056558+JuanCS-Dev@users.noreply.github.com>
JuanCS-Dev
deleted the
test-circuit-breaker-improvement-7174230857590657879
branch
1 participant
Added comprehensive unit tests for
CircuitBreakercovering state transitions (CLOSED, OPEN, HALF_OPEN) and error handling. Fixed a bug whereHALF_OPENstate would deadlock ifsuccess_threshold > 1.Verified with new tests passing. Existing resilience tests (e.g.,tests/test_recovery_enhancements.py) also pass. Note:tests/providers/test_resilience.pyis currently broken due to unrelated import errors and API mismatches.PR created automatically by Jules for task 7174230857590657879 started by @JuanCS-Dev