Repository navigation
Run interceptor and compression tests once against sync and async clients - #387
Conversation
`haberdasher_client` and `haberdasher_client_sync` build a client against `http://localhost` over a given transport, replacing the `Client`/`SyncClient` + generated-client boilerplate repeated in most tests. They take a transport rather than an app so tests can still inspect or wrap it. Signed-off-by: Stefan VanBuren <stefan@vanburen.xyz>
`test_roundtrip_google_compat.py` keeps its own construction since it uses a different generated client. Signed-off-by: Stefan VanBuren <stefan@vanburen.xyz>
A `client` fixture parametrized over ASGI and WSGI replaces the duplicated `client_async`/`client_sync` fixtures, and `test_intercept` and `test_intercept_error` table the four RPC types instead of one test per type and mode. `call` in `_util.py` hides the await/iteration differences, running sync clients in a worker thread. Signed-off-by: Stefan VanBuren <stefan@vanburen.xyz>
A `new_client` fixture parametrized over ASGI and WSGI builds a client for a shared service with the given server compressions, replacing three `_sync`/`_async` pairs. Signed-off-by: Stefan VanBuren <stefan@vanburen.xyz>
`unary_client` builds an ASGI or WSGI server whose `MakeHat` calls a plain handler function, so tests whose service only implements `MakeHat` can define it once. Merges the `test_roundtrip`, GET, message-limit, and `test_headers` `_sync`/`_async` pairs; `call` now forwards call options such as `use_get`. Signed-off-by: Stefan VanBuren <stefan@vanburen.xyz>
|
|
||
| async def call( | ||
| client: HaberdasherClient | HaberdasherClientSync, | ||
| method: str, |
There was a problem hiding this comment.
This should probably be an enum.
Alternatively, arguably less type-safe but easier for the callers, I think we can accept the method itself, and isinstance(method.__self__, HaberdasherClientSync) method(req, **kwargs) etc (I am less concerned about the type safety of the method than kwargs)
There was a problem hiding this comment.
Done in d2cfce5: call takes the bound method and dispatches on method.__self__.
| raise ValueError(msg) | ||
|
|
||
|
|
||
| def haberdasher_client(transport: Transport, **kwargs) -> HaberdasherClient: |
There was a problem hiding this comment.
Throughout the file, the lack of type safety (more importantly IDE completion) on kwargs is a regression I think. IIRC Python unfortunately doesn't have a pattern for typing delegate method kwargs so if we can't, I think it's worth the small extra boilerplate at the callers to have the IDE support
There was a problem hiding this comment.
Agreed, dropped both helpers in 9a8dee8; tests construct clients directly again.
| client: HaberdasherClient | HaberdasherClientSync, | ||
| method: str, | ||
| request: Size | list[Size], | ||
| **kwargs, |
There was a problem hiding this comment.
This method seems more useful than above while still having the kwargs problem. FWIW I avoid this sort of helper in all my repos for a preference to have IDE-supported / well typed code over reducing boilerplate. I wouldn't block this though
There was a problem hiding this comment.
call no longer takes **kwargs (d2cfce5); its only user, the GET test, is back to separate sync/async tests.
|
|
||
| A sync client runs in a worker thread, as it would in a sync program. |
There was a problem hiding this comment.
| A sync client runs in a worker thread, as it would in a sync program. |
|
|
||
| def unary_client( | ||
| mode: Literal["async", "sync"], | ||
| make_hat: Callable[[Size, RequestContext], Hat], |
There was a problem hiding this comment.
There doesn't seem to be a good reason to accept make_hat over haberdashers, they are minimal boilerplate. mode would go away. But this method has a ergonomic issue of having two untyped kwargs type of parameters ;) I don't know if it's worth having this
There was a problem hiding this comment.
Agreed it wasn't worth it; removed in 983e285, which restores the tests it merged.
Its handler-function parameter and untyped `app_options`/`**kwargs` lose type checking and IDE completion for application and client options, which outweighs the duplication it removed. Reverts c19ad02. Signed-off-by: Stefan VanBuren <stefan@vanburen.xyz>
`call` dispatches on the method's client instead of taking the client and a method name, and no longer forwards call options as untyped `**kwargs`. Signed-off-by: Stefan VanBuren <stefan@vanburen.xyz>
`haberdasher_client` and `haberdasher_client_sync` forwarded client options as untyped `**kwargs`, losing type checking and IDE completion at every call site for a few lines of boilerplate. Tests construct clients directly again, and files the helpers only touched match `main`. The compression tests' `new_client` factory takes the client options it uses as typed keyword arguments. Signed-off-by: Stefan VanBuren <stefan@vanburen.xyz>
stefanvanburen
changed the title
anuraaga
left a comment
anuraaga
left a comment
There was a problem hiding this comment.
FWIW it would be fine to still combine some of the tests that were reverted, just with inline branching without helpers. Anyways this PR looks good now
Merges the sync and async variants of `test_headers`, `test_roundtrip`, `test_roundtrip_connect_get_empty_request`, `test_message_limit_unary_error`, and `test_message_limit_default` into one test each, parametrized over `mode`. Each test builds its server and client inline under `if mode == "async"`, so application and client options stay typed, and shares the call and assertions through `call`. Signed-off-by: Stefan VanBuren <stefan@vanburen.xyz>
done in 0a1057b - LMK if that wasn't what you were thinking, but seemed better to me. |
2 participants
Runs the interceptor and compression tests once against both servers instead of writing each test twice for sync and async. A
callhelper intest/_util.pyinvokes a bound sync or async client method uniformly, passing and returning streams as lists and running sync clients in a worker thread; per-file fixtures parametrized overasync/syncbuild the server and client with typed arguments. The set of cases is unchanged.Pairs that differ in their transports, service implementations, or streaming timing stay as separate tests, since merging them needed helpers that forward untyped options.