Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Flaky tests after migration to GitHubAction #349

Open
Onvistlex opened this issue Sep 2, 2024 · 1 comment
Open

Flaky tests after migration to GitHubAction #349

Onvistlex opened this issue Sep 2, 2024 · 1 comment

Comments

@Onvistlex
Copy link

First, thanks for this nice library :)

We have a lot of unit tests that we've been running locally without any problems in the past. However, after migrating these tests to a GitHub Action, we noticed that some of the tested StateFlows were collapsing multiple expected states into one, which resulted in assertion errors. It took me a while to reproduce this locally as a minimal working example. It seems that the sheer number of tests is causing this behavior:

@Suppress("OPT_IN_USAGE")
class ExampleUnitTest {

    init { Dispatchers.setMain(UnconfinedTestDispatcher()) }

    @Test
    fun `tests if success is emitted`() = runTest {
        repeat(2000) {
            SimpleViewModel().stateFlow.test {
                assertEquals("Run count: $it", "Loading", awaitItem())
                assertEquals("Run count: $it", "Success", awaitItem())
                cancelAndIgnoreRemainingEvents()
            }
        }
    }
}

suspend fun read3rdPartyData(): String = withContext(Dispatchers.IO) { "Success" }

internal class SimpleViewModel : ViewModel() {

    val stateFlow = flow {
        emit("Loading")
        emit(read3rdPartyData())
    }.catch {
        emit("Error")
    }.stateIn(
        scope = viewModelScope,
        started = SharingStarted.WhileSubscribed(5000),
        initialValue = "Loading"
    )
}

Because many of our tests are similar to this example, we wrote a more sophisticated awaitItem() function that can skip states that are not of interest (e.g., skipping the loading state when checking for the success state):

/**
 * Skips items of other types until the wanted item type is received and its predicate passes.
 */
suspend inline fun <reified T> ReceiveTurbine<in T>.awaitItemType(
    predicate: (T) -> Boolean = { true }
): T {
    var item = awaitItem()
    var predicatePassed = (item as? T)?.let(predicate) ?: false
    while (item !is T || predicatePassed.not()) {
        item = awaitItem()
        predicatePassed = (item as? T)?.let(predicate) ?: false
    }
    return item
}

Would it be possible to add a similar function to Turbine? Or did we miss something in our test setup? Any help would be appreciated! 😊

@jingibus
Copy link
Collaborator

jingibus commented Sep 4, 2024

My only concern with a function like that is that it can hide legitimate test issues: by skipping items that are not of interest, we also prevent failures that may occur when something weird is emitted that shouldn't have been.

In addition to that, I would advise auditing the code in question for usage of Dispatchers.Default or other multithreaded dispatchers in test. Multithreading will always introduce the potential for flaky tests, which may consistently pass locally and then fail on CI. This happens because the OS thread scheduler is not controlled by the test; depending on how it chooses to run those threads, different results may occur. Without multithreading, otoh, the execution path should be completely unambiguous, so no flakiness (for that reason, at least) can occur.

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

No branches or pull requests

2 participants