chore: Move and run Python SDK integration tests

[edwinjosechittilappilly]

This pull request introduces improvements to integration and SDK testing workflows, enhances test output clarity, and adds a comprehensive QA checklist for the Python SDK. The changes also refine error handling for conversation deletion and streamline onboarding logic for SDK tests. The most significant updates are grouped below.

Test Workflow & Output Enhancements

• Integration and SDK tests are now grouped and visually separated in the Makefile using GitHub Actions-style output (::group::), making CI logs easier to navigate. [1] [2] [3] [4] [5]
• SDK integration tests now run from tests/integration/sdk/ instead of a single test file, and use a dedicated install step for the Python SDK. [1] [2] [3]

SDK Integration Test Infrastructure

• Added tests/integration/sdk/conftest.py to provide shared fixtures, API key management, onboarding, and test file creation for SDK tests.
• Added a detailed QA checklist in tests/integration/sdk/README.md covering 51 tests across 8 domains for the Python SDK.
• Introduced test_auth.py for authentication and API key behavior tests in the SDK integration suite.
• Modified backend onboarding logic to skip in-process setup when SDK_TESTS_ONLY=true, preventing unwanted state resets during SDK integration tests.

Error Handling Improvements

• Improved conversation deletion logic in the backend (chat_service.py and chat.py) to return False if the conversation does not exist, and to respond with HTTP 404 when appropriate. [1] [2] [3]

Test Scoping & Directory Structure

• Integration tests for the core system are now scoped to tests/integration/core/ for both CI and local runs, separating them from SDK tests. [1] [2] [3]

---

These changes collectively improve test clarity, reliability, and coverage for both core and SDK integration workflows.

[Copilot]

Pull request overview

This PR reorganizes and streamlines SDK integration testing by moving Python SDK integration tests into the main repo’s tests/ tree and updating CI/local Makefile targets to run core integration tests separately from SDK tests against an already-running stack. It also adjusts chat deletion semantics so “missing conversation” can be treated as a not-found case, and updates the Python SDK to return False on 404 for delete.

Changes:

• Add a new tests/integration/sdk/ suite (split by domain) and remove the old sdks/python/tests/test_integration.py.
• Update Makefile targets (test-integration, test-ci, test-ci-local, test-sdk) to run core integration tests separately and run SDK tests with SDK_TESTS_ONLY=true.
• Update backend + Python SDK behavior so deleting a nonexistent conversation returns False (via 404 on the API + NotFound handling in the Python SDK).

Reviewed changes

Copilot reviewed 17 out of 21 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
tests/integration/sdk/conftest.py	Adds fixtures for SDK integration tests against an external OpenRAG instance.
tests/integration/sdk/test_*.py	New Python SDK integration test coverage across auth/chat/docs/search/settings/models/filters/errors/e2e.
tests/integration/sdk/README.md	Documents the SDK integration test checklist and how to run it.
sdks/python/tests/test_integration.py	Removes the old SDK integration test module from the SDK package.
tests/conftest.py	Skips in-process backend onboarding/cleanup when SDK_TESTS_ONLY=true.
src/services/chat_service.py	Changes delete-session behavior to distinguish “not found” from success (local-first).
src/api/v1/chat.py	Returns HTTP 404 for deleting a missing conversation.
sdks/python/openrag_sdk/chat.py	Catches NotFoundError on delete and returns False.
Makefile	Updates test targets to run core integration tests and SDK tests separately; installs SDK editable for SDK test runs.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

[mpawlow]

@edwinjosechittilappilly

Code Review 1

• See: PR comments: (1a) to (1n)

[mpawlow]

(1d) [Normal] Thread-unsafe global API key cache; pytest.skip called outside a fixture

• Problem: get_api_key() uses a module-level _cached_api_key global with no lock.
• Under any parallel fixture execution, two callers can both observe _cached_api_key is None simultaneously, each call httpx.post(...), and create two API keys (leaking credentials).
• Proposed Solution: Use a threading.Lock around cache population, or convert this to a session-scoped async fixture that yields the key.

[mpawlow]

🚀 Partially fixed.

• The module-level _cached_api_key global still has no asyncio.Lock, so two coroutines racing through _fetch_api_key() before either completes can create two API keys

(2a) [Minor] TOCTOU race in _fetch_api_key() - two coroutines can both create API keys

• Problem: In asyncio, two coroutines that both enter _fetch_api_key() before either reaches the await ac.post(...) checkpoint will both pass the if _cached_api_key is not None guard and both issue API key
  creation requests, producing two keys on the server. The second key is permanently orphaned.
• Proposed Solution: Use an asyncio.Lock to guard the check-and-set:

_api_key_lock = asyncio.Lock()
async def _fetch_api_key():
    global _cached_api_key
    async with _api_key_lock:
        if _cached_api_key is not None:
            return _cached_api_key
        ...

• Note: This issue is Minor severity. Please feel free to optionally fix or ignore

[mpawlow]

(1e) [Normal] pytest.skip called outside a fixture

• Problem: pytest.skip() is called from a non-fixture helper function, which can cause obscure DID NOT RAISE errors.

[mpawlow]

📝

[mpawlow]

(1j) [Minor] Test files import private _base_url directly instead of using the fixture

• Also, see: tests/integration/sdk/test_errors.py:9
• Problem: from .conftest import _base_url imports a module-level private variable directly, bypassing pytest's fixture mechanism.
• If OPENRAG_URL is modified after module import, the test files will use a stale value.
• Proposed Solution: Use the base_url fixture already defined in conftest.py:74–77.
• Note: This issue is Minor severity. Please feel free to optionally fix or ignore

[mpawlow]

📝

[mpawlow]

(1k) [Normal] Chat tests create conversations with no cleanup

• Problem: Tests creating conversations via client.chat.create(...) never delete them. In a long-lived development environment these accumulate indefinitely.
• Proposed Solution: Add await client.chat.delete(response.chat_id) in a finally block, or use a fixture for conversation lifecycle management.

[mpawlow]

(1l) [Minor] test_knowledge_filter_crud is very monolithic and any step failure leaks the filter resource

• Problem: The full CRUD lifecycle is a single test. If create succeeds but a later step fails, the filter is never deleted.
• i.e. There's no try/finally around the delete call.
• Proposed Solution: Wrap the body in try/finally to ensure deletion, or split into separate tests with a session-scoped fixture managing filter lifecycle.
• Note: This issue is Minor severity. Please feel free to optionally fix or ignore

[mpawlow]

📝

[mpawlow]

(1m) [Normal] test_search_returns_result_fields silently passes with zero results

• Problem: For result in results.results: is a no-op if the list is empty.
• The test passes green even though nothing about result fields was verified.
• Proposed Solution: Add assert len(results.results) > 0 before the loop.

[mpawlow]

📝

[mpawlow]

(1n) [Minor] asyncio_mode not configured in root pyproject.toml

• Note: Feedback for pyproject.toml file
• Applicable to all SDK test files
• Problem: Tests use @pytest.mark.asyncio on every async method.
• The root pyproject.toml does not set asyncio_mode = "auto" (only sdks/python/pyproject.toml does).
• Running pytest tests/integration/sdk/ from the project root may produce collection warnings or failures depending on the installed pytest-asyncio version.
• Proposed Solution: Add asyncio_mode = "auto" to the [tool.pytest.ini_options] section in the root pyproject.toml.
• Note: This issue is Minor severity. Please feel free to optionally fix or ignore

[mpawlow]

📝

[edwinjosechittilappilly]

Rebased to release branch.

[mpawlow]

@edwinjosechittilappilly

Code Review 2

• LGTM / Approved (partial)
• See PR Comments: (2a) to (2c)

[mpawlow]

(2b) [Major] tests/conftest.py SDK_TESTS_ONLY guard is inside the fixture body; module-level imports already executed at collection time

• Related Lines: ~18–20
• Problem: The fix added a yield; return guard inside the onboard_system fixture body, but from config.settings import clients, from session_manager import SessionManager, and from main import generate_jwt_keys are module-level imports that execute during pytest's collection phase, before any fixture runs.
• In a pure SDK-only environment without backend packages installed, pytest fails during collection with ModuleNotFoundError.
• Proposed Solution: Move the imports inside the fixture body, guarded by the SDK_TESTS_ONLY check:

@pytest.fixture(scope="session", autouse=True)
def onboard_system():
    if os.environ.get("SDK_TESTS_ONLY") == "true":
        yield
        return
    from config.settings import clients
    from session_manager import SessionManager
    from main import generate_jwt_keys
    ...

[edwinjosechittilappilly]

Good catch I belive I already moved it inside onboard_system ?

[mpawlow]

(2c) [Normal] connector_sync mutation of connector.bucket_names is not concurrency-safe

• Problem: If two concurrent sync requests arrive for the same connector with different bucket_filter values, both requests write to connector.bucket_names on what is likely a shared/cached connector instance.
• The finally: connector.bucket_names = original_buckets is per-request; whichever finally runs last wins, meaning one request may use the other's bucket filter mid-flight.
• Proposed Solution: Deep-copy the connector before mutating it, or use a per-request lock keyed on the connector's connection ID.

[edwinjosechittilappilly]

can we create this as an ticket so we can have it in backlog?
Good Catch!