You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Thanks for the fix — the intent (fail-fast on missing DB) is right and the production change is small and focused. A few items worth addressing on the test side, plus a couple of small suggestions.
error(\"Unable to connect to the database\") is idiomatic Kotlin and gets the job done (it throws IllegalStateException). Two minor nits to consider:
Consider logging at error level just before throwing, so operators get the same clear log line they had before in addition to the stack trace. Right now the only context downstream is the exception message.
A more domain-specific exception (e.g. DatabaseConnectionException) would communicate intent better and let callers/tests assert on the type unambiguously, but IllegalStateException is acceptable for fail-fast init.
Agree with the PR description that retry belongs at the orchestration layer (k8s / docker-compose restart), so leaving that out is fine.
Missing package declaration. Every other file under src/test/kotlin declares its package (e.g. package com.wire.apps.polls.services in PollServiceTest.kt). This new file has none, which puts SetupTest in the root/default package — inconsistent with the rest of the suite and likely to trip ktlint/style checks. Add:
packagecom.wire.apps.polls.setup
Test likely passes for the wrong reason.connectDatabase() does:
val dbConfig by closestDI().instance<DatabaseConfiguration>()
DatabaseSetup.connect(dbConfig)
if (DatabaseSetup.isConnected()) { ... } else { error(...) }
dbConfig is a lazy delegate — accessing it triggers a Kodein lookup. testApplication { } does not install Kodein, so when DatabaseSetup.connect(dbConfig) is evaluated, reading dbConfig throws (Kodein's not-found / DI-missing error, which surfaces as some flavor of IllegalStateException). The test would still satisfy assertFailsWith<IllegalStateException>, but the isConnected() == false branch is never reached — so the test doesn't actually exercise the new fail-fast code path.
To genuinely cover the change, install Kodein inside testApplication and bind a stub DatabaseConfiguration, e.g.:
You could also assert on the exception message (assertFailsWith<IllegalStateException> { ... }.message contains "Unable to connect") to make the test specific to the new branch rather than any IllegalStateException.
No unmockkObject / unmockkAll cleanup.mockkObject(DatabaseSetup) registers a global static mock for the duration of the JVM; without cleanup it leaks into any other test that touches DatabaseSetup in the same run. Add an @AfterEach (or try { ... } finally { unmockkObject(DatabaseSetup) }) to clean up.
testApplication may be heavier than needed. Since connectDatabase() only needs an Application receiver, and you're mocking DatabaseSetup anyway, you could likely just construct a lightweight test application without spinning up the full ktor test host — but testApplication is acceptable if you prefer the consistency.
build.gradle.kts
Adding ktor-server-test-host as testImplementation is fine. Pinning it to ktorVersion keeps versions aligned — good.
Summary
Production change LGTM. Please:
add the missing package declaration,
ensure the test actually drives the isConnected() == false branch (install Kodein with a stub DatabaseConfiguration, and ideally assert on the exception message),
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Submission Checklist for internal contributors
The PR Title
SQPIT-764The PR Description
What's new in this PR?
Issues
The program execution continued despite no database connection which makes it unusable.
Solutions
Throw on connection failure.
Testing
Test Coverage
How to Test
Run the app without running database. The program should terminate with clear error.
Notes
I didn't implement retry mechanism as this can be done at container level if necessary.
References
feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.