test: extract pure shortcut matching so it can be table-tested - #208
Merged
Conversation
AppDelegateShortcutRoutingTests spends 4684 lines and 87 tests proving things about key matching, and pays for it by creating real windows, spinning the main run loop for 50ms at a time in ~58 places, and resetting global defaults in every teardown. CI isolates the class, runs it serial, permanently skips one test, and absorbs a retry because it is timing-sensitive. Most of that machinery is not buying anything. The matching logic was already pure -- it just lived as private methods on AppDelegate and read one instance property, so the only way to reach it was through a window. Moves matchShortcutStroke/matchShortcut into a free ShortcutRouting enum with the layout provider passed in rather than read from self. The bodies move verbatim; AppDelegate keeps thin forwarders with the same private signatures, so no call site changes. Adds ShortcutRoutingTests: 18 table rows plus 2 chord tests over the same matrix the slow suite covers -- ANSI keycode fallback, Dvorak, Russian, ISO, AZERTY, shift-symbol normalization, keypad Enter. No windows, no run loop, no UserDefaults. Three helpers used only by the moved code came with it. Two more (normalizedShortcutEventCharacter, digitForNumberKeyCode) are shared with numberedShortcutDigit, which stays behind, so they became nonisolated static in place rather than being duplicated. The old tests are untouched and still run. They are the safety net proving this move changed nothing; retiring them and unwinding the CI special-casing is a later step, once these have proven equivalent over real runs. Refs #186
This was referenced Jul 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First slice of #186. Does not close it.
What this does
AppDelegateShortcutRoutingTestsis 4,684 lines and 87 tests, and it's slow and flaky for a reason that turns out to be avoidable: the key-matching logic it exercises was already pure, but it lived as private methods onAppDelegateand read one instance property, so the only way to reach it was to build a real window. CI has been paying for that — the class is isolated, run serially, one test permanently skipped, and a retry absorbed because it's timing-sensitive.This moves the matching into a free function you can call directly, and table-tests it.
Summary
Sources/ShortcutRouting.swift(new) —matchStrokeandmatch, bodies moved verbatim. The one change: the layout character provider is a parameter instead ofself.shortcutLayoutCharacterProvider, which is what made them impure.Sources/AppDelegate.swift— the two methods become thin forwarders with identical private signatures. No call site changes. 197 lines out, 30 in.programaTests/ShortcutRoutingTests.swift(new) — 18 table rows plus 2 chord tests. No windows, noRunLoop, noUserDefaults, noKeyboardShortcutSettings.Matrix covered: plain modifier match/mismatch, Return vs keypad Enter, AZERTY digit-symbol ANSI fallback, digit shortcut rejected on the wrong physical key, shifted-digit symbols, Ctrl+H as an ASCII control char, Cmd+Shift+] fallback on a non-US layout, ISO angle-bracket rejection, Russian resolving via the injected provider and its fallback when translation also fails, three Dvorak physical-key cases, and shift-symbol normalization (
?→/,{→[).The old tests stay
Deliberately. They're the safety net proving this move changed nothing — deleting them in the same PR that moves the code would remove the only evidence the move was safe. Retiring them, and unwinding the
split-statefulisolation and retry inscripts/ci-run-unit-tests.sh, is the next slice once these have proven equivalent over some real runs.One judgment call worth a look
Five helpers were only reachable from the moved code. Three had no other callers and moved wholesale. Two —
normalizedShortcutEventCharacteranddigitForNumberKeyCode— are also used bynumberedShortcutDigit, which stays onAppDelegate. Rather than duplicate them into two sources of truth, they becamenonisolated staticin place. They touch no instance state, so dropping the@MainActorisolation is safe, but it's the one non-mechanical decision here.Test plan
xcodebuild -scheme programa→** BUILD SUCCEEDED **(done)xcodebuild -scheme programa-unit build-for-testing→** TEST BUILD SUCCEEDED **, withShortcutRoutingTests.swiftconfirmed in the compile log (done — a file missing from the pbxproj Sources phase silently doesn't compile rather than erroring, so this was checked explicitly)AppDelegateShortcutRoutingTests— if the move broke anything, that suite is what catches it