Skip to content

Fix/issue 1111 status dependency probe tests - #1140

Open
kic635 wants to merge 2 commits into
oceanbase:mainfrom
kic635:fix/issue-1111-status-dependency-probe-tests
Open

Fix/issue 1111 status dependency probe tests#1140
kic635 wants to merge 2 commits into
oceanbase:mainfrom
kic635:fix/issue-1111-status-dependency-probe-tests

Conversation

@kic635

@kic635 kic635 commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

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.

@wayyoungboy wayyoungboy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@kic635
kic635 force-pushed the fix/issue-1111-status-dependency-probe-tests branch from 853425d to 672176a Compare July 6, 2026 11:11
… 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>
@kic635
kic635 force-pushed the fix/issue-1111-status-dependency-probe-tests branch from 6dec25f to c941e7c Compare July 6, 2026 11:44
…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>
@kic635

kic635 commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

@wayyoungboy Thanks for the review — addressed.

The dependency-probe endpoint tests are now in the required PR test path:

  • tests/unit/server/test_system_status_dependency_probes.py (6 endpoint tests)
  • tests/unit/server/conftest.py (shared fixtures for this file)

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.

Copy link
Copy Markdown
Member

Thanks for the update. This is still not ready to merge while the previous CHANGES_REQUESTED review is active.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add endpoint-level regression tests for dependency status probes

2 participants