Creating a SavedStateHandle by supplying an object for the route parameter introduces an Android dependency [340966212]

Fixed

Bug

Status Update

No update yet.

Description

do...@google.com

created issue #1

May 16, 2024 01:54AM

Component used: Navigation Version used: 2.8.0-beta01 Devices/Android versions reproduced on: all

aosp/3073537 introduced a new API for creating a SavedStateHandle using an object for the route parameter e.g. SavedStateHandle(route = MyDestination).

Internally, this constructor relies on android.net.Uri which means that you can only call it from an instrumented test, not a unit test.

This is undesirable since one of the use cases for this API is to be able to unit test ViewModels by passing them a SavedStateHandle containing a route.

Example: https://github.com/android/nowinandroid/pull/1413/files#diff-bbcd9ff3a5be72096922a84b290125441e34d8183492cd27536e343a903579a8R57

Comments

do...@google.com <do...@google.com> #2May 16, 2024 02:02AM

In case it's useful, here's the stacktrace when running the following tests: https://github.com/android/nowinandroid/pull/1413/files#diff-bbcd9ff3a5be72096922a84b290125441e34d8183492cd27536e343a903579a8R43

java.lang.RuntimeException: Method encode in android.net.Uri not mocked. See https://developer.android.com/r/studio-ui/build/not-mocked for details.
	at android.net.Uri.encode(Uri.java)
	at androidx.navigation.NavType$Companion$StringType$1.serializeAsValue(NavType.kt:834)
	at androidx.navigation.NavType$Companion$StringType$1.serializeAsValue(NavType.kt:803)
	at androidx.navigation.serialization.RouteBuilder$Filled$addArg$2.invoke(RouteBuilder.kt:114)
	at androidx.navigation.serialization.RouteBuilder$Filled$addArg$2.invoke(RouteBuilder.kt:110)
	at androidx.navigation.serialization.RouteBuilder$Builder.apply(RouteBuilder.kt:214)
	at androidx.navigation.serialization.RouteBuilder$Filled.addArg(RouteBuilder.kt:110)
	at androidx.navigation.serialization.RouteEncoder.encodeSerializableValue(RouteEncoder.kt:66)
	at kotlinx.serialization.encoding.Encoder$DefaultImpls.encodeNullableSerializableValue(Encoding.kt:299)
	at kotlinx.serialization.encoding.AbstractEncoder.encodeNullableSerializableValue(AbstractEncoder.kt:18)
	at kotlinx.serialization.encoding.AbstractEncoder.encodeNullableSerializableElement(AbstractEncoder.kt:90)
	at com.google.samples.apps.nowinandroid.feature.interests.navigation.InterestsDestination.write$Self$interests_demoDebug(InterestsNavigation.kt:25)
	at com.google.samples.apps.nowinandroid.feature.interests.navigation.InterestsDestination$$serializer.serialize(InterestsNavigation.kt:25)
	at com.google.samples.apps.nowinandroid.feature.interests.navigation.InterestsDestination$$serializer.serialize(InterestsNavigation.kt:25)
	at kotlinx.serialization.encoding.Encoder$DefaultImpls.encodeSerializableValue(Encoding.kt:279)
	at kotlinx.serialization.encoding.AbstractEncoder.encodeSerializableValue(AbstractEncoder.kt:18)
	at androidx.navigation.serialization.RouteEncoder.encodeRouteWithArgs(RouteEncoder.kt:49)
	at androidx.navigation.serialization.RouteSerializerKt.generateRouteWithArgs(RouteSerializer.kt:141)
	at androidx.navigation.testing.SavedStateHandleFactoryKt.invoke(SavedStateHandleFactory.kt:48)
	at androidx.navigation.testing.SavedStateHandleFactoryKt.invoke$default(SavedStateHandleFactory.kt:38)
	at com.google.samples.apps.nowinandroid.interests.InterestsViewModelTest.setup(InterestsViewModelTest.kt:61)

jb...@google.com <jb...@google.com> May 17, 2024 01:27PM

Assigned to cl...@google.com.

ap...@google.com <ap...@google.com> #3Jun 6, 2024 11:43PM

Project: platform/frameworks/support
Branch: androidx-main

commit ee37cb1535e3b4d9b27d5288e2a4c98b4773d4eb
Author: Clara Fok <clarafok@google.com>
Date: Thu Jun 06 15:53:07 2024

Refactor generateRoutePattern

Add new internal KSerializer<T>.forEachIndexed() function and applies an operation lambda on each child argument of the serializer. This function takes a Map<KType, NavType<*>>, maps each argument back to a NavType<Any?>, and provides this navType into the operation lambda.

Then extracted common RouteBuilder logic shared between RoutePattern and RouteFilled into a separate RouteBuilder class (old RouteBuilder renamed to OldRouteBuilder temporarily, entire OldRouteBuilder class to be deleted in next CL).

Finally refactored generateRoutePattern to use the new forEachIndexed() and new RouteBuilder.

Test: existing tests
Bug: 340966212
Change-Id: I7dc524d2f49fdcdc6925a5cb8c9649aa8c26bef2

M navigation/navigation-common/src/main/java/androidx/navigation/serialization/RouteBuilder.kt
M navigation/navigation-common/src/main/java/androidx/navigation/serialization/RouteEncoder.kt
M navigation/navigation-common/src/main/java/androidx/navigation/serialization/RouteSerializer.kt

https://android-review.googlesource.com/3119131

ap...@google.com <ap...@google.com> #4Jun 6, 2024 11:43PM

Project: platform/frameworks/support
Branch: androidx-main

commit dd314d59da662fc85fa187f2df9b3a42b93fc586
Author: Clara Fok <clarafok@google.com>
Date: Thu Jun 06 16:08:08 2024

Refactor generateRouteWithArgs

Add new internal KSerializer<T>.forEachIndexed() function and applies an operation lambda on each child argument of the serializer. This function takes a Map<String, NavType<Any?>> and provides this navType into the operation lambda.

Refactored RouteEncoder to output a map of argName to List<String> (the value is the output of NavType.serializeAsValue/serializeAsValues).

Finally refactored generateRouteWithArgs to use the new forEachIndexed(), new RouteBuilder, and new RouteEncoder.

Test: existing tests
Bug: 340966212
Change-Id: Ic2ebe6cc7c396254c10a03c26639432d9c6c6fe3

M navigation/navigation-common/src/main/java/androidx/navigation/serialization/RouteBuilder.kt
M navigation/navigation-common/src/main/java/androidx/navigation/serialization/RouteEncoder.kt
M navigation/navigation-common/src/main/java/androidx/navigation/serialization/RouteSerializer.kt

https://android-review.googlesource.com/3118323

ap...@google.com <ap...@google.com> #5Jun 14, 2024 11:11PM

Project: platform/frameworks/support
Branch: androidx-main

commit b333eceb8ade397c6418f8a6ecc14f24e88c484f
Author: Clara Fok <clarafok@google.com>
Date: Fri Jun 14 12:33:38 2024

Refactor SavedStateHandleFactory

Previous implementation delegates to NavDeepLink operations which has dependencies on Uri, thus limiting this API to androidTests.

Refactor away from NavDeepLink and instead manually replicate the steps that an argument value would go through.

However, since bundle is deeply integrated into the parsing of arguments by NavType, the non-instrumented test will require robolectric in order to properly support bundle.

Test: ./gradlew navigation:navigation-testing:test
Bug: 340966212
Relnote: "SavedStateHandleFactory test api can now be used in non-android tests but will require robolectric to support argument parsing with bundles."
Change-Id: I76cdc4b40474680dbdb58e6b8a6451f0e228f9b1

M navigation/navigation-common/src/main/java/androidx/navigation/serialization/RouteEncoder.kt
M navigation/navigation-common/src/main/java/androidx/navigation/serialization/RouteSerializer.kt
M navigation/navigation-testing/build.gradle
M navigation/navigation-testing/src/main/java/androidx/navigation/testing/SavedStateHandleFactory.kt
M navigation/navigation-testing/src/test/java/androidx/navigation/testing/TestSavedStateHandleFactory.kt

https://android-review.googlesource.com/3117919

cl...@google.com <cl...@google.com> #6Jun 14, 2024 11:35PM

Marked as fixed.

Fixed internally and available in navigation-2.8.0-rc01.

Note that Android Bundle is deeply integrated into the parsing of arguments by NavType and cannot be removed, hence non-instrumented tests will require robolectric in order to properly support bundles.

ap...@google.com <ap...@google.com> #7Jun 18, 2024 06:44PM

Project: platform/frameworks/support
Branch: androidx-main

commit df6f97f72af9fa61365b44af75905ddd9039bb26
Author: Clara Fok <clarafok@google.com>
Date: Tue Jun 18 10:47:42 2024

Update SavedStateHandleFactory kdoc

Clarify that android Bundles cannot be avoided at this stage and non-instrumented tests need to be run with robolectric.

Test: TH
Bug: 340966212
Relnote: "To use SavedStateHandle test factory in non-instrumented tests, robolectric is required. This is because android Bundle is necessarily integrated into the parsing of nav arguments."
Change-Id: I8047c1a2d2dff4cb2efd0452407287738f2ce157

M navigation/navigation-testing/src/main/java/androidx/navigation/testing/SavedStateHandleFactory.kt

https://android-review.googlesource.com/3136074

do...@google.com <do...@google.com> #8Jun 25, 2024 03:58PM

Thanks for doing this. Whilst this is technically an improvement, the fact remains that if you want to test a ViewModel which accepts navigation arguments you now need to introduce a dependency on Robolectric which is a fairly major blocker for many developers. Robolectric slows down tests and reduces test fidelity.

Is there no way of removing the Bundle dependency? Or perhaps moving Bundle into AndroidX?

pr...@google.com <pr...@google.com> #9Jun 26, 2024 05:43PM

The following release(s) address this bug.It is possible this bug has only been partially addressed:

androidx.navigation:navigation-common:2.8.0-beta04
androidx.navigation:navigation-testing:2.8.0-beta04

do...@google.com <do...@google.com> #10Jun 27, 2024 03:07PM

I created a follow up bug to track the Android dependency forcing unit->instrumented tests for ViewModels. https://issuetracker.google.com/349807172