Summary
SoundManager.shared is reached directly from the code paths that decide whether a notification sound fires, and it has no injection point or observable record of what it played. The result is that no sound behaviour anywhere in the app can be asserted in a test — it can only be reasoned about by reading.
This is not a defect on its own, but it is a coverage hole in a decision that has already drifted once without anyone noticing.
Why it matters
The "should this event make a sound" decision is not trivial, and it is not the same question as "should a card open". In handlePermissionRequest the two conditions were folded into a single if for a long time:
if permissionQueue.count == 1 {
...open the card...
SoundManager.shared.handleEvent("PermissionRequest")
}count == 1 happens to mean "this request starts a new burst", which is right for the sound and only accidentally right for the card. Any change to the card condition silently changes the sound's meaning too — e.g. making a burst of five queued approvals play five sounds instead of one. There is no test anywhere that would catch that, because there is nothing to assert against.
The same singleton pattern is used from the other event paths, so this applies to completion and question sounds as well.
Suggested fix
A minimal seam is enough — no framework, no protocol proliferation:
- give
SoundManager an injectable handler (or make handleEvent a closure property that defaults to the real implementation), and
- let tests substitute a recorder that appends the event names it receives.
That makes assertions like "a burst of three queued approvals plays one sound" and "a suppressed card still/never plays" expressible. Today they are not.
Context
Found while working on #309. The fix there had to separate the card condition from the sound condition, and the sound half is the one that ships with no test coverage — not for lack of trying, but because writing that test means adding this seam, which is a larger and more invasive change than the bug fix it would be attached to. Filing it separately rather than smuggling it in.
Summary
SoundManager.sharedis reached directly from the code paths that decide whether a notification sound fires, and it has no injection point or observable record of what it played. The result is that no sound behaviour anywhere in the app can be asserted in a test — it can only be reasoned about by reading.This is not a defect on its own, but it is a coverage hole in a decision that has already drifted once without anyone noticing.
Why it matters
The "should this event make a sound" decision is not trivial, and it is not the same question as "should a card open". In
handlePermissionRequestthe two conditions were folded into a singleiffor a long time:count == 1happens to mean "this request starts a new burst", which is right for the sound and only accidentally right for the card. Any change to the card condition silently changes the sound's meaning too — e.g. making a burst of five queued approvals play five sounds instead of one. There is no test anywhere that would catch that, because there is nothing to assert against.The same singleton pattern is used from the other event paths, so this applies to completion and question sounds as well.
Suggested fix
A minimal seam is enough — no framework, no protocol proliferation:
SoundManageran injectable handler (or makehandleEventa closure property that defaults to the real implementation), andThat makes assertions like "a burst of three queued approvals plays one sound" and "a suppressed card still/never plays" expressible. Today they are not.
Context
Found while working on #309. The fix there had to separate the card condition from the sound condition, and the sound half is the one that ships with no test coverage — not for lack of trying, but because writing that test means adding this seam, which is a larger and more invasive change than the bug fix it would be attached to. Filing it separately rather than smuggling it in.