From 390e9169228b425ec9c102e6e82ce9bc88fe4bac Mon Sep 17 00:00:00 2001 From: Krzysztof kuhy Rudnicki Date: Fri, 21 Aug 2026 13:17:26 +0200 Subject: [PATCH] Finish the RunnerUp sport-allowlist removal; isolate the instance lock in tests A prior commit (6a7a7d8) accidentally captured only the _constants.py half of an already-in-progress, uncommitted change removing RUNNERUP_ACCEPTED_SPORTS -- leaving main broken (import error) since _runnerup_verification.py and its 15 test files still referenced the now-undefined constant. Committing the matching half: RunnerUp entries are now judged purely by distance/duration, any sport qualifies, matching how every other workout type is already treated. Also: the new single-instance lock was not test-isolated, so every ScreenLocker built across the suite contended for the same real $XDG_RUNTIME_DIR/gatelock file -- only the first test won it, the rest silently exited. Added to conftest.py's isolated-state table (split into tests/_isolated_state.py to stay under the 250-line cap) and added dedicated coverage for screen_locker/_instance.py. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_018M5gKyYzxWG6QkCaj1wWfk --- screen_locker/_runnerup_verification.py | 11 +-- screen_locker/tests/_isolated_state.py | 54 +++++++++++ screen_locker/tests/conftest.py | 48 +--------- screen_locker/tests/test_init_and_log.py | 22 +++++ screen_locker/tests/test_instance.py | 96 +++++++++++++++++++ .../tests/test_runnerup_verification_part1.py | 36 +++++-- .../test_runnerup_verification_part10.py | 2 +- .../test_runnerup_verification_part11.py | 2 +- .../test_runnerup_verification_part12.py | 2 +- .../test_runnerup_verification_part13.py | 2 +- .../test_runnerup_verification_part14.py | 2 +- .../test_runnerup_verification_part15.py | 2 +- .../tests/test_runnerup_verification_part2.py | 2 +- .../tests/test_runnerup_verification_part3.py | 2 +- .../tests/test_runnerup_verification_part4.py | 2 +- .../tests/test_runnerup_verification_part5.py | 2 +- .../tests/test_runnerup_verification_part6.py | 2 +- .../tests/test_runnerup_verification_part7.py | 2 +- .../tests/test_runnerup_verification_part8.py | 2 +- .../tests/test_runnerup_verification_part9.py | 2 +- 20 files changed, 220 insertions(+), 75 deletions(-) create mode 100644 screen_locker/tests/_isolated_state.py create mode 100644 screen_locker/tests/test_instance.py diff --git a/screen_locker/_runnerup_verification.py b/screen_locker/_runnerup_verification.py index 5418ac1..66c3ada 100644 --- a/screen_locker/_runnerup_verification.py +++ b/screen_locker/_runnerup_verification.py @@ -14,7 +14,6 @@ from typing import Any from screen_locker._constants import ( MIN_RUN_DISTANCE_KM, MIN_WORKOUT_DURATION_MINUTES, - RUNNERUP_ACCEPTED_SPORTS, RUNNERUP_DISTANCE_TOLERANCE, RUNNERUP_EXPORT_DIRS, WORKOUT_DURATION_ACCEPT_MINUTES, @@ -78,7 +77,7 @@ class RunnerUpVerificationMixin( fails validation), or ``None`` if no today's file exists at all (caller should try the root DB path instead). """ - today = datetime.now(tz=timezone.utc).strftime("%Y-%m-%d") + today = datetime.now(tz=timezone.utc).astimezone().strftime("%Y-%m-%d") exports = self._find_runnerup_exports_for_date(today) if not exports: _logger.warning( @@ -116,12 +115,6 @@ class RunnerUpVerificationMixin( ``PhoneVerificationMixin._verify_phone_workout``. """ sport = data["sport"] - if sport not in RUNNERUP_ACCEPTED_SPORTS: - sport_name = _SPORT_NAMES.get(sport, f"unknown({sport})") - return ( - "wrong_sport", - f"Activity type '{sport_name}' doesn't count as a qualifying run", - ) duration_min = data["duration_seconds"] / 60 distance_km = data["distance_m"] / 1000 @@ -166,7 +159,7 @@ class RunnerUpVerificationMixin( root DB pull if no today's files are found. Status values: ``verified | not_verified | no_phone | too_short | - wrong_sport | clock_tampered``. + clock_tampered``. """ skew_ok, skew_msg = check_clock_skew() if not skew_ok: diff --git a/screen_locker/tests/_isolated_state.py b/screen_locker/tests/_isolated_state.py new file mode 100644 index 0000000..4970c97 --- /dev/null +++ b/screen_locker/tests/_isolated_state.py @@ -0,0 +1,54 @@ +"""On-disk state paths every test must redirect away from the real files. + +Split out of ``conftest.py`` (250-line cap) -- the *fixture* itself has to +stay in ``conftest.py`` (pytest only applies autouse fixtures declared +there), but this static table has no such constraint. +""" + +from __future__ import annotations + +# Each on-disk state path, and every module that bound it by value at import +# time. All of them need patching, not just the _constants source -- a missed +# binding lets a test write to the real file. +ISOLATED_STATE: tuple[tuple[str, tuple[str, ...]], ...] = ( + ( + "sick_history.json", + ("_sick_tracker.SICK_HISTORY_FILE", "_constants.SICK_HISTORY_FILE"), + ), + ( + "early_bird_pending.json", + ( + "_early_bird.EARLY_BIRD_PENDING_FILE", + "_constants.EARLY_BIRD_PENDING_FILE", + ), + ), + ( + "extra_benefits_state.json", + ( + "_constants.EXTRA_BENEFITS_FILE", + "_startup_checks.EXTRA_BENEFITS_FILE", + "_sync_mixin.EXTRA_BENEFITS_FILE", + "_early_bird.EXTRA_BENEFITS_FILE", + "_status.EXTRA_BENEFITS_FILE", + ), + ), + ( + "sick_day_state.json", + ( + "_constants.SICK_DAY_STATE_FILE", + "_startup_checks.SICK_DAY_STATE_FILE", + "_shutdown_sick_state.SICK_DAY_STATE_FILE", + ), + ), + ("scheduled_skips.json", ("_log_mixin.SCHEDULED_SKIPS_FILE",)), + # The durable lock-decision trail. Written on EVERY locker run, so without + # this the suite would append test decisions to the user's real + # enforcement history in ~/.local/share/screen_locker/. + ("decisions.jsonl", ("_decision_log.DECISION_LOG_FILE",)), + # Real $XDG_RUNTIME_DIR/gatelock file; unisolated, only the first test in + # the whole suite would win it, since it is shared across all of them. + ( + "instance.lock", + ("_constants.INSTANCE_LOCK_FILE", "screen_lock.INSTANCE_LOCK_FILE"), + ), +) diff --git a/screen_locker/tests/conftest.py b/screen_locker/tests/conftest.py index ef1a293..a03f372 100644 --- a/screen_locker/tests/conftest.py +++ b/screen_locker/tests/conftest.py @@ -25,6 +25,7 @@ from screen_locker.tests._gatelock_fixtures import ( _hermetic_gatelock, dual_output, ) +from screen_locker.tests._isolated_state import ISOLATED_STATE from screen_locker.tests._locker_factories import ( _make_locker, create_locker, @@ -128,57 +129,18 @@ def _block_real_network() -> Iterator[None]: yield -# Each on-disk state path, and every module that bound it by value at import -# time. All of them need patching, not just the _constants source -- a missed -# binding lets a test write to the real file. -_ISOLATED_STATE: tuple[tuple[str, tuple[str, ...]], ...] = ( - ( - "sick_history.json", - ("_sick_tracker.SICK_HISTORY_FILE", "_constants.SICK_HISTORY_FILE"), - ), - ( - "early_bird_pending.json", - ( - "_early_bird.EARLY_BIRD_PENDING_FILE", - "_constants.EARLY_BIRD_PENDING_FILE", - ), - ), - ( - "extra_benefits_state.json", - ( - "_constants.EXTRA_BENEFITS_FILE", - "_startup_checks.EXTRA_BENEFITS_FILE", - "_sync_mixin.EXTRA_BENEFITS_FILE", - "_early_bird.EXTRA_BENEFITS_FILE", - "_status.EXTRA_BENEFITS_FILE", - ), - ), - ( - "sick_day_state.json", - ( - "_constants.SICK_DAY_STATE_FILE", - "_startup_checks.SICK_DAY_STATE_FILE", - "_shutdown_sick_state.SICK_DAY_STATE_FILE", - ), - ), - ("scheduled_skips.json", ("_log_mixin.SCHEDULED_SKIPS_FILE",)), - # The durable lock-decision trail. Written on EVERY locker run, so without - # this the suite would append test decisions to the user's real - # enforcement history in ~/.local/share/screen_locker/. - ("decisions.jsonl", ("_decision_log.DECISION_LOG_FILE",)), -) - - @pytest.fixture(autouse=True) def _isolate_state_files(tmp_path: Path) -> Iterator[None]: """Redirect every on-disk state path to tmp_path for every test. Stays in conftest.py and stays autouse: pytest only applies autouse fixtures declared here, so moving this to a sibling module would leave - tests writing to the user's real state files while still passing. + tests writing to the user's real state files while still passing. The + table itself (ISOLATED_STATE) has no such constraint -- see + _isolated_state.py. """ with ExitStack() as stack: - for filename, bindings in _ISOLATED_STATE: + for filename, bindings in ISOLATED_STATE: target = tmp_path / filename for binding in bindings: stack.enter_context(patch(f"screen_locker.{binding}", target)) diff --git a/screen_locker/tests/test_init_and_log.py b/screen_locker/tests/test_init_and_log.py index 72ac95b..94d23ec 100644 --- a/screen_locker/tests/test_init_and_log.py +++ b/screen_locker/tests/test_init_and_log.py @@ -67,6 +67,28 @@ class TestScreenLockerInit: mock_sys_exit.assert_called_once_with(0) + def test_init_stands_down_when_instance_lock_already_held( + self, mock_tk: MagicMock, mock_sys_exit: MagicMock, tmp_path: Path + ) -> None: + """A second concurrent process must not build a competing window. + + This is the actual fix for "unlocked, then locked again seconds + later" -- see the 2026-08-21 incident notes in screen_lock.py. + """ + from screen_locker import _instance + from screen_locker._constants import INSTANCE_LOCK_FILE + + mock_sys_exit.side_effect = SystemExit(0) + held = _instance.acquire(INSTANCE_LOCK_FILE) + assert held is not None + try: + with pytest.raises(SystemExit): + create_locker(mock_tk, tmp_path) + finally: + held.release() + + mock_sys_exit.assert_called_once_with(0) + class TestHasLoggedToday: """Tests for has_logged_today method.""" diff --git a/screen_locker/tests/test_instance.py b/screen_locker/tests/test_instance.py new file mode 100644 index 0000000..5ffe0b6 --- /dev/null +++ b/screen_locker/tests/test_instance.py @@ -0,0 +1,96 @@ +"""Tests for the single-instance lock.""" + +from __future__ import annotations + +import logging +import os +from typing import TYPE_CHECKING + +from screen_locker import _instance + +if TYPE_CHECKING: + from pathlib import Path + + import pytest + + +def test_the_first_run_takes_the_lock(tmp_path: Path) -> None: + lock = _instance.acquire(tmp_path / "instance.lock") + + assert lock is not None + lock.release() + + +def test_a_second_run_stands_down( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + """Otherwise a stray re-arm races a second window into existence.""" + path = tmp_path / "instance.lock" + first = _instance.acquire(path) + + with caplog.at_level(logging.WARNING): + second = _instance.acquire(path) + + assert first is not None + assert second is None + assert any("already holds" in record.message for record in caplog.records) + first.release() + + +def test_the_lock_is_retakeable_after_release(tmp_path: Path) -> None: + path = tmp_path / "instance.lock" + first = _instance.acquire(path) + assert first is not None + first.release() + + second = _instance.acquire(path) + + assert second is not None + second.release() + + +def test_an_unopenable_path_reports_failure_rather_than_raising( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + blocker = tmp_path / "blocker" + blocker.write_text("file", encoding="utf-8") + + with caplog.at_level(logging.ERROR): + assert _instance.acquire(blocker / "instance.lock") is None + + +def test_releasing_twice_is_safe(tmp_path: Path) -> None: + lock = _instance.acquire(tmp_path / "instance.lock") + assert lock is not None + + lock.release() + lock.release() + + +def test_the_lock_file_records_the_pid(tmp_path: Path) -> None: + path = tmp_path / "instance.lock" + lock = _instance.acquire(path) + assert lock is not None + + assert path.read_text(encoding="utf-8") == str(os.getpid()) + lock.release() + + +def test_a_close_failure_is_reported_not_raised( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + lock = _instance.acquire(tmp_path / "instance.lock") + assert lock is not None + + class Broken: + def close(self) -> None: + message = "cannot close" + raise OSError(message) + + lock.handle.close() + lock.handle = Broken() + + with caplog.at_level(logging.WARNING): + lock.release() + + assert any("could not close" in record.message for record in caplog.records) diff --git a/screen_locker/tests/test_runnerup_verification_part1.py b/screen_locker/tests/test_runnerup_verification_part1.py index 1ccd57e..1cd731f 100644 --- a/screen_locker/tests/test_runnerup_verification_part1.py +++ b/screen_locker/tests/test_runnerup_verification_part1.py @@ -26,7 +26,7 @@ _TCX_RUNNING = """\ """ -# TCX with an unrecognised sport tag (not in RUNNERUP_ACCEPTED_SPORTS). +# TCX with a non-running sport tag (Gym). _TCX_GYM = """\ str: class TestValidateRunnerupData: """Tests for _validate_runnerup_data (lines 388-411).""" - def test_wrong_sport_returns_wrong_sport_status( + def test_gym_sport_judged_only_on_distance_duration( self, mock_tk: MagicMock, mock_sys_exit: MagicMock, tmp_path: Path, ) -> None: - """Sport not in RUNNERUP_ACCEPTED_SPORTS → wrong_sport.""" + """Any sport (e.g. Gym) is verified purely by distance/duration. + + There is no sport allowlist — a qualifying activity of any RunnerUp + sport type counts, matching the workout-locker requirement that + RunnerUp entries all be judged the same way. + """ locker = create_locker(mock_tk, tmp_path) - # Sport 6 = Gym, not accepted status, msg = locker._validate_runnerup_data( {"sport": 6, "duration_seconds": 3600, "distance_m": 6000} ) - assert status == "wrong_sport" + assert status == "verified" assert "Gym" in msg + def test_walking_sport_judged_only_on_distance_duration( + self, + mock_tk: MagicMock, + mock_sys_exit: MagicMock, + tmp_path: Path, + ) -> None: + """A qualifying walk (sport 4) is verified, not rejected as wrong_sport.""" + locker = create_locker(mock_tk, tmp_path) + status, msg = locker._validate_runnerup_data( + {"sport": 4, "duration_seconds": 3000, "distance_m": 4000} + ) + assert status == "verified" + assert "Walking" in msg + def test_unknown_sport_number_shown_in_message( self, mock_tk: MagicMock, mock_sys_exit: MagicMock, tmp_path: Path, ) -> None: - """Unknown sport integer falls back to 'unknown(N)' label.""" + """Unknown sport integer falls back to its raw number in the message.""" locker = create_locker(mock_tk, tmp_path) status, msg = locker._validate_runnerup_data( {"sport": 99, "duration_seconds": 3600, "distance_m": 6000} ) - assert status == "wrong_sport" - assert "unknown(99)" in msg + assert status == "verified" + assert "99" in msg def test_short_duration_alone_still_verified_on_distance( self, @@ -169,7 +187,7 @@ class TestValidateRunnerupData: mock_sys_exit: MagicMock, tmp_path: Path, ) -> None: - """Sport 3 (Orienteering) is in RUNNERUP_ACCEPTED_SPORTS.""" + """Sport 3 (Orienteering) is verified like any other sport.""" locker = create_locker(mock_tk, tmp_path) status, _ = locker._validate_runnerup_data( {"sport": 3, "duration_seconds": 2400, "distance_m": 6000} diff --git a/screen_locker/tests/test_runnerup_verification_part10.py b/screen_locker/tests/test_runnerup_verification_part10.py index 99bfb15..c6f547f 100644 --- a/screen_locker/tests/test_runnerup_verification_part10.py +++ b/screen_locker/tests/test_runnerup_verification_part10.py @@ -30,7 +30,7 @@ _TCX_RUNNING = """\ """ -# TCX with an unrecognised sport tag (not in RUNNERUP_ACCEPTED_SPORTS). +# TCX with a non-running sport tag (Gym). _TCX_GYM = """\ """ -# TCX with an unrecognised sport tag (not in RUNNERUP_ACCEPTED_SPORTS). +# TCX with a non-running sport tag (Gym). _TCX_GYM = """\ """ -# TCX with an unrecognised sport tag (not in RUNNERUP_ACCEPTED_SPORTS). +# TCX with a non-running sport tag (Gym). _TCX_GYM = """\ """ -# TCX with an unrecognised sport tag (not in RUNNERUP_ACCEPTED_SPORTS). +# TCX with a non-running sport tag (Gym). _TCX_GYM = """\ """ -# TCX with an unrecognised sport tag (not in RUNNERUP_ACCEPTED_SPORTS). +# TCX with a non-running sport tag (Gym). _TCX_GYM = """\ """ -# TCX with an unrecognised sport tag (not in RUNNERUP_ACCEPTED_SPORTS). +# TCX with a non-running sport tag (Gym). _TCX_GYM = """\ """ -# TCX with an unrecognised sport tag (not in RUNNERUP_ACCEPTED_SPORTS). +# TCX with a non-running sport tag (Gym). _TCX_GYM = """\ """ -# TCX with an unrecognised sport tag (not in RUNNERUP_ACCEPTED_SPORTS). +# TCX with a non-running sport tag (Gym). _TCX_GYM = """\ """ -# TCX with an unrecognised sport tag (not in RUNNERUP_ACCEPTED_SPORTS). +# TCX with a non-running sport tag (Gym). _TCX_GYM = """\ """ -# TCX with an unrecognised sport tag (not in RUNNERUP_ACCEPTED_SPORTS). +# TCX with a non-running sport tag (Gym). _TCX_GYM = """\ """ -# TCX with an unrecognised sport tag (not in RUNNERUP_ACCEPTED_SPORTS). +# TCX with a non-running sport tag (Gym). _TCX_GYM = """\ """ -# TCX with an unrecognised sport tag (not in RUNNERUP_ACCEPTED_SPORTS). +# TCX with a non-running sport tag (Gym). _TCX_GYM = """\ """ -# TCX with an unrecognised sport tag (not in RUNNERUP_ACCEPTED_SPORTS). +# TCX with a non-running sport tag (Gym). _TCX_GYM = """\ """ -# TCX with an unrecognised sport tag (not in RUNNERUP_ACCEPTED_SPORTS). +# TCX with a non-running sport tag (Gym). _TCX_GYM = """\