yummyPancake2607 commented on PR #4065:
URL: https://github.com/apache/iggy/pull/4065#issuecomment-5551314648
@hubcio
## Bare pytest.raises(RuntimeError)
Fixed — both occurrences in
`test_user_without_read_servers_cannot_list_clients`
now use `pytest.raises(RuntimeError, match="Unauthorized")`. Confirmed
`IggyError::Unauthorized`'s `Display` is exactly `"Unauthorized"`
(`core/common/src/error/iggy_error.rs:96`) and the binding forwards
`e.to_string()` verbatim, so it survives the wire round trip unchanged.
## Cleanup only on the success path
Fixed at all three spots:
- `test_get_me_reports_joined_consumer_group`: join, assertions and leave
now run inside `try`, with `delete_consumer_group` in `finally`.
- `test_user_with_read_servers_can_list_clients` (line 215) and
`test_user_without_read_servers_cannot_list_clients` (line 235): user
creation is followed by `try`/`finally: await
iggy_client.delete_user(...)`.
## get_clients best-effort caveat missing from the docstring
Added to both `get_client` and `get_clients` in `client.rs` (confirmed both
go through the same `shard.list_all_clients()` scatter-gather in
`core/server/src/dispatch/reads.rs`, bounded by
`LIST_CLIENTS_GATHER_TIMEOUT` — 3s — in `core/shard/src/lib.rs:312`):
`get_clients` states a shard missing the gather timeout is dropped from the
result; `get_client` states a connected client on such a shard can
transiently resolve to `None`. Regenerated the stub, both notes are in
`apache_iggy.pyi` now.
## Bare next() risking StopIteration
Fixed both (the topic pick and the group pick in
`test_get_me_reports_joined_consumer_group`): each now takes `None` as the
`next()` default and asserts not-None before unpacking, same shape as the
`mine = next(..., None); assert mine is not None` pattern already used for
`get_clients()` lookups.
## Unnamed literals (1_000_000, 3)
Named as module-level constants: `UNUSED_CLIENT_ID_MARGIN = 1_000_000` and
`DISTINCT_ID_ATTEMPTS = 3`, replacing all three call sites (`+ 1_000_000`
and both `range(3)` loops).
## Hand-rolled ConsumerGroupInfo copy
Derived `Clone, Copy` on `iggy_common::ConsumerGroupInfo`
(`core/common/src/types/client/client_info.rs`) — it's three plain `u32`
fields, nothing blocked it. `client_info.rs`'s `From<&RustConsumerGroupInfo>`
now collapses to `Self { inner: *group }`.
## consumer_groups docstring naming the wrong surprise
Rewritten to say what a caller actually hits: `ConsumerGroupInfo` has no
`__eq__`, so two instances from separate reads compare unequal even with
identical fields — bind the list once and compare fields, not objects.
Dropped the list-mutation sentence, which wasn't the real gotcha.
## user_id None branch untested
Not deliberate — added
`test_get_clients_reports_none_user_id_before_login`: connects a second
client without `login_user`, confirms it shows up in `get_clients()`
(diffed against the pre-connect snapshot to avoid depending on client-id
ordering) with `user_id is None`.
## transport equality can't catch a wire-code shift
Fixed at both call sites (`test_get_clients_contains_this_client` and
`test_get_client_by_id_matches_get_me`): both now assert `== "TCP"` instead
of comparing against `me.transport`, which comes from the same
`transport_kind_to_wire` mapping and so couldn't have caught a shift.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
