Repository navigation
test(abstraction-improver): use the SD-card importer seam instead of deleting it — 11 tests over the Import All loop - #437
Conversation
…deleting it — 11 tests over the Import All loop Investigated `ISdCardSessionImporter` and `DeviceLogsViewModel`'s `_importerOverride` / `ResolveImporter()` as a possible twin of the firewall seams deleted in #420: one interface, one implementation, no fake anywhere, and a constructor parameter every existing test passes null to. It is not the same shape. The seam is the only route into `ImportAllFiles`, which holds the decision `SdCardFailure.IsCardUnavailable` exists for — a failure that may be about one file is skipped and the batch carries on, while a failure about the card stops the run. Getting that backwards is silent and expensive in both directions: the listing comes back in the same order every time, so aborting on a per-file fault makes every file after it permanently unreachable through IMPORT ALL, and carrying on past a card-wide one grinds the user through one multi-second failure per remaining file. `ImportAllSummaryTests` covers the other half — how a finished `ImportAllOutcome` is worded — but it builds the outcome by hand, so nothing exercised the loop that fills it. Deleting the seam would have made that loop untestable for good, which is a loss, not a simplification. So this uses it. No production file changes. Covered, via a fake importer on the existing internal constructor: - every listed file is asked for, in order; - a per-file fault (`SdCardTransferErrorException`) is skipped and the batch carries on; - a card-wide fault (`SdCardNotPresentException`) stops it; - a stall ends the batch only for the two reasons that are evidence about the device (`TransferTimeout`, `TransportClosed`) and not for the per-read `NoDataReceived`; - a result carrying no persisted session is a skip, not a stop; - a device that drops mid-batch stops the run at the iteration re-check; - a per-file failure leaves the card panel alone (`HasFiles` requires `SdCardState == Ok`, so painting it would hide the list the batch is still importing into) while a card-wide one paints it; - `ImportFile`'s device snapshot: a failure is not painted onto a device the user switched to while the download was in flight. Mutation-tested. Six deliberate breaks of those decisions each failed at least one test: inverting the `IsCardUnavailable` abort either way, dropping the mid-batch connectivity re-check, widening the panel-painting guard past `IsCardUnavailable`, dropping its `ReferenceEquals(SelectedDevice, device)` half, and reading the live selection in place of `ImportFile`'s snapshot. The success path is deliberately absent, and the test class says so: a persisted import calls `AddImportedSession`, which reaches `LoggingManager.Instance` — a singleton whose constructor resolves off a null `App.ServiceProvider` outside the app host — through `Dispatcher.UIThread.Invoke`, which never returns in a process with no pumped dispatcher. Both are global, neither is injectable, and this project stands up no application lifetime by design. A seventh mutation (dropping the skip path's `continue`) confirmed it: it hangs rather than fails. `RecordingStreamingDevice` gains settable `IsConnected` and `SdCardFiles`, both defaulting to what they returned before. Gate: desktop build, restore, iOS head Release build, `dotnet test` — all green, 1156 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
PR Summary by QodoTest SD-card Import All batch decisions through importer seam
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
…asked for Qodo R1 (Medium): the no-session test checked only that all three files were requested, so dropping outcome.RecordSkip while keeping the continue left it green. The summary dialog is the only place the batch's skip/abort accounting becomes observable, so the internal test-seam constructor now also takes the IMessageBoxService (defaulting to AvaloniaMessageBoxService, as before) and the batch tests read the Import Complete text. Mutations: dropping the no-session RecordSkip fails 3; dropping the catch's RecordSkip fails 1. Both were green before. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit d50ef98 |
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit d50ef98 |
|
Qodo-clean, CI green — ready for review. Head What the builder left. One Qodo round on Fix, Qodo on
Both reviews name Gate, run locally on CI run 35649810309 ran on The three-way merge with #435 and #436 is in the next comment. 🤖 Generated with Claude Code |
Three-way merge:
|
| PR | Ready-note SHA | Head now | CI green on head | CI merge base |
|---|---|---|---|---|
| #435 | 2bacbf3 |
2bacbf3 ✓ |
yes (run 35545404407) | 4716ce1, before #418 #419 #420 #422 #423 #425 #426 #428 #431 #432 |
| #436 | b36c347 |
b36c347 ✓ |
yes (run 35547335403) | 4716ce1, same 10 merges missed |
| #437 | this PR's note above | d50ef98 ✓ |
yes (run 35649810309) | 97be4df = current main |
So the greens for #435 and #436 come from before the last ten merges to main. This merged tree is the build that covers them against current main. It passes on desktop, iOS and tests, but not Android.
Collision audit (what changed across the three PRs):
- Removed or renamed:
- fix(#433): a channel is the same colour on the mobile live plot as it is everywhere else — and the third palette goes away #435 deletes
MobileShellViewModel.Palette, which is private. It also moves the nestedShellTestDeviceandAdoptout ofMobileShellSilentStreamWatchdogTests, into a top-levelinternal Daqifi.Avalonia.Tests.Views.ShellTestDevice.
- fix(#433): a channel is the same colour on the mobile live plot as it is everywhere else — and the third palette goes away #435 deletes
- Converted:
- test(abstraction-improver): use the SD-card importer seam instead of deleting it — 11 tests over the Import All loop #437 changes
DeviceLogsViewModel._messageBoxServicefromstaticto instance. It is private. - test(abstraction-improver): use the SD-card importer seam instead of deleting it — 11 tests over the Import All loop #437 adds a trailing optional parameter to the internal constructor. The only other test caller,
DeviceLogsSelectionTests, passes 3 positional arguments and still binds.
- test(abstraction-improver): use the SD-card importer seam instead of deleting it — 11 tests over the Import All loop #437 changes
- New top-level types:
ShellTestDevice,MobileShellSeriesColorTests,DeleteSessionFailureTests,DeviceLogsImportBatchTests. Each name exists exactly once in the merged tree. - Nested helpers:
RecordingHostappears 4 times,RecordingSessionImporterandRecordingMessageBoxonce each. They are allprivate sealedinside different outer classes, so none of them collide. - None of the three PRs touches a file another one touches.
- No collisions.
🤖 Generated with Claude Code
Sweep: five-way merge of
|
|
Sweep update: the six-way merge of |
The investigation
Looked at
ISdCardSessionImporterandDeviceLogsViewModel's_importerOverride/ResolveImporter()as a possible twin of the firewall bootstrap seams deleted in #420. On the surface it matched: one interface, one implementation, no fake anywhere in the source tree, and a constructor parameter that every existing test passesnullto.It is not the same shape, and deleting it would have been a loss.
The seam is the only route into
ImportAllFiles, which holds the decisionSdCardFailure.IsCardUnavailableexists for: a failure that may be about one file is skipped and the batch carries on, while a failure that is about the card stops the run.SdCardFailureClassifierdocuments at length why that line is drawn where it is — and getting it backwards is silent and expensive in both directions. The listing comes back in the same order every time, so aborting on a per-file fault makes every file listed after it permanently unreachable through IMPORT ALL; carrying on past a card-wide one grinds the user through one multi-second failure per remaining file.ImportAllSummaryTestscovers the other half — how a finishedImportAllOutcomeis worded — but it builds the outcome by hand. Nothing exercised the loop that fills it. Flattening the interface would have made that loop untestable for good.So this PR uses the seam rather than deleting it. One production change, a test seam only: the internal constructor also takes the
IMessageBoxService(null →AvaloniaMessageBoxService, exactly as before; the field moves fromstaticto instance). It is how the tests read the Import All summary, which is the only place the batch's skip/abort accounting becomes observable — added ind50ef98after Qodo R1 showed the no-session row could not see that accounting at all.What is now covered
Through a fake importer on the existing internal constructor:
SdCardTransferErrorException) is skipped and the batch carries onSdCardNotPresentException) stops itTransferTimeout,TransportClosed), not for the per-readNoDataReceived— the case a healthy device produces on a momentary gapImported 0 of 3,Skipped 3, each name listed, guidance once)Import stopped at <file>:and not as a skip; a mid-batch disconnect says soHasFilesrequiresSdCardState == Ok, so painting it would hide the list the batch is still importing into) while a card-wide one paints itImportFile's device snapshot: a failure is not painted onto a device the user switched to while the download was in flightMutation-tested
Eight deliberate breaks, each caught by at least one test:
IsCardUnavailableReferenceEquals(SelectedDevice, device)half of that guard droppedImportFilereads the live selection instead of its snapshotoutcome.RecordSkip(keepscontinue) — green befored50ef98outcome.RecordSkip— green befored50ef98What is deliberately not covered, and why
The success path. A persisted import calls
AddImportedSession, which reachesLoggingManager.Instance— a singleton whose constructor resolves offApp.ServiceProvider, null outside the app host — throughDispatcher.UIThread.Invoke, which never returns in a process with no pumped dispatcher. Both are global, neither is injectable here, and this test project stands up no application lifetime by design (see the comment at the top of its csproj).A seventh mutation confirmed this rather than assuming it: dropping the skip path's
continuepushes execution intoAddImportedSessionand hangs instead of failing. The test class documents the limitation in its remarks, so the next person does not rediscover it by watching CI time out.Covering it honestly would mean injecting
LoggingManagerinto the view model — a production change, and a larger one than this investigation warrants. Flagged rather than done.Notes
RecordingStreamingDevicegains settableIsConnectedandSdCardFiles, both defaulting to exactly what they returned before, so no existing test changes behaviour.SdCardSessionImporter.cs— this PR does not.origin/mainat 04c3c17 (after refactor(logic-simplifier): the duplicate-device check answers with the match, not a flag beside it #425 merged) and re-run.Gate
Desktop build →
restore→ iOS head-c Release -r iossimulator-arm64 --no-restore→dotnet test. All exit 0; 1156 passed, 0 failed — re-run by the shepherd ond50ef98(test count unchanged: the fix adds assertions to existing rows, not new rows).🤖 Generated with Claude Code