Conversation
stevensJourney
approved these changes
Apr 7, 2025
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This refactors our tests opening databases (
DatabaseTestandSyncIntegrationTest) to:GlobalScopewhen opening databases. Instead, we use the test scope we receive fromrunTest. Thanks to structured concurrency, this ensures that no async database code can "leak" out of the test.kotlin-test. These assertions are more expressive and lead to much better error messages (e.g.list shouldHaveSize 3will actually print contents of the list on violations,assertEquals(3, list.size)is much less helpful).The first step required some other changes, in particular:
@Setupin tests to open databases, because that function is non-suspending and doesn't run in the test's coroutine scope. Instead, I wrote a utility (thedatabaseTestfunction) that creates anActiveDatabaseTestinstance encapsulating all that logic. This ensures that the full lifecycle (initializing databases, running tests, closing databases) happens within the test's coroutine scope.runBlockinginitializer inPowerSyncDatabaseImplis incompatible with the new tests. I think this is because the connection pool also uses the coroutine scope from tests now (which is no longer aGlobalScope). And therunBlockingthen prevents that scope from progressing? I'm not 100% sure about this, but there was some deadlock withrunBlockingwhen opening the database.A workaround that I think is also beneficial for apps is to move the initialization into a
Jobthat is then awaited before each other method. This is the same pattern we use for Dart and JavaScript as well, and ensures we're not accidentally blocking any UI thread that might be opening the PowerSync database.