cookie solution - #180
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #180 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 10 10
Lines 529 549 +20
=========================================
+ Hits 529 549 +20 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The autouse
isolate_browser_cookiesfixture globally monkeypatches the cookie helpers for all tests, which may hide issues in tests that intentionally need real cookie behavior—consider scoping this fixture to specific test modules or using a marker instead.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The autouse `isolate_browser_cookies` fixture globally monkeypatches the cookie helpers for all tests, which may hide issues in tests that intentionally need real cookie behavior—consider scoping this fixture to specific test modules or using a marker instead.
## Individual Comments
### Comment 1
<location path="tests/unit/test_session_state.py" line_range="6-8" />
<code_context>
+from open_cups import session_state
+
+
+def test_load_returns_existing_cookie(monkeypatch: pytest.MonkeyPatch) -> None:
+ monkeypatch.setattr(session_state, "_read_session_cookie", lambda: "existing-id")
+ assert session_state._load_or_create_session_id() == "existing-id" # noqa: SLF001
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test that exercises `SessionState` itself, not just the helper function.
These tests only cover `_load_or_create_session_id` and not `SessionState.__init__` or its use of `st.session_state`. Please add a test that constructs `SessionState` with mocked `st.session_state` and `st.context.cookies`, and verifies `session_id` is correctly set when a cookie exists and when it does not. This will validate the documented behavior and protect the public API from regressions during helper refactors.
Suggested implementation:
```python
import types
import pytest
from open_cups import session_state
```
```python
def test_load_returns_existing_cookie(monkeypatch: pytest.MonkeyPatch) -> None:
monkeypatch.setattr(session_state, "_read_session_cookie", lambda: "existing-id")
assert session_state._load_or_create_session_id() == "existing-id" # noqa: SLF001
def test_session_state_uses_existing_cookie_for_session_id(
monkeypatch: pytest.MonkeyPatch,
) -> None:
fake_st = types.SimpleNamespace(
session_state={},
context=types.SimpleNamespace(cookies={"session_id": "existing-id"}),
)
monkeypatch.setattr(session_state, "st", fake_st)
state = session_state.SessionState()
assert state.session_id == "existing-id"
assert fake_st.session_state.get("session_id") == "existing-id"
def test_session_state_mints_session_id_when_cookie_absent(
monkeypatch: pytest.MonkeyPatch,
) -> None:
fake_st = types.SimpleNamespace(
session_state={},
context=types.SimpleNamespace(cookies={}),
)
monkeypatch.setattr(session_state, "st", fake_st)
state = session_state.SessionState()
# When no cookie exists, SessionState should mint a new non-empty ID
assert isinstance(state.session_id, str)
assert state.session_id
assert fake_st.session_state.get("session_id") == state.session_id
def test_load_mints_and_persists_when_cookie_absent(
monkeypatch: pytest.MonkeyPatch,
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| def test_load_returns_existing_cookie(monkeypatch: pytest.MonkeyPatch) -> None: | ||
| monkeypatch.setattr(session_state, "_read_session_cookie", lambda: "existing-id") | ||
| assert session_state._load_or_create_session_id() == "existing-id" # noqa: SLF001 |
There was a problem hiding this comment.
suggestion (testing): Add a test that exercises SessionState itself, not just the helper function.
These tests only cover _load_or_create_session_id and not SessionState.__init__ or its use of st.session_state. Please add a test that constructs SessionState with mocked st.session_state and st.context.cookies, and verifies session_id is correctly set when a cookie exists and when it does not. This will validate the documented behavior and protect the public API from regressions during helper refactors.
Suggested implementation:
import types
import pytest
from open_cups import session_statedef test_load_returns_existing_cookie(monkeypatch: pytest.MonkeyPatch) -> None:
monkeypatch.setattr(session_state, "_read_session_cookie", lambda: "existing-id")
assert session_state._load_or_create_session_id() == "existing-id" # noqa: SLF001
def test_session_state_uses_existing_cookie_for_session_id(
monkeypatch: pytest.MonkeyPatch,
) -> None:
fake_st = types.SimpleNamespace(
session_state={},
context=types.SimpleNamespace(cookies={"session_id": "existing-id"}),
)
monkeypatch.setattr(session_state, "st", fake_st)
state = session_state.SessionState()
assert state.session_id == "existing-id"
assert fake_st.session_state.get("session_id") == "existing-id"
def test_session_state_mints_session_id_when_cookie_absent(
monkeypatch: pytest.MonkeyPatch,
) -> None:
fake_st = types.SimpleNamespace(
session_state={},
context=types.SimpleNamespace(cookies={}),
)
monkeypatch.setattr(session_state, "st", fake_st)
state = session_state.SessionState()
# When no cookie exists, SessionState should mint a new non-empty ID
assert isinstance(state.session_id, str)
assert state.session_id
assert fake_st.session_state.get("session_id") == state.session_id
def test_load_mints_and_persists_when_cookie_absent(
monkeypatch: pytest.MonkeyPatch,5113672 to
82299a0
Compare
No description provided.