test: add instrumented tests for active session

[matthiasemde]

Story

This PR adds tests for the active session. As you can imagine, this is kinda complicated because the active session has many dependencies like multiple services, use cases and repositories.

Modifications to the main code

NotificationManager

One thing I stumbled over at first was the fact that even the simplest test would fail with the error Bad notification for startForeground(). This lead me down the rabbit hole of trying to "fix" the service or somehow inject a test service. On the way I wrote a simple test for the session service, which doesn't really do much, because the service only shows a notification and has no public methods which could be tested.

Back to the error in the active session test. The Bad notification for startForeground() didn't have anything to do with the service per se, but rather with the non-existence of the notification channels which are normally created by the Musikus : Application() class. However, the application appears not to get instantiated for instrumented tests so I had to find a solution.

The answer, as so many times with testing, is "Dependency injection". Creating a dedicated NotificationModule which provides the NotificationManager while also creating the notification channels.

@Module
@InstallIn(SingletonComponent::class)
object NotificationModule {

  @Provides
  @Singleton
  fun provideNotificationManager(
    @ApplicationContext context: Context
  ): NotificationManager {
    createNotificationChannels(context)
    return context.getSystemService(Context.NOTIFICATION_SERVICE) as NotificationManager
  }
}

Works like a charm 👌

TimeProvider

A potentially controversial change in this PR is the discretization of the clock in TimeProvider. The interface now looks like this:

interface TimeProvider {
    fun now(): ZonedDateTime

    val clock: Flow<ZonedDateTime>

    fun localZoneId(): ZoneId = now().zone
    …
}

The interesting thing is the difference in implementation between TimeProviderImpl and FakeTimeProvider:

class TimeProviderImpl(scope: CoroutineScope) : TimeProvider {

    private var isClockRunning = false

    override val clock: StateFlow<ZonedDateTime> = flow {
        while (true) {
            emit(ZonedDateTime.now())
            delay(100.milliseconds)
        }
    }.onStart {
        isClockRunning = true
    }.onCompletion {
        isClockRunning = false
    }.stateIn(
        scope = scope,
        started = SharingStarted.WhileSubscribed(),
        initialValue = ZonedDateTime.now()
    )

    override fun now(): ZonedDateTime {
        return if (isClockRunning) {
            clock.value
        } else {
            ZonedDateTime.now()
        }
    }
}

class FakeTimeProvider : TimeProvider {
    private val _clock = MutableStateFlow(START_TIME)
    override val clock: Flow<ZonedDateTime> get() = _clock.asStateFlow()

    override fun now(): ZonedDateTime {
        return _clock.value
    }

    fun setCurrentDateTime(dateTime: ZonedDateTime) {
        _clock.update { dateTime }
    }

    fun advanceTimeBy(duration: Duration) {
        _clock.update { it.plus(duration.toJavaDuration()) }
    }
    …
}

This implementation has two main advantages:

• All UI elements which depend on the clock will get updated automatically when advanceTimeBy or setCurrentDateTime is called.
• All the get**Duration use cases are now more easily testable after being converted to compute**Duration use cases

ActiveSessionViewModel

Problems

• A problem which I ran into, is the fact that our active session UI is updated by a clock. So when trying to advance the time using fakeTimeProvider.advanceTimeBy(90.seconds) the UI didn't actually update a delay(100) had passed. This, of course, is not a nice thing to add to a test. I think we should look into modifying the TimeProvider to provide a clock-like flow of some kind, which we can then use in all our screens which need to update automatically. The FakeTimeProvider could then send the updates immediately. Done 👌
• The "Pause" button, or more specific the PauseActiveSessionUseCase does not allow the user to pause the session if the session timer is still at zero. In reality this is never a problem, but for testing it was quite tricky to figure out why pressing "Pause" wasn't working. The check could probably go into another use case... Done 👌

Learnings

Use Cases

Important

To reduce undefined behavior and improve testability, use cases should not fetch data from the repository and do too much processing at the same time, especially if the data gets combined with state from the view model.

Here is an example which led to this conclusion:

private val timerUiState = combine(
    sessionState,
    timeProvider.clock // should update with clock
) { timerState, now ->
    val pause = timerState == ActiveSessionState.PAUSED

    val practiceDuration = try {
        activeSessionUseCases.getPracticeDuration(now) // suspended call
    } catch (e: IllegalStateException) {
        Duration.ZERO // Session not yet started
    }
    val pauseDurStr = getDurationString(
        activeSessionUseCases.getOngoingPauseDuration(now),  // suspended call
        DurationFormat.MS_DIGITAL
    )
    ActiveSessionTimerUiState(
        timerText = getFormattedTimerText(practiceDuration),
        subHeadingText =
        if (pause) {
            UiText.StringResource(R.string.active_session_timer_subheading_paused, pauseDurStr)
        } else {
            UiText.StringResource(R.string.active_session_timer_subheading)
        },
    )
}

This code can lead to a race condition... Can you spot it?

Since both getPracticeDuration and getOngoingPauseDuration first request the session state from the active session repository, a race condition occurs whenever the user presses "Pause" while the code execution is somewhere in front of getOngoingPauseDuration and the clock has updated (10 Hz). In this case, currentPauseStartTimestamp in the session state will get updated with the new clock value but the value of now is still original value which leads to a negative pause duration.

Adds to: #18

[mipro98]

leaving comments so far

[mipro98]

This should be the different node, of the current section, shouldn't it? So maybe select the node more explicitly, e.g. by contentDescription.

(1) this will make it clear that it's a different node and
(2) prevents the test from passing if the modalBottomSheet does somehow not close and you are asserting the same Item as before.

[mipro98]

can you add a comment why the UUID has to be 6 exactly here (and the other ones as well)?