diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index a5b99d503..f981aa9b1 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -8,6 +8,9 @@ jobs: test: runs-on: ${{ matrix.os }} strategy: + # Don't cancel the other OS legs when one flakes, so a single intermittent + # failure doesn't hide the real per-OS result. + fail-fast: false matrix: os: [ ubuntu-latest, windows-latest, macos-latest ] steps: @@ -19,7 +22,7 @@ jobs: distribution: 'temurin' java-version: 21 - name: Install dependencies and run JVM tests - run: ./gradlew jvmTest + run: ./gradlew jvmTest --info - name: Run macOS native tests if: runner.os == 'macOS' run: ./gradlew macosArm64Test diff --git a/core/src/commonMain/kotlin/dev/kdriver/core/browser/Browser.kt b/core/src/commonMain/kotlin/dev/kdriver/core/browser/Browser.kt index fbf1eb842..b64e80989 100644 --- a/core/src/commonMain/kotlin/dev/kdriver/core/browser/Browser.kt +++ b/core/src/commonMain/kotlin/dev/kdriver/core/browser/Browser.kt @@ -40,7 +40,7 @@ interface Browser { * * If you want to tabs, consider using [tabs] property instead. */ - val targets: MutableList + val targets: List /** * The WebSocket URL for the browser's debugger. diff --git a/core/src/commonMain/kotlin/dev/kdriver/core/browser/DefaultBrowser.kt b/core/src/commonMain/kotlin/dev/kdriver/core/browser/DefaultBrowser.kt index 1e2b0c750..a91c01bf7 100644 --- a/core/src/commonMain/kotlin/dev/kdriver/core/browser/DefaultBrowser.kt +++ b/core/src/commonMain/kotlin/dev/kdriver/core/browser/DefaultBrowser.kt @@ -15,6 +15,7 @@ import io.ktor.util.logging.* import kotlinx.coroutines.* import kotlinx.coroutines.sync.Mutex import kotlinx.coroutines.sync.withLock +import kotlin.concurrent.Volatile import kotlin.time.Duration.Companion.seconds /** @@ -36,16 +37,25 @@ open class DefaultBrowser( override var info: ContraDict? = null - override val targets: MutableList = mutableListOf() + // The canonical registry: mutated only while holding [updateTargetInfoMutex]. After each + // mutation an immutable copy is published to [targetsSnapshot] so the non-suspend getters + // below can read a consistent view without the lock (ISSUE-5). + private val mutableTargets = mutableListOf() + + @Volatile + private var targetsSnapshot: List = emptyList() + + override val targets: List + get() = targetsSnapshot override val websocketUrl: String get() = info?.webSocketDebuggerUrl ?: error("Browser not yet started. Call start() first") override val mainTab: Tab? - get() = targets.filterIsInstance().maxByOrNull { it.type == "page" } + get() = targetsSnapshot.filterIsInstance().maxByOrNull { it.type == "page" } override val tabs: List - get() = targets.filterIsInstance().filter { it.type == "page" } + get() = targetsSnapshot.filterIsInstance().filter { it.type == "page" } /* override val cookies: CookieJar @@ -65,6 +75,9 @@ open class DefaultBrowser( companion object { + private const val TARGET_WAIT_TIMEOUT_MS = 10_000L + private const val TARGET_POLL_INTERVAL_MS = 50L + /** * The entry point for creating a new Browser instance. * @@ -96,6 +109,67 @@ open class DefaultBrowser( protected open fun createTab(websocketUrl: String, targetInfo: Target.TargetInfo): Tab = DefaultTab(websocketUrl, coroutineScope, targetInfo, this) + /** + * Adds the target if unknown — page targets become [Tab]s, everything else a plain connection — + * or updates the [Target.TargetInfo] of the existing entry. Dedupes by `targetId` so a target + * discovered by both [updateTargets] and a `targetCreated` event can't appear twice, and so page + * targets are consistently [Tab]s (ISSUE-6). + * + * Must be called while holding [updateTargetInfoMutex]. + */ + private fun upsertTarget(targetInfo: Target.TargetInfo) { + val existing = mutableTargets.firstOrNull { it.targetId == targetInfo.targetId } + if (existing != null) { + existing.targetInfo = targetInfo + } else { + val wsUrl = "ws://${config.host}:${config.port}/devtools/${targetInfo.type}/${targetInfo.targetId}" + val created = if (targetInfo.type == "page") createTab(wsUrl, targetInfo) + else createConnection(wsUrl, targetInfo) + mutableTargets.add(created) + logger.debug("target added => $created") + } + publishTargetsSnapshot() + } + + /** Publishes an immutable copy of [mutableTargets]. Must be called while holding the mutex. */ + private fun publishTargetsSnapshot() { + targetsSnapshot = mutableTargets.toList() + } + + private fun findPageTab(predicate: (Tab) -> Boolean): Tab? = + targetsSnapshot.filterIsInstance().firstOrNull { it.type == "page" && predicate(it) } + + /** + * Waits for a page [Tab] matching [predicate] to appear in the registry, instead of assuming it + * is already present. The matching `Tab` is only registered once the asynchronous `targetCreated` + * event is processed, which races `createTarget()` returning (ISSUE-8). + */ + private suspend fun awaitPageTab(timeoutMillis: Long, predicate: (Tab) -> Boolean): Tab = + withTimeoutOrNull(timeoutMillis) { + var found = findPageTab(predicate) + while (found == null) { + delay(TARGET_POLL_INTERVAL_MS) + found = findPageTab(predicate) + } + found + } ?: throw IllegalStateException("Timed out waiting for a page target after ${timeoutMillis}ms") + + /** + * Best-effort wait, during [start], for the initial page [Tab] to be registered, so `mainTab` + * (and tests built on `browser.mainTab`) are ready when `start()` returns instead of racing the + * asynchronous target discovery (ISSUE-6 residual). Re-runs [updateTargets] each poll so it + * works whether or not auto-discovery events are enabled. Does NOT throw on timeout — a browser + * may legitimately have no page yet, in which case `mainTab` stays null as before. + */ + internal suspend fun awaitInitialPageTab() { + withTimeoutOrNull(TARGET_WAIT_TIMEOUT_MS) { + while (findPageTab { true } == null) { + delay(TARGET_POLL_INTERVAL_MS) + updateTargets() + } + } + } + override suspend fun wait(timeout: Long): Browser { delay(timeout) return this @@ -121,11 +195,11 @@ open class DefaultBrowser( newWindow = newWindow, enableBeginFrameControl = true ) - targets.filterIsInstance().first { it.type == "page" && it.targetId == targetId.targetId }.also { + awaitPageTab(TARGET_WAIT_TIMEOUT_MS) { it.targetId == targetId.targetId }.also { if (it is OwnedConnection) it.owner = this } } else { - targets.filterIsInstance().first { it.type == "page" }.also { + awaitPageTab(TARGET_WAIT_TIMEOUT_MS) { true }.also { it.page.navigate(url) if (it is OwnedConnection) it.owner = this } @@ -224,49 +298,27 @@ open class DefaultBrowser( } updateTargets() + // Ensure the initial page target is registered before returning, so mainTab is ready + // (closes the residual ISSUE-6 startup race that flakes browser.mainTab ?: error(...)). + awaitInitialPageTab() return this } private suspend fun handleTargetUpdate(event: Any) = updateTargetInfoMutex.withLock { when (event) { - is Target.TargetInfoChangedParameter -> { - val targetInfo = event.targetInfo - val currentTab = targets.firstOrNull { it.targetId == targetInfo.targetId } ?: run { - logger.warn("TargetInfoChangedParameter: Target with ID ${targetInfo.targetId} not found in current targets.") - return - } - val currentTarget = currentTab.targetInfo - - logger.debug("target #${targets.indexOf(currentTab)} has changed") - /* - if (logger.isLoggable(Level.FINE)) { - val changes = compareTargetInfo(currentTarget, targetInfo) - val changesString = changes.joinToString("\n") { (key, old, new) -> - "$key: $old => $new" - } - logger.fine("target #${targets.indexOf(currentTab)} has changed: \n$changesString") - } - */ - - currentTab.targetInfo = targetInfo - } - - is Target.TargetCreatedParameter -> { - val targetInfo = event.targetInfo - val wsUrl = "ws://${config.host}:${config.port}/devtools/${targetInfo.type}/${targetInfo.targetId}" - - val newTarget = createTab(wsUrl, targetInfo) - targets.add(newTarget) - logger.debug("target ${targets.size - 1} created => $newTarget") - } + // Both create and info-changed go through the same dedupe-by-targetId upsert, so an + // info-changed for an as-yet-unregistered target registers it rather than being dropped. + is Target.TargetInfoChangedParameter -> upsertTarget(event.targetInfo) + is Target.TargetCreatedParameter -> upsertTarget(event.targetInfo) is Target.TargetDestroyedParameter -> { - val currentTab = targets.firstOrNull { it.targetId == event.targetId } ?: run { + val current = mutableTargets.firstOrNull { it.targetId == event.targetId } ?: run { logger.warn("TargetDestroyedParameter: Target with ID ${event.targetId} not found in current targets.") - return + return@withLock } - logger.debug("target removed. id ${targets.indexOf(currentTab)} => $currentTab") - targets.remove(currentTab) + logger.debug("target removed => $current") + mutableTargets.remove(current) + publishTargetsSnapshot() } is Target.TargetCrashedParameter -> { @@ -318,16 +370,10 @@ open class DefaultBrowser( } override suspend fun updateTargets() { - getTargets().forEach { t -> - val existingTab = this.targets.firstOrNull { it.targetInfo?.targetId == t.targetId } - - if (existingTab != null) { - existingTab.targetInfo = t.copy() // ou update manuellement les champs si nécessaire - } else { - val wsUrl = "ws://${config.host}:${config.port}/devtools/page/${t.targetId}" - val newConnection = createConnection(wsUrl, t) - this.targets.add(newConnection) - } + // Fetch outside the lock (it's a network round-trip), then apply atomically (ISSUE-5). + val targetInfos = getTargets() + updateTargetInfoMutex.withLock { + targetInfos.forEach { upsertTarget(it) } } yield() // equivalent to asyncio.sleep(0) diff --git a/core/src/jvmTest/kotlin/dev/kdriver/core/browser/TargetRegistryTest.kt b/core/src/jvmTest/kotlin/dev/kdriver/core/browser/TargetRegistryTest.kt new file mode 100644 index 000000000..cc9e67823 --- /dev/null +++ b/core/src/jvmTest/kotlin/dev/kdriver/core/browser/TargetRegistryTest.kt @@ -0,0 +1,146 @@ +package dev.kdriver.core.browser + +import dev.kdriver.cdp.CommandMode +import dev.kdriver.cdp.Serialization +import dev.kdriver.cdp.domain.Target +import dev.kdriver.core.connection.DefaultConnection +import dev.kdriver.core.tab.DefaultTab +import dev.kdriver.core.tab.Tab +import kotlinx.coroutines.CompletableDeferred +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.launch +import kotlinx.coroutines.test.advanceUntilIdle +import kotlinx.coroutines.test.runCurrent +import kotlinx.coroutines.test.runTest +import kotlinx.io.files.Path +import kotlinx.serialization.json.JsonElement +import kotlinx.serialization.json.encodeToJsonElement +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertNotNull +import kotlin.test.assertNull +import kotlin.test.assertTrue + +/** + * Target-registry behavior of [DefaultBrowser] (audit ISSUE-6 / ISSUE-8): page targets are + * registered as [Tab]s and deduped, and `get()` waits for the target to appear rather than + * assuming it is already present. + * + * Driven through a fake [connection] (a [DefaultConnection] whose `callCommand` is stubbed), so no + * real browser is required. + */ +class TargetRegistryTest { + + private fun pageInfo(id: String) = Target.TargetInfo( + targetId = id, + type = "page", + title = "", + url = "about:blank", + attached = true, + canAccessOpener = false, + ) + + /** Fake connection: answers `Target.getTargets` with [targetInfos]; everything else is a no-op. */ + private class FakeConnection( + scope: CoroutineScope, + var targetInfos: List, + ) : DefaultConnection("ws://stub/devtools/browser/stub", scope) { + override suspend fun callCommand(method: String, parameter: JsonElement?, mode: CommandMode): JsonElement? = + when (method) { + "Target.getTargets" -> + Serialization.json.encodeToJsonElement(Target.GetTargetsReturn(targetInfos = targetInfos)) + else -> null + } + } + + /** A [DefaultTab] whose CDP calls (e.g. `page.navigate`) are stubbed out. */ + private class FakeTab( + scope: CoroutineScope, + targetInfo: Target.TargetInfo, + ) : DefaultTab("ws://stub/devtools/page/stub", scope, targetInfo) { + override suspend fun callCommand(method: String, parameter: JsonElement?, mode: CommandMode): JsonElement? = + when (method) { + // navigate() decodes a non-null NavigateReturn; everything else is unused here. + "Page.navigate" -> Serialization.json.parseToJsonElement("""{"frameId":"frame1"}""") + else -> null + } + } + + private class TestBrowser(scope: CoroutineScope) : DefaultBrowser( + scope, + Config(browserExecutablePath = Path("/fake/chrome"), host = "127.0.0.1", port = 9222, autoDiscoverTargets = false), + ) { + override fun createTab(websocketUrl: String, targetInfo: Target.TargetInfo): Tab = FakeTab(coroutineScope, targetInfo) + } + + @Test + fun updateTargets_registersPageTargetAsTab() = runTest { + val browser = TestBrowser(this) + browser.connection = FakeConnection(this, listOf(pageInfo("t1"))) + + browser.updateTargets() + + assertEquals(1, browser.targets.size) + assertEquals(1, browser.tabs.size, "a page target must be registered as a Tab (visible to tabs/mainTab)") + assertTrue(browser.targets.first() is Tab) + } + + @Test + fun updateTargets_dedupesByTargetId() = runTest { + val browser = TestBrowser(this) + browser.connection = FakeConnection(this, listOf(pageInfo("t1"))) + + browser.updateTargets() + browser.updateTargets() // same target discovered again + + assertEquals(1, browser.targets.size, "repeat discovery must not duplicate a target") + } + + @Test + fun get_waitsForPageTabToAppear_insteadOfThrowing() = runTest { + val browser = TestBrowser(this) + val connection = FakeConnection(this, emptyList()) // no page tab registered yet + browser.connection = connection + + val outcome = CompletableDeferred>() + launch { + outcome.complete(runCatching { browser.get("https://example.com", newTab = false, newWindow = false) }) + } + + // Let get() start and reach its wait (no page tab exists yet). + runCurrent() + + // The page target now appears (e.g. the initial page registering after start-up). + connection.targetInfos = listOf(pageInfo("t1")) + browser.updateTargets() + + advanceUntilIdle() + + val result = outcome.await() + assertTrue(result.isSuccess, "get() should wait for the page target, not throw NoSuchElementException") + } + + @Test + fun awaitInitialPageTab_makesMainTabReadyOncePageAppears() = runTest { + val browser = TestBrowser(this) + val connection = FakeConnection(this, emptyList()) // no page target yet + browser.connection = connection + + // Without waiting, mainTab is null right after discovery — the startup race that flakes + // `browser.mainTab ?: error(...)`. + browser.updateTargets() + assertNull(browser.mainTab) + + // start()'s tail waits for the page; run it concurrently. + val job = launch { browser.awaitInitialPageTab() } + runCurrent() + assertNull(browser.mainTab, "still no page registered while waiting") + + // The initial page now appears. + connection.targetInfos = listOf(pageInfo("t1")) + advanceUntilIdle() + + assertNotNull(browser.mainTab, "mainTab must be ready once awaitInitialPageTab returns") + job.join() + } +} diff --git a/core/src/jvmTest/kotlin/dev/kdriver/core/network/FetchInterceptionTest.kt b/core/src/jvmTest/kotlin/dev/kdriver/core/network/FetchInterceptionTest.kt index 84b34284e..af6fdc5a9 100644 --- a/core/src/jvmTest/kotlin/dev/kdriver/core/network/FetchInterceptionTest.kt +++ b/core/src/jvmTest/kotlin/dev/kdriver/core/network/FetchInterceptionTest.kt @@ -23,8 +23,8 @@ class FetchInterceptionTest { Network.ResourceType.XHR ) { tab.get(sampleFile("profile.html")) - val originalResponse = withTimeout(3000L) { getResponseBody() } - withTimeout(3000L) { continueRequest() } + val originalResponse = withTimeout(10_000L) { getResponseBody() } + withTimeout(10_000L) { continueRequest() } originalResponse } @@ -42,8 +42,8 @@ class FetchInterceptionTest { Fetch.RequestStage.RESPONSE, ) { tab.get(sampleFile("profile.html")) - val originalResponse = withTimeout(3000L) { getResponseBody() } - withTimeout(3000L) { continueRequest() } + val originalResponse = withTimeout(10_000L) { getResponseBody() } + withTimeout(10_000L) { continueRequest() } originalResponse } @@ -62,12 +62,12 @@ class FetchInterceptionTest { Network.ResourceType.XHR ) { tab.get(sampleFile("profile.html")) - withTimeout(3000L) { continueRequest() } + withTimeout(10_000L) { continueRequest() } reset() tab.reload() - val originalResponse = withTimeout(3000L) { getResponseBody() } - withTimeout(3000L) { continueRequest() } + val originalResponse = withTimeout(10_000L) { getResponseBody() } + withTimeout(10_000L) { continueRequest() } originalResponse } diff --git a/core/src/jvmTest/kotlin/dev/kdriver/core/network/RequestExpectationTest.kt b/core/src/jvmTest/kotlin/dev/kdriver/core/network/RequestExpectationTest.kt index 19df15b97..4c6e8962c 100644 --- a/core/src/jvmTest/kotlin/dev/kdriver/core/network/RequestExpectationTest.kt +++ b/core/src/jvmTest/kotlin/dev/kdriver/core/network/RequestExpectationTest.kt @@ -20,9 +20,9 @@ class RequestExpectationTest { tab.expect(Regex("groceries.html")) { tab.get(sampleFile("groceries.html")) - val request = withTimeout(3000L) { this@expect.getRequest() } - val response = withTimeout(3000L) { this@expect.getResponse() } - val responseBody = withTimeout(3000L) { this@expect.getRawResponseBody() } + val request = withTimeout(10_000L) { this@expect.getRequest() } + val response = withTimeout(10_000L) { this@expect.getResponse() } + val responseBody = withTimeout(10_000L) { this@expect.getRawResponseBody() } assertEquals(request.url, response.url) assertEquals(200, response.status) @@ -40,9 +40,9 @@ class RequestExpectationTest { tab.expect { tab.get(sampleFile("groceries.html")) - val request = withTimeout(3000L) { this@expect.getRequest() } - val response = withTimeout(3000L) { this@expect.getResponse() } - val responseBody = withTimeout(3000L) { this@expect.getRawResponseBody() } + val request = withTimeout(10_000L) { this@expect.getRequest() } + val response = withTimeout(10_000L) { this@expect.getResponse() } + val responseBody = withTimeout(10_000L) { this@expect.getRawResponseBody() } assertEquals(request.url, response.url) assertEquals(200, response.status) @@ -64,9 +64,9 @@ class RequestExpectationTest { tab.reload() tab.waitForReadyState(ReadyState.COMPLETE) - val request = withTimeout(3000L) { this@expect.getRequest() } - val response = withTimeout(3000L) { this@expect.getResponse() } - val responseBody = withTimeout(3000L) { this@expect.getRawResponseBody() } + val request = withTimeout(10_000L) { this@expect.getRequest() } + val response = withTimeout(10_000L) { this@expect.getResponse() } + val responseBody = withTimeout(10_000L) { this@expect.getRawResponseBody() } assertEquals(request.url, response.url) assertEquals(200, response.status) @@ -90,13 +90,13 @@ class RequestExpectationTest { val pageExp = expectations[pagePattern] ?: error("Missing expectation for profile.html") val apiExp = expectations[apiPattern] ?: error("Missing expectation for user-data.json") - val pageResponse = withTimeout(3000L) { pageExp.getResponse() } - val apiResponse = withTimeout(3000L) { apiExp.getResponse() } + val pageResponse = withTimeout(10_000L) { pageExp.getResponse() } + val apiResponse = withTimeout(10_000L) { apiExp.getResponse() } assertEquals(200, pageResponse.status) assertEquals(200, apiResponse.status) - val userData = withTimeout(3000L) { apiExp.getResponseBody() } + val userData = withTimeout(10_000L) { apiExp.getResponseBody() } assertEquals("Zendriver", userData.name) } diff --git a/core/src/jvmTest/kotlin/dev/kdriver/core/tab/TabTest.kt b/core/src/jvmTest/kotlin/dev/kdriver/core/tab/TabTest.kt index 2c3660d12..f97cfec70 100644 --- a/core/src/jvmTest/kotlin/dev/kdriver/core/tab/TabTest.kt +++ b/core/src/jvmTest/kotlin/dev/kdriver/core/tab/TabTest.kt @@ -180,8 +180,8 @@ class TabTest { tab.reload() tab.waitForReadyState(ReadyState.COMPLETE) - withTimeout(1000) { assertTrue(handle1Called.await()) } - withTimeout(1000) { assertTrue(handle2Called.await()) } + withTimeout(10_000) { assertTrue(handle1Called.await()) } + withTimeout(10_000) { assertTrue(handle2Called.await()) } job1.cancel() job2.cancel()