Skip to content

Commit

Permalink
Test that we don't leak services
Browse files Browse the repository at this point in the history
This reveals that we don't expose enough state information
on TreehouseApp.
  • Loading branch information
squarejesse committed May 29, 2024
1 parent 3988a9b commit 87013a6
Show file tree
Hide file tree
Showing 7 changed files with 99 additions and 13 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import assertk.assertions.isEmpty
import assertk.assertions.isEqualTo
import com.example.redwood.testapp.testing.TextInputValue
import kotlin.test.Test
import kotlinx.coroutines.flow.first
import kotlinx.coroutines.test.runTest

class LeaksTest {
Expand Down Expand Up @@ -55,4 +56,30 @@ class LeaksTest {

treehouseApp.stop()
}

@Test
fun serviceNotLeaked() = runTest {
val tester = TreehouseTester(this)
val treehouseApp = tester.loadApp()
treehouseApp.start()
tester.eventLog.takeEvent("test_app.codeLoadSuccess()", skipOthers = true)

// Wait for Zipline to be ready.
// TODO(jwilson): consider deferring events or exposing the TreehouseApp state. As-is the
// codeLoadSuccess() event occurs on the Zipline dispatcher but we don't have an instance
// here until we bounce that information to the main dispatcher.
treehouseApp.zipline.first { it != null }

val serviceLeakWatcher = LeakWatcher {
tester.hostApi // The first instance of HostApi is held by the current run of the test app.
}

// Stop referencing this HostApi from our test harness.
tester.hostApi = FakeHostApi()

// Stop the app. Even though we still reference the app, it stops referencing hostApi.
treehouseApp.stop()
tester.eventLog.takeEvent("test_app.codeUnloaded()", skipOthers = true)
serviceLeakWatcher.assertNotLeaked()
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
/*
* Copyright (C) 2024 Square, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package app.cash.redwood.treehouse

import com.example.redwood.testapp.treehouse.HostApi

class FakeHostApi : HostApi {
override suspend fun httpCall(url: String, headers: Map<String, String>): String {
error("unexpected call")
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ import okio.SYSTEM
internal class TreehouseTester(
private val testScope: TestScope,
) {
private val eventLog = EventLog()
val eventLog = EventLog()

@OptIn(ExperimentalStdlibApi::class)
private val testDispatcher = testScope.coroutineContext[CoroutineDispatcher.Key] as TestDispatcher
Expand Down Expand Up @@ -96,11 +96,7 @@ internal class TreehouseTester(
override fun create(scope: CoroutineScope, dispatchers: TreehouseDispatchers) = frameClock
}

private val hostApi = object : HostApi {
override suspend fun httpCall(url: String, headers: Map<String, String>): String {
error("unexpected call")
}
}
var hostApi: HostApi = FakeHostApi()

private val treehouseAppFactory = TreehouseApp.Factory(
platform = platform,
Expand All @@ -118,14 +114,14 @@ internal class TreehouseTester(

private val appSpec = object : TreehouseApp.Spec<TestAppPresenter>() {
override val name: String
get() = "test-app"
get() = "test_app"
override val manifestUrl: Flow<String>
get() = this@TreehouseTester.manifestUrl
override val loadCodeFromNetworkOnly: Boolean
get() = true

override fun bindServices(zipline: Zipline) {
zipline.bind<HostApi>("HostApi", hostApi)
zipline.bind("HostApi", hostApi)
}

override fun create(zipline: Zipline): TestAppPresenter {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,13 @@
*/
package app.cash.redwood.treehouse

import app.cash.zipline.Zipline
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.SupervisorJob
import kotlinx.coroutines.cancel
import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.MutableStateFlow
import kotlinx.coroutines.flow.StateFlow
import kotlinx.coroutines.job
import kotlinx.coroutines.launch

Expand Down Expand Up @@ -56,6 +59,12 @@ internal abstract class CodeHost<A : AppService>(

private var state: State<A> = State.Idle()

/** Updated with [State]. */
private val mutableZipline = MutableStateFlow<Zipline?>(null)

val zipline: StateFlow<Zipline?>
get() = mutableZipline

private val codeSessionListener = object : CodeSession.Listener<A> {
override fun onUncaughtException(codeSession: CodeSession<A>, exception: Throwable) {
}
Expand All @@ -69,6 +78,7 @@ internal abstract class CodeHost<A : AppService>(
val previous = state
if (previous is State.Running) {
state = State.Crashed(previous.codeUpdatesScope)
mutableZipline.value = null
}
}
}
Expand Down Expand Up @@ -104,6 +114,7 @@ internal abstract class CodeHost<A : AppService>(
previous.codeSession?.stop()

state = State.Idle()
mutableZipline.value = null
}

fun restart() {
Expand All @@ -118,6 +129,7 @@ internal abstract class CodeHost<A : AppService>(

val codeUpdatesScope = newCodeUpdatesScope()
state = State.Starting(codeUpdatesScope)
mutableZipline.value = null
codeUpdatesScope.collectCodeUpdates()
}

Expand Down Expand Up @@ -152,14 +164,15 @@ internal abstract class CodeHost<A : AppService>(
previous.codeSession?.stop()

// If the codeUpdatesScope is null, we're stopped. Discard the newly-loaded code.
val codeUpdatesScope = state.codeUpdatesScope
val codeUpdatesScope = previous.codeUpdatesScope
if (codeUpdatesScope == null) {
next.stop()
return@launch
}

// Boot up the new code.
state = State.Running(codeUpdatesScope, next)
mutableZipline.value = (next as? ZiplineCodeSession)?.zipline
next.addListener(codeSessionListener)
next.start()

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import app.cash.zipline.loader.ZiplineLoader
import kotlin.native.ObjCName
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.StateFlow
import kotlinx.coroutines.flow.mapNotNull
import kotlinx.serialization.modules.EmptySerializersModule
import kotlinx.serialization.modules.SerializersModule
Expand Down Expand Up @@ -74,8 +75,8 @@ public class TreehouseApp<A : AppService> private constructor(
* It is unwise to use this instance for anything beyond measurement and monitoring, because the
* instance may be replaced if new code is loaded.
*/
public val zipline: Zipline?
get() = (codeHost.codeSession as? ZiplineCodeSession)?.zipline
public val zipline: StateFlow<Zipline?>
get() = codeHost.zipline

/**
* Create content for [source].
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,14 @@ class EventLog {
return events.receive()
}

suspend fun takeEvent(event: String) {
assertThat(takeEvent()).isEqualTo(event)
suspend fun takeEvent(event: String, skipOthers: Boolean = false) {
while (true) {
val actual = takeEvent()
if (skipOthers && actual != event) continue

assertThat(actual).isEqualTo(event)
return
}
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ package app.cash.redwood.treehouse

import app.cash.redwood.protocol.EventTag
import app.cash.redwood.protocol.WidgetTag
import app.cash.zipline.Zipline
import app.cash.zipline.ZiplineManifest

class FakeEventListener(
private val eventLog: EventLog,
Expand All @@ -29,6 +31,23 @@ class FakeEventListener(
FakeEventListener(eventLog, app)
}

override fun codeLoadStart(): Any? {
eventLog += "${app.spec.name}.codeLoadStart()"
return null
}

override fun codeLoadSuccess(manifest: ZiplineManifest, zipline: Zipline, startValue: Any?) {
eventLog += "${app.spec.name}.codeLoadSuccess()"
}

override fun codeLoadFailed(exception: Exception, startValue: Any?) {
eventLog += "${app.spec.name}.codeLoadFailed()"
}

override fun codeUnloaded() {
eventLog += "${app.spec.name}.codeUnloaded()"
}

override fun unknownEvent(widgetTag: WidgetTag, tag: EventTag) {
eventLog += "${app.spec.name}.unknownEvent($widgetTag, $tag)"
}
Expand Down

0 comments on commit 87013a6

Please sign in to comment.