Skip to content

RHIDP-14000: update mcp and query interrupt tests#1888

Merged
tisnik merged 2 commits into
lightspeed-core:mainfrom
Jdubrick:RHIDP-14000-mcp-interrupt-tests
Jun 10, 2026
Merged

RHIDP-14000: update mcp and query interrupt tests#1888
tisnik merged 2 commits into
lightspeed-core:mainfrom
Jdubrick:RHIDP-14000-mcp-interrupt-tests

Conversation

@Jdubrick

@Jdubrick Jdubrick commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Description

MCP

  • test_delete_mcp_server_toolgroup_not_found_is_idempotent
    • dynamic server in local config; Llama Stack returns NotFoundError on unregister → still returns 200 with deleted=True and removes local config
  • test_list_mcp_servers_configuration_not_loaded
    • list handler returns 500 when configuration is not loaded

Benefits: Covers the idempotent delete path when the Llama Stack toolgroup is already gone (previously untested). Aligns list handler with config-not-loaded coverage on other endpoints.

Stream Interrupt

Unit tests:

  • test_stream_interrupt_registry_concurrent_cancel_and_deregister
    • races cancel_stream vs deregister_stream under the registry lock; asserts no exceptions and CI-friendly timeouts
  • test_stream_interrupt_endpoint_double_interrupt
    • second interrupt after successful cancel returns interrupted=False (no error on client retry)

Integration:

  • test_stream_interrupt_nonexistent_request_returns_404
    • HTTP POST with valid SUID but no active stream → 404 through full FastAPI stack
  • test_stream_interrupt_full_round_trip
    • fixed to load config via test_config so @authorize resolves correctly

Benefits: Protects thread-safe registry behavior and double-interrupt semantics. The HTTP 404 test validates routing/auth/handler wiring that direct handler calls miss.

Type of change

  • Refactor
  • New feature
  • Bug fix
  • CVE fix
  • Optimization
  • Documentation Update
  • Configuration Update
  • Bump-up service version
  • Bump-up dependent library
  • Bump-up library or tool used for development (does not change the final image)
  • CI configuration change
  • Konflux configuration change
  • Unit tests improvement
  • Integration tests improvement
  • End to end tests improvement
  • Benchmarks improvement

Tools used to create PR

Identify any AI code assistants used in this PR (for transparency and review context)

  • Assisted-by: Composer 2.5
  • Generated by: (e.g., tool name and version; N/A if not used)

Related Tickets & Documents

Checklist before requesting a review

  • I have performed a self-review of my code.
  • PR has passed all pre-merge test jobs.
  • If it is a core feature, I have added thorough tests.

Testing

  • Please provide detailed steps to perform tests related to this code change.
  • How were the fix/results from this change verified? Please provide relevant screenshots or results.

Summary by CodeRabbit

  • Tests
    • Added integration test verifying interrupt endpoint returns 404 for unknown requests.
    • Expanded streaming interrupt tests to cover double-interrupt idempotency and concurrent cancel/deregister race conditions.
    • Added unit tests ensuring MCP server delete remains idempotent when upstream toolgroup is missing.
    • Added unit test asserting listing servers returns a clear error when configuration is not loaded.

@coderabbitai

coderabbitai Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 3b69e8c7-1845-4391-a160-7f11e9a812d7

📥 Commits

Reviewing files that changed from the base of the PR and between bdd73aa and 0cc9905.

📒 Files selected for processing (4)
  • tests/integration/endpoints/test_stream_interrupt_integration.py
  • tests/unit/app/endpoints/test_mcp_servers.py
  • tests/unit/app/endpoints/test_stream_interrupt.py
  • tests/unit/pydantic_ai_lightspeed/llamastack/test_transport.py
📜 Recent review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
  • GitHub Check: Pylinter
  • GitHub Check: spectral
  • GitHub Check: build-pr
  • GitHub Check: Pyright
  • GitHub Check: mypy
  • GitHub Check: unit_tests (3.13)
  • GitHub Check: unit_tests (3.12)
  • GitHub Check: E2E: server mode / ci / group 1
  • GitHub Check: E2E: library mode / ci / group 2
  • GitHub Check: E2E: library mode / ci / group 3
  • GitHub Check: E2E: server mode / ci / group 2
  • GitHub Check: E2E: server mode / ci / group 3
  • GitHub Check: E2E: library mode / ci / group 1
  • GitHub Check: E2E Tests for Lightspeed Evaluation job
  • GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
🧰 Additional context used
📓 Path-based instructions (1)
tests/**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

tests/**/*.py: Use pytest for all unit and integration tests; do not use unittest
Use pytest.mark.asyncio marker for async tests

Files:

  • tests/unit/app/endpoints/test_mcp_servers.py
  • tests/integration/endpoints/test_stream_interrupt_integration.py
  • tests/unit/pydantic_ai_lightspeed/llamastack/test_transport.py
  • tests/unit/app/endpoints/test_stream_interrupt.py
🔇 Additional comments (9)
tests/unit/app/endpoints/test_mcp_servers.py (1)

9-10: LGTM!

Also applies to: 393-427, 429-449

tests/integration/endpoints/test_stream_interrupt_integration.py (3)

7-8: LGTM!

Also applies to: 11-11, 16-16


30-36: LGTM!


73-84: LGTM!

tests/unit/app/endpoints/test_stream_interrupt.py (3)

4-4: LGTM!

Also applies to: 13-13, 20-23


156-197: LGTM!


200-227: LGTM!

tests/unit/pydantic_ai_lightspeed/llamastack/test_transport.py (2)

6-6: LGTM!


49-49: LGTM!

Also applies to: 63-63, 138-138, 321-321


Walkthrough

This PR adds test coverage for error handling and edge cases across streaming interrupt and MCP server endpoints. Stream interrupt integration tests now cover 404 responses for non-existent request IDs, unit tests verify concurrent registry safety and endpoint idempotency, and MCP server tests ensure deletion succeeds idempotently when toolgroups are missing and list operations handle missing configuration.

Changes

Stream interrupt and MCP server error handling test coverage

Layer / File(s) Summary
Stream interrupt 404 integration test
tests/integration/endpoints/test_stream_interrupt_integration.py
Adds FastAPI status import and REQUEST_ID_NOT_IN_REGISTRY constant, updates test_stream_interrupt_full_round_trip to accept test_config: AppConfig, and introduces test_stream_interrupt_nonexistent_request_returns_404 to POST an unknown request ID and assert HTTP 404.
Stream interrupt concurrency and idempotency unit tests
tests/unit/app/endpoints/test_stream_interrupt.py
Adds threading import and timeout constants, then introduces test_stream_interrupt_registry_concurrent_cancel_and_deregister to verify safe concurrent access to the registry and test_stream_interrupt_endpoint_double_interrupt to verify endpoint idempotency (first interrupt returns interrupted=True, second returns interrupted=False).
MCP server deletion idempotency and configuration error tests
tests/unit/app/endpoints/test_mcp_servers.py
Adds fastapi.status and NotFoundError imports, then introduces test_delete_mcp_server_toolgroup_not_found_is_idempotent to verify deletion succeeds when upstream reports missing toolgroup, and test_list_mcp_servers_configuration_not_loaded to verify HTTP 500 with configuration error when AppConfig._configuration is unset.
Transport test typing adjustments
tests/unit/pydantic_ai_lightspeed/llamastack/test_transport.py
Adds AsyncGenerator import and explicit AsyncGenerator[...] return annotations to mocked async generators in several transport tests (no behavioral changes).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested reviewers

  • tisnik
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 61.90% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'RHIDP-14000: update mcp and query interrupt tests' directly corresponds to the PR's main objective: adding test coverage for MCP server endpoints and stream interrupt functionality, as evidenced by updates to four test files focusing on these areas.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
✨ Simplify code
  • Create PR with simplified code

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Jdubrick added 2 commits June 10, 2026 10:15
Signed-off-by: Jordan Dubrick <jdubrick@redhat.com>
Signed-off-by: Jordan Dubrick <jdubrick@redhat.com>
@Jdubrick Jdubrick force-pushed the RHIDP-14000-mcp-interrupt-tests branch from 73e6d19 to 0cc9905 Compare June 10, 2026 14:15

@tisnik tisnik left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@tisnik tisnik merged commit b4216ef into lightspeed-core:main Jun 10, 2026
31 of 32 checks passed
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.

2 participants