Skip to content

feat(clustering): allow injecting coroutine contexts into ClusterManager - #1822

Merged
dkhawk merged 2 commits into
mainfrom
feat/cluster-manager-dispatchers
Oct 20, 2026
Merged

dkhawk merged 2 commits into
mainfrom
feat/cluster-manager-dispatchers

Conversation

@kikoso

@kikoso kikoso commented Oct 17, 2026

Copy link
Copy Markdown
Collaborator

ClusterManager hardcodes Dispatchers.Main for delivering clusters to the renderer and Dispatchers.Default for computing them. Tests can swap Main with Dispatchers.setMain(), but there is no way to replace Default, so cluster() always hops to a real background thread and tests that depend on it are flaky.

This adds a constructor that takes a mainContext and a backgroundContext:

ClusterManager<MyItem>(context, map, markerManager, mainDispatcher, backgroundDispatcher)

The existing constructors are unchanged and keep using Dispatchers.Main / Dispatchers.Default. The new parameters live on a separate constructor rather than as extra default arguments, because adding defaulted parameters would change the synthetic $default constructor that already-compiled Kotlin callers link against. apiDump shows only one added constructor.

Note that this covers the clustering computation in ClusterManager.cluster(). DefaultClusterRenderer still uses its own executor and main-looper handler for marker rendering.

Testing

  • New ClusterManagerTest.testClusterRunsOnInjectedContexts uses a StandardTestDispatcher for both contexts and checks clusters reach the renderer only after the scheduler advances.
  • ./gradlew :clustering:testDebugUnitTest :clustering:apiCheck spotlessCheck :clustering:lint passes.

@kikoso
kikoso marked this pull request as ready for review October 17, 2026 09:29
@googlemaps-bot

googlemaps-bot commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor

Code Coverage

Overall Project 60.61% -0.11% 🍏
Files changed 66.18% 🍏

Module Coverage
Kover Gradle Plugin XML report for :clustering 33.84% -0.45% 🍏
Files
Module File Coverage
Kover Gradle Plugin XML report for :clustering ClusterManager.kt 78.14% -7.92% 🍏

@dkhawk

dkhawk commented Oct 19, 2026

Copy link
Copy Markdown
Contributor

Thanks for this PR, @kikoso! Injecting dispatchers into ClusterManager is a great addition for test determinism.

Here are a few recommendations before landing:

1. Standardize on map parameter name

In the primary constructor, the parameter is still mMap: GoogleMap, while in the secondary constructor it was renamed to map: GoogleMap. Let's standardize on map in both constructors to retire the legacy Hungarian notation consistently.

2. Convenience overloads omitting MarkerManager

Right now, injecting contexts requires supplying all 5 arguments including MarkerManager(map). Adding these convenience overloads makes testing and usage much more ergonomic:

/**
 * Injects both main and background contexts while using default MarkerManager.
 */
public constructor(
    context: Context,
    map: GoogleMap,
    mainContext: CoroutineContext,
    backgroundContext: CoroutineContext,
) : this(context, map, MarkerManager(map), mainContext, backgroundContext)

/**
 * Injects background context for off-main-thread cluster computation,
 * while defaulting to Dispatchers.Main and default MarkerManager.
 */
public constructor(
    context: Context,
    map: GoogleMap,
    backgroundContext: CoroutineContext,
) : this(context, map, MarkerManager(map), Dispatchers.Main, backgroundContext)

3. Guard scope with SupervisorJob

private val scope = CoroutineScope(mainContext + SupervisorJob())

If mainContext is a dispatcher (without a job), CoroutineScope creates a regular Job(), which cancels permanently if any child task encounters an unhandled exception. Adding SupervisorJob() prevents a single failure from disabling subsequent cluster() calls.

4. Test assertion clarity (ClusterManagerTest.kt)

Because setting manager.renderer = renderer executes an initial cluster() run during setup, renderer.onClustersChanged was already called once before manager.addItem(...). As written, verify(exactly = 1) right after manager.cluster() looks like it is asserting that cluster() executed immediately.

Clearing the mock after setup makes the intention explicit:

val renderer = mockk<ClusterRenderer<TestItem>>(relaxed = true)
manager.renderer = renderer
scheduler.advanceUntilIdle()
clearMocks(renderer)

manager.addItem(TestItem(10.0, 10.0))
manager.cluster()
verify(exactly = 0) { renderer.onClustersChanged(any()) }

scheduler.advanceUntilIdle()
verify(exactly = 1) { renderer.onClustersChanged(match { it.size == 1 }) }

Rename mMap to map, add convenience constructors without MarkerManager,
use a SupervisorJob for the cluster scope and clear the renderer mock
in the injected contexts test.
@kikoso

kikoso commented Oct 20, 2026

Copy link
Copy Markdown
Collaborator Author

Let's standardize on map in both constructors to retire the legacy Hungarian notation consistently.

Done.

Adding these convenience overloads makes testing and usage much more ergonomic

Added both. The types don't clash with the existing (Context, GoogleMap, MarkerManager) constructor, so Kotlin and Java callers both pick the right one.

Adding SupervisorJob() prevents a single failure from disabling subsequent cluster() calls.

Good point, changed to CoroutineScope(mainContext + SupervisorJob()).

Clearing the mock after setup makes the intention explicit

Agreed, it was confusing to read. The test now clears the mock after the initial run and checks exactly = 0 before advancing the scheduler.

All in 0a89a83. Thanks for the review, Dale!

@dkhawk
dkhawk merged commit f1f71f2 into main Oct 20, 2026
13 checks passed
@dkhawk
dkhawk deleted the feat/cluster-manager-dispatchers branch October 20, 2026 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants