From be0e699fcc7fdce61ca475e9220c49962914eb28 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Fri, 3 Jul 2026 14:54:12 -0500 Subject: [PATCH 1/2] =?UTF-8?q?test(coverage):=20lane=202=20=E2=80=94=20sy?= =?UTF-8?q?nc,=20workers,=20transport=20&=20auth=20to=20>=3D95%?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Test-only (zero production changes). Raises JVM unit-test LINE coverage for the sync/worker, IMAP/SMTP/Graph transport, and OAuth packages: data/sync 98.6%, mail 96.7%, auth 100.0% LINE. New/extended cover: - SendWorker outbox drain (SMTP/Graph, may-have-sent, SMTP fallback, staged attachments), MailConnectionFactory token cache/refresh, MailSyncer.syncAll, SendScheduler, MailBackfiller pre-existing-row refresh. - ImapConnectionCache reuse + drop-retry, ImapClient fetchAttachment/setFlag/ deleteMessage/idle + edge cases, GraphSender.send transport. - OutlookAuthManager token exchange/refresh + failure branches, OAuth models. Instruction/branch coverage stays lower (coroutine suspend-state synthetics under synchronous mocks) — a known JaCoCo x coroutines limitation, not untested logic; JaCoCo config is untouched (owned by the capstone lane). Co-Authored-By: Claude Opus 4.8 --- .../org/libremail/auth/OAuthModelsTest.kt | 32 ++ .../libremail/auth/OutlookAuthManagerTest.kt | 286 ++++++++++++++++++ .../libremail/data/sync/MailBackfillerTest.kt | 21 ++ .../data/sync/MailConnectionFactoryTest.kt | 175 +++++++++++ .../org/libremail/data/sync/MailSyncerTest.kt | 84 +++++ .../libremail/data/sync/SendSchedulerTest.kt | 41 +++ .../org/libremail/data/sync/SendWorkerTest.kt | 223 +++++++++++++- .../org/libremail/mail/GraphSenderSendTest.kt | 138 +++++++++ .../org/libremail/mail/ImapClientTest.kt | 154 ++++++++++ .../libremail/mail/ImapConnectionCacheTest.kt | 140 +++++++++ 10 files changed, 1283 insertions(+), 11 deletions(-) create mode 100644 app/src/test/kotlin/org/libremail/auth/OAuthModelsTest.kt create mode 100644 app/src/test/kotlin/org/libremail/auth/OutlookAuthManagerTest.kt create mode 100644 app/src/test/kotlin/org/libremail/data/sync/MailConnectionFactoryTest.kt create mode 100644 app/src/test/kotlin/org/libremail/data/sync/SendSchedulerTest.kt create mode 100644 app/src/test/kotlin/org/libremail/mail/GraphSenderSendTest.kt create mode 100644 app/src/test/kotlin/org/libremail/mail/ImapConnectionCacheTest.kt diff --git a/app/src/test/kotlin/org/libremail/auth/OAuthModelsTest.kt b/app/src/test/kotlin/org/libremail/auth/OAuthModelsTest.kt new file mode 100644 index 0000000..d19e153 --- /dev/null +++ b/app/src/test/kotlin/org/libremail/auth/OAuthModelsTest.kt @@ -0,0 +1,32 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.auth + +import org.junit.Test +import kotlin.test.assertEquals +import kotlin.test.assertNotEquals +import kotlin.test.assertNull + +/** Value-semantics cover for the two OAuth result carriers. */ +class OAuthModelsTest { + + @Test + fun `OAuthResult exposes its fields and value semantics`() { + val result = OAuthResult(email = "me@example.com", accessToken = "at", authStateJson = "{json}") + + assertEquals("me@example.com", result.email) + assertEquals("at", result.accessToken) + assertEquals("{json}", result.authStateJson) + assertEquals(result, result.copy()) + assertNotEquals(result, result.copy(accessToken = "other")) + } + + @Test + fun `FreshToken defaults its expiry to null and supports copy`() { + val fresh = FreshToken(accessToken = "at", authStateJson = "{json}") + + assertNull(fresh.accessTokenExpiry) + assertEquals(4_200L, fresh.copy(accessTokenExpiry = 4_200L).accessTokenExpiry) + assertEquals("at", fresh.accessToken) + assertEquals(fresh, FreshToken("at", "{json}")) + } +} diff --git a/app/src/test/kotlin/org/libremail/auth/OutlookAuthManagerTest.kt b/app/src/test/kotlin/org/libremail/auth/OutlookAuthManagerTest.kt new file mode 100644 index 0000000..f7c874e --- /dev/null +++ b/app/src/test/kotlin/org/libremail/auth/OutlookAuthManagerTest.kt @@ -0,0 +1,286 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.auth + +import android.content.Context +import android.content.Intent +import android.content.pm.PackageManager +import android.net.Uri +import android.text.TextUtils +import android.util.Base64 +import io.mockk.every +import io.mockk.just +import io.mockk.mockk +import io.mockk.mockkConstructor +import io.mockk.mockkStatic +import io.mockk.runs +import io.mockk.unmockkAll +import kotlinx.coroutines.test.runTest +import net.openid.appauth.AuthState +import net.openid.appauth.AuthorizationException +import net.openid.appauth.AuthorizationRequest +import net.openid.appauth.AuthorizationResponse +import net.openid.appauth.AuthorizationService +import net.openid.appauth.AuthorizationServiceConfiguration +import net.openid.appauth.ResponseTypeValues +import net.openid.appauth.TokenRequest +import net.openid.appauth.TokenResponse +import org.json.JSONObject +import org.junit.After +import org.junit.Test +import org.libremail.BuildConfig +import kotlin.test.assertEquals +import kotlin.test.assertFailsWith + +/** + * Drives the Outlook/Microsoft OAuth wrapper in a pure JVM test. The AppAuth types it builds + * (requests, responses, token responses) carry data in final fields, so those are constructed for + * real; only the boundaries are faked — the Android statics AppAuth reaches for ([Uri], [Base64], + * [TextUtils], [Intent]), the browser-less [PackageManager] that lets [AuthorizationService] + * construct, and the token-endpoint call itself (its constructor is mocked, the callback invoked with + * a canned [TokenResponse]). This pins the two-resource token exchange (Exchange token minted from the + * auth-code grant), the email extraction from the id_token (with its preferred_username fallback), the + * refresh paths, and every failure branch — none of which had unit cover before. + */ +class OutlookAuthManagerTest { + + @After + fun tearDown() = unmockkAll() + + @Test + fun `isConfigured reflects whether a client id ships with the build`() { + installStatics() + assertEquals(BuildConfig.OUTLOOK_OAUTH_CLIENT_ID.isNotBlank(), authManager().isConfigured) + } + + @Test + fun `createAuthIntent builds and returns the AppAuth sign-in intent`() { + installStatics() + mockkConstructor(AuthorizationService::class) + every { anyConstructed().dispose() } just runs + val intent = mockk() + every { anyConstructed().getAuthorizationRequestIntent(any()) } returns intent + + assertEquals(intent, authManager().createAuthIntent()) + } + + @Test + fun `exchangeToken mints an Exchange token and reads the account email from the id_token`() = runTest { + installStatics() + stubResponse(authResponseWithCode("auth-code")) + stubTokenRequests( + tokenResponse(access = "code-access", idToken = jwt("email" to "me@example.com"), refresh = "rt"), + tokenResponse(access = "outlook-access", idToken = null, refresh = "rt"), + ) + + val result = authManager().exchangeToken(mockk()) + + assertEquals("me@example.com", result.email) + assertEquals("outlook-access", result.accessToken) // the Exchange-scoped token, not the code one + } + + @Test + fun `exchangeToken falls back to preferred_username when the email claim is blank`() = runTest { + installStatics() + stubResponse(authResponseWithCode("auth-code")) + stubTokenRequests( + tokenResponse("code-access", jwt("email" to "", "preferred_username" to "alt@example.com"), "rt"), + tokenResponse("outlook-access", null, "rt"), + ) + + assertEquals("alt@example.com", authManager().exchangeToken(mockk()).email) + } + + @Test + fun `exchangeToken throws when the authorization was cancelled`() = runTest { + installStatics() + mockkStatic(AuthorizationResponse::class) + every { AuthorizationResponse.fromIntent(any()) } returns null + mockkStatic(AuthorizationException::class) + every { AuthorizationException.fromIntent(any()) } returns null + + assertFailsWith { authManager().exchangeToken(mockk()) } + } + + @Test + fun `exchangeToken throws when no authorization code was returned`() = runTest { + installStatics() + stubResponse(authResponseWithCode(null)) // a response with no code + mockkConstructor(AuthorizationService::class) + every { anyConstructed().dispose() } just runs + + assertFailsWith { authManager().exchangeToken(mockk()) } + } + + @Test + fun `exchangeToken throws when the id_token carries no usable email`() = runTest { + installStatics() + stubResponse(authResponseWithCode("auth-code")) + stubTokenRequests(tokenResponse("code-access", jwt(), "rt")) // empty claims -> no email + + assertFailsWith { authManager().exchangeToken(mockk()) } + } + + @Test + fun `exchangeToken throws on an unparseable id_token`() = runTest { + installStatics() + stubResponse(authResponseWithCode("auth-code")) + stubTokenRequests(tokenResponse("code-access", idToken = "not-a-jwt", refresh = "rt")) + + assertFailsWith { authManager().exchangeToken(mockk()) } + } + + @Test + fun `freshOutlookToken refreshes and returns the Exchange access token`() = runTest { + installStatics() + stubDeserializedAuthState(refreshToken = "rt") + stubTokenRequests(tokenResponse("exchange-at", idToken = null, refresh = "rt")) + + val fresh = authManager().freshOutlookToken("{stored}") + + assertEquals("exchange-at", fresh.accessToken) + assertEquals("{serialized}", fresh.authStateJson) + } + + @Test + fun `freshGraphToken refreshes and returns the Graph access token`() = runTest { + installStatics() + stubDeserializedAuthState(refreshToken = "rt") + stubTokenRequests(tokenResponse("graph-at", idToken = null, refresh = "rt")) + + assertEquals("graph-at", authManager().freshGraphToken("{stored}").accessToken) + } + + @Test + fun `a refresh without a stored refresh token asks the user to sign in again`() = runTest { + installStatics() + stubDeserializedAuthState(refreshToken = null) + mockkConstructor(AuthorizationService::class) + every { anyConstructed().dispose() } just runs + + assertFailsWith { authManager().freshGraphToken("{stored}") } + } + + @Test + fun `exchangeToken surfaces a token endpoint failure`() = runTest { + installStatics() + stubResponse(authResponseWithCode("auth-code")) + stubFailingTokenRequest() + + assertFailsWith { authManager().exchangeToken(mockk()) } + } + + @Test + fun `a refresh surfaces a token endpoint failure`() = runTest { + installStatics() + stubDeserializedAuthState(refreshToken = "rt") + stubFailingTokenRequest() + + assertFailsWith { authManager().freshGraphToken("{stored}") } + } + + // --- test fixtures ----------------------------------------------------------------------------- + + private fun authManager() = OutlookAuthManager(androidContext()) + + /** Installs the Android statics AppAuth touches so its real objects can be built off-device. */ + private fun installStatics() { + mockkStatic(TextUtils::class) + every { TextUtils.isEmpty(any()) } answers { (firstArg()?.length ?: 0) == 0 } + every { TextUtils.join(any(), any>()) } answers { + secondArg>().joinToString(firstArg().toString()) + } + mockkStatic(Uri::class) + val uri = mockk(relaxed = true) + every { uri.scheme } returns "org.libremail" + every { Uri.parse(any()) } returns uri + every { Uri.fromParts(any(), any(), any()) } returns uri + mockkStatic(Base64::class) + every { Base64.encodeToString(any(), any()) } answers { + java.util.Base64.getUrlEncoder().withoutPadding().encodeToString(firstArg()) + } + every { Base64.decode(any(), any()) } answers { + java.util.Base64.getUrlDecoder().decode(firstArg()) + } + // BrowserSelector's static init builds a probe Intent; keep its fluent chain from touching stubs. + mockkConstructor(Intent::class) + every { anyConstructed().setAction(any()) } answers { self as Intent } + every { anyConstructed().addCategory(any()) } answers { self as Intent } + every { anyConstructed().setData(any()) } answers { self as Intent } + } + + /** A context whose PackageManager reports no browsers, so AuthorizationService constructs cleanly. */ + private fun androidContext(): Context { + val pm = mockk(relaxed = true) + every { pm.resolveActivity(any(), any()) } returns null + every { pm.queryIntentActivities(any(), any()) } returns emptyList() + return mockk(relaxed = true).also { + every { it.packageManager } returns pm + every { it.applicationContext } returns it + } + } + + private val config get() = AuthorizationServiceConfiguration(Uri.parse("authorize"), Uri.parse("token")) + + private fun stubResponse(response: AuthorizationResponse) { + mockkStatic(AuthorizationResponse::class) + every { AuthorizationResponse.fromIntent(any()) } returns response + mockkStatic(AuthorizationException::class) + every { AuthorizationException.fromIntent(any()) } returns null + } + + private fun authResponseWithCode(code: String?): AuthorizationResponse { + val request = AuthorizationRequest.Builder(config, "client", ResponseTypeValues.CODE, Uri.parse("redirect")) + .setScope("openid") + .build() + return AuthorizationResponse.Builder(request).apply { code?.let { setAuthorizationCode(it) } }.build() + } + + /** Mocks the token-endpoint call, handing back [responses] in order to each performTokenRequest. */ + private fun stubTokenRequests(vararg responses: TokenResponse) { + mockkConstructor(AuthorizationService::class) + every { anyConstructed().dispose() } just runs + val queue = ArrayDeque(responses.toList()) + every { anyConstructed().performTokenRequest(any(), any()) } answers { + secondArg().onTokenRequestCompleted(queue.removeFirst(), null) + } + } + + /** Mocks the token endpoint to report failure (null response, null exception) to its callback. */ + private fun stubFailingTokenRequest() { + mockkConstructor(AuthorizationService::class) + every { anyConstructed().dispose() } just runs + every { anyConstructed().performTokenRequest(any(), any()) } answers { + secondArg().onTokenRequestCompleted(null, null) + } + } + + /** Makes AuthState.jsonDeserialize hand back a mock carrying [refreshToken]. */ + private fun stubDeserializedAuthState(refreshToken: String?) { + val authState = mockk(relaxed = true) + every { authState.refreshToken } returns refreshToken + every { authState.update(any(), any()) } just runs + every { authState.jsonSerializeString() } returns "{serialized}" + mockkStatic(AuthState::class) + every { AuthState.jsonDeserialize(any()) } returns authState + } + + private fun tokenResponse(access: String, idToken: String?, refresh: String?): TokenResponse { + val request = TokenRequest.Builder(config, "client") + .setGrantType("authorization_code") + .setAuthorizationCode("code") + .setRedirectUri(Uri.parse("redirect")) + .build() + return TokenResponse.Builder(request) + .setAccessToken(access) + .setIdToken(idToken) + .setRefreshToken(refresh) + .setAccessTokenExpirationTime(System.currentTimeMillis() + 3_600_000L) + .build() + } + + private fun jwt(vararg claims: Pair): String { + val payload = java.util.Base64.getUrlEncoder().withoutPadding() + .encodeToString(JSONObject(mapOf(*claims)).toString().toByteArray()) + return "header.$payload.signature" + } +} diff --git a/app/src/test/kotlin/org/libremail/data/sync/MailBackfillerTest.kt b/app/src/test/kotlin/org/libremail/data/sync/MailBackfillerTest.kt index 7cbd260..c145cd1 100644 --- a/app/src/test/kotlin/org/libremail/data/sync/MailBackfillerTest.kt +++ b/app/src/test/kotlin/org/libremail/data/sync/MailBackfillerTest.kt @@ -332,6 +332,27 @@ class MailBackfillerTest { coVerify(exactly = 0) { imapClient.fetchOlderThan(any(), any(), match { it <= 1L }, any()) } } + /** + * A backfilled page whose ids already exist (e.g. former search-only rows) must be *refreshed* + * (markSynced + header update), not just IGNORE-inserted — this covers persistBatch's + * pre-existing-row branch, which the all-brand-new happy paths above never hit. + */ + @Test + fun `re-inserting a pre-existing header refreshes it rather than only inserting`() = runTest { + appendMessages(60) + seedForegroundWindow() + val backfiller = backfiller(AccountSettings("acct")) + // Report every offered id as already present, so persistBatch takes the refresh branch. + coEvery { lastMessageDao!!.existingIds(any()) } answers { firstArg() } + + backfiller.runBackfill() + + coVerify(atLeast = 1) { lastMessageDao!!.markSynced(any()) } + coVerify(atLeast = 1) { + lastMessageDao!!.updateHeaderContent(any(), any(), any(), any(), any(), any()) + } + } + private fun fetchedMessage(uid: String) = FetchedMessage( uid = uid, sender = "Sender", diff --git a/app/src/test/kotlin/org/libremail/data/sync/MailConnectionFactoryTest.kt b/app/src/test/kotlin/org/libremail/data/sync/MailConnectionFactoryTest.kt new file mode 100644 index 0000000..5256161 --- /dev/null +++ b/app/src/test/kotlin/org/libremail/data/sync/MailConnectionFactoryTest.kt @@ -0,0 +1,175 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.data.sync + +import io.mockk.coEvery +import io.mockk.coVerify +import io.mockk.every +import io.mockk.mockk +import kotlinx.coroutines.flow.flowOf +import kotlinx.coroutines.test.runTest +import org.junit.Test +import org.libremail.auth.FreshToken +import org.libremail.auth.OutlookAuthManager +import org.libremail.data.security.CredentialStore +import org.libremail.data.settings.AppSettings +import org.libremail.data.settings.SettingsRepository +import org.libremail.domain.model.Account +import org.libremail.domain.model.AuthType +import org.libremail.domain.model.MailSecurity +import org.libremail.domain.model.ServerConfig +import kotlin.test.assertEquals +import kotlin.test.assertFailsWith +import kotlin.test.assertFalse +import kotlin.test.assertTrue + +/** + * [MailConnectionFactory] turns a stored credential into connection params, refreshing (and caching) + * OAuth access tokens on demand. These tests pin the password path, the OAuth refresh-and-cache + * behaviour (a still-valid token is reused; an expired or unknown-expiry one is refreshed), the + * persist-only-when-changed rule, and the missing-credential errors — all without a real network. + */ +class MailConnectionFactoryTest { + + private val credentialStore = mockk(relaxed = true) + private val outlookAuthManager = mockk() + private val settingsRepository = mockk { + every { settings } returns flowOf(AppSettings()) + } + + private fun factory() = MailConnectionFactory(credentialStore, outlookAuthManager, settingsRepository) + + private val passwordAccount = Account( + id = "acct", + email = "a@example.org", + displayName = "A", + authType = AuthType.PASSWORD_IMAP, + imap = ServerConfig("imap.example.org", 993, MailSecurity.SSL_TLS), + smtp = ServerConfig("smtp.example.org", 465, MailSecurity.SSL_TLS), + ) + + private val outlookAccount = Account.outlook("me@example.com") + + private fun token(access: String, json: String, expiry: Long?) = + FreshToken(accessToken = access, authStateJson = json, accessTokenExpiry = expiry) + + private val future get() = System.currentTimeMillis() + 3_600_000L + + @Test + fun `password imapParams resolve the stored secret and never use XOAUTH2`() = runTest { + coEvery { credentialStore.loadSecret("acct") } returns "app-password" + + val params = factory().imapParamsFor(passwordAccount) + + assertEquals("app-password", params.secret) + assertEquals("a@example.org", params.username) + assertFalse(params.useXoauth2) + assertTrue(params.strictStartTls) // allowStartTls defaults false -> strict + } + + @Test + fun `password smtpParams resolve the stored secret`() = runTest { + coEvery { credentialStore.loadSecret("acct") } returns "app-password" + + val params = factory().smtpParamsFor(passwordAccount) + + assertEquals("app-password", params.secret) + assertFalse(params.useXoauth2) + } + + @Test + fun `a missing password credential is a hard error`() = runTest { + coEvery { credentialStore.loadSecret("acct") } returns null + + assertFailsWith { factory().imapParamsFor(passwordAccount) } + } + + @Test + fun `allowing STARTTLS relaxes the strict flag`() = runTest { + every { settingsRepository.settings } returns flowOf(AppSettings(allowStartTls = true)) + coEvery { credentialStore.loadSecret("acct") } returns "pw" + + assertFalse(factory().imapParamsFor(passwordAccount).strictStartTls) + } + + @Test + fun `outlook imapParams mint an Exchange token and mark XOAUTH2`() = runTest { + coEvery { credentialStore.loadSecret(outlookAccount.id) } returns "stored" + coEvery { outlookAuthManager.freshOutlookToken("stored") } returns token("outlook-at", "refreshed", future) + + val params = factory().imapParamsFor(outlookAccount) + + assertEquals("outlook-at", params.secret) + assertTrue(params.useXoauth2) + // The AuthState changed, so the refreshed one is persisted for next time. + coVerify { credentialStore.saveSecret(outlookAccount.id, "refreshed") } + } + + @Test + fun `an unchanged AuthState is not re-persisted`() = runTest { + coEvery { credentialStore.loadSecret(outlookAccount.id) } returns "stored" + coEvery { outlookAuthManager.freshOutlookToken("stored") } returns token("outlook-at", "stored", future) + + factory().imapParamsFor(outlookAccount) + + coVerify(exactly = 0) { credentialStore.saveSecret(any(), any()) } + } + + @Test + fun `a still-valid cached token is reused without a second refresh`() = runTest { + coEvery { credentialStore.loadSecret(outlookAccount.id) } returns "stored" + coEvery { outlookAuthManager.freshOutlookToken("stored") } returns token("outlook-at", "stored", future) + val factory = factory() + + factory.imapParamsFor(outlookAccount) + val second = factory.imapParamsFor(outlookAccount) + + assertEquals("outlook-at", second.secret) + coVerify(exactly = 1) { outlookAuthManager.freshOutlookToken(any()) } + } + + @Test + fun `an expired cached token forces a refresh`() = runTest { + coEvery { credentialStore.loadSecret(outlookAccount.id) } returns "stored" + val past = System.currentTimeMillis() - 1_000L + coEvery { outlookAuthManager.freshOutlookToken("stored") } returns token("outlook-at", "stored", past) + val factory = factory() + + factory.imapParamsFor(outlookAccount) + factory.imapParamsFor(outlookAccount) + + coVerify(exactly = 2) { outlookAuthManager.freshOutlookToken(any()) } + } + + @Test + fun `an unknown expiry is never trusted from cache`() = runTest { + coEvery { credentialStore.loadSecret(outlookAccount.id) } returns "stored" + coEvery { outlookAuthManager.freshOutlookToken("stored") } returns token("outlook-at", "stored", null) + val factory = factory() + + factory.imapParamsFor(outlookAccount) + factory.imapParamsFor(outlookAccount) + + coVerify(exactly = 2) { outlookAuthManager.freshOutlookToken(any()) } + } + + @Test + fun `graphToken mints a Graph-scoped token cached separately from the Exchange one`() = runTest { + coEvery { credentialStore.loadSecret(outlookAccount.id) } returns "stored" + coEvery { outlookAuthManager.freshGraphToken("stored") } returns token("graph-at", "stored", future) + coEvery { outlookAuthManager.freshOutlookToken("stored") } returns token("outlook-at", "stored", future) + val factory = factory() + + assertEquals("graph-at", factory.graphTokenFor(outlookAccount)) + // The Exchange scope has its own cache slot, so it still refreshes independently. + assertEquals("outlook-at", factory.imapParamsFor(outlookAccount).secret) + coVerify(exactly = 1) { outlookAuthManager.freshGraphToken(any()) } + coVerify(exactly = 1) { outlookAuthManager.freshOutlookToken(any()) } + } + + @Test + fun `a missing OAuth credential is a hard error`() = runTest { + coEvery { credentialStore.loadSecret(outlookAccount.id) } returns null + + assertFailsWith { factory().graphTokenFor(outlookAccount) } + } +} diff --git a/app/src/test/kotlin/org/libremail/data/sync/MailSyncerTest.kt b/app/src/test/kotlin/org/libremail/data/sync/MailSyncerTest.kt index 1574f54..03f9beb 100644 --- a/app/src/test/kotlin/org/libremail/data/sync/MailSyncerTest.kt +++ b/app/src/test/kotlin/org/libremail/data/sync/MailSyncerTest.kt @@ -28,7 +28,9 @@ import org.libremail.mail.ImapClient import org.libremail.notifications.MailNotifier import org.libremail.power.BatteryStatus import org.libremail.power.BatteryStatusProvider +import java.io.IOException import kotlin.test.assertEquals +import kotlin.test.assertTrue class MailSyncerTest { @@ -257,6 +259,88 @@ class MailSyncerTest { ) } + @Test + fun `syncAll succeeds with a zero total when there are no accounts`() = runTest { + val syncer = syncAllSyncer(accounts = emptyList()) + + val result = syncer.syncAll() + + assertEquals(0, result.getOrNull()) + } + + @Test + fun `syncAll syncs every account inbox and sums the fetched counts`() = runTest { + val syncer = syncAllSyncer(accounts = listOf(accountEntity("one"), accountEntity("two"))) + + val result = syncer.syncAll() + + assertTrue(result.isSuccess) + assertEquals(2, result.getOrNull()) // one message fetched per account + } + + @Test + fun `syncAll still succeeds when one account fails but another syncs`() = runTest { + val syncer = syncAllSyncer( + accounts = listOf(accountEntity("ok"), accountEntity("bad")), + failingIds = setOf("bad"), + ) + + val result = syncer.syncAll() + + assertTrue(result.isSuccess, "at least one account synced") + assertEquals(1, result.getOrNull()) + } + + @Test + fun `syncAll fails only when every account fails`() = runTest { + val syncer = syncAllSyncer( + accounts = listOf(accountEntity("bad1"), accountEntity("bad2")), + failingIds = setOf("bad1", "bad2"), + ) + + assertTrue(syncer.syncAll().isFailure) + } + + private fun accountEntity(id: String) = account.copy(id = id, email = "$id@example.org") + + /** + * A syncer for the [MailSyncer.syncAll] path: [accountDao.getAll] returns [accounts], each inbox + * fetch yields one message (so a successful account contributes 1 to the total), and any account + * in [failingIds] fails its connection so its per-account sync errors out. + */ + private fun syncAllSyncer(accounts: List, failingIds: Set = emptySet()): MailSyncer { + val accountDao = mockk() + coEvery { accountDao.getAll() } returns accounts + val messageDao = mockk(relaxed = true) + coEvery { messageDao.getSyncedIds(any(), any()) } returns emptyList() + coEvery { messageDao.getUnfetchedIds(any(), any()) } returns emptyList() + val imapClient = mockk() + coEvery { imapClient.fetchRecent(any(), any(), any()) } returns listOf( + FetchedMessage("1", "Ada", "ada@example.org", "Hi", 1_000L, isRead = true, isFlagged = false), + ) + val connectionFactory = mockk() + coEvery { connectionFactory.imapParamsFor(match { it.id in failingIds }) } throws IOException("no network") + coEvery { connectionFactory.imapParamsFor(match { it.id !in failingIds }) } returns mockk() + val settingsRepository = mockk() + coEvery { settingsRepository.fetchPolicy() } returns FetchPolicy.ON_DEMAND + coEvery { settingsRepository.isNewMailNotificationsEnabled() } returns false + every { settingsRepository.settings } returns flowOf(AppSettings()) + val accountSettingsRepository = mockk() + coEvery { accountSettingsRepository.get(any()) } returns AccountSettings("acct") + return MailSyncer( + context = mockk(relaxed = true), + accountDao = accountDao, + messageDao = messageDao, + imapClient = imapClient, + connectionFactory = connectionFactory, + settingsRepository = settingsRepository, + accountSettingsRepository = accountSettingsRepository, + batteryStatusProvider = batteryProvider(BatteryStatus(percent = 100, isCharging = false)), + notifier = mockk(relaxed = true), + mailRepository = mockk(relaxed = true), + ) + } + /** A context whose active network reports the given metered state via ConnectivityManager. */ private fun networkContext(unmetered: Boolean): Context { val capabilities = mockk() diff --git a/app/src/test/kotlin/org/libremail/data/sync/SendSchedulerTest.kt b/app/src/test/kotlin/org/libremail/data/sync/SendSchedulerTest.kt new file mode 100644 index 0000000..8a99dbf --- /dev/null +++ b/app/src/test/kotlin/org/libremail/data/sync/SendSchedulerTest.kt @@ -0,0 +1,41 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.data.sync + +import androidx.work.ExistingWorkPolicy +import androidx.work.OneTimeWorkRequest +import androidx.work.WorkManager +import io.mockk.every +import io.mockk.mockk +import io.mockk.mockkObject +import io.mockk.unmockkAll +import io.mockk.verify +import org.junit.After +import org.junit.Test + +/** + * [SendScheduler] is a thin wrapper over WorkManager; this pins the behaviour that carries meaning — + * the outbox drain is enqueued as a single unique job with REPLACE, so a newly-queued message (or a + * manual retry) starts a fresh drain rather than waiting behind a pending backoff. + */ +class SendSchedulerTest { + + @After + fun tearDown() = unmockkAll() + + @Test + fun `sendNow enqueues a unique send-outbox job with REPLACE`() { + mockkObject(WorkManager.Companion) + val workManager = mockk(relaxed = true) + every { WorkManager.getInstance(any()) } returns workManager + + SendScheduler(mockk(relaxed = true)).sendNow() + + verify { + workManager.enqueueUniqueWork( + "libremail_send_outbox", + ExistingWorkPolicy.REPLACE, + any(), + ) + } + } +} diff --git a/app/src/test/kotlin/org/libremail/data/sync/SendWorkerTest.kt b/app/src/test/kotlin/org/libremail/data/sync/SendWorkerTest.kt index a32729d..2b9361d 100644 --- a/app/src/test/kotlin/org/libremail/data/sync/SendWorkerTest.kt +++ b/app/src/test/kotlin/org/libremail/data/sync/SendWorkerTest.kt @@ -1,52 +1,111 @@ // SPDX-License-Identifier: GPL-3.0-or-later package org.libremail.data.sync +import android.content.Context +import android.util.Log import androidx.work.ListenableWorker.Result import dagger.Lazy import io.mockk.coEvery import io.mockk.coVerify import io.mockk.every import io.mockk.mockk +import io.mockk.mockkStatic +import io.mockk.slot +import io.mockk.unmockkAll import io.mockk.verify import kotlinx.coroutines.test.runTest +import org.junit.After +import org.junit.Before import org.junit.Test import org.libremail.data.attachment.AttachmentUriGrants import org.libremail.data.local.dao.AccountDao import org.libremail.data.local.dao.OutboxDao +import org.libremail.data.local.entity.AccountEntity +import org.libremail.data.local.entity.OutboxEntity +import org.libremail.data.local.entity.ServerConfigEmbedded +import org.libremail.data.local.toOutgoingAttachmentsJson import org.libremail.data.security.EncryptedCacheGuard +import org.libremail.domain.model.OutgoingAttachment +import org.libremail.domain.model.SmtpParams +import org.libremail.mail.GraphSendException import org.libremail.mail.GraphSender +import org.libremail.mail.SendableAttachment import org.libremail.mail.SmtpSender +import java.io.File import kotlin.test.assertEquals +import kotlin.test.assertFalse +import kotlin.test.assertTrue /** - * [SendWorker] already gates on [EncryptedCacheGuard]; this locks that invariant in — while the cache - * is locked it must defer without resolving any of its (`Lazy`) DB-backed dependencies (resolving any - * of them opens the Room DB, which blocks on the passphrase await). + * [SendWorker] first gates on [EncryptedCacheGuard] (regression cover for the pre-auth-DB class of + * bug — while locked it must defer without resolving any of its `Lazy` DB-backed deps), then drains + * the outbox: sending each queued message over SMTP or Microsoft Graph, deleting it on success, + * flagging failures for a retry, and — crucially for the "may have sent" case — never auto-retrying + * a Graph request that might already have delivered. */ class SendWorkerTest { - private val outboxDao = mockk() + private val outboxDao = mockk(relaxed = true) private val accountDao = mockk() - private val connectionFactory = mockk() - private val attachmentUriGrants = mockk() + private val smtpSender = mockk(relaxed = true) + private val graphSender = mockk(relaxed = true) + private val connectionFactory = mockk(relaxed = true) + private val attachmentUriGrants = mockk(relaxed = true) private val lazyOutbox = mockk> { every { get() } returns outboxDao } private val lazyAccount = mockk> { every { get() } returns accountDao } private val lazyConnection = mockk> { every { get() } returns connectionFactory } private val lazyGrants = mockk> { every { get() } returns attachmentUriGrants } private val cacheGuard = mockk() + private lateinit var cacheDir: File + private lateinit var appContext: Context + + @Before + fun setUp() { + cacheDir = java.nio.file.Files.createTempDirectory("sendworker").toFile() + appContext = mockk(relaxed = true) + every { appContext.cacheDir } returns cacheDir + coEvery { cacheGuard.isCacheLocked() } returns false + } + + @After + fun tearDown() { + cacheDir.deleteRecursively() + unmockkAll() + } + private fun worker() = SendWorker( - mockk(relaxed = true), + appContext, mockk(relaxed = true), lazyOutbox, lazyAccount, - mockk(), - mockk(), + smtpSender, + graphSender, lazyConnection, cacheGuard, lazyGrants, ) + private fun account(id: String, authType: String) = AccountEntity( + id = id, + email = "$id@example.org", + displayName = id, + authType = authType, + imap = ServerConfigEmbedded("imap.example.org", 993, "SSL_TLS"), + smtp = ServerConfigEmbedded("smtp.example.org", 465, "SSL_TLS"), + ) + + private fun entity(id: String = "m1", accountId: String = "acct", attachments: String = "") = OutboxEntity( + id = id, + accountId = accountId, + toAddresses = "bob@example.org", + ccAddresses = "", + subject = "Hi", + body = "Body", + createdAt = 0L, + attachments = attachments, + ) + @Test fun `retries without resolving any DB dependency when the cache is locked`() = runTest { coEvery { cacheGuard.isCacheLocked() } returns true @@ -60,12 +119,154 @@ class SendWorkerTest { } @Test - fun `drains the outbox when the cache is unlocked`() = runTest { - coEvery { cacheGuard.isCacheLocked() } returns false + fun `an empty outbox succeeds without sending`() = runTest { coEvery { outboxDao.getAll() } returns emptyList() assertEquals(Result.success(), worker().doWork()) coVerify(exactly = 1) { outboxDao.getAll() } + coVerify(exactly = 0) { smtpSender.send(any(), any(), any(), any()) } + } + + @Test + fun `drops a queued message whose account was removed`() = runTest { + val row = entity() + coEvery { outboxDao.getAll() } returns listOf(row) + coEvery { accountDao.getById("acct") } returns null + + assertEquals(Result.success(), worker().doWork()) + + coVerify { outboxDao.delete("m1") } + coVerify { attachmentUriGrants.releaseUnreferenced(any()) } + coVerify(exactly = 0) { smtpSender.send(any(), any(), any(), any()) } + } + + @Test + fun `sends a password account message over SMTP then clears it`() = runTest { + coEvery { outboxDao.getAll() } returns listOf(entity()) + coEvery { accountDao.getById("acct") } returns account("acct", "PASSWORD_IMAP") + coEvery { connectionFactory.smtpParamsFor(any()) } returns mockk() + + assertEquals(Result.success(), worker().doWork()) + + coVerify { smtpSender.send(any(), "acct@example.org", any(), any()) } + coVerify { outboxDao.delete("m1") } + coVerify { attachmentUriGrants.releaseUnreferenced(any()) } + } + + @Test + fun `an SMTP failure flags the row and retries`() = runTest { + coEvery { outboxDao.getAll() } returns listOf(entity()) + coEvery { accountDao.getById("acct") } returns account("acct", "PASSWORD_IMAP") + coEvery { connectionFactory.smtpParamsFor(any()) } returns mockk() + coEvery { smtpSender.send(any(), any(), any(), any()) } throws RuntimeException("smtp down") + + assertEquals(Result.retry(), worker().doWork()) + + coVerify { outboxDao.setError("m1", "smtp down") } + coVerify(exactly = 0) { outboxDao.delete(any()) } + } + + @Test + fun `sends an Outlook message over Graph`() = runTest { + coEvery { outboxDao.getAll() } returns listOf(entity()) + coEvery { accountDao.getById("acct") } returns account("acct", "OAUTH_OUTLOOK") + coEvery { connectionFactory.graphTokenFor(any()) } returns "graph-token" + + assertEquals(Result.success(), worker().doWork()) + + coVerify { graphSender.send("graph-token", any(), any()) } + coVerify { outboxDao.delete("m1") } + coVerify(exactly = 0) { smtpSender.send(any(), any(), any(), any()) } + } + + @Test + fun `a Graph send that may have sent is left queued and not retried`() = runTest { + coEvery { outboxDao.getAll() } returns listOf(entity()) + coEvery { accountDao.getById("acct") } returns account("acct", "OAUTH_OUTLOOK") + coEvery { connectionFactory.graphTokenFor(any()) } returns "graph-token" + coEvery { graphSender.send(any(), any(), any()) } throws + GraphSendException("maybe sent", mayHaveSent = true) + + // Not a failure: WorkManager must NOT auto-retry, or the message could be duplicated. + assertEquals(Result.success(), worker().doWork()) + + coVerify { outboxDao.setError("m1", match { it.contains("check your Sent folder") }) } + coVerify(exactly = 0) { outboxDao.delete(any()) } + coVerify(exactly = 0) { smtpSender.send(any(), any(), any(), any()) } // must not fall back + } + + @Test + fun `a Graph rejection falls back to SMTP`() = runTest { + coEvery { outboxDao.getAll() } returns listOf(entity()) + coEvery { accountDao.getById("acct") } returns account("acct", "OAUTH_OUTLOOK") + coEvery { connectionFactory.graphTokenFor(any()) } returns "graph-token" + coEvery { connectionFactory.smtpParamsFor(any()) } returns mockk() + coEvery { graphSender.send(any(), any(), any()) } throws + GraphSendException("rejected", mayHaveSent = false) + + assertEquals(Result.success(), worker().doWork()) + + coVerify { smtpSender.send(any(), "acct@example.org", any(), any()) } + coVerify { outboxDao.delete("m1") } + } + + @Test + fun `a Graph transport error falls back to SMTP`() = runTest { + mockkStatic(Log::class) + every { Log.w(any(), any(), any()) } returns 0 + coEvery { outboxDao.getAll() } returns listOf(entity()) + coEvery { accountDao.getById("acct") } returns account("acct", "OAUTH_OUTLOOK") + coEvery { connectionFactory.graphTokenFor(any()) } throws RuntimeException("token refresh failed") + coEvery { connectionFactory.smtpParamsFor(any()) } returns mockk() + + assertEquals(Result.success(), worker().doWork()) + + coVerify { smtpSender.send(any(), "acct@example.org", any(), any()) } + coVerify { outboxDao.delete("m1") } + } + + @Test + fun `stages attachments pairing an inline image with its file and content id`() = runTest { + val attachmentsJson = listOf( + OutgoingAttachment(uri = "content://1", name = "logo.png", contentId = "logo@x", isInline = true), + OutgoingAttachment(uri = "content://2", name = "doc.pdf"), + ).toOutgoingAttachmentsJson() + stageFile("m1", index = 0, name = "logo.png", bytes = byteArrayOf(1, 2)) + stageFile("m1", index = 1, name = "doc.pdf", bytes = byteArrayOf(3, 4)) + coEvery { outboxDao.getAll() } returns listOf(entity(attachments = attachmentsJson)) + coEvery { accountDao.getById("acct") } returns account("acct", "PASSWORD_IMAP") + coEvery { connectionFactory.smtpParamsFor(any()) } returns mockk() + val sent = slot>() + coEvery { smtpSender.send(any(), any(), any(), capture(sent)) } returns Unit + + worker().doWork() + + assertEquals(2, sent.captured.size) + val inline = sent.captured.single { it.isInline } + assertEquals("logo@x", inline.contentId) + assertTrue(sent.captured.any { !it.isInline }) + } + + @Test + fun `falls back to positional staged files when there is no attachment metadata`() = runTest { + stageFile("m1", index = 0, name = "a.txt", bytes = byteArrayOf(1)) + stageFile("m1", index = 1, name = "b.txt", bytes = byteArrayOf(2)) + coEvery { outboxDao.getAll() } returns listOf(entity(attachments = "")) + coEvery { accountDao.getById("acct") } returns account("acct", "PASSWORD_IMAP") + coEvery { connectionFactory.smtpParamsFor(any()) } returns mockk() + val sent = slot>() + coEvery { smtpSender.send(any(), any(), any(), capture(sent)) } returns Unit + + worker().doWork() + + assertEquals(2, sent.captured.size) + assertFalse(sent.captured.any { it.isInline }) // positional restore is always plain attachments + } + + /** Stages a file the way the compose pipeline does: cacheDir/outbox///. */ + private fun stageFile(messageId: String, index: Int, name: String, bytes: ByteArray) { + File(cacheDir, "outbox/$messageId/$index").apply { mkdirs() } + .resolve(name).writeBytes(bytes) } } diff --git a/app/src/test/kotlin/org/libremail/mail/GraphSenderSendTest.kt b/app/src/test/kotlin/org/libremail/mail/GraphSenderSendTest.kt new file mode 100644 index 0000000..dbcb6de --- /dev/null +++ b/app/src/test/kotlin/org/libremail/mail/GraphSenderSendTest.kt @@ -0,0 +1,138 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.mail + +import kotlinx.coroutines.test.runTest +import org.junit.After +import org.junit.Test +import org.libremail.domain.model.OutgoingMessage +import java.io.ByteArrayOutputStream +import java.io.IOException +import java.io.InputStream +import java.io.OutputStream +import java.net.HttpURLConnection +import java.net.URL +import java.net.URLConnection +import java.net.URLStreamHandler +import java.net.URLStreamHandlerFactory +import java.util.concurrent.atomic.AtomicReference +import kotlin.test.assertEquals +import kotlin.test.assertFailsWith +import kotlin.test.assertFalse +import kotlin.test.assertTrue + +/** + * Exercises the [GraphSender.send] transport path (its JSON payload builder is unit-tested separately + * in [GraphSenderTest]). The Graph endpoint is a fixed https URL the sender news up itself, so the + * test routes https through a process-wide [URLStreamHandlerFactory] to a per-test fake connection — + * no network, no production seam. The behaviour pinned: a 2xx succeeds; a non-2xx is a safe-to-retry + * rejection; a lost response is flagged [GraphSendException.mayHaveSent] (must NOT retry/fall back); + * a transmit failure is not; and the connection is always disconnected. + */ +class GraphSenderSendTest { + + @After + fun tearDown() = armed.set(null) + + private val message = + OutgoingMessage(accountId = "outlook:me@x.com", to = "bob@example.org", subject = "Hi", body = "Body") + + private fun arm( + status: Int = 202, + body: String = "", + failOutput: Boolean = false, + failResponse: Boolean = false, + ): AtomicReference { + val last = AtomicReference(null) + armed.set { u -> FakeGraphConnection(u, status, body, failOutput, failResponse).also { last.set(it) } } + return last + } + + @Test + fun `a 2xx response completes the send`() = runTest { + val last = arm(status = 202) + + GraphSender().send("token", message) + + assertTrue(last.get()!!.disconnected, "the connection must be disconnected when done") + } + + @Test + fun `a non-2xx response is a safe-to-retry rejection`() = runTest { + arm(status = 400, body = "{\"error\":\"bad request\"}") + + val ex = assertFailsWith { GraphSender().send("token", message) } + + assertFalse(ex.mayHaveSent, "an explicit rejection means Graph did not send") + assertTrue(ex.message!!.contains("HTTP 400"), ex.message!!) + } + + @Test + fun `a lost response is flagged as maybe-sent`() = runTest { + arm(failResponse = true) + + val ex = assertFailsWith { GraphSender().send("token", message) } + + assertTrue(ex.mayHaveSent, "the request was fully sent, so it may already have delivered") + } + + @Test + fun `a transmit failure is not maybe-sent`() = runTest { + arm(failOutput = true) + + val ex = assertFailsWith { GraphSender().send("token", message) } + + assertFalse(ex.mayHaveSent, "the request never reached Graph, so a retry is safe") + } + + @Test + fun `GraphSendException carries its message, flag and cause`() { + val cause = IOException("boom") + val ex = GraphSendException("failed", mayHaveSent = true, cause = cause) + + assertEquals("failed", ex.message) + assertTrue(ex.mayHaveSent) + assertEquals(cause, ex.cause) + } + + private class FakeGraphConnection( + url: URL, + private val status: Int, + private val body: String, + private val failOutput: Boolean, + private val failResponse: Boolean, + ) : HttpURLConnection(url) { + var disconnected = false + override fun connect() = Unit + override fun disconnect() { + disconnected = true + } + override fun usingProxy() = false + override fun getOutputStream(): OutputStream = + if (failOutput) throw IOException("cannot transmit") else ByteArrayOutputStream() + override fun getResponseCode(): Int = if (failResponse) throw IOException("no response") else status + override fun getInputStream(): InputStream = body.byteInputStream() + override fun getErrorStream(): InputStream? = if (body.isEmpty()) null else body.byteInputStream() + } + + companion object { + private val armed = AtomicReference<((URL) -> HttpURLConnection)?>(null) + + // Set once per JVM: route https opens to whatever the running test armed. Only this test opens + // https in the unit-test JVM (ReportUploadWorker's endpoint is blank and never opened), so a + // permanent https handler is safe here. + init { + URL.setURLStreamHandlerFactory( + URLStreamHandlerFactory { protocol -> + if (protocol == "https") { + object : URLStreamHandler() { + override fun openConnection(u: URL): URLConnection = + armed.get()?.invoke(u) ?: throw IOException("no fake connection armed") + } + } else { + null + } + }, + ) + } + } +} diff --git a/app/src/test/kotlin/org/libremail/mail/ImapClientTest.kt b/app/src/test/kotlin/org/libremail/mail/ImapClientTest.kt index a4c11ed..f8726cc 100644 --- a/app/src/test/kotlin/org/libremail/mail/ImapClientTest.kt +++ b/app/src/test/kotlin/org/libremail/mail/ImapClientTest.kt @@ -1,10 +1,15 @@ // SPDX-License-Identifier: GPL-3.0-or-later package org.libremail.mail +import android.util.Log import com.icegreen.greenmail.util.GreenMail import com.icegreen.greenmail.util.GreenMailUtil import com.icegreen.greenmail.util.ServerSetupTest +import io.mockk.every +import io.mockk.mockkStatic +import io.mockk.unmockkAll import jakarta.activation.DataHandler +import jakarta.mail.Flags import jakarta.mail.Folder import jakarta.mail.Message import jakarta.mail.Part @@ -14,13 +19,20 @@ import jakarta.mail.internet.MimeBodyPart import jakarta.mail.internet.MimeMessage import jakarta.mail.internet.MimeMultipart import jakarta.mail.util.ByteArrayDataSource +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.cancelAndJoin +import kotlinx.coroutines.channels.Channel +import kotlinx.coroutines.launch +import kotlinx.coroutines.runBlocking import kotlinx.coroutines.test.runTest +import kotlinx.coroutines.withTimeout import org.junit.After import org.junit.Before import org.junit.Test import org.libremail.domain.model.ImapConnectionParams import org.libremail.domain.model.MailSecurity import java.util.Properties +import kotlin.test.assertContentEquals import kotlin.test.assertEquals import kotlin.test.assertFailsWith import kotlin.test.assertFalse @@ -41,6 +53,7 @@ class ImapClientTest { @After fun tearDown() { greenMail.stop() + unmockkAll() } private fun params(secret: String = "secret") = ImapConnectionParams( @@ -187,6 +200,142 @@ class ImapClientTest { assertEquals(setOf("Inbox subject"), inbox.map { it.subject }.toSet()) } + @Test + fun `fetchAttachment downloads a part's bytes by its index`() = runTest { + appendInlineImageDigest() // part 0 = inline logo.png, part 1 = invoice.pdf + val uid = client.fetchRecent(params(), "INBOX", limit = 50).first().uid + + val inline = client.fetchAttachment(params(), "INBOX", uid, 0) + val attachment = client.fetchAttachment(params(), "INBOX", uid, 1) + + assertEquals("logo.png", inline.filename) + assertContentEquals(byteArrayOf(1, 2, 3, 4), inline.bytes) + assertEquals("invoice.pdf", attachment.filename) + assertContentEquals(byteArrayOf(5, 6, 7), attachment.bytes) + assertTrue(attachment.mimeType.contains("pdf", ignoreCase = true), attachment.mimeType) + } + + @Test + fun `fetchAttachment fails for an out-of-range part index`() = runTest { + appendInlineImageDigest() + val uid = client.fetchRecent(params(), "INBOX", limit = 50).first().uid + + assertFailsWith { client.fetchAttachment(params(), "INBOX", uid, 99) } + } + + @Test + fun `fetchAttachment fails when the message is not found`() = runTest { + GreenMailUtil.sendTextEmailTest("alice@example.org", "bob@example.org", "Solo", "Body") + greenMail.waitForIncomingEmail(1) + + assertFailsWith { client.fetchAttachment(params(), "INBOX", "999999", 0) } + } + + @Test + fun `setFlag marks a message flagged on the server`() = runTest { + GreenMailUtil.sendTextEmailTest("alice@example.org", "bob@example.org", "Star me", "Body") + greenMail.waitForIncomingEmail(1) + val uid = client.fetchRecent(params(), "INBOX", limit = 50).first().uid + + client.setFlag(params(), "INBOX", uid, Flags.Flag.FLAGGED, value = true) + + assertTrue(client.fetchRecent(params(), "INBOX", limit = 50).first().isFlagged, "should be flagged") + } + + @Test + fun `setFlag on an unknown uid is a no-op`() = runTest { + GreenMailUtil.sendTextEmailTest("alice@example.org", "bob@example.org", "Present", "Body") + greenMail.waitForIncomingEmail(1) + + // No message has this uid, so getMessageByUID returns null and setFlag simply does nothing. + client.setFlag(params(), "INBOX", "999999", Flags.Flag.FLAGGED, value = true) + + assertFalse(client.fetchRecent(params(), "INBOX", limit = 50).first().isFlagged) + } + + @Test + fun `deleteMessage expunges the message from the folder`() = runTest { + GreenMailUtil.sendTextEmailTest("alice@example.org", "bob@example.org", "Delete me", "Body") + GreenMailUtil.sendTextEmailTest("alice@example.org", "bob@example.org", "Keep me", "Body") + greenMail.waitForIncomingEmail(2) + val uid = client.fetchRecent(params(), "INBOX", limit = 50).first { it.subject == "Delete me" }.uid + + client.deleteMessage(params(), "INBOX", uid) + + val remaining = client.fetchRecent(params(), "INBOX", limit = 50).map { it.subject } + assertFalse(remaining.contains("Delete me"), "remaining=$remaining") + assertTrue(remaining.contains("Keep me"), "remaining=$remaining") + } + + @Test + fun `fetchRecent returns empty for an empty folder`() = runTest { + createFolder("Empty") + + assertTrue(client.fetchRecent(params(), "Empty", limit = 50).isEmpty()) + } + + @Test + fun `body and reply fetches fail for an unknown uid`() = runTest { + GreenMailUtil.sendTextEmailTest("alice@example.org", "bob@example.org", "Present", "Body") + greenMail.waitForIncomingEmail(1) + + assertFailsWith { client.fetchBodyMarkingSeen(params(), "INBOX", "999999") } + assertFailsWith { client.fetchBodyPeek(params(), "INBOX", "999999") } + assertFailsWith { client.fetchForReply(params(), "INBOX", "999999") } + } + + @Test + fun `STARTTLS and XOAUTH2 params drive the corresponding connection properties`() = runTest { + // GreenMail here is plaintext with no XOAUTH2, so the connect fails — but buildProps has already + // run, which is the STARTTLS + XOAUTH2 property wiring this exercises. + val params = ImapConnectionParams( + host = "127.0.0.1", + port = greenMail.imap.port, + security = MailSecurity.STARTTLS, + username = "alice@example.org", + secret = "secret", + useXoauth2 = true, + strictStartTls = true, + ) + + assertFailsWith { client.listFolders(params) } + } + + @Test + fun `idle syncs once on connect, again on newly delivered mail, and stops on cancel`() = runBlocking { + mockkStatic(Log::class) // idle() logs connect/push at debug level + every { Log.d(any(), any()) } returns 0 + val activity = Channel(Channel.UNLIMITED) + val job = launch(Dispatchers.IO) { client.idle(params()) { activity.send(Unit) } } + try { + // A sync fires immediately on connect to catch anything already waiting... + withTimeout(IDLE_TIMEOUT_MS) { activity.receive() } + // ...then an IMAP IDLE push fires another when new mail arrives. + GreenMailUtil.sendTextEmailTest("alice@example.org", "bob@example.org", "Pushed", "Body") + withTimeout(IDLE_TIMEOUT_MS) { activity.receive() } + } finally { + job.cancelAndJoin() // cancelling closes the connection and unblocks idle() + } + assertTrue(job.isCompleted, "the idle loop must terminate on cancellation") + } + + /** Creates [folderName] (holding messages) if it does not already exist. */ + private fun createFolder(folderName: String) { + val props = Properties().apply { + put("mail.store.protocol", "imap") + put("mail.imap.host", "127.0.0.1") + put("mail.imap.port", greenMail.imap.port.toString()) + } + val store = Session.getInstance(props).getStore("imap") + store.connect("127.0.0.1", greenMail.imap.port, "alice@example.org", "secret") + try { + val folder = store.getFolder(folderName) + if (!folder.exists()) folder.create(Folder.HOLDS_MESSAGES) + } finally { + store.close() + } + } + /** * Appends a rich digest to the INBOX: a `multipart/mixed` of a `multipart/related` (HTML body * referencing an inline image via `cid:logo1`) plus a genuine PDF attachment — the shape that @@ -275,4 +424,9 @@ class ImapClientTest { store.close() } } + + private companion object { + /** Generous ceiling for the in-process IDLE round trip so the assertion never races the server. */ + const val IDLE_TIMEOUT_MS = 15_000L + } } diff --git a/app/src/test/kotlin/org/libremail/mail/ImapConnectionCacheTest.kt b/app/src/test/kotlin/org/libremail/mail/ImapConnectionCacheTest.kt new file mode 100644 index 0000000..4e02d7e --- /dev/null +++ b/app/src/test/kotlin/org/libremail/mail/ImapConnectionCacheTest.kt @@ -0,0 +1,140 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.mail + +import io.mockk.mockk +import io.mockk.verify +import jakarta.mail.Folder +import jakarta.mail.FolderClosedException +import jakarta.mail.MessagingException +import jakarta.mail.Store +import jakarta.mail.StoreClosedException +import kotlinx.coroutines.test.runTest +import org.junit.Test +import org.libremail.domain.model.ImapConnectionParams +import org.libremail.domain.model.MailSecurity +import java.io.IOException +import kotlin.test.assertEquals +import kotlin.test.assertFailsWith + +/** + * The connection-reuse cache (issue #125 spike): one authenticated [Store] per account behind a + * mutex, established lazily and kept open. These tests pin the reuse guarantee and the lazy + * catch-and-retry-once stale handling — a dropped connection is rebuilt and the op retried, a second + * failure clears the slot, and a genuine protocol error is propagated without ever reconnecting. + */ +class ImapConnectionCacheTest { + + private val params = ImapConnectionParams( + host = "127.0.0.1", + port = 143, + security = MailSecurity.NONE, + username = "alice@example.org", + secret = "secret", + useXoauth2 = false, + ) + + private var connects = 0 + + /** A cache whose connect step counts calls and returns [supply] (a fresh relaxed [Store] by default). */ + private fun cache(supply: () -> Store = { mockk(relaxed = true) }) = ImapConnectionCache { + connects++ + supply() + } + + @Test + fun `establishes one connection and reuses it across calls`() = runTest { + val cache = cache() + + assertEquals("a", cache.withStore(params) { "a" }) + assertEquals("b", cache.withStore(params) { "b" }) + + assertEquals(1, connects, "the second op reuses the first connection") + } + + @Test + fun `rebuilds the socket once and retries when the connection drops`() = runTest { + val opened = mutableListOf() + val cache = cache { + mockk(relaxed = true).also { opened += it } + } + var attempts = 0 + + val result = cache.withStore(params) { + attempts++ + if (attempts == 1) throw IOException("dropped") else "recovered" + } + + assertEquals("recovered", result) + assertEquals(2, connects, "a dropped connection is rebuilt exactly once") + verify { opened[0].close() } // the stale socket is torn down before reconnecting + } + + @Test + fun `a second failure after reconnect clears the slot so the next call reconnects`() = runTest { + val cache = cache() + + assertFailsWith { cache.withStore(params) { throw IOException("still down") } } + assertEquals(2, connects, "initial connect plus one rebuild") + + cache.withStore(params) { "ok" } + assertEquals(3, connects, "the cleared slot forces a fresh connect") + } + + @Test + fun `a genuine protocol error is propagated without reconnecting`() = runTest { + val cache = cache() + + assertFailsWith { + cache.withStore(params) { throw IllegalStateException("bad login") } + } + + assertEquals(1, connects, "a non-drop error must not trigger a reconnect") + } + + @Test + fun `folder-closed, store-closed, IO and IO-caused messaging errors all count as drops`() = runTest { + retriesOn(FolderClosedException(mockk(relaxed = true))) + retriesOn(StoreClosedException(mockk(relaxed = true))) + retriesOn(IOException("socket")) + retriesOn(MessagingException("wrapped", IOException("socket"))) + } + + @Test + fun `a messaging error without an IO cause is not a drop`() = runTest { + val cache = cache() + + assertFailsWith { + cache.withStore(params) { throw MessagingException("server said no") } + } + + assertEquals(1, connects) + } + + @Test + fun `closeAll tears down and forgets every cached connection`() = runTest { + val store = mockk(relaxed = true) + val cache = cache { store } + + cache.withStore(params) { "a" } + cache.closeAll() + + verify { store.close() } + cache.withStore(params) { "b" } + assertEquals(2, connects, "after closeAll the next op reconnects") + } + + /** Asserts [error] is treated as a dropped connection: rebuilt once, the retry succeeds. */ + private suspend fun retriesOn(error: Throwable) { + connects = 0 + val cache = cache() + var attempts = 0 + + val result = cache.withStore(params) { + attempts++ + if (attempts == 1) throw error else "ok" + } + + assertEquals("ok", result) + assertEquals(2, connects, "${error.javaClass.simpleName} should have been retried on a fresh socket") + } +} From 6333dd451158bf2dda6c364dda24a3358e2e9dda Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Fri, 3 Jul 2026 15:15:10 -0500 Subject: [PATCH 2/2] fix(reporting): gate startup crash prompt to a legitimate <24h crash, first re-open only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The auto-submit crash prompt over-triggered: it re-surfaced the newest saved crash report on every launch, with no age bound, so a pre-update crash kept popping "LibreMail crashed" long after the crash was fixed (#255). Gate StartupReportViewModel.pendingCrash so a crash is auto-offered: - first re-open only — dismiss() now persists a "surfaced" marker instead of an in-memory-only hide, so a report is offered at most once across launches; it stays in the store (still listed in Problem Reports) and only discard() deletes. - < 24h only — inject a clock provider and filter to createdAtMillis within 24h. - legitimate crash only — reports come solely from CrashReporter's uncaught- exception handler, so update / force-stop / user-close create none; made explicit and covered by a test. The marker is a minimal additive `surfaced` flag on DebugReport (persisted in storage JSON, kept out of the submission payload; a missing flag = not surfaced) plus ReportStore.markSurfaced(id). Extracted StartupCrashPrompt from LibreMailApp so the real dialog + gating is E2E-testable. Co-Authored-By: Claude Opus 4.8 --- .../ui/reporting/StartupCrashPromptTest.kt | 145 ++++++++++++++++++ .../org/libremail/reporting/DebugReport.kt | 12 +- .../org/libremail/reporting/ReportStore.kt | 14 ++ .../kotlin/org/libremail/ui/LibreMailApp.kt | 23 ++- .../ui/reporting/StartupReportViewModel.kt | 47 ++++-- .../reporting/CrashReporterInstallTest.kt | 23 +++ .../libremail/reporting/DebugReportTest.kt | 28 ++++ .../libremail/reporting/ReportStoreTest.kt | 21 +++ .../reporting/StartupReportViewModelTest.kt | 137 ++++++++++++----- 9 files changed, 387 insertions(+), 63 deletions(-) create mode 100644 app/src/androidTest/kotlin/org/libremail/ui/reporting/StartupCrashPromptTest.kt diff --git a/app/src/androidTest/kotlin/org/libremail/ui/reporting/StartupCrashPromptTest.kt b/app/src/androidTest/kotlin/org/libremail/ui/reporting/StartupCrashPromptTest.kt new file mode 100644 index 0000000..5f4413b --- /dev/null +++ b/app/src/androidTest/kotlin/org/libremail/ui/reporting/StartupCrashPromptTest.kt @@ -0,0 +1,145 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.ui.reporting + +import androidx.activity.ComponentActivity +import androidx.compose.runtime.MutableState +import androidx.compose.runtime.mutableStateOf +import androidx.compose.ui.test.assertIsDisplayed +import androidx.compose.ui.test.junit4.createAndroidComposeRule +import androidx.compose.ui.test.onAllNodesWithText +import androidx.compose.ui.test.onNodeWithText +import androidx.compose.ui.test.performClick +import androidx.test.ext.junit.runners.AndroidJUnit4 +import androidx.test.platform.app.InstrumentationRegistry +import org.junit.After +import org.junit.Assert.assertNotNull +import org.junit.Assert.assertNull +import org.junit.Before +import org.junit.Rule +import org.junit.Test +import org.junit.runner.RunWith +import org.libremail.R +import org.libremail.reporting.DebugReport +import org.libremail.reporting.ReportKind +import org.libremail.reporting.ReportStore +import org.libremail.ui.StartupCrashPrompt +import org.libremail.ui.theme.LibreMailTheme +import java.io.File + +/** + * E2E for the #255 startup-crash-prompt gating, driving the real [StartupCrashPrompt] composable over a + * real file-backed [ReportStore]: a legitimate recent crash pops the dialog exactly once (and never + * again after a simulated relaunch reads the persisted `surfaced` flag), while a stale (> 24h) crash + * never pops it. A fixed clock keeps the age gate independent of the device wall clock. + */ +@RunWith(AndroidJUnit4::class) +class StartupCrashPromptTest { + + @get:Rule + val composeTestRule = createAndroidComposeRule() + + private val now = 1_000_000_000_000L + private val dayMs = 24L * 60 * 60 * 1000 + private lateinit var dir: File + + @Before + fun setUp() { + val context = InstrumentationRegistry.getInstrumentation().targetContext + dir = File(context.cacheDir, "startup_crash_prompt_test_${System.nanoTime()}") + dir.deleteRecursively() + dir.mkdirs() + } + + @After + fun tearDown() { + dir.deleteRecursively() + } + + private fun string(resId: Int) = composeTestRule.activity.getString(resId) + + private fun store() = ReportStore(dir) + + private fun crash(id: String, createdAt: Long) = DebugReport( + id = id, + createdAtMillis = createdAt, + kind = ReportKind.CRASH, + appVersionName = "0.1.0", + appVersionCode = 1, + androidRelease = "14", + androidSdkInt = 34, + deviceManufacturer = "Google", + deviceModel = "Pixel", + stackTrace = null, + settings = emptyMap(), + logs = emptyList(), + ) + + private fun viewModel(store: ReportStore) = StartupReportViewModel(store, now = { now }) + + /** Renders the prompt against [vmState]; swapping its value simulates a fresh process on relaunch. */ + private fun render(vmState: MutableState) { + composeTestRule.setContent { + LibreMailTheme(darkTheme = false, dynamicColor = false) { + StartupCrashPrompt(viewModel = vmState.value, onReview = {}) + } + } + } + + private fun awaitDialogShown() = composeTestRule.waitUntil(WAIT_MS) { + composeTestRule.onAllNodesWithText(string(R.string.crash_prompt_title)).fetchSemanticsNodes().isNotEmpty() + } + + private fun awaitDialogGone() = composeTestRule.waitUntil(WAIT_MS) { + composeTestRule.onAllNodesWithText(string(R.string.crash_prompt_title)).fetchSemanticsNodes().isEmpty() + } + + @Test + fun recentCrash_popsDialogOnce_andNotAgainOnRelaunch() { + val store = store() + store.save(crash("c", createdAt = now - 60_000L)) + val vmState = mutableStateOf(viewModel(store)) + render(vmState) + + // First re-open after the crash: the prompt is offered. + awaitDialogShown() + composeTestRule.onNodeWithText(string(R.string.crash_prompt_title)).assertIsDisplayed() + + // "Not now" hides it and persistently marks it surfaced (the report itself stays saved). + composeTestRule.onNodeWithText(string(R.string.crash_prompt_later)).performClick() + awaitDialogGone() + assertNotNull(store().find("c")) + + // Relaunch: a fresh store + VM over the same dir reads the persisted flag → no re-nag. + composeTestRule.runOnUiThread { vmState.value = viewModel(store()) } + composeTestRule.waitForIdle() + composeTestRule.onNodeWithText(string(R.string.crash_prompt_title)).assertDoesNotExist() + } + + @Test + fun staleCrash_doesNotPopDialog() { + val store = store() + store.save(crash("old", createdAt = now - dayMs - 60_000L)) + render(mutableStateOf(viewModel(store))) + + composeTestRule.waitForIdle() + composeTestRule.onNodeWithText(string(R.string.crash_prompt_title)).assertDoesNotExist() + } + + @Test + fun discard_deletesTheReport() { + val store = store() + store.save(crash("c", createdAt = now - 60_000L)) + render(mutableStateOf(viewModel(store))) + + awaitDialogShown() + composeTestRule.onNodeWithText(string(R.string.crash_prompt_title)).assertIsDisplayed() + composeTestRule.onNodeWithText(string(R.string.crash_prompt_discard)).performClick() + awaitDialogGone() + + assertNull(store().find("c")) + } + + private companion object { + const val WAIT_MS = 5_000L + } +} diff --git a/app/src/main/kotlin/org/libremail/reporting/DebugReport.kt b/app/src/main/kotlin/org/libremail/reporting/DebugReport.kt index 2c782f7..8f12a52 100644 --- a/app/src/main/kotlin/org/libremail/reporting/DebugReport.kt +++ b/app/src/main/kotlin/org/libremail/reporting/DebugReport.kt @@ -31,12 +31,19 @@ data class DebugReport( val userComment: String = "", /** Reply-to address the user supplied when submitting (see #159); required for online submit. */ val userEmail: String = "", + /** + * Whether the startup crash prompt has already auto-offered this report (see #255). Internal + * bookkeeping only: persisted with the report but deliberately kept out of [toSubmissionPayload] + * so it never leaks into what the user reviews or submits. A missing flag (older stored reports) + * reads as `false` — not yet surfaced. + */ + val surfaced: Boolean = false, ) { /** The exact text shown for review, copied, saved to a file, and POSTed on submit. */ fun toSubmissionPayload(): String = toJson().toString(JSON_INDENT) - /** Compact form used for on-disk persistence. */ - fun toStorageJson(): String = toJson().toString() + /** Compact form used for on-disk persistence; adds the internal [surfaced] bookkeeping flag. */ + fun toStorageJson(): String = toJson().put("surfaced", surfaced).toString() private fun toJson(): JSONObject { val app = JSONObject() @@ -92,6 +99,7 @@ data class DebugReport( logs = logs, userComment = json.optString("userComment", ""), userEmail = json.optString("userEmail", ""), + surfaced = json.optBoolean("surfaced", false), ) } } diff --git a/app/src/main/kotlin/org/libremail/reporting/ReportStore.kt b/app/src/main/kotlin/org/libremail/reporting/ReportStore.kt index 247b1ed..ffef53b 100644 --- a/app/src/main/kotlin/org/libremail/reporting/ReportStore.kt +++ b/app/src/main/kotlin/org/libremail/reporting/ReportStore.kt @@ -27,6 +27,20 @@ class ReportStore(private val directory: File) { fun find(id: String): DebugReport? = _reports.value.firstOrNull { it.id == id } + /** + * Persistently marks a report as auto-surfaced so the startup crash prompt offers it at most once + * across launches (see #255). The report itself stays in the store (still listed under Problem + * Reports); only [delete] removes it. No-op if the report is missing or already surfaced. + */ + fun markSurfaced(id: String) { + synchronized(lock) { + val report = _reports.value.firstOrNull { it.id == id } ?: return + if (report.surfaced) return + File(directory, fileName(id)).writeText(report.copy(surfaced = true).toStorageJson()) + _reports.value = scan() + } + } + fun delete(id: String) { synchronized(lock) { File(directory, fileName(id)).delete() diff --git a/app/src/main/kotlin/org/libremail/ui/LibreMailApp.kt b/app/src/main/kotlin/org/libremail/ui/LibreMailApp.kt index c81f587..9ee1125 100644 --- a/app/src/main/kotlin/org/libremail/ui/LibreMailApp.kt +++ b/app/src/main/kotlin/org/libremail/ui/LibreMailApp.kt @@ -69,7 +69,6 @@ fun LibreMailApp( val start = startDestination ?: return val licenseAlreadyAccepted = licenseAccepted ?: return val navController = rememberNavController() - val pendingCrash by startupViewModel.pendingCrash.collectAsStateWithLifecycle() // A mailto:/share intent opens compose on top of the mailbox, pre-filled. Keyed on the request so // it fires once per intent (and again for a new intent delivered while the app is alive). @@ -256,14 +255,28 @@ fun LibreMailApp( } // On launch, offer any saved crash report for review — never sent without the user's action. + StartupCrashPrompt( + viewModel = startupViewModel, + onReview = { reportId -> navController.navigate(Routes.reportReview(reportId)) }, + ) +} + +/** + * Offers any pending crash report for review on launch (see #255). [StartupReportViewModel] gates this + * to a legitimate crash from the last 24h, shown at most once; this only renders its decision. "Review" + * and "Not now" both mark the report surfaced so it never re-nags; only "Discard" deletes it. + */ +@Composable +internal fun StartupCrashPrompt(viewModel: StartupReportViewModel, onReview: (String) -> Unit) { + val pendingCrash by viewModel.pendingCrash.collectAsStateWithLifecycle() pendingCrash?.let { crash -> CrashReportDialog( onReview = { - startupViewModel.dismiss() - navController.navigate(Routes.reportReview(crash.id)) + viewModel.dismiss(crash.id) + onReview(crash.id) }, - onLater = startupViewModel::dismiss, - onDiscard = { startupViewModel.discard(crash.id) }, + onLater = { viewModel.dismiss(crash.id) }, + onDiscard = { viewModel.discard(crash.id) }, ) } } diff --git a/app/src/main/kotlin/org/libremail/ui/reporting/StartupReportViewModel.kt b/app/src/main/kotlin/org/libremail/ui/reporting/StartupReportViewModel.kt index 89cfdec..b70c1ac 100644 --- a/app/src/main/kotlin/org/libremail/ui/reporting/StartupReportViewModel.kt +++ b/app/src/main/kotlin/org/libremail/ui/reporting/StartupReportViewModel.kt @@ -4,43 +4,58 @@ package org.libremail.ui.reporting import androidx.lifecycle.ViewModel import androidx.lifecycle.viewModelScope import dagger.hilt.android.lifecycle.HiltViewModel -import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.SharingStarted import kotlinx.coroutines.flow.StateFlow -import kotlinx.coroutines.flow.combine +import kotlinx.coroutines.flow.map import kotlinx.coroutines.flow.stateIn import kotlinx.coroutines.launch import org.libremail.reporting.ReportKind import org.libremail.reporting.ReportStore import javax.inject.Inject -/** Surfaces a pending crash report (if any) so the app can offer it for review on launch. */ +/** + * Surfaces a pending crash report (if any) so the app can offer it for review on launch. The prompt is + * gated (see #255) so it fires at most once, only for a legitimate recent crash: + * + * - **First re-open only:** a report is auto-offered once, then persistently marked surfaced; it never + * re-nags on later launches. It stays in the store (still listed under Problem Reports for manual + * review); only [discard] deletes it. + * - **< 24h only:** older crashes are never auto-surfaced (they may still be reviewed manually). + * - **Legitimate crash only:** only `CrashReporter`'s uncaught-exception handler ever creates a + * [ReportKind.CRASH] report, so an app update, a user-initiated close, or a force-stop create no + * report and therefore never pop this prompt. + * + * @param now clock provider, injected so the age gate is unit-testable. + */ @HiltViewModel -class StartupReportViewModel @Inject constructor(private val store: ReportStore) : ViewModel() { +class StartupReportViewModel(private val store: ReportStore, private val now: () -> Long) : ViewModel() { - private val dismissed = MutableStateFlow(false) + @Inject + constructor(store: ReportStore) : this(store, { System.currentTimeMillis() }) val pendingCrash: StateFlow = - combine(store.reports, dismissed) { reports, isDismissed -> - if (isDismissed) { - null - } else { - reports.firstOrNull { it.kind == ReportKind.CRASH } - ?.let { ReportSummary(it.id, it.kind, it.createdAtMillis) } - } + store.reports.map { reports -> + reports.firstOrNull { report -> + report.kind == ReportKind.CRASH && + !report.surfaced && + report.createdAtMillis >= now() - CRASH_MAX_AGE_MS + }?.let { ReportSummary(it.id, it.kind, it.createdAtMillis) } }.stateIn(viewModelScope, SharingStarted.WhileSubscribed(SUBSCRIBE_MS), null) - /** Hides the prompt for this launch; the report stays saved and is offered again next launch. */ - fun dismiss() { - dismissed.value = true + /** + * "Not now" / "Review": persistently marks the crash surfaced so it is auto-offered at most once + * across launches. The report stays saved (still shown in Problem Reports); only [discard] deletes. + */ + fun dismiss(id: String) { + viewModelScope.launch { store.markSurfaced(id) } } fun discard(id: String) { - dismissed.value = true viewModelScope.launch { store.delete(id) } } private companion object { const val SUBSCRIBE_MS = 5_000L + const val CRASH_MAX_AGE_MS = 24L * 60 * 60 * 1000 } } diff --git a/app/src/test/kotlin/org/libremail/reporting/CrashReporterInstallTest.kt b/app/src/test/kotlin/org/libremail/reporting/CrashReporterInstallTest.kt index 32354a7..d72c745 100644 --- a/app/src/test/kotlin/org/libremail/reporting/CrashReporterInstallTest.kt +++ b/app/src/test/kotlin/org/libremail/reporting/CrashReporterInstallTest.kt @@ -11,6 +11,7 @@ import org.junit.Test import org.junit.rules.TemporaryFolder import org.libremail.data.settings.SettingsRepository import kotlin.test.assertEquals +import kotlin.test.assertTrue /** * Covers [CrashReporter.install]: the installed handler must persist the crash locally AND still chain @@ -63,4 +64,26 @@ class CrashReporterInstallTest { // ...and the OS's original handler still ran, so the system crash still surfaces. verify { previous.uncaughtException(thread, crash) } } + + @Test + fun `only a genuine uncaught exception creates a crash report - update, force-stop, swipe-away do not`() { + Thread.setDefaultUncaughtExceptionHandler(mockk(relaxed = true)) + val store = ReportStore(tempFolder.root) + val buffer = RingLogBuffer() + val collector = DiagnosticsCollector(appVersion, settingsRepository, buffer) + val reporter = CrashReporter(collector, store, buffer) + reporter.install() + val installed = requireNotNull(Thread.getDefaultUncaughtExceptionHandler()) + + // An app update (killDueToPackageUpdate), a force-stop, and a user swipe-away/task-removal all + // end the process WITHOUT delivering an uncaught throwable to this handler, so none of them + // creates a report and the startup prompt stays silent (#255 criterion 3). Reports come solely + // from a genuine uncaught crash routing through the installed handler. + assertTrue(store.reports.value.isEmpty()) + + installed.uncaughtException(Thread.currentThread(), IllegalStateException("real crash")) + + assertEquals(1, store.reports.value.size) + assertEquals(ReportKind.CRASH, store.reports.value.single().kind) + } } diff --git a/app/src/test/kotlin/org/libremail/reporting/DebugReportTest.kt b/app/src/test/kotlin/org/libremail/reporting/DebugReportTest.kt index 4cf670b..0ea96a3 100644 --- a/app/src/test/kotlin/org/libremail/reporting/DebugReportTest.kt +++ b/app/src/test/kotlin/org/libremail/reporting/DebugReportTest.kt @@ -1,8 +1,10 @@ // SPDX-License-Identifier: GPL-3.0-or-later package org.libremail.reporting +import org.json.JSONObject import org.junit.Test import kotlin.test.assertEquals +import kotlin.test.assertFalse import kotlin.test.assertNull import kotlin.test.assertTrue @@ -93,4 +95,30 @@ class DebugReportTest { assertTrue(payload.contains("reporter@example.com")) } + + @Test + fun `surfaced flag round-trips through storage json`() { + val original = sample().copy(surfaced = true) + + val restored = DebugReport.fromStorageJson(original.toStorageJson()) + + assertTrue(restored.surfaced) + assertEquals(original, restored) + } + + @Test + fun `a legacy stored report without the surfaced flag reads as not surfaced`() { + val legacy = JSONObject(sample().toStorageJson()).apply { remove("surfaced") }.toString() + + val restored = DebugReport.fromStorageJson(legacy) + + assertFalse(restored.surfaced) + } + + @Test + fun `the internal surfaced flag never appears in the submission payload`() { + val payload = sample().copy(surfaced = true).toSubmissionPayload() + + assertFalse(payload.contains("surfaced")) + } } diff --git a/app/src/test/kotlin/org/libremail/reporting/ReportStoreTest.kt b/app/src/test/kotlin/org/libremail/reporting/ReportStoreTest.kt index 02e7072..eda881e 100644 --- a/app/src/test/kotlin/org/libremail/reporting/ReportStoreTest.kt +++ b/app/src/test/kotlin/org/libremail/reporting/ReportStoreTest.kt @@ -69,6 +69,27 @@ class ReportStoreTest { assertEquals("persisted", reopened.find("persisted")?.id) } + @Test + fun `markSurfaced flags the report and persists across a fresh instance`() { + val store = ReportStore(tempFolder.root) + store.save(report("a")) + + store.markSurfaced("a") + + assertTrue(store.find("a")!!.surfaced) + // Survives a fresh instance over the same directory (the next launch reads it as surfaced). + assertTrue(ReportStore(tempFolder.root).find("a")!!.surfaced) + } + + @Test + fun `markSurfaced is a no-op for a missing report`() { + val store = ReportStore(tempFolder.root) + + store.markSurfaced("missing") + + assertTrue(store.reports.value.isEmpty()) + } + @Test fun `ignores unparseable files`() { File(tempFolder.root, "garbage.json").writeText("not json at all") diff --git a/app/src/test/kotlin/org/libremail/ui/reporting/StartupReportViewModelTest.kt b/app/src/test/kotlin/org/libremail/ui/reporting/StartupReportViewModelTest.kt index e10e73b..83f6901 100644 --- a/app/src/test/kotlin/org/libremail/ui/reporting/StartupReportViewModelTest.kt +++ b/app/src/test/kotlin/org/libremail/ui/reporting/StartupReportViewModelTest.kt @@ -1,15 +1,10 @@ // SPDX-License-Identifier: GPL-3.0-or-later package org.libremail.ui.reporting -import io.mockk.Runs -import io.mockk.every -import io.mockk.just -import io.mockk.mockk -import io.mockk.verify import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.ExperimentalCoroutinesApi -import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.launch +import kotlinx.coroutines.test.TestScope import kotlinx.coroutines.test.UnconfinedTestDispatcher import kotlinx.coroutines.test.advanceUntilIdle import kotlinx.coroutines.test.resetMain @@ -18,25 +13,41 @@ import kotlinx.coroutines.test.runTest import kotlinx.coroutines.test.setMain import org.junit.After import org.junit.Before +import org.junit.Rule import org.junit.Test +import org.junit.rules.TemporaryFolder import org.libremail.reporting.DebugReport import org.libremail.reporting.ReportKind import org.libremail.reporting.ReportStore import kotlin.test.assertEquals +import kotlin.test.assertNotNull import kotlin.test.assertNull +import kotlin.test.assertTrue +/** + * Verifies the #255 gating: the startup crash prompt surfaces a [ReportKind.CRASH] report only when it + * is fresh (< 24h), unseen (first re-open only, persisted across relaunches), and a real crash. Uses a + * real file-backed [ReportStore] over a temp dir so the persisted `surfaced` flag round-trips exactly + * as it would across a process restart, and a fixed clock so the age gate is deterministic. + */ @OptIn(ExperimentalCoroutinesApi::class) class StartupReportViewModelTest { + @get:Rule + val tempFolder = TemporaryFolder() + private val dispatcher = UnconfinedTestDispatcher() + private val now = 1_000_000_000_000L + private val dayMs = 24L * 60 * 60 * 1000 + @Before fun setUp() = Dispatchers.setMain(dispatcher) @After fun tearDown() = Dispatchers.resetMain() - private fun report(id: String, kind: ReportKind, createdAt: Long = 1L) = DebugReport( + private fun report(id: String, kind: ReportKind, createdAt: Long) = DebugReport( id = id, createdAtMillis = createdAt, kind = kind, @@ -51,59 +62,105 @@ class StartupReportViewModelTest { logs = emptyList(), ) - @Test - fun `pendingCrash surfaces the first crash report`() = runTest(dispatcher) { - val store = mockk(relaxed = true) - every { store.reports } returns - MutableStateFlow(listOf(report("m", ReportKind.MANUAL), report("c", ReportKind.CRASH, createdAt = 9L))) - val vm = StartupReportViewModel(store) + private fun crash(id: String, createdAt: Long) = report(id, ReportKind.CRASH, createdAt) + private fun store() = ReportStore(tempFolder.root) + + private fun viewModel(store: ReportStore) = StartupReportViewModel(store, now = { now }) + + /** Subscribes to [StartupReportViewModel.pendingCrash] so the `WhileSubscribed` flow starts. */ + private fun TestScope.subscribe(vm: StartupReportViewModel) { backgroundScope.launch { vm.pendingCrash.collect {} } runCurrent() - - assertEquals(ReportSummary("c", ReportKind.CRASH, 9L), vm.pendingCrash.value) } @Test - fun `pendingCrash is null when only manual reports exist`() = runTest(dispatcher) { - val store = mockk(relaxed = true) - every { store.reports } returns MutableStateFlow(listOf(report("m", ReportKind.MANUAL))) - val vm = StartupReportViewModel(store) + fun `surfaces a fresh unseen crash`() = runTest(dispatcher) { + val store = store() + store.save(crash("c", createdAt = now - 1_000L)) + val vm = viewModel(store) + subscribe(vm) - backgroundScope.launch { vm.pendingCrash.collect {} } - runCurrent() + assertEquals(ReportSummary("c", ReportKind.CRASH, now - 1_000L), vm.pendingCrash.value) + } + + @Test + fun `does not surface a crash older than 24h`() = runTest(dispatcher) { + val store = store() + store.save(crash("old", createdAt = now - dayMs - 1)) + val vm = viewModel(store) + subscribe(vm) assertNull(vm.pendingCrash.value) } @Test - fun `dismiss hides the prompt for this launch without deleting the report`() = runTest(dispatcher) { - val store = mockk(relaxed = true) - every { store.reports } returns MutableStateFlow(listOf(report("c", ReportKind.CRASH))) - val vm = StartupReportViewModel(store) + fun `surfaces a crash exactly at the 24h boundary`() = runTest(dispatcher) { + val store = store() + store.save(crash("edge", createdAt = now - dayMs)) + val vm = viewModel(store) + subscribe(vm) - backgroundScope.launch { vm.pendingCrash.collect {} } - runCurrent() - vm.dismiss() - runCurrent() - - assertNull(vm.pendingCrash.value) - verify(exactly = 0) { store.delete(any()) } + assertEquals("edge", vm.pendingCrash.value?.id) } @Test - fun `discard hides the prompt and deletes the report`() = runTest(dispatcher) { - val store = mockk(relaxed = true) - every { store.reports } returns MutableStateFlow(listOf(report("c", ReportKind.CRASH))) - every { store.delete(any()) } just Runs - val vm = StartupReportViewModel(store) + fun `ignores non-crash reports`() = runTest(dispatcher) { + val store = store() + store.save(report("m", ReportKind.MANUAL, createdAt = now)) + val vm = viewModel(store) + subscribe(vm) + + assertNull(vm.pendingCrash.value) + } + + @Test + fun `surfaces the newest eligible crash, skipping surfaced and stale ones`() = runTest(dispatcher) { + val store = store() + store.save(crash("stale", createdAt = now - dayMs - 1)) + store.save(crash("fresh", createdAt = now - 2_000L)) + store.save(crash("seen", createdAt = now - 1_000L)) + store.markSurfaced("seen") + val vm = viewModel(store) + subscribe(vm) + + assertEquals("fresh", vm.pendingCrash.value?.id) + } + + @Test + fun `dismiss marks the crash surfaced so it does not reappear on relaunch`() = runTest(dispatcher) { + val store = store() + store.save(crash("c", createdAt = now)) + val vm = viewModel(store) + subscribe(vm) + assertNotNull(vm.pendingCrash.value) + + vm.dismiss("c") + advanceUntilIdle() + + // Hidden this launch, still stored (not deleted), and now flagged surfaced. + assertNull(vm.pendingCrash.value) + assertNotNull(store.find("c")) + assertTrue(store.find("c")!!.surfaced) + + // A fresh store + VM (a new process) reads the persisted flag → never re-nags. + val relaunchStore = store() + val relaunchVm = viewModel(relaunchStore) + subscribe(relaunchVm) + assertNull(relaunchVm.pendingCrash.value) + } + + @Test + fun `discard deletes the report`() = runTest(dispatcher) { + val store = store() + store.save(crash("c", createdAt = now)) + val vm = viewModel(store) + subscribe(vm) - backgroundScope.launch { vm.pendingCrash.collect {} } - runCurrent() vm.discard("c") advanceUntilIdle() assertNull(vm.pendingCrash.value) - verify(exactly = 1) { store.delete("c") } + assertNull(store.find("c")) } }