Skip to content

test: cover python lifecycle and foreground APIs#817

Closed
haosenwang1018 wants to merge 1 commit intogithub:mainfrom
haosenwang1018:test/python-lifecycle-and-foreground-apis
Closed

test: cover python lifecycle and foreground APIs#817
haosenwang1018 wants to merge 1 commit intogithub:mainfrom
haosenwang1018:test/python-lifecycle-and-foreground-apis

Conversation

@haosenwang1018
Copy link

Summary

  • add Python client tests for get_foreground_session_id() request wiring
  • add Python client tests for set_foreground_session_id() success and failure handling
  • add a lifecycle subscription test that verifies client.on() dispatches wildcard and typed handlers and unsubscribes cleanly

Why

Issue #815 tracks several public Python client APIs that were shipped but never exercised in tests. This PR covers the lifecycle subscription and foreground-session APIs so regressions in RPC method names, request payloads, or unsubscribe behavior are caught in CI.

Validation

  • python -m pytest -q python/test_client.py -k 'foreground or dispatches_wildcard'

@SteveSandersonMS
Copy link
Contributor

Thanks @haosenwang1018 for submitting all these PRs with test coverage.

As it stands these are not something we would merge because:

  1. They only cover Python, and we need to cover all the languages we support. Making the test suites diverge significantly will make it harder to keep the languages in sync.
  2. They are largely mock-oriented, which is not the test pattern we're trying to focus on. Using mocks means we're locking down internal implementation details instead of observing the true behavior. For tests that describe interactions with the underlying CLI server, we would prefer to use the E2E test infrastructure. This also helps to ensure we're only testing things that can actually happen in reality.

If you're interested in working with us on test coverage that's wonderful and I'd be happy to discuss more. I'd recommend not submitting large numbers of PRs without coordination though as we are really focused on keeping the project moving and we won't necessarily have capacity to review and merge things that are not on our roadmap.

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