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

Handle date/time conversion in runtime #258

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

drewhamilton
Copy link
Collaborator

Closes #230.

This implements the first solution I proposed in #230. I'm not overly attached to this approach, just wanted to prove it out.

This pulls all android.icu imports out of generated code and into the runtime, allowing substitution of non-Android ICU dependencies in JVM unit tests.

Generated code now uses a generic DateTimeConverter interface to format date/time parameters. The default AndroidDateTimeConverter implementation instantiates android.icu.util.Calendars. JVM tests can substitute a JvmDateTimeConverter into their FormattedResources object to instantiate com.ibm.icu.util.Calendars, avoiding the need for Robolectric to format date/time strings.

Comment on lines +34 to +38
private val Iso8601Locale by lazy(LazyThreadSafetyMode.NONE) {
ULocale.Builder()
.setExtension('u', "ca-iso8601")
.build()
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since AndroidDateTimeConverter is an object, this ULocale instantiation was happening immediately when FormattedResources was accessed, causing the JVM test to (still) crash. Loading it lazily avoids this.

This prevents the ULocale from being instantiated at class-loading time, which would crash JVM tests due to the missing Android implementation.
@drewhamilton drewhamilton force-pushed the drew/handle-date-time-conversion-in-runtime branch from 7fa0ec6 to 83c9523 Compare November 3, 2023 22:13
Copy link
Collaborator

@JakeWharton JakeWharton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely not sure how I feel about this. It's basically Dispatchers.setMain which is my mortal enemy.

Comment on lines +35 to +36
@Before fun substituteDateTimeConverter() {
FormattedResources.dateTimeConverter = JvmDateTimeConverter
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Painful 🙈

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. Nothing better came to mind quickly but I can stew on it a bit longer.

@drewhamilton drewhamilton marked this pull request as draft November 7, 2023 00:59
@theisenp
Copy link
Collaborator

I just realized I never responded to this, so sorry about that!

I think that in practice this approach probably would work fine (at least for our setup at Cash), even if we're leaking state across tests. Definitely agree that swapping out the static reference is painful though and it would be great to avoid it. We could potentially limit the API surface a bit by hiding the assignment behind a JUnit @Rule, but that doesn't fix the underlying problem.

Another thing we could explore is making the resources themselves injectable. Something like:

val FormattedResources = BaseFormattedResources(dateConverter = AndroidDateConverter)

class BaseFormattedResources(private val dateConverter : DateConverter<*>) {
  fun my_string(date: LocalDate): FormattedResource { ... }
}

People could still reference the default resources statically, but they could also pass in an instance if they need to for testing:

class MyPresenter(private val formattedResources: BaseFormattedResources = FormattedResources) { ... }

class MyPresenterTest {
  private val presenter = MyPresenter(BaseFormattedResources(JvmDateConverter))
}

Lots of options here in terms of naming, the exact configuration, how much we encourage static use vs injecting instances, etc.

@drewhamilton
Copy link
Collaborator Author

Makes sense. I'll give this a try.

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

Successfully merging this pull request may close these issues.

Improve JVM testing for date/time format args
3 participants