Credibility pass: comments state constraints, not development history
A skeptical-cloner review flagged the patterns that read as AI-iteration residue: test comments and section headers narrating the review process that produced them, "legacy format" framing in a days-old repo, shadow re-imports appended without reading file headers, one genuine machine leftover (FIXTURE_CACHE = FIXTURE_CACHE), and a few register slips. Every history-narrating comment is rewritten as the timeless invariant it was guarding, test sections are grouped by behavior, function-local imports are hoisted, and the README loses its one marketing clause and heaviest dash runs. No behavior changes; 176 tests unchanged and green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Eric Wagoner
+28
-30
@@ -15,6 +15,7 @@ from bggpipe.config import Config
|
||||
from bggpipe.models import CollectionItem
|
||||
from bggpipe.upload import (
|
||||
UPLOAD_LOG_COLUMNS,
|
||||
LoginError,
|
||||
UploadJob,
|
||||
_scrub,
|
||||
build_queue,
|
||||
@@ -23,7 +24,7 @@ from bggpipe.upload import (
|
||||
)
|
||||
|
||||
|
||||
def NOW() -> str:
|
||||
def _now() -> str:
|
||||
return "2026-08-01T00:00:00+00:00"
|
||||
|
||||
|
||||
@@ -69,7 +70,7 @@ def _log_row(action="add", bgg_id="1", collid="", version_id="", status="added")
|
||||
"name": "Game",
|
||||
"version_id": version_id,
|
||||
"status": status,
|
||||
"timestamp": NOW(),
|
||||
"timestamp": _now(),
|
||||
"error": "",
|
||||
}
|
||||
|
||||
@@ -170,7 +171,7 @@ def test_real_run_refuses_while_stub_marker_exists(tmp_path):
|
||||
(cfg.cache_dir / "STUB_FIXTURES.marker").write_text("stub")
|
||||
_seed_data(tmp_path, to_add=[_add_row()])
|
||||
with pytest.raises(typer.Exit):
|
||||
run_upload(cfg, uploader=FakeUploader(), sleep=lambda s: None, now=NOW)
|
||||
run_upload(cfg, uploader=FakeUploader(), sleep=lambda s: None, now=_now)
|
||||
|
||||
|
||||
def test_dry_run_allowed_with_stub_marker_and_writes_nothing(tmp_path, capsys):
|
||||
@@ -178,7 +179,7 @@ def test_dry_run_allowed_with_stub_marker_and_writes_nothing(tmp_path, capsys):
|
||||
cfg.cache_dir.mkdir(parents=True)
|
||||
(cfg.cache_dir / "STUB_FIXTURES.marker").write_text("stub")
|
||||
_seed_data(tmp_path, to_add=[_add_row()], to_update=[_update_row()])
|
||||
run_upload(cfg, dry_run=True, now=NOW)
|
||||
run_upload(cfg, dry_run=True, now=_now)
|
||||
out = capsys.readouterr().out
|
||||
assert "WARNING" in out and "SYNTHETIC" in out
|
||||
assert "would add Wingspan" in out
|
||||
@@ -197,7 +198,7 @@ def test_run_logs_every_attempt_and_continues_past_failures(tmp_path):
|
||||
to_update=[_update_row(collid="9")],
|
||||
)
|
||||
fake = FakeUploader(failures={"Catan"})
|
||||
results = run_upload(cfg, uploader=fake, sleep=lambda s: None, now=NOW)
|
||||
results = run_upload(cfg, uploader=fake, sleep=lambda s: None, now=_now)
|
||||
assert [r["status"] for r in results] == ["added", "failed", "updated"]
|
||||
logged = list(csv.DictReader((tmp_path / "upload_log.csv").open()))
|
||||
assert len(logged) == 3
|
||||
@@ -209,10 +210,10 @@ def test_rerun_skips_completed_work(tmp_path):
|
||||
cfg = _cfg(tmp_path)
|
||||
_seed_data(tmp_path, to_add=[_add_row(bgg_id="1"), _add_row(bgg_id="2", name="C")])
|
||||
fake = FakeUploader()
|
||||
run_upload(cfg, uploader=fake, sleep=lambda s: None, now=NOW)
|
||||
run_upload(cfg, uploader=fake, sleep=lambda s: None, now=_now)
|
||||
assert len(fake.calls) == 2
|
||||
again = FakeUploader()
|
||||
results = run_upload(cfg, uploader=again, sleep=lambda s: None, now=NOW)
|
||||
results = run_upload(cfg, uploader=again, sleep=lambda s: None, now=_now)
|
||||
assert again.calls == [] and results == []
|
||||
|
||||
|
||||
@@ -225,7 +226,7 @@ def test_pacing_sleeps_2_to_4s_between_games_only(tmp_path):
|
||||
uploader=FakeUploader(),
|
||||
sleep=sleeps.append,
|
||||
rng=random.Random(42),
|
||||
now=NOW,
|
||||
now=_now,
|
||||
)
|
||||
assert len(sleeps) == 3 # between games, not before the first
|
||||
assert all(2.0 <= s <= 4.0 for s in sleeps)
|
||||
@@ -235,7 +236,7 @@ def test_limit_caps_the_queue(tmp_path):
|
||||
cfg = _cfg(tmp_path)
|
||||
_seed_data(tmp_path, to_add=[_add_row(bgg_id=str(i)) for i in range(1, 5)])
|
||||
fake = FakeUploader()
|
||||
run_upload(cfg, uploader=fake, limit=2, sleep=lambda s: None, now=NOW)
|
||||
run_upload(cfg, uploader=fake, limit=2, sleep=lambda s: None, now=_now)
|
||||
assert len(fake.calls) == 2
|
||||
|
||||
|
||||
@@ -303,22 +304,20 @@ def test_fresh_clone_marker_blocks_upload_without_cache_dir(tmp_path):
|
||||
(tmp_path / "STUB_DATA.marker").write_text("stub-derived CSVs")
|
||||
_seed_data(tmp_path, to_add=[_add_row()])
|
||||
with pytest.raises(typer.Exit):
|
||||
run_upload(cfg, uploader=FakeUploader(), sleep=lambda s: None, now=NOW)
|
||||
run_upload(cfg, uploader=FakeUploader(), sleep=lambda s: None, now=_now)
|
||||
|
||||
|
||||
# -- audit-fix regressions ----------------------------------------------
|
||||
# -- failure isolation and resume ---------------------------------------
|
||||
|
||||
|
||||
def test_login_error_aborts_without_poisoning_the_log(tmp_path):
|
||||
from bggpipe.upload import LoginError
|
||||
|
||||
class BrokenLogin(FakeUploader):
|
||||
def add_game(self, job):
|
||||
raise LoginError("Cloudflare is challenging this browser")
|
||||
|
||||
cfg = _cfg(tmp_path)
|
||||
_seed_data(tmp_path, to_add=[_add_row(bgg_id=str(i)) for i in range(1, 4)])
|
||||
results = run_upload(cfg, uploader=BrokenLogin(), sleep=lambda s: None, now=NOW)
|
||||
results = run_upload(cfg, uploader=BrokenLogin(), sleep=lambda s: None, now=_now)
|
||||
assert results == [] # nothing logged: next run retries everything
|
||||
assert not (tmp_path / "upload_log.csv").exists()
|
||||
|
||||
@@ -327,7 +326,7 @@ def test_three_identical_failures_abort_as_systemic(tmp_path):
|
||||
cfg = _cfg(tmp_path)
|
||||
_seed_data(tmp_path, to_add=[_add_row(bgg_id=str(i)) for i in range(1, 6)])
|
||||
fake = FakeUploader(failures={"Wingspan"}) # every job shares the name
|
||||
results = run_upload(cfg, uploader=fake, sleep=lambda s: None, now=NOW)
|
||||
results = run_upload(cfg, uploader=fake, sleep=lambda s: None, now=_now)
|
||||
assert len(results) == 3 # aborted after the third identical failure
|
||||
logged = list(csv.DictReader((tmp_path / "upload_log.csv").open()))
|
||||
assert len(logged) == 3 # jobs 4-5 left unlogged and retryable
|
||||
@@ -341,10 +340,10 @@ def test_added_no_version_is_done_and_verify_tolerates_it(tmp_path):
|
||||
|
||||
cfg = _cfg(tmp_path)
|
||||
_seed_data(tmp_path, to_add=[_add_row(version_id="99", version_name="4th ed.")])
|
||||
run_upload(cfg, uploader=NoVersionPicker(), sleep=lambda s: None, now=NOW)
|
||||
run_upload(cfg, uploader=NoVersionPicker(), sleep=lambda s: None, now=_now)
|
||||
# done: re-running must NOT re-add (a duplicate collection entry)
|
||||
again = FakeUploader()
|
||||
assert run_upload(cfg, uploader=again, sleep=lambda s: None, now=NOW) == []
|
||||
assert run_upload(cfg, uploader=again, sleep=lambda s: None, now=_now) == []
|
||||
assert again.calls == []
|
||||
# verify: game present without the version is the EXPECTED outcome
|
||||
log = list(csv.DictReader((tmp_path / "upload_log.csv").open()))
|
||||
@@ -354,7 +353,7 @@ def test_added_no_version_is_done_and_verify_tolerates_it(tmp_path):
|
||||
def test_missing_to_add_csv_is_a_loud_precondition_failure(tmp_path):
|
||||
cfg = _cfg(tmp_path) # no diff outputs seeded at all
|
||||
with pytest.raises(typer.Exit):
|
||||
run_upload(cfg, uploader=FakeUploader(), sleep=lambda s: None, now=NOW)
|
||||
run_upload(cfg, uploader=FakeUploader(), sleep=lambda s: None, now=_now)
|
||||
|
||||
|
||||
def test_second_update_for_same_game_is_deferred(tmp_path):
|
||||
@@ -369,11 +368,11 @@ def test_second_update_for_same_game_is_deferred(tmp_path):
|
||||
],
|
||||
)
|
||||
fake = FakeUploader()
|
||||
results = run_upload(cfg, uploader=fake, sleep=lambda s: None, now=NOW)
|
||||
results = run_upload(cfg, uploader=fake, sleep=lambda s: None, now=_now)
|
||||
assert [r["collid"] for r in results] == ["9"]
|
||||
# after the first lands, the next run picks up the deferred one
|
||||
again = FakeUploader()
|
||||
results = run_upload(cfg, uploader=again, sleep=lambda s: None, now=NOW)
|
||||
results = run_upload(cfg, uploader=again, sleep=lambda s: None, now=_now)
|
||||
assert [j.collid for j in again.calls] == ["10"]
|
||||
|
||||
|
||||
@@ -383,7 +382,7 @@ def test_real_run_without_credentials_exits_before_any_browser(tmp_path, monkeyp
|
||||
cfg = _cfg(tmp_path)
|
||||
_seed_data(tmp_path, to_add=[_add_row()])
|
||||
with pytest.raises(typer.Exit):
|
||||
run_upload(cfg, sleep=lambda s: None, now=NOW) # uploader=None: real path
|
||||
run_upload(cfg, sleep=lambda s: None, now=_now) # uploader=None: real path
|
||||
assert not (tmp_path / "upload_log.csv").exists()
|
||||
|
||||
|
||||
@@ -395,15 +394,15 @@ def test_same_key_second_copy_survives_limit_and_interrupts(tmp_path):
|
||||
_seed_data(tmp_path, to_add=[dict(twin), dict(twin)])
|
||||
|
||||
first = FakeUploader()
|
||||
run_upload(cfg, uploader=first, limit=1, sleep=lambda s: None, now=NOW)
|
||||
run_upload(cfg, uploader=first, limit=1, sleep=lambda s: None, now=_now)
|
||||
assert len(first.calls) == 1
|
||||
|
||||
second = FakeUploader()
|
||||
run_upload(cfg, uploader=second, sleep=lambda s: None, now=NOW)
|
||||
run_upload(cfg, uploader=second, sleep=lambda s: None, now=_now)
|
||||
assert len(second.calls) == 1 # the second copy, not zero, not two
|
||||
|
||||
third = FakeUploader()
|
||||
assert run_upload(cfg, uploader=third, sleep=lambda s: None, now=NOW) == []
|
||||
assert run_upload(cfg, uploader=third, sleep=lambda s: None, now=_now) == []
|
||||
|
||||
|
||||
def test_consecutive_failure_counter_resets_on_success(tmp_path):
|
||||
@@ -417,7 +416,7 @@ def test_consecutive_failure_counter_resets_on_success(tmp_path):
|
||||
cfg = _cfg(tmp_path)
|
||||
_seed_data(tmp_path, to_add=[_add_row(bgg_id=str(i)) for i in range(1, 7)])
|
||||
fake = FlakyPairs()
|
||||
run_upload(cfg, uploader=fake, sleep=lambda s: None, now=NOW)
|
||||
run_upload(cfg, uploader=fake, sleep=lambda s: None, now=_now)
|
||||
assert len(fake.calls) == 6 # fail,fail,ok,fail,fail,ok — never aborts
|
||||
|
||||
|
||||
@@ -425,14 +424,13 @@ def test_empty_game_name_is_refused_not_uploaded(tmp_path):
|
||||
cfg = _cfg(tmp_path)
|
||||
_seed_data(tmp_path, to_add=[_add_row(bgg_id="42", name="")])
|
||||
fake = FakeUploader()
|
||||
results = run_upload(cfg, uploader=fake, sleep=lambda s: None, now=NOW)
|
||||
results = run_upload(cfg, uploader=fake, sleep=lambda s: None, now=_now)
|
||||
assert results == [] and fake.calls == []
|
||||
|
||||
|
||||
def test_verify_shortfall_reported_once_per_game(tmp_path):
|
||||
# two DONE adds (different versions) of one game, one copy on BGG:
|
||||
# exactly ONE shortfall problem (the old _job_key guard was dead code
|
||||
# and double-reported)
|
||||
# the shortfall is reported once per game, not once per logged add
|
||||
log = [
|
||||
_log_row(action="add", bgg_id="7", version_id="1", status="added"),
|
||||
_log_row(action="add", bgg_id="7", version_id="2", status="added"),
|
||||
@@ -456,7 +454,7 @@ def test_run_upload_verify_wiring(tmp_path, capsys):
|
||||
# verify=True must re-fetch the LIVE collection (refresh) and cross-check
|
||||
cfg = _cfg(tmp_path)
|
||||
_seed_data(tmp_path, to_add=[_add_row(bgg_id="1", name="Wingspan")])
|
||||
run_upload(cfg, uploader=FakeUploader(), sleep=lambda s: None, now=NOW)
|
||||
run_upload(cfg, uploader=FakeUploader(), sleep=lambda s: None, now=_now)
|
||||
client = _VerifyClient([_item(1, 10, name="Wingspan")])
|
||||
run_upload(
|
||||
cfg,
|
||||
@@ -464,7 +462,7 @@ def test_run_upload_verify_wiring(tmp_path, capsys):
|
||||
verify=True,
|
||||
client=client,
|
||||
sleep=lambda s: None,
|
||||
now=NOW,
|
||||
now=_now,
|
||||
)
|
||||
assert client.calls == [{"username": "tester", "refresh": True}]
|
||||
assert "Verification OK" in capsys.readouterr().out
|
||||
|
||||
Reference in New Issue
Block a user