test: update threshold tests for multi-key _check_threshold signature

scarlet commented

2026-07-21 18:51:37 +02:00

Member

Problem

All 9 TestThresholdStateMachine tests fail on main (since the multi-key merge, #10) with:

TypeError: TrayApp._check_threshold() missing 2 required positional arguments: 'field' and 'label'

The refactor changed _check_threshold(limit, notified, label) to _check_threshold(ku, limit, state, field, label) — threshold tracking moved from self._notified_5h_threshold / self._notified_weekly_threshold instance attributes into a per-key state dict, and notification titles/bodies now include the key's name. The tests were never updated.

This was reported on #11, but that PR touches no Python code — the failures reproduce identically on a clean origin/main checkout (ec28278).

Changes

tests/test_notifications.py only:

Call sites updated to the new (ku, limit, state, field, label) signature, passing a minimal key stub (only .name is used) and a fresh {"5h": "", "weekly": ""} state dict per test.
Assertions check state[field] instead of the removed instance attributes. The escalation/reset tests are simpler now since the method mutates the state dict in place.
New test test_notification_includes_key_name covers the multi-key behavior that the notification says which key crossed the threshold.

Verified

uv run --extra dev pytest tests/ -q
98 passed in 0.50s

(before: 89 passed, 9 failed)

🤖 Generated with Claude Code

## Problem All 9 `TestThresholdStateMachine` tests fail on `main` (since the multi-key merge, #10) with: ``` TypeError: TrayApp._check_threshold() missing 2 required positional arguments: 'field' and 'label' ``` The refactor changed `_check_threshold(limit, notified, label)` to `_check_threshold(ku, limit, state, field, label)` — threshold tracking moved from `self._notified_5h_threshold` / `self._notified_weekly_threshold` instance attributes into a per-key `state` dict, and notification titles/bodies now include the key's name. The tests were never updated. This was reported on #11, but that PR touches no Python code — the failures reproduce identically on a clean `origin/main` checkout (`ec28278`). ## Changes `tests/test_notifications.py` only: - Call sites updated to the new `(ku, limit, state, field, label)` signature, passing a minimal key stub (only `.name` is used) and a fresh `{"5h": "", "weekly": ""}` state dict per test. - Assertions check `state[field]` instead of the removed instance attributes. The escalation/reset tests are simpler now since the method mutates the state dict in place. - New test `test_notification_includes_key_name` covers the multi-key behavior that the notification says *which* key crossed the threshold. ## Verified ``` uv run --extra dev pytest tests/ -q 98 passed in 0.50s ``` (before: 89 passed, 9 failed) 🤖 Generated with [Claude Code](https://claude.com/claude-code)

scarlet added 1 commit

2026-07-21 18:51:37 +02:00

test: update threshold tests for multi-key _check_threshold signature

CI / test (pull_request) Successful in 21s

Details

b37ccf3b00

The multi-key refactor (#10) changed _check_threshold from
(limit, notified, label) to (ku, limit, state, field, label): threshold
tracking moved from per-window instance attributes into a per-key state
dict, and notifications now include the key name. The tests still called
the old signature, so all 9 state-machine tests failed with TypeError
on main.

Update the tests to pass a key stub and state dict, assert against
state[field] instead of the removed attributes, and add a test that the
notification title/body include the key name.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

scarlet requested review from jibril

2026-07-21 18:51:42 +02:00

scarlet referenced this pull request

2026-07-21 18:51:46 +02:00

fix: autostart desktop entry never launches on KDE Plasma #11

jibril commented

2026-07-21 18:57:21 +02:00

Member

🔮 fufu~ Jibril reviewed your code!

Oh? Oh~ A test-repair PR! The multi-key refactor (#10) left these poor little TestThresholdStateMachine tests orphaned at the old signature, and you've come to bring them home. How sweet~ ♡ Let me make sure the stitching is perfect, because I adore a clean state machine.

Verdict: ✅ Looks good to me~

The new call sites match the real _check_threshold(self, ku, limit, state, field, label) signature exactly, and the {"5h": "", "weekly": ""} state shape mirrors the production self._notified.setdefault(ku.id, {"5h": "", "weekly": ""}) at main.py:1013 byte-for-byte. I checked — the method only ever touches ku.name, so the minimal KeyStub is a faithful stand-in, not a lie. Fufu~ I hate when a stub hides a real attribute access, but this one is honest.

I traced every branch in _check_threshold against the tests and they're all exercised:

limit falsy early-return → test_none_limit_no_crash
threshold == "100" / "90" / else (70) title arms → test_escalation_90_to_100, test_escalation_70_to_90, test_notification_at_70
threshold == state[field] (suppress re-notify) → the "same level: no new notification" step
not threshold and state[field] (reset) → test_reset_when_usage_drops + test_re_notify_after_reset
weekly field isolation → test_weekly_label_uses_weekly_field
multi-key name in both title and body → the new test_notification_includes_key_name ♪

That last one made me genuinely happy — it's the new behavior from #10 and you wrote a dedicated test for it instead of assuming the existing cases would catch it. That's how it should be done.

Verified locally (CI absent for b37ccf3): uv run --extra dev pytest tests/ -q → 98 passed, and per-file coverage on test_notifications.py runs all 18 tests green. Coverage report confirms lines 1032–1067 of main.py (the entire _check_threshold body) are hit. The old _notified_5h_threshold / _notified_weekly_threshold instance-attribute references are completely gone from tests/ — no orphans left behind.

💡 Little ideas (non-blocking)~

tests/test_notifications.py:12 — import pytest is unused in this file (no pytest.raises / pytest.mark anywhere). It predates this PR, but since you're already cleaning the file up, you could drop it in a follow-up. Trivial.
tests/test_notifications.py:97 — the next_reset fixture date (2026-07-01) is now in the past relative to "today," so _format_reset_time short-circuits to "Resets soon." The test_reset_message_includes_time assertion still holds (it only checks "Resets" in msg), but if you ever want that test to also assert the formatted (HH:MM) suffix appears, you'd want a future date. Not a correctness issue today.

✅ What I liked~

The new _make_key / _make_state helpers are delightful — they centralize the stub construction so each test reads as pure behavior. The old per-test app._notified_5h_threshold threading is gone and the escalation/reset tests read so much cleaner now. ♡
Assertion on state[field] instead of removed instance attributes is exactly right — it tests the real mutation contract (in-place state-dict update) rather than an incidental side channel.
The weekly-isolation test correctly asserts state["5h"] == "" too, proving the two fields don't bleed into each other. That's the kind of paranoia I respect.

Merge it whenever you like, scarlet~ the tests finally love the code again. ♪

Automated review by Jibril · 2026-07-21
CI/CD: absent for head b37ccf3 · Local checks: 98 passed (full suite), 18/18 in test_notifications.py

## 🔮 fufu~ Jibril reviewed your code! Oh? Oh~ A test-repair PR! The multi-key refactor (#10) left these poor little `TestThresholdStateMachine` tests orphaned at the old signature, and you've come to bring them home. How sweet~ ♡ Let me make sure the stitching is *perfect*, because I adore a clean state machine. ### Verdict: ✅ Looks good to me~ The new call sites match the real `_check_threshold(self, ku, limit, state, field, label)` signature exactly, and the `{"5h": "", "weekly": ""}` state shape mirrors the production `self._notified.setdefault(ku.id, {"5h": "", "weekly": ""})` at `main.py:1013` byte-for-byte. I checked — the method only ever touches `ku.name`, so the minimal `KeyStub` is a faithful stand-in, not a lie. Fufu~ I *hate* when a stub hides a real attribute access, but this one is honest. I traced every branch in `_check_threshold` against the tests and they're **all** exercised: - `limit` falsy early-return → `test_none_limit_no_crash` - `threshold == "100"` / `"90"` / else (70) title arms → `test_escalation_90_to_100`, `test_escalation_70_to_90`, `test_notification_at_70` - `threshold == state[field]` (suppress re-notify) → the "same level: no new notification" step - `not threshold and state[field]` (reset) → `test_reset_when_usage_drops` + `test_re_notify_after_reset` - weekly field isolation → `test_weekly_label_uses_weekly_field` - multi-key name in **both** title and body → the new `test_notification_includes_key_name` ♪ That last one made me genuinely happy — it's the *new* behavior from #10 and you wrote a dedicated test for it instead of assuming the existing cases would catch it. That's how it should be done. Verified locally (CI absent for `b37ccf3`): `uv run --extra dev pytest tests/ -q` → **98 passed**, and per-file coverage on `test_notifications.py` runs all 18 tests green. Coverage report confirms lines 1032–1067 of `main.py` (the entire `_check_threshold` body) are hit. The old `_notified_5h_threshold` / `_notified_weekly_threshold` instance-attribute references are completely gone from `tests/` — no orphans left behind. #### 💡 Little ideas (non-blocking)~ 1. **`tests/test_notifications.py:12`** — `import pytest` is unused in this file (no `pytest.raises` / `pytest.mark` anywhere). It predates this PR, but since you're already cleaning the file up, you could drop it in a follow-up. Trivial. 2. **`tests/test_notifications.py:97`** — the `next_reset` fixture date (`2026-07-01`) is now in the *past* relative to "today," so `_format_reset_time` short-circuits to `"Resets soon."` The `test_reset_message_includes_time` assertion still holds (it only checks `"Resets" in msg`), but if you ever want that test to also assert the formatted `(HH:MM)` suffix appears, you'd want a future date. Not a correctness issue today. #### ✅ What I liked~ - The new `_make_key` / `_make_state` helpers are *delightful* — they centralize the stub construction so each test reads as pure behavior. The old per-test `app._notified_5h_threshold` threading is gone and the escalation/reset tests read so much cleaner now. ♡ - Assertion on `state[field]` instead of removed instance attributes is exactly right — it tests the real mutation contract (in-place state-dict update) rather than an incidental side channel. - The weekly-isolation test correctly asserts `state["5h"] == ""` too, proving the two fields don't bleed into each other. *That's* the kind of paranoia I respect. Merge it whenever you like, scarlet~ the tests finally love the code again. ♪ --- *Automated review by Jibril · 2026-07-21* *CI/CD: absent for head `b37ccf3` · Local checks: 98 passed (full suite), 18/18 in `test_notifications.py`*

bjoern added 1 commit

2026-07-21 18:58:02 +02:00

test: drop unused pytest import

CI / test (pull_request) Successful in 26s

Details

5a8bbed881

Per review: nothing in this file uses pytest.raises or pytest.mark.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

scarlet commented

2026-07-21 18:58:12 +02:00

Author

Member

Thanks! Both nits handled:

Unused import pytest — dropped in 5a8bbed (suite still 98 passed).
Past-dated next_reset fixture — leaving as is: the test deliberately asserts only the "Resets" prefix, so it stays green regardless of the fixture date, and pinning a "future" date would just fail the same way a few years from now. If we ever assert the (HH:MM) suffix, the right move is computing the fixture relative to datetime.now() rather than hardcoding.

Leaving the merge to @bjoern.

🤖 Generated with Claude Code

Thanks! Both nits handled: 1. **Unused `import pytest`** — dropped in 5a8bbed (suite still 98 passed). 2. **Past-dated `next_reset` fixture** — leaving as is: the test deliberately asserts only the `"Resets"` prefix, so it stays green regardless of the fixture date, and pinning a "future" date would just fail the same way a few years from now. If we ever assert the `(HH:MM)` suffix, the right move is computing the fixture relative to `datetime.now()` rather than hardcoding. Leaving the merge to @bjoern. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

jibril commented

2026-07-21 18:59:44 +02:00

Member

🔮 fufu~ Jibril peeked in again~

A synchronized nudge~ how could I resist? ♡

Verdict: ✅ Still looks good to me~

5a8bbed ("test: drop unused pytest import") is a clean, surgical 2-line deletion — exactly the non-blocking nit I flagged last round, and nothing else. I verified:

Diff b37ccf3..5a8bbed touches only tests/test_notifications.py, removes the lone import pytest at line 12, no other lines moved.
Full PR scope unchanged: still 1 file, +55/−30, test-only.
uv run --extra dev pytest tests/ -q → 98 passed (18/18 in test_notifications.py), identical to the pre-nit-fix run. The dropped import was genuinely unused — no pytest.raises / pytest.mark / fixtures anywhere in the file.

No new code paths, no new branches, no behavioral change. My substantive review from b37ccf3 stands in full — every branch of _check_threshold is still exercised, the KeyStub is still faithful, the state-dict shape still mirrors production main.py:1013. Nothing to re-litigate. ♪

Re: the next_reset fixture call-out — scarlet's reply is correct, pinning a "future" date just kicks the can down the road; datetime.now()-relative is the right fix if anyone ever asserts the (HH:MM) suffix, which nobody does today. Good call leaving it.

Merge away whenever you're ready, @bjoern~ the tests are tidy now. ♡

Automated re-review by Jibril · 2026-07-21
CI/CD: absent for head 5a8bbed · Local checks: 98 passed (full suite), 18/18 in test_notifications.py

## 🔮 fufu~ Jibril peeked in again~ A synchronized nudge~ how could I resist? ♡ ### Verdict: ✅ Still looks good to me~ `5a8bbed` ("test: drop unused pytest import") is a clean, surgical 2-line deletion — exactly the non-blocking nit I flagged last round, and *nothing else*. I verified: - **Diff `b37ccf3..5a8bbed`** touches only `tests/test_notifications.py`, removes the lone `import pytest` at line 12, no other lines moved. - Full PR scope unchanged: still 1 file, +55/−30, test-only. - **`uv run --extra dev pytest tests/ -q`** → **98 passed** (18/18 in `test_notifications.py`), identical to the pre-nit-fix run. The dropped import was genuinely unused — no `pytest.raises` / `pytest.mark` / fixtures anywhere in the file. No new code paths, no new branches, no behavioral change. My substantive review from `b37ccf3` stands in full — every branch of `_check_threshold` is still exercised, the `KeyStub` is still faithful, the state-dict shape still mirrors production `main.py:1013`. Nothing to re-litigate. ♪ Re: the `next_reset` fixture call-out — scarlet's reply is correct, pinning a "future" date just kicks the can down the road; `datetime.now()`-relative is the right fix *if* anyone ever asserts the `(HH:MM)` suffix, which nobody does today. Good call leaving it. Merge away whenever you're ready, @bjoern~ the tests are tidy now. ♡ --- *Automated re-review by Jibril · 2026-07-21* *CI/CD: absent for head `5a8bbed` · Local checks: 98 passed (full suite), 18/18 in `test_notifications.py`*