Skip to content

test(integration): swap bad-auth probe to portfolio.balance()#127

Merged
TexasCoding merged 1 commit into
mainfrom
fix/integration-bad-auth-endpoint
May 17, 2026
Merged

test(integration): swap bad-auth probe to portfolio.balance()#127
TexasCoding merged 1 commit into
mainfrom
fix/integration-bad-auth-endpoint

Conversation

@TexasCoding

Copy link
Copy Markdown
Owner

Summary

test_bad_auth_returns_auth_error was calling client.markets.list(limit=1) to exercise the auth-error path. Kalshi started serving public market data unauthenticated, so the call now returns 200 with bad credentials and the test reports "DID NOT RAISE KalshiAuthError".

Swapped to client.portfolio.balance() — account-scoped, always auth-required. Added a docstring note so future maintainers know why endpoint choice matters here.

Verification

  • tests/integration/test_errors.py::TestErrorPaths::test_bad_auth_returns_auth_error ✅ against live demo

Progress on #124

Closes 1 of 11. Running tally: A (#126 = 4) + B (this = 1) = 5 of 11. Remaining 6 cluster around not_found errors (test_order_groups × 3, test_orders × 3) and the demo subaccount-slot exhaustion (1). Each in a follow-up PR.

Refs #124

Test plan

  • Local integration run against demo passes
  • CI green

….balance

markets.list() no longer exercises the auth path — Kalshi began serving
public market data unauthenticated, so a client with bad credentials
gets a 200 back instead of 401/403 and the test "DID NOT RAISE".

Switched to portfolio.balance(), which is account-scoped and always
auth-required. Added a docstring note so future maintainers know why
the endpoint choice matters.

Closes 1 of 11 integration-test failures (#124).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented May 17, 2026

Copy link
Copy Markdown

Code Review — PR #127

Overview

This PR fixes test_bad_auth_returns_auth_error, which broke when Kalshi began serving GET /markets unauthenticated. The fix swaps the probe endpoint from client.markets.list(limit=1) to client.portfolio.balance() and adds a docstring explaining the choice. Small diff (6 additions, 1 deletion), good motivation.


✅ What's Done Well

  • Surgical change. Touches exactly what the bug requires — the one call that no longer exercises the auth path. Nothing else is modified.
  • Docstring explains the WHY. Per project conventions in CLAUDE.md, comments should document non-obvious constraints. The note that markets.list() was previously used and why it stopped working is precisely the kind of hidden constraint future maintainers need. Well placed.
  • Endpoint choice is sound. portfolio.balance() calls self._require_auth() at the resource layer, so it has an explicit SDK-level auth guard in addition to server-side enforcement. It's as stable an auth probe as you'll find in this API.
  • No new imports or dependencies. client.portfolio is already wired up; no scaffolding needed.
  • Test structure unchanged. The throwaway-client pattern (valid RSA key, wrong key_id) is preserved correctly, and the try/finally: client.close() guard is still in place.

Suggestions / Minor Notes

1. Consider asserting on the error message or endpoint in the docstring (non-blocking)

The docstring says why portfolio.balance() was chosen, but doesn't mention what to look for if this test starts failing silently again in the future (i.e., if Kalshi makes GET /portfolio/balance public). A single sentence like:

If this test starts returning 200 instead of raising, check whether Kalshi has made /portfolio/balance public and pick another account-scoped endpoint.

would be a useful forward-looking hint, since you've now burned one endpoint already.

2. sync_client fixture not used (intentional, but worth confirming)

The other tests in this class accept sync_client: KalshiClient as a fixture arg. This test correctly builds its own client with invalid credentials. Just confirming this is intentional and not a missed parameter — it is, since you need the bad-auth client, not the configured one.

3. Async coverage (pre-existing gap, not introduced here)

The module docstring notes that error mapping is tested sync-only because _map_error() is shared. That's fine and consistent with the existing test philosophy. No action needed for this PR.


Verdict

Approve. The fix is correct, minimal, and well-documented. The PR description clearly articulates the root cause (Kalshi changed server behavior) and the verification steps. No issues blocking merge.

@TexasCoding TexasCoding merged commit 97725cd into main May 17, 2026
4 checks passed
@TexasCoding TexasCoding deleted the fix/integration-bad-auth-endpoint branch May 17, 2026 14:11
TexasCoding added a commit that referenced this pull request May 17, 2026
…128)

POST /portfolio/subaccounts returns the new subaccount_number immediately,
but GET /portfolio/subaccounts/balances lags by ~1.0-1.5s before the new
account appears in the list. The ephemeral_subaccount fixture asserted
the number was visible immediately after create, so it failed when the
list hadn't propagated yet.

Replaced the immediate return with a 10s poll loop on list_balances
(0.5s interval). Soft-skips with a clear message if the new number never
appears, so future demo-side issues don't masquerade as test bugs.

Closes 1 of 11 integration-test failures (#124).
Running tally: A (#126=4) + B (#127=1) + E (this=1) = 6 of 11.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
TexasCoding added a commit that referenced this pull request May 17, 2026
…ers, order_groups) (#129)

* test(integration): poll for query-exchange visibility after POST

Demo's query-exchange replica lags writes by ~10 seconds. Orders and
order_groups created via POST aren't immediately readable via their
GET-by-id endpoints — the failing tests asserted GET success
immediately after CREATE and reliably hit 404 ("not_found" /
service: query-exchange).

Probe evidence (orders):
  create -> order_id X
  poll 1-3 (~1.5s): 404 not_found
  ...
  poll @ 10s: GET works, status=resting

Same pattern for order_groups (~9.5s).

- Added wait_for_resource (sync) and await_resource (async) helpers in
  tests/integration/helpers.py — bounded poll loop that swallows
  KalshiNotFoundError until timeout (default 15s, interval 0.5s) and
  re-raises if the resource never propagates.
- Wrapped GET-after-POST sites in test_orders.py
  (test_order_fill_lifecycle, test_create_get_cancel sync + async) and
  test_order_groups.py (test_create_and_get, test_update_limit,
  TestOrderGroupsAsync.test_create_get_delete).
- Dropped the now-redundant `await asyncio.sleep(0.5)` workaround +
  unused asyncio import from test_order_groups.py.

Verified locally: tests/integration/test_orders.py +
tests/integration/test_order_groups.py — 22/22 passed.

Closes 5 of 11 integration-test failures (#124).
Running tally: A (#126=4) + B (#127=1) + E (#128=1) + C (this=5) = 11/11.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* review(#129): drop dead last_error var, use PEP 695 type params

Per bot review on PR #129:

- Removed unused last_error assignment in wait_for_resource (ruff F841;
  caused CI lint failure). bare 'raise' re-raises the active exc
  directly, so the captured ref was dead.
- Switched both helpers to PEP 695 type parameter syntax
  (def wait_for_resource[T](...)) to clear UP047. Drops the standalone
  TypeVar import.
- Expanded await_resource one-line docstring with the demo-lag context
  so the file reads top-to-bottom.

Verified: uv run ruff check . clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.

1 participant