From e920c4b0fe73183a3bb51e648066ebd5f7be3e8c Mon Sep 17 00:00:00 2001 From: Andreas Rossbacher Date: Wed, 28 Aug 2024 09:37:07 -0700 Subject: [PATCH] Log a better error message that contains the type of the state class that could not be persisted for easier debugging. (#720) Update unit tests to also check the exception message. Co-authored-by: Andreas Rossbacher --- .../kotlin/com/airbnb/mvrx/PersistState.kt | 3 +- .../com/airbnb/mvrx/PersistedStateTest.kt | 85 ++++++++++++++----- 2 files changed, 67 insertions(+), 21 deletions(-) diff --git a/mvrx/src/main/kotlin/com/airbnb/mvrx/PersistState.kt b/mvrx/src/main/kotlin/com/airbnb/mvrx/PersistState.kt index 5b163d73..92de86e7 100644 --- a/mvrx/src/main/kotlin/com/airbnb/mvrx/PersistState.kt +++ b/mvrx/src/main/kotlin/com/airbnb/mvrx/PersistState.kt @@ -97,7 +97,8 @@ private fun Bundle.putAny(key: String?, value: T): Bundle { is Parcelable -> putParcelable(key, value) is Serializable -> putSerializable(key, value) null -> putString(key, null) - else -> error("Cannot persist $key. It must be null, Serializable, or Parcelable.") + // The null case is covered above so can avoid the null check here. + else -> error("Cannot persist '$key': Class ${value!!::class.java.name} must be null, Serializable, or Parcelable.") } return this } diff --git a/mvrx/src/test/kotlin/com/airbnb/mvrx/PersistedStateTest.kt b/mvrx/src/test/kotlin/com/airbnb/mvrx/PersistedStateTest.kt index 587d9c9e..1d8b030f 100644 --- a/mvrx/src/test/kotlin/com/airbnb/mvrx/PersistedStateTest.kt +++ b/mvrx/src/test/kotlin/com/airbnb/mvrx/PersistedStateTest.kt @@ -6,6 +6,7 @@ import android.os.Parcelable import kotlinx.parcelize.Parcelize import org.junit.Assert.assertEquals import org.junit.Assert.assertNull +import org.junit.Assert.assertThrows import org.junit.Test import org.mockito.Mockito @@ -44,12 +45,21 @@ class PersistedStateTest : BaseTest() { assertEquals(5, newState.count) } - @Test(expected = IllegalStateException::class) + @Test fun validatesMissingKeyInBundle() { - data class State(@PersistState val count: Int = 5) : MavericksState + val exception: IllegalStateException = assertThrows( + IllegalStateException::class.java + ) { + data class State(@PersistState val count: Int = 5) : MavericksState - val newState = restorePersistedMavericksState(Bundle(), State(), validation = true) - assertEquals(5, newState.count) + val newState = restorePersistedMavericksState(Bundle(), State(), validation = true) + assertEquals(5, newState.count) + } + assertEquals( + "savedInstanceState bundle should have a key for state property at position 0 but it was missing.", + // The mockito mock adds a random id string at the end. + exception.message?.substringBeforeLast("$") + ) } @Test @@ -188,11 +198,20 @@ class PersistedStateTest : BaseTest() { assertEquals(3, state.data[0].count) } - @Test(expected = IllegalStateException::class) + @Test() fun testNonParcelableList() { - data class State2(@PersistState val data: List = listOf(Mockito.mock(Context::class.java))) : MavericksState + val exception: IllegalStateException = assertThrows( + IllegalStateException::class.java + ) { + data class State2(@PersistState val data: List = listOf(Mockito.mock(Context::class.java))) : MavericksState - persistMavericksState(State2(), validation = true) + persistMavericksState(State2(), validation = true) + } + assertEquals( + "Cannot parcel android.content.Context\$MockitoMock", + // The mockito mock adds a random id string at the end. + exception.message?.substringBeforeLast("$") + ) } @Test @@ -204,11 +223,20 @@ class PersistedStateTest : BaseTest() { assertEquals(3, state.data.take(1).first().count) } - @Test(expected = IllegalStateException::class) + @Test() fun testNonParcelableSet() { - data class State2(@PersistState val data: Set = setOf(Mockito.mock(Context::class.java))) : MavericksState + val exception: IllegalStateException = assertThrows( + IllegalStateException::class.java + ) { + data class State2(@PersistState val data: Set = setOf(Mockito.mock(Context::class.java))) : MavericksState - persistMavericksState(State2(), validation = true) + persistMavericksState(State2(), validation = true) + } + assertEquals( + "Cannot parcel android.content.Context\$MockitoMock", + // The mockito mock adds a random id string at the end. + exception.message?.substringBeforeLast("$") + ) } @Test @@ -222,20 +250,37 @@ class PersistedStateTest : BaseTest() { assertEquals(3, state.data["foo"]?.count) } - @Test(expected = IllegalStateException::class) + @Test() fun testNonParcelableMap() { - data class State2( - @PersistState val data: Map = mapOf("foo" to Mockito.mock(Context::class.java)) - ) : MavericksState - - persistMavericksState(State2(), validation = true) + val exception: IllegalStateException = assertThrows( + IllegalStateException::class.java + ) { + data class State2( + @PersistState val data: Map = mapOf("foo" to Mockito.mock(Context::class.java)) + ) : MavericksState + + persistMavericksState(State2(), validation = true) + } + assertEquals( + "Cannot parcel android.content.Context\$MockitoMock", + // The mockito mock adds a random id string at the end. + exception.message?.substringBeforeLast("$") + ) } - @Test(expected = IllegalStateException::class) + @Test() fun failOnNonParcelable() { - class NonParcelableClass - data class State(@PersistState val data: NonParcelableClass = NonParcelableClass()) : MavericksState - persistMavericksState(State()) + val exception: IllegalStateException = assertThrows( + IllegalStateException::class.java + ) { + class NonParcelableClass + data class State(@PersistState val data: NonParcelableClass = NonParcelableClass()) : MavericksState + persistMavericksState(State()) + } + assertEquals( + "Cannot persist '0': Class com.airbnb.mvrx.PersistedStateTest\$failOnNonParcelable\$exception\$1\$NonParcelableClass must be null, Serializable, or Parcelable.", + exception.message + ) } @Test