Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ Remaining before 1.0:
- PyPI release automation (CI itself is now in place — see GITHUB section).
- Document types `02`/`03`/`04`/`11`-`14` (only `01` Invoice is modelled).
- Live sandbox verification against LHDN preprod.
- `tests/unit/test_auth.py::test_token_manager_refresh_margin` is **vacuous**: it never calls `get_token()`, so `is_valid()` returns False only because no token was ever acquired. Proven by re-running it with `expires_in=3600` (far outside the 60s margin) — still passes, 0 calls to the token endpoint. It would pass with the refresh-margin logic deleted. The async twin was strengthened in the Phase 6b coverage PR; fix the sync one the same way (two responses, two `get_token()` calls, assert re-acquisition).
- Lesson from the two refresh-margin tests: **a passing test is not a testing test.** `test_token_manager_refresh_margin` was vacuous for the whole life of the sync suite — it never called `get_token()`, so `is_valid` was False merely because no token existed, and it would have passed with the refresh-ahead logic deleted. `auth.py` sat at 95% line coverage throughout, because coverage counts executed lines, not meaningful assertions. Both twins now drive the real path: acquire a token that expires inside the **default 60s margin**, call `get_token()` again, and assert it returns the fresh token with the route hit twice. Their sensitivity was confirmed by a throwaway **mutation run** — a scratch copy with `refresh_margin=0`, where the second call returns the cached token and the assertions fail. That mutation check is *not* part of the suite; re-do it by hand if you touch the refresh-ahead logic. When a test pins behaviour that matters, check it can fail.
- `TokenManager` creates its own `httpx.Client` when none is injected but exposes no `close()`, while `AsyncTokenManager` tracks `_owns_client` and has `aclose()`. **Not a live leak** — both clients pass their own `httpx` client in, so the manager never builds one on the normal path; it only bites a bare `TokenManager()`, which is not exported. Low priority.

## CODE_STATE
Expand Down
40 changes: 35 additions & 5 deletions tests/unit/test_auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
from __future__ import annotations

from typing import Any
from urllib.parse import parse_qs

import httpx
import pytest
Expand Down Expand Up @@ -65,7 +66,7 @@ def test_oauth2_token_rejects_missing_access() -> None:


def test_token_manager_acquires_token(respx_mock: Any, token_url: str, mgr: TokenManager) -> None:
respx_mock.post(token_url).mock(
route = respx_mock.post(token_url).mock(
return_value=httpx.Response(200, json=_token_response("TOK-1", expires_in=3600))
)

Expand All @@ -74,6 +75,18 @@ def test_token_manager_acquires_token(respx_mock: Any, token_url: str, mgr: Toke
assert tok.access_token == "TOK-1"
assert mgr.is_valid is True

# Pin the outgoing form too. Nothing else asserts the grant payload, so a
# typo in `_build_form` (e.g. "client_credential") would otherwise pass
# every test and only fail against the real LHDN token endpoint.
# Mirrors `test_async_auth.py::test_async_token_manager_acquires_token`.
sent = parse_qs(route.calls.last.request.content.decode())
assert sent == {
"client_id": ["cid"],
"client_secret": ["csecret"],
"grant_type": ["client_credentials"],
"scope": ["InvoicingAPI"],
}


def test_token_manager_caches_until_expiry(
respx_mock: Any, token_url: str, mgr: TokenManager
Expand Down Expand Up @@ -109,14 +122,31 @@ def test_token_manager_reacquires_after_expiry(


def test_token_manager_refresh_margin(respx_mock: Any, token_url: str, mgr: TokenManager) -> None:
# expires_in 30 → within default 60s refresh margin → manager treats fresh
# token as already-stale so the next get_token() must re-hit the wire.
respx_mock.post(token_url).mock(
return_value=httpx.Response(200, json=_token_response("TOK", expires_in=30))
"""A token expiring inside the refresh margin must be re-acquired.

Asserting only ``is_valid`` would be weak: ``get_token`` does not consult
that property, it re-checks ``is_expired`` itself. So this drives the real
path -- a second ``get_token`` must go back to the wire and return the new
token, not hand back the still-unexpired-but-stale first one.

Mirrors ``test_async_auth.py::test_async_token_manager_refresh_margin``.
"""
route = respx_mock.post(token_url).mock(
side_effect=[
# expires_in 30 is inside the default 60s refresh margin, so this
# token counts as stale the moment it arrives.
httpx.Response(200, json=_token_response("STALE", expires_in=30)),
httpx.Response(200, json=_token_response("FRESH", expires_in=3600)),
]
)

assert mgr.get_token().access_token == "STALE"
assert mgr.is_valid is False

assert mgr.get_token().access_token == "FRESH"
assert route.call_count == 2
assert mgr.is_valid is True

Comment thread
coderabbitai[bot] marked this conversation as resolved.

def test_token_manager_raises_on_auth_failure(
respx_mock: Any, token_url: str, mgr: TokenManager
Expand Down
Loading