Fix/issue 1111 status dependency probe tests - #1140
Conversation
wayyoungboy
left a comment
There was a problem hiding this comment.
Requesting changes because the new issue #1111 coverage is not exercised by the required PR test workflow.
This PR adds the dependency-probe tests under tests/regression/test_system_status_dependency_probes.py, but the regular Tests workflow runs pytest --ignore=tests/regression -m "not e2e and not e2e_config". The Regression Tests workflow is also skipped for fork PRs unless it is manually dispatched. I verified the new regression test file locally and it passes, but the green required PR checks do not actually validate this new coverage.
Please move or duplicate a minimal dependency-probe regression into the required test path, or make sure a regression workflow run for this PR is required/attached before merge.
853425d to
672176a
Compare
… CI (oceanbase#1111) Exercise /system/status and /system/health probe behavior through HTTP in tests/unit/server so the default PR test workflow covers issue oceanbase#1111. Share fixtures via tests/helpers, keep a regression re-export, and harden CI stability with async probe cleanup, auth dependency override, and wall-clock TTL expiry. Co-authored-by: Cursor <cursoragent@cursor.com>
6dec25f to
c941e7c
Compare
…base#1111) Drop tests/helpers and regression re-exports; keep endpoint coverage in the required CI path only. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@wayyoungboy Thanks for the review — addressed. The dependency-probe endpoint tests are now in the required PR test path:
They are exercised by the regular Tests workflow: pytest --ignore=tests/regression -m "not e2e and not e2e_config" The old tests/regression/test_system_status_dependency_probes.py entry point has been removed, so we no longer rely on the Regression Tests workflow (which is skipped for fork PRs). CI test (3.11) and test (3.12) are green on the latest commit. No production code changes. |
|
Thanks for the update. This is still not ready to merge while the previous Please request re-review after confirming that the dependency-probe endpoint regression tests are exercised by the required PR test workflow, not only by a non-required path. Once the blocking review is cleared, this can be reconsidered for merge. |
Summary
Fix #1111
PR #1085 added bounded, non-blocking dependency status probes for /api/v1/system/status. The existing unit tests in tests/unit/server/test_health_check.py cover the helper layer, but there was no endpoint-level regression coverage through the public HTTP routes.
This PR adds a regression suite under tests/regression/ that exercises /api/v1/system/health and /api/v1/system/status directly, so the probe timeout, single-flight coalescing, dedicated executor, TTL cache, and fallback behavior introduced in #1085 stay protected going forward.
Solution Description
Added tests/regression/test_system_status_dependency_probes.py with six endpoint-level regression tests:
Blocked status probe does not block health — while a database probe is stuck, /api/v1/system/health still responds promptly with 200.
Parallel bounded timeouts via dedicated executor — both database and LLM probes time out in parallel within the configured bound; work goes through _DEPENDENCY_PROBE_EXECUTOR, not the default executor.
Single-flight reuse for stuck probes — repeated /status polls while a timed-out probe is still in-flight do not submit additional workers.
TTL cache reuse and refresh — cached dependency statuses are reused within TTL and refreshed after expiry (deterministic via mocked monotonic clock, no flaky sleep).
Cached degraded status preserved — degraded dependency results and error_message are served from cache on subsequent polls within TTL.
Probe exception fallback — when a dependency probe raises, /status returns 200 with dependencies={} instead of a 500.
Also registered the regression pytest marker in pyproject.toml so the suite can be run selectively:
pytest -m regression tests/regression/test_system_status_dependency_probes.py -v
No production code changes — test coverage only.