arkivanov / MVIKotlin

Extendable MVI framework for Kotlin Multiplatform with powerful debugging tools (logging and time travel)
https://arkivanov.github.io/MVIKotlin
Apache License 2.0
847 stars 32 forks source link

Failure creating stores in tests when Paparazzi plugin applied #91

Closed dalewking closed 1 year ago

dalewking commented 1 year ago

I am trying to add Paparazzi snapshot testing. When the paparazzi plugin is applied I get failures when it tries to create a LoggingStore. I do not get the failure when Paparazzi plugin is not applied and isAssertOnMainThreadEnabled = false does not fix it either.

It seems to be that a failure trying to log should not cause an exception so adding a try catch might be a good idea

Note also a previous issue I reported regarding the check for main thread #35

'boolean android.util.Log.isLoggable(java.lang.String, int)'
java.lang.UnsatisfiedLinkError: 'boolean android.util.Log.isLoggable(java.lang.String, int)'
    at android.util.Log.isLoggable(Native Method)
    at com.arkivanov.mvikotlin.utils.internal.Logs.isAndroidLoggerAvailable(Logs.kt:36)
    at com.arkivanov.mvikotlin.utils.internal.Logs.createLogger(Logs.kt:32)
    at com.arkivanov.mvikotlin.utils.internal.Logs.ensureLogger(Logs.kt:24)
    at com.arkivanov.mvikotlin.utils.internal.Logs.logE(Logs.kt:17)
    at com.arkivanov.mvikotlin.utils.internal.MainThreadAssert.getMainThreadId(MainThreadAssert.kt:11)
    at com.arkivanov.mvikotlin.utils.internal.MainThreadAssertKt.isMainThread(MainThreadAssert.kt:27)
    at com.arkivanov.mvikotlin.rx.internal.BaseSubject$1.invoke(BaseSubject.kt:13)
    at com.arkivanov.mvikotlin.rx.internal.BaseSubject$1.invoke(BaseSubject.kt:13)
    at com.arkivanov.mvikotlin.rx.internal.BaseSubject.subscribe(BaseSubject.kt:23)
    at com.arkivanov.mvikotlin.main.store.DefaultStore.init(DefaultStore.kt:38)
    at com.arkivanov.mvikotlin.logging.store.LoggingStore.init(LoggingStore.kt:14)
dalewking commented 1 year ago

The root of the problem (and with #35) is that this code does not honor isAssertOnMainThreadEnabled

https://github.com/arkivanov/MVIKotlin/blob/master/utils-internal/src/commonMain/kotlin/com/arkivanov/mvikotlin/utils/internal/MainThreadAssert.kt#L24

arkivanov commented 1 year ago

Thanks for raising. When running vanilla JUnit tests, accessing Android classes throws "java.lang.RuntimeException: Method isLoggable in android.util.Log not mocked". This error is caught here and further checks are disabled. For some reason Paparazzi throws UnsatisfiedLinkError which is an Error and is not currently caught. We should catch Throwable instead.

The method you are referring to (isMainThread) should not honor isAssertOnMainThreadEnabled, because that flag only affects main thread asserts, not checks. This check should go away eventually when the support of the old K/N memory model will be removed. Currently, we should perform this check only on K/N and only under the old memory model.

I will fix that.

arkivanov commented 1 year ago

Actually you are correct. It's better to not check for the main thread if asserts are disabled.