From 2f3524fa8a3ce2a05ae12de1ee29393a6a5ca6c0 Mon Sep 17 00:00:00 2001 From: ecgang Date: Wed, 26 Aug 2026 10:15:16 -0600 Subject: [PATCH] Make the launch and panic-wipe tests able to fail Three tests here could pass while the behaviour was wrong. A review pass found all three; the mutation runs below are what settled them. The launch decision had no test at all. Deleting shouldPresentConversationList along with the restorability predicate took the seam with it, leaving `presentation == .conversationList -> showSidebar = true` executed only in production. That one comparison decides whether a public timeline or a conversation list ends up on screen. It comes back as a pure one-case function with both branches asserted. The panic-wipe test asserted only what launch presents, and an earlier version of its comment claimed that distinguished "pointer absent" from "pointer rewritten as .mesh". It does distinguish that one. It does not distinguish a surviving .direct record, which also presents as .conversationList - so a regression where finishPanicWipe stopped deleting the key, while suppression kept working, would leave the peer id on disk and the test would stay green. For a panic wipe that is the whole feature. It now reads the stored key directly. Proved rather than assumed: with the deletion removed from finishPanicWipe, exactly one of nine tests fails, on the new XCTAssertNil (ConversationStoreLastActiveTests.swift:162, "45 bytes"). The presentation assertion passes against that mutant, which is the point - it could not have caught it. The chrome panic test asserted one arm of isPeopleSheetPresented while its own comment named both. It now selects a private peer first and asserts the selection is cleared too, so it covers the cross-model invariant it claimed rather than the half this PR happens to own. 9 XCTest + 31 swift-testing, 0 failures, exit 0 (macOS). Co-Authored-By: Claude Opus 5 (1M context) --- bitchat/App/AppRuntime.swift | 18 +++++++++- bitchatTests/AppArchitectureTests.swift | 7 ++++ .../ConversationStoreLastActiveTests.swift | 34 ++++++++++++++++--- 3 files changed, 53 insertions(+), 6 deletions(-) diff --git a/bitchat/App/AppRuntime.swift b/bitchat/App/AppRuntime.swift index d21d0507..15b8e88a 100644 --- a/bitchat/App/AppRuntime.swift +++ b/bitchat/App/AppRuntime.swift @@ -171,11 +171,27 @@ final class AppRuntime: ObservableObject { /// recent-chats section, so the last conversation is one tap away rather /// than lost. private func restoreLastActiveConversationOnLaunch() { - if conversations.restoreLastActiveConversation() == .conversationList { + if Self.shouldPresentConversationList(for: conversations.restoreLastActiveConversation()) { appChromeModel.showSidebar = true } } + /// The launch decision itself, split out as a pure function so the mapping + /// is testable without standing up an `AppRuntime`. It is one comparison, + /// but it is the comparison that decides whether a public timeline or a + /// list ends up on screen, and inverting it silently is exactly the kind of + /// mistake the store-level tests cannot see. + static func shouldPresentConversationList( + for presentation: ConversationStore.LaunchPresentation + ) -> Bool { + switch presentation { + case .conversationList: + return true + case .deferToChannelRestore: + return false + } + } + /// Drops media that has outlived the retention window, then applies the /// explicit protection class to files that older builds wrote without /// one. Expiry runs first so the migration never touches files the diff --git a/bitchatTests/AppArchitectureTests.swift b/bitchatTests/AppArchitectureTests.swift index 270ad76d..ca4fd621 100644 --- a/bitchatTests/AppArchitectureTests.swift +++ b/bitchatTests/AppArchitectureTests.swift @@ -506,6 +506,12 @@ struct AppArchitectureTests { // left set covers the root timeline exactly like the others. Launch // restore (#1064) can raise it without the person touching anything. chromeModel.showSidebar = true + // The OTHER arm of `isPeopleSheetPresented`. Asserting only the latch + // would leave the test green while a selected DM kept the sheet over + // the wipe result — the selection is cleared by pre-existing code in + // ChatViewModel.panicClearAllData, so this pins a cross-model + // invariant that neither half owns alone. + viewModel.startPrivateChat(with: PeerID(str: "1122334455667788")) chromeModel.panicClearAllData() @@ -514,6 +520,7 @@ struct AppArchitectureTests { #expect(!chromeModel.isNoticesSheetPresented) #expect(chromeModel.showingFingerprintFor == nil) #expect(!chromeModel.showSidebar) + #expect(viewModel.selectedPrivateChatPeer == nil) } @Test("Recent chats list direct conversations with people absent from the rosters") diff --git a/bitchatTests/ConversationStoreLastActiveTests.swift b/bitchatTests/ConversationStoreLastActiveTests.swift index b67bc992..d04d6908 100644 --- a/bitchatTests/ConversationStoreLastActiveTests.swift +++ b/bitchatTests/ConversationStoreLastActiveTests.swift @@ -135,9 +135,15 @@ final class ConversationStoreLastActiveTests: XCTestCase { // beginPanicWipe → setSelectedPrivatePeer(nil) → setActiveChannel(.mesh) → finishPanicWipe // Without the fix those resets re-persist a `.mesh` record, and the next // launch would `.deferToChannelRestore` instead of `.conversationList`. - // That is what makes this assertion load-bearing rather than trivially - // true: `.conversationList` is also the no-record answer, so the test - // distinguishes "pointer absent" from "pointer rewritten as .mesh". + // + // The presentation assertion alone is NOT enough, and the earlier + // version of this comment overclaimed. `.conversationList` is the + // answer for an absent record AND for a surviving `.direct` one, so a + // regression where `finishPanicWipe` stopped removing the key while + // suppression still worked would leave the peer id on disk and this + // test would stay green. For a panic wipe that is the whole point of + // the feature, so assert the stored key is gone, not just that launch + // looks right. let storage = makeStorage() let session = ConversationStore(storage: storage) @@ -149,14 +155,32 @@ final class ConversationStoreLastActiveTests: XCTestCase { session.setActiveChannel(.mesh) // re-persist trigger #2 (suppressed) session.finishPanicWipe() // removes the pointer once - // A fresh store on the SAME storage finds NO record and falls back to - // the conversation list — the pointer is truly absent, not `.mesh`. + // The pointer is truly absent, not rewritten as `.mesh` and not left + // behind as `.direct`. Read the key directly: this is the only + // assertion that separates "erased" from "still on disk", since both + // present as `.conversationList` at launch. + XCTAssertNil(storage.data(forKey: "conversation.lastActive")) + + // And launch presents the list rather than deferring to a resurrected + // `.mesh` record — the suppression half of the same fix. XCTAssertEqual( ConversationStore(storage: storage).restoreLastActiveConversation(), .conversationList ) } + // MARK: - Launch decision + + func test_launchPresentsTheListForAConversationListPresentation() { + XCTAssertTrue(AppRuntime.shouldPresentConversationList(for: .conversationList)) + } + + func test_launchLeavesAPublicChannelRestoreAlone() { + // Inverting this is how a geohash restore would end up buried under the + // people sheet, or a DM launch would land on the public timeline. + XCTAssertFalse(AppRuntime.shouldPresentConversationList(for: .deferToChannelRestore)) + } + func test_firstLaunchPresentsConversationList() { let storage = makeStorage()