Finish the RunnerUp sport-allowlist removal; isolate the instance lock in tests
Some checks are pending
pre-commit / pre-commit (push) Waiting to run
Tests / test (3.10) (push) Waiting to run
Tests / test (3.11) (push) Waiting to run
Tests / test (3.12) (push) Waiting to run
Tests / lock-popup-safety (push) Waiting to run
release-apk / release (APK) (push) Waiting to run

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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018M5gKyYzxWG6QkCaj1wWfk
This commit is contained in:
Krzysztof Rudnicki 2026-08-21 13:17:26 +02:00
parent 957e4566c2
commit 390e916922
20 changed files with 220 additions and 75 deletions

View File

@ -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:

View File

@ -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"),
),
)

View File

@ -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))

View File

@ -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."""

View File

@ -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)

View File

@ -26,7 +26,7 @@ _TCX_RUNNING = """\
</TrainingCenterDatabase>
"""
# TCX with an unrecognised sport tag (not in RUNNERUP_ACCEPTED_SPORTS).
# TCX with a non-running sport tag (Gym).
_TCX_GYM = """\
<?xml version="1.0" encoding="UTF-8"?>
<TrainingCenterDatabase
@ -83,34 +83,52 @@ def _write_tcx(tmp_path: Path, content: str, name: str = "activity.tcx") -> 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}

View File

@ -30,7 +30,7 @@ _TCX_RUNNING = """\
</TrainingCenterDatabase>
"""
# TCX with an unrecognised sport tag (not in RUNNERUP_ACCEPTED_SPORTS).
# TCX with a non-running sport tag (Gym).
_TCX_GYM = """\
<?xml version="1.0" encoding="UTF-8"?>
<TrainingCenterDatabase

View File

@ -26,7 +26,7 @@ _TCX_RUNNING = """\
</TrainingCenterDatabase>
"""
# TCX with an unrecognised sport tag (not in RUNNERUP_ACCEPTED_SPORTS).
# TCX with a non-running sport tag (Gym).
_TCX_GYM = """\
<?xml version="1.0" encoding="UTF-8"?>
<TrainingCenterDatabase

View File

@ -30,7 +30,7 @@ _TCX_RUNNING = """\
</TrainingCenterDatabase>
"""
# TCX with an unrecognised sport tag (not in RUNNERUP_ACCEPTED_SPORTS).
# TCX with a non-running sport tag (Gym).
_TCX_GYM = """\
<?xml version="1.0" encoding="UTF-8"?>
<TrainingCenterDatabase

View File

@ -28,7 +28,7 @@ _TCX_RUNNING = """\
</TrainingCenterDatabase>
"""
# TCX with an unrecognised sport tag (not in RUNNERUP_ACCEPTED_SPORTS).
# TCX with a non-running sport tag (Gym).
_TCX_GYM = """\
<?xml version="1.0" encoding="UTF-8"?>
<TrainingCenterDatabase

View File

@ -26,7 +26,7 @@ _TCX_RUNNING = """\
</TrainingCenterDatabase>
"""
# TCX with an unrecognised sport tag (not in RUNNERUP_ACCEPTED_SPORTS).
# TCX with a non-running sport tag (Gym).
_TCX_GYM = """\
<?xml version="1.0" encoding="UTF-8"?>
<TrainingCenterDatabase

View File

@ -30,7 +30,7 @@ _TCX_RUNNING = """\
</TrainingCenterDatabase>
"""
# TCX with an unrecognised sport tag (not in RUNNERUP_ACCEPTED_SPORTS).
# TCX with a non-running sport tag (Gym).
_TCX_GYM = """\
<?xml version="1.0" encoding="UTF-8"?>
<TrainingCenterDatabase

View File

@ -26,7 +26,7 @@ _TCX_RUNNING = """\
</TrainingCenterDatabase>
"""
# TCX with an unrecognised sport tag (not in RUNNERUP_ACCEPTED_SPORTS).
# TCX with a non-running sport tag (Gym).
_TCX_GYM = """\
<?xml version="1.0" encoding="UTF-8"?>
<TrainingCenterDatabase

View File

@ -23,7 +23,7 @@ _TCX_RUNNING = """\
</TrainingCenterDatabase>
"""
# TCX with an unrecognised sport tag (not in RUNNERUP_ACCEPTED_SPORTS).
# TCX with a non-running sport tag (Gym).
_TCX_GYM = """\
<?xml version="1.0" encoding="UTF-8"?>
<TrainingCenterDatabase

View File

@ -28,7 +28,7 @@ _TCX_RUNNING = """\
</TrainingCenterDatabase>
"""
# TCX with an unrecognised sport tag (not in RUNNERUP_ACCEPTED_SPORTS).
# TCX with a non-running sport tag (Gym).
_TCX_GYM = """\
<?xml version="1.0" encoding="UTF-8"?>
<TrainingCenterDatabase

View File

@ -31,7 +31,7 @@ _TCX_RUNNING = """\
</TrainingCenterDatabase>
"""
# TCX with an unrecognised sport tag (not in RUNNERUP_ACCEPTED_SPORTS).
# TCX with a non-running sport tag (Gym).
_TCX_GYM = """\
<?xml version="1.0" encoding="UTF-8"?>
<TrainingCenterDatabase

View File

@ -27,7 +27,7 @@ _TCX_RUNNING = """\
</TrainingCenterDatabase>
"""
# TCX with an unrecognised sport tag (not in RUNNERUP_ACCEPTED_SPORTS).
# TCX with a non-running sport tag (Gym).
_TCX_GYM = """\
<?xml version="1.0" encoding="UTF-8"?>
<TrainingCenterDatabase

View File

@ -27,7 +27,7 @@ _TCX_RUNNING = """\
</TrainingCenterDatabase>
"""
# TCX with an unrecognised sport tag (not in RUNNERUP_ACCEPTED_SPORTS).
# TCX with a non-running sport tag (Gym).
_TCX_GYM = """\
<?xml version="1.0" encoding="UTF-8"?>
<TrainingCenterDatabase

View File

@ -27,7 +27,7 @@ _TCX_RUNNING = """\
</TrainingCenterDatabase>
"""
# TCX with an unrecognised sport tag (not in RUNNERUP_ACCEPTED_SPORTS).
# TCX with a non-running sport tag (Gym).
_TCX_GYM = """\
<?xml version="1.0" encoding="UTF-8"?>
<TrainingCenterDatabase

View File

@ -27,7 +27,7 @@ _TCX_RUNNING = """\
</TrainingCenterDatabase>
"""
# TCX with an unrecognised sport tag (not in RUNNERUP_ACCEPTED_SPORTS).
# TCX with a non-running sport tag (Gym).
_TCX_GYM = """\
<?xml version="1.0" encoding="UTF-8"?>
<TrainingCenterDatabase