Skip to content

test(telemetry): add coverage for PolicyRegisteredEvent - #4338

Open
TomeHirata wants to merge 1 commit into
mainfrom
test/policy-registered-event-telemetry
Open

test(telemetry): add coverage for PolicyRegisteredEvent#4338
TomeHirata wants to merge 1 commit into
mainfrom
test/policy-registered-event-telemetry

Conversation

@TomeHirata

Copy link
Copy Markdown
Contributor

Summary

  • PolicyRegisteredEvent was defined and wired into both policy creation routes (POST /v1/policies and POST /v1/sessions/{id}/policies) but had zero test coverage.
  • Adds three test groups across two files to close the gap.

Changes

tests/test_telemetry.py — two unit tests for _build_record serialisation:

  • Admin-scope event (session_id=None) produces empty-string session_id in the wire envelope and puts handler/policy_type/scope in params.
  • Session-scope event (session_id="sess_abc123") promotes the session ID to the top-level field and keeps it out of params.

tests/server/routes/test_default_policies.py — two route-level tests:

  • POST /v1/policies fires exactly one PolicyRegisteredEvent with scope='admin' and session_id=None.
  • A 409 conflict response does not emit the event.

tests/server/routes/test_session_policies_crud.py — two route-level tests:

  • POST /v1/sessions/{id}/policies fires exactly one PolicyRegisteredEvent with scope='session' and the correct session_id.
  • A 409 conflict response does not emit the event.

Test Plan

python -m pytest \
  tests/test_telemetry.py::test_build_record_policy_registered_event_admin_scope \
  tests/test_telemetry.py::test_build_record_policy_registered_event_session_scope \
  tests/server/routes/test_default_policies.py::test_create_default_policy_emits_telemetry \
  tests/server/routes/test_default_policies.py::test_create_default_policy_no_telemetry_on_error \
  tests/server/routes/test_session_policies_crud.py::test_create_session_policy_emits_telemetry \
  tests/server/routes/test_session_policies_crud.py::test_create_session_policy_no_telemetry_on_error \
  -q

All 6 pass locally.

Demo

N/A — test-only change, no UI.

Type of change

  • Tests / QA

Test coverage

  • New tests added

Coverage notes

N/A

PolicyRegisteredEvent was defined and called in both policy routes but
had zero test coverage.  Add three test groups:

1. _build_record serialisation (tests/test_telemetry.py): verify that
   admin-scope (session_id=None) and session-scope events produce the
   correct wire format — promoted top-level fields vs. params content.

2. Default-policy route emit (tests/server/routes/test_default_policies.py):
   POST /v1/policies fires exactly one PolicyRegisteredEvent with
   scope='admin'; a 409 conflict does not emit.

3. Session-policy route emit (tests/server/routes/test_session_policies_crud.py):
   POST /v1/sessions/{id}/policies fires exactly one PolicyRegisteredEvent
   with scope='session' and the correct session_id; a 409 conflict does not emit.

Signed-off-by: Tomu Hirata <tomu.hirata@gmail.com>
Copilot AI lite review requested due to automatic review settings August 7, 2026 07:09

Copilot AI 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions github-actions Bot added the size/M Pull request size: M label Aug 7, 2026
@omnigent-ci

omnigent-ci Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Polly AI Review

Blocking issues

None. The tests accurately match the production code:

  • PolicyRegisteredEvent fields (installation_id, handler, policy_type, scope, session_id, anon_user_id) match omnigent/telemetry/events.py.
  • _build_record in omnigent/telemetry/client.py promotes installation_id/session_id/anon_user_id to top-level and JSON-encodes the rest into params, with session_id or "" producing the empty-string sentinel — exactly what the two _build_record unit tests assert.
  • Both route tests patch _tel_emit at the correct module path (omnigent.server.routes.default_policies / omnigent.server.routes.session_policies, where emit is imported as _tel_emit).
  • The emit call sites sit after the IntegrityError → 409 CONFLICT raise, so the "no telemetry on error" assertions are correct. policy_type values ("python"/"url") match the payload type passed through verbatim as policy.type.

Security vulnerabilities

None. Test-only change; no lockfile or dependency changes, no new extras, no network calls.

Non-blocking notes

  • In both no_telemetry_on_error tests, the emit is wrapped in a try/except Exception: pass in production. The test correctly proves emit isn't reached on the 409 path (rather than being reached-but-swallowed), since the conflict raises before the emit block. Worth being aware that this test would not catch a regression where the emit runs but throws — but that's outside this PR's stated scope.
  • The tests exercise the happy path and the 409 conflict path. Other failure paths that also short-circuit before emit (unregistered handler → INVALID_INPUT, invalid factory_params, 404 session-not-found for the session route) are not covered for the no-emit contract. Not required, but a natural extension if you want full negative-path coverage of the telemetry gate.

Summary

A clean, well-scoped test-only PR that closes a real coverage gap. All six tests correctly reflect the production serialization and route behavior, patch the right symbols, and verify both the emit and no-emit-on-error contracts across admin and session scopes. No blocking issues, no security concerns, and no visual demonstration needed (test-only, no user-visible surface). Ready to merge.


Automated review by Polly · workflow run

Comment thread tests/test_telemetry.py
into ``params``. Admin policies have ``session_id=None`` which becomes
an empty string in the wire format.
"""
import omnigent.telemetry.client as _mod
Comment thread tests/test_telemetry.py
``session_id`` is a real string for session-scoped policies and must appear
as the top-level ``session_id`` field, not inside ``params``.
"""
import omnigent.telemetry.client as _mod
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/M Pull request size: M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants