From bcc2f659665f3e33bde9e5c597e0b2a68ef047ae Mon Sep 17 00:00:00 2001 From: "Shane B." Date: Wed, 19 Aug 2026 20:24:55 +0200 Subject: [PATCH 1/2] Add a Save logs button, and take the obvious identifiers out on the way Asking somebody to find ~/Library/Logs/OpenTagViewer/exporter.log reliably produces one of two wrong things: a screenshot of an error dialog, which never has the useful part in it, or their export zip - which holds the keys to their tags and cannot be un-shared once it is posted. The button takes Cancel's place, which did exactly what the window's own close button does and so earned none of that space. What it writes is deliberately unmistakable: a `.txt` with `logs` in the name, and a dialog that names both files rather than assuming the difference is obvious. **And it redacts, best effort.** Apple IDs, home directories, peer hashes, device serials, the name somebody gave their phone, keychain attributes printed verbatim, and record UUIDs. Values are numbered rather than blanked, so `` is the same serial everywhere it appears - "these two lines are about one device" is often the whole diagnosis, and blanket asterisks destroy exactly that. The model beside a device name stays, because it is not personal and is usually the point of the line. A library was considered and rejected. scrubadub pulls scikit-learn, textblob and faker; presidio pulls spacy, numpy and pydantic. Both are large in a frozen bundle, and neither knows what an Apple serial, a Cuttlefish peer hash or a keychain acct field looks like - they are built for prose. Everything worth removing here has a known shape. Nothing claims the result is clean, and the dialog says so: the text comes from a library reading Apple's structures, so a field that is harmless on one account may not be on another. Two suites. test_redact.py covers the patterns without a display, including that timestamps, module names, byte counts and model numbers survive - redaction that eats the diagnosis just earns a second request for the original. test_save_logs_button.py drives the real window, because a window writing an unredacted file looks exactly like one writing a redacted file. Every identifier in those fixtures is randomly generated. The first draft used values from a real user's bug report, which is precisely what this feature exists to prevent and would have put them in the history permanently. --- python/exporter/redact.py | 145 +++++++++++++++++++ python/exporter/wizard.py | 66 ++++++++- python/test/test_redact.py | 140 ++++++++++++++++++ python/test/test_save_logs_button.py | 203 +++++++++++++++++++++++++++ 4 files changed, 553 insertions(+), 1 deletion(-) create mode 100644 python/exporter/redact.py create mode 100644 python/test/test_redact.py create mode 100644 python/test/test_save_logs_button.py diff --git a/python/exporter/redact.py b/python/exporter/redact.py new file mode 100644 index 00000000..068df8e2 --- /dev/null +++ b/python/exporter/redact.py @@ -0,0 +1,145 @@ +""" +Take the obvious personal identifiers out of a log, so posting one is less costly. + +**Best effort, and the wording everywhere says so.** This cannot promise a clean file: the text +comes from a library reading Apple's structures, and a field that is innocuous on one account may +not be on another. What it can do is remove the things that are *known* to appear and are +recognisable by shape, which is most of what a person would otherwise have to find by hand. + +**A library was considered and rejected.** `scrubadub` pulls scikit-learn, textblob, dateparser +and faker; `presidio-analyzer` pulls spacy, numpy and pydantic. Both are large enough to matter in +a frozen desktop bundle, and neither knows what an Apple device serial, a Cuttlefish peer hash or +a keychain `acct` field looks like - they are built for prose. Everything worth removing here has +a known shape, and a known shape is a regex. + +**Values are numbered rather than blanked**, so `` is the same serial everywhere it +appears. A log where every identifier became `***` loses the one thing that makes it a log: that +this record and that record are about the same device. Redaction that destroys the diagnosis +defeats the reason for sending the file. +""" + +from __future__ import annotations + +import re +from collections import Counter +from dataclasses import dataclass + + +@dataclass(frozen=True) +class Rule: + """One kind of identifier, and how to find it.""" + + name: str + pattern: re.Pattern[str] + group: int = 0 + """Which capture group holds the value. 0 means the whole match.""" + + +# Order matters where two rules could claim the same text. Paths run before names so that a +# username inside a home directory is taken as a path rather than left for something looser, and +# the labelled fields run before the loose shapes so their placeholder names stay meaningful. +RULES: tuple[Rule, ...] = ( + # An Apple ID, wherever it turns up - an error message, a prompt echoed back, a keychain + # attribute. The one identifier here that names a person rather than a device. + Rule("email", re.compile(r"\b[\w.+-]+@[\w-]+\.[\w.-]+\b")), + + # `/Users/paula/…`, `/home/amity/…`, `C:\Users\paula\…`. Tracebacks are full of these, and the + # account name is very often the person's actual name. + Rule("user", re.compile(r"(?i)(?:/Users/|/home/|[A-Z]:\\Users\\)([^/\\\s'\"]+)"), group=1), + + # Cuttlefish peers, as `Peer SHA256:=`. Pseudonymous, stable, one per device on the + # account - so a set of them fingerprints the account even without a name attached. + Rule("peer", re.compile(r"SHA256:[A-Za-z0-9+/]{20,}={0,2}")), + + # How escrow records describe themselves: "…, serial F2LX9Q…, escrowed 2024-03-11". + Rule("serial", re.compile(r"\bserial\s+([A-Z0-9]{6,20})\b"), group=1), + + # The device name an escrow record carries, which is whatever the user called their phone - + # very often their own name. Both of the lines FindMy.py logs it on. + # + # **MULTILINE is not decoration.** Without it `$` matches only at the end of the whole file, so + # this fired on the last line and nothing else - it reported zero devices on a real log full of + # them, which is the failure mode where redaction looks like it ran. + Rule( + "device", + re.compile(r"(?:escrow recovery for|escrow record)\s+(.+?)(?=,|$)", re.MULTILINE), + group=1, + ), + + # How `EscrowRecord.describe()` renders: ", , serial …, escrowed …". The name is + # whatever somebody called their phone, and is frequently their own name; the model beside it + # is not personal and is worth keeping, since it is often the point of the line. + # + # Anchored on a model followed by `serial` so it cannot run away with a whole sentence. + # The anchor allows a list marker, because this is most often read off a numbered menu of + # recovery options - " 1. Paula's iPhone, iPhone15,2, serial …" - and anchoring on the bare + # line start missed every one of them. + Rule( + "device", + re.compile( + r"(?:^[ \t]*(?:\d+\.[ \t]*)?|\?[ \t]|:[ \t])" + r"([^,\n]{1,60}?),[ \t]+" + # Bounded rather than comma-free: an Apple model identifier contains a comma of its + # own - `iPhone15,2` - so a lookahead that stops at the first comma never reaches the + # `serial` it is anchored on, and matched only the models that happen not to have one. + r"(?=(?:iPhone|iPad|iPod|Mac|MacBook|iMac|Watch).{0,40}?,[ \t]+serial\b)", + re.MULTILINE, + ), + group=1, + ), + + # `describe_item` prints keychain attributes verbatim: acct='…', labl='…'. What is in them is + # Apple's choice, so the safe assumption is that it identifies somebody. + # + # The quotes are matched but *not* captured, so an empty attribute stays `acct=''` instead of + # becoming `acct=` - a placeholder standing in for nothing, which reads as though + # something was hidden and makes the count wrong. + Rule("item", re.compile(r"\b(?:acct|labl|srvr|agrp)=(['\"])([^'\"]*)\1"), group=2), + + # Record and beacon identifiers. Not secret, but unique to one account's accessories. + Rule("uuid", re.compile(r"\b[0-9a-fA-F]{8}-(?:[0-9a-fA-F]{4}-){3}[0-9a-fA-F]{12}\b")), +) + + +def redact(text: str) -> tuple[str, Counter[str]]: + """ + Replace recognised identifiers with stable placeholders. + + :returns: The cleaned text, and how many *distinct* values were replaced per kind - which is + what a person needs to sanity-check the result. Ten devices found in a log from somebody + who owns two is a rule matching too much, and no serials found at all is a rule that has + stopped matching, and both are worth seeing. + """ + seen: dict[str, dict[str, str]] = {rule.name: {} for rule in RULES} + + for rule in RULES: + def substitute(match: re.Match[str], rule: Rule = rule) -> str: + value = match.group(rule.group) + + # Nothing to do, and replacing it would turn an empty field into a fake identifier. + if not value or not value.strip(): + return match.group(0) + + known = seen[rule.name] + if value not in known: + known[value] = f"<{rule.name}-{len(known) + 1}>" + + # Only the captured part is replaced, so the surrounding text - which is what makes + # the line readable - survives. "serial F2LX9Q" becomes "serial ", not + # "". + start, end = match.span(rule.group) + return match.group(0)[: start - match.start()] + known[value] + match.group(0)[end - match.start():] + + text = rule.pattern.sub(substitute, text) + + return text, Counter({name: len(values) for name, values in seen.items() if values}) + + +def summarise(counts: Counter[str]) -> str: + """One line naming what was taken out, for showing to the person about to send the file.""" + if not counts: + return "Nothing recognisable was found to remove." + + parts = [f"{count} {name}{'s' if count != 1 else ''}" for name, count in sorted(counts.items())] + + return "Replaced " + ", ".join(parts) + "." diff --git a/python/exporter/wizard.py b/python/exporter/wizard.py index 0e45ef51..9ac77d0d 100644 --- a/python/exporter/wizard.py +++ b/python/exporter/wizard.py @@ -39,6 +39,7 @@ is_verification_code, verification_code, ) +from exporter.redact import redact, summarise from exporter.tkutil import centre_on_screen, centre_over from exporter.custom_tags import ( CustomTagError, @@ -199,10 +200,73 @@ def _build(self) -> None: help_label.grid(row=0, column=2) help_label.bind("", lambda _event: webbrowser.open(WIKI_LINK, new=2, autoraise=True)) - ttk.Button(buttons, text="Cancel", command=self.destroy).grid(row=0, column=3, padx=(0, 8)) + # **Where Cancel used to be**, which did exactly what the window's own close button does + # and so earned none of that space. This does something nothing else here can. + # + # A button rather than a link, because being findable is the whole point: somebody asked + # for "the logs" who cannot find them attaches one of two wrong things instead - a + # screenshot of an error dialog, which never contains the useful part, or their export zip, + # which holds the keys to their tags and cannot be un-shared once it is posted. + ttk.Button(buttons, text="Save logs…", command=self._save_logs).grid( + row=0, column=3, padx=(0, 8), + ) self.confirm_button = ttk.Button(buttons, text="Export…", command=self._export, state="disabled") self.confirm_button.grid(row=0, column=4) + def _save_logs(self) -> None: + """ + Copy the log somewhere the user can find, under a name nothing else here could be. + + **The point is the name, as much as the copy.** The two files this program produces are a + bundle of tag keys and a log, and one of them can be handed out safely. Somebody asked for + "the file" attaches whichever they can find, and the wrong answer publishes the keys to + their tags permanently - so the log is written as `.txt`, with `logs` in its name, and the + dialog afterwards says which is which rather than assuming it is obvious. + """ + log = log_file() + + if not log.is_file() or log.stat().st_size == 0: + messagebox.showinfo( + "Save logs", + "There is nothing in the log yet.\n\n" + "It is written as the exporter runs, so try again after the step that went wrong.", + parent=self, + ) + return + + stamp = datetime.datetime.now().strftime("%Y%m%d-%H%M%S") + chosen = asksaveasfilename( + parent=self, + title="Save logs", + # Deliberately not .zip. The extension is the fastest way to tell this from an export, + # and a text file is also one somebody can read before sending, which is the whole ask. + defaultextension=".txt", + initialfile=f"OpenTagViewer-logs-{stamp}.txt", + filetypes=[("Text files", "*.txt"), ("All files", "*.*")], + ) + + if not chosen: + return + + try: + cleaned, counts = redact(log.read_text(encoding="utf-8", errors="replace")) + Path(chosen).write_text(cleaned, encoding="utf-8") + except OSError as e: + logger.exception("Could not save the log") + messagebox.showerror("Save logs", f"Could not write that file:\n\n{e}", parent=self) + return + + messagebox.showinfo( + "Save logs", + f"Saved to:\n{chosen}\n\n" + f"{summarise(counts)} The copy on disk is untouched.\n\n" + "This is the log, not your tags. Your tags are the .zip the Export… button writes.\n\n" + "**Read it before you post it.** Identifiers were removed by pattern-matching, which" + " cannot promise it caught everything - the text comes from a library reading Apple's" + " structures, and a field that is harmless on one account may not be on another.", + parent=self, + ) + def _read_label(self) -> str: """What the button that reads the account says, which is not the same on both routes.""" return "Read this Mac's Find My files" if self.route.is_local else "Sign in to Apple…" diff --git a/python/test/test_redact.py b/python/test/test_redact.py new file mode 100644 index 00000000..961bdafd --- /dev/null +++ b/python/test/test_redact.py @@ -0,0 +1,140 @@ +""" +Taking personal identifiers out of a log before somebody posts it. + +Two properties, and the second is the one that is easy to forget. + +**It removes what it claims to.** Every rule here exists because that identifier was observed in a +real log, so a rule that stops matching is a regression with a privacy cost. + +**And it leaves the diagnosis intact.** Redaction that eats timestamps, module names, byte counts +or model numbers produces a file nobody can debug from, and the person sending it has no way to +tell. A log stripped to `***` is not safer in any way that matters - it just gets a second request +for the unredacted one. +""" + +from __future__ import annotations + +import pytest + +from exporter.redact import redact, summarise + + +def clean(text: str) -> str: + return redact(text)[0] + + +class TestWhatItRemoves: + def test_an_apple_id(self): + assert "someone@example.com" not in clean("Attempting authentication for user someone@example.com") + + def test_a_home_directory_names_a_person(self): + # Tracebacks are full of these and the account name is very often somebody's real name. + out = clean('File "/home/kmvaesp/OpenTagViewer/python/exporter/cli.py", line 893') + + assert "kmvaesp" not in out + assert "OpenTagViewer/python/exporter/cli.py" in out, "the useful half of the path stays" + + @pytest.mark.parametrize("path", [ + "/Users/kmvaesp/Library/Logs/x.log", + "/home/kmvaesp/x.log", + r"C:\Users\kmvaesp\AppData\x.log", + ]) + def test_every_platform_spelling_of_a_home_directory(self, path): + assert "kmvaesp" not in clean(path) + + def test_a_peer_hash(self): + line = "Peer SHA256:7Yt2Rg90+i3fSELB8nD25LePIq+Op/V3P18oT8t7qPI= carries no signing key" + + out = clean(line) + + assert "7Yt2Rg90+i3fSEL" not in out + assert "carries no signing key" in out + + def test_a_device_serial(self): + assert "BWBPC2K1ZR" not in clean("iPad, iPad Pro, serial BWBPC2K1ZR, escrowed 2024-03-11") + + def test_the_name_somebody_gave_their_phone(self): + # The model beside it is not personal and is often the point of the line, so it stays. + out = clean(" 1. Sam's iPhone, iPhone15,2, serial BWBPC2, escrowed 2024-03-11") + + assert "Sam" not in out + assert "iPhone15,2" in out + + def test_keychain_attributes_printed_verbatim(self): + # What Apple puts in these is Apple's choice, so the safe assumption is that it identifies + # somebody. + out = clean("Service key item: class='genp', acct='someone@example.com', labl='Home WiFi'") + + assert "someone@example.com" not in out + assert "Home WiFi" not in out + + def test_record_identifiers(self): + assert "725A989D" not in clean("Record 725A989D-D871-49A7-B2FE-948C24F356AB carries a field") + + +class TestWhatItKeeps: + """ + A log that cannot be read is not a safer log, it is a second round trip. + """ + + @pytest.mark.parametrize("line", [ + "INFO findmy.keychain.session: Reading the Manatee view with classA: 64 bytes", + "DEBUG findmy.cloudkit.client: Zone ProtectedCloudStorage fully synced after page 1", + "INFO findmy.keychain.items: View holds 61 item(s) and 29 pointer(s)", + "2026-08-16 12:09:48,557 WARNING exporter.privacy: ====", + "DerError: Truncated DER: element claims 109 bytes, 61 remain", + "The Manatee view holds 141 elliptic-curve key(s)", + ]) + def test_the_lines_that_make_a_log_worth_having(self, line): + assert clean(line) == line + + def test_a_traceback_still_points_at_code(self): + out = clean(' File "/home/kmvaesp/x/findmy/keychain/servicekey.py", line 297, in service_keys_from_der') + + assert "servicekey.py" in out + assert "line 297" in out + assert "service_keys_from_der" in out + + +class TestTheSameValueGetsTheSameName: + """ + Numbered rather than blanked, because "these two lines are about one device" is often the + whole diagnosis - and blanket `***` destroys exactly that. + """ + + def test_one_value_twice_is_one_placeholder(self): + out = clean("serial ABC123 said no\nserial ABC123 said no again\n") + + assert out.count("") == 2 + + def test_two_values_are_told_apart(self): + out = clean("serial ABC123 here\nserial XYZ789 there\n") + + assert "" in out + assert "" in out + + def test_the_count_is_of_distinct_values(self): + # What a person checks the result against: two devices on a two-device account is right, + # and ten is a rule matching too much. + _, counts = redact("serial ABC123 x\nserial ABC123 y\nserial XYZ789 z\n") + + assert counts["serial"] == 2 + + +class TestSayingWhatHappened: + def test_it_names_what_was_replaced(self): + _, counts = redact("user a@b.com and serial ABC123") + + assert summarise(counts) == "Replaced 1 email, 1 serial." + + def test_finding_nothing_is_said_plainly(self): + # Not silence: "nothing was found" and "the redactor did not run" look identical otherwise, + # and only one of them is fine. + assert "Nothing recognisable" in summarise(redact("View holds 61 item(s)")[1]) + + +class TestItDoesNotInventIdentifiers: + def test_an_empty_field_is_left_alone(self): + out = clean("Service key item: acct='', labl='x'") + + assert "acct=''" in out, "an empty attribute must not become a fake identifier" diff --git a/python/test/test_save_logs_button.py b/python/test/test_save_logs_button.py new file mode 100644 index 00000000..a23b4926 --- /dev/null +++ b/python/test/test_save_logs_button.py @@ -0,0 +1,203 @@ +""" +The "Save logs…" button, driven through the actual window. + +**Separate from `test_redact.py` on purpose.** That file proves the patterns work on text. This one +proves the button is wired to them, which is the part that breaks silently: a window that writes an +unredacted file looks exactly like one that writes a redacted file. + +Every identifier below is randomly generated and belongs to nobody. That is deliberate - the first +draft of this file used values lifted from a real user's bug report, which is precisely what the +feature under test exists to prevent, and would have committed them to the history permanently. +""" + +from __future__ import annotations + +import tkinter as tk +from unittest import mock + +import pytest + +from exporter import wizard + +# Random, and shaped like the real thing: a peer hash is base64 of a SHA-256, a serial is ten +# uppercase alphanumerics, a home directory carries somebody's login name. +PEER = "7Yt2Rg90+i3fSELB8nD25LePIq+Op/V3P18oT8t7qPI=" +SERIAL = "BWBPC2K1ZR" +USER = "kmvaesp" + +# One of everything the redactor should find, in the shapes FindMy.py actually writes them - so +# this fails if the wiring is right and the patterns have rotted. +LOG = f"""\ +2026-08-16 12:09:48 WARNING exporter.privacy: NOT safe to publish +2026-08-16 12:09:49 INFO findmy.reports.account: Attempting authentication for user someone@example.com +2026-08-16 12:09:50 INFO findmy.keychain.recovery: Beginning escrow recovery for Sam's iPhone +2026-08-16 12:09:51 WARNING findmy.keychain.peers: Peer SHA256:{PEER} carries no signing key +2026-08-16 12:09:52 INFO findmy.keychain.items: Service key item: acct='someone@example.com', labl='Home WiFi' + 1. Sam's iPhone, iPhone15,2, serial {SERIAL}, escrowed 2024-03-11 + File "/home/{USER}/OpenTagViewer/python/exporter/cli.py", line 893, in run +2026-08-16 12:09:53 INFO findmy.keychain.session: The Manatee view holds 141 elliptic-curve key(s) +""" + +SECRETS = [ + "someone@example.com", + "Sam's iPhone", + PEER[:15], + SERIAL, + f"/home/{USER}/", + "Home WiFi", +] + + +@pytest.fixture(scope="module") +def window(): + """ + One window for the whole module. + + **Built once rather than per test.** Repeatedly creating and destroying a `Tk` in one process + is unstable on macOS - the first attempt at this crashed the interpreter during collection, + before a single assertion ran. Nothing here mutates window state, so one is enough. + """ + try: + app = wizard.WizardApp() + except tk.TclError as e: # pragma: no cover - depends on the machine, not on the code + pytest.skip(f"needs a display to build a window: {e}") + + app.withdraw() + try: + yield app + finally: + app.destroy() + + +@pytest.fixture +def log(tmp_path): + """A log file this test owns.""" + path = tmp_path / "exporter.log" + path.write_text(LOG, encoding="utf-8") + + return path + + +def click_save_logs(window, log, saving_to): + """Press the button, answering its save dialog with this path. None cancels the dialog.""" + with mock.patch.object(wizard, "log_file", return_value=log), \ + mock.patch.object(wizard, "asksaveasfilename", + return_value=str(saving_to) if saving_to else ""), \ + mock.patch.object(wizard.messagebox, "showinfo") as info, \ + mock.patch.object(wizard.messagebox, "showerror") as error: + window._save_logs() + + return info, error + + +def button_labels(widget) -> list[str]: + found: list[str] = [] + + for child in widget.winfo_children(): + try: + label = child.cget("text") + except tk.TclError: + label = "" + if label: + found.append(str(label)) + found.extend(button_labels(child)) + + return found + + +class TestTheButtonExists: + def test_it_is_in_the_window(self, window): + assert "Save logs…" in button_labels(window) + + def test_cancel_is_not(self, window): + # It did exactly what the window's own close button does, and this took its place. + assert "Cancel" not in button_labels(window) + + +class TestWhatItWrites: + def test_no_identifier_survives_into_the_saved_file(self, window, log, tmp_path): + """ + The whole reason the button exists. + + Asserted per identifier rather than by comparing whole files, so a failure names which one + leaked instead of only saying that something did. + """ + target = tmp_path / "out.txt" + + click_save_logs(window, log, target) + + saved = target.read_text(encoding="utf-8") + for secret in SECRETS: + assert secret not in saved, f"{secret!r} reached a file a user is about to post" + + def test_the_log_is_still_worth_reading(self, window, log, tmp_path): + # Redaction that eats the diagnosis just produces a second request for the original. + target = tmp_path / "out.txt" + + click_save_logs(window, log, target) + + saved = target.read_text(encoding="utf-8") + assert "The Manatee view holds 141 elliptic-curve key(s)" in saved + assert "carries no signing key" in saved + assert "iPhone15,2" in saved, "a model is not personal and is often the point of the line" + assert "line 893, in run" in saved + + def test_the_original_on_disk_is_left_alone(self, window, log, tmp_path): + # The copy the user keeps stays complete; only what leaves the machine is redacted. + click_save_logs(window, log, tmp_path / "out.txt") + + assert log.read_text(encoding="utf-8") == LOG + + def test_it_says_what_it_took_out(self, window, log, tmp_path): + info, _ = click_save_logs(window, log, tmp_path / "out.txt") + + body = info.call_args[0][1] + assert "Replaced" in body + assert "serial" in body + + def test_it_does_not_promise_the_file_is_clean(self, window, log, tmp_path): + # The one claim that must never be made. Pattern-matching cannot know what Apple put in a + # field, and somebody told "this is safe now" will not read it. + info, _ = click_save_logs(window, log, tmp_path / "out.txt") + + body = info.call_args[0][1].lower() + assert "read it before you post it" in body + assert "cannot promise" in body + + def test_it_distinguishes_the_log_from_the_tags(self, window, log, tmp_path): + info, _ = click_save_logs(window, log, tmp_path / "out.txt") + + assert "not your tags" in info.call_args[0][1] + + +class TestWhenThereIsNothingToSave: + def test_a_missing_log_does_not_open_a_save_dialog(self, window, log): + log.unlink() + + with mock.patch.object(wizard, "log_file", return_value=log), \ + mock.patch.object(wizard, "asksaveasfilename") as save, \ + mock.patch.object(wizard.messagebox, "showinfo") as info: + window._save_logs() + + assert not save.called, "asking where to save nothing wastes the one action they took" + assert "nothing in the log yet" in info.call_args[0][1].lower() + + def test_an_empty_log_is_treated_the_same(self, window, log): + log.write_text("", encoding="utf-8") + + with mock.patch.object(wizard, "log_file", return_value=log), \ + mock.patch.object(wizard, "asksaveasfilename") as save, \ + mock.patch.object(wizard.messagebox, "showinfo"): + window._save_logs() + + assert not save.called + + +class TestWhenTheUserBacksOut: + def test_cancelling_the_dialog_writes_nothing(self, window, log, tmp_path): + before = set(tmp_path.iterdir()) + + info, error = click_save_logs(window, log, None) + + assert set(tmp_path.iterdir()) == before + assert not info.called and not error.called From ca44c9ac7dd2bebfd11469a233023a9add77fed3 Mon Sep 17 00:00:00 2001 From: "Shane B." Date: Wed, 19 Aug 2026 21:06:43 +0200 Subject: [PATCH 2/2] Skip the window tests where there is no Tk, instead of failing the run CI went red on a test file that passes locally. `import tkinter` at module scope runs during collection, so a Python built without Tk fails the whole session before any fixture can decide to skip - the `pytest.skip` in the window fixture never got a chance. That is the same lesson as "Stop the headless CLI needing a window", which moved code out of wizard.py for exactly this reason, and I did not apply it to the test that came with it. Worth knowing for reading the run: the matrix is fail-fast, so 3.12 failed and cancelled the other three - and `gh pr checks` reported three failures whose every step had succeeded. Only 3.12 had anything wrong with it, and only in that its interpreter had no _tkinter while 3.13's did. `pytest.importorskip` now guards the module, before `exporter.wizard` is imported, since that pulls in tkinter too. Verified by hiding _tkinter from the import system: the file skips rather than erroring, and still runs its eleven tests where Tk exists. --- python/test/test_save_logs_button.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/python/test/test_save_logs_button.py b/python/test/test_save_logs_button.py index a23b4926..7b1fa693 100644 --- a/python/test/test_save_logs_button.py +++ b/python/test/test_save_logs_button.py @@ -12,12 +12,18 @@ from __future__ import annotations -import tkinter as tk from unittest import mock import pytest -from exporter import wizard +# **Before anything that imports tkinter, `exporter.wizard` included.** A `pytest.skip` inside a +# fixture is too late: the module is imported during collection, so a Python built without Tk +# fails the whole run before any fixture can decline. Not hypothetical - CI runs a matrix, and +# 3.12 resolved to a framework interpreter with no `_tkinter` while 3.13 had one, so this took +# down four jobs and reported the failure against the three it cancelled. +tk = pytest.importorskip("tkinter", reason="needs a Python built with Tk") + +from exporter import wizard # noqa: E402 - has to follow the importorskip above # Random, and shaped like the real thing: a peer hash is base64 of a SHA-256, a serial is ten # uppercase alphanumerics, a home directory carries somebody's login name.