Convert ReactPropConstantsTest to Kotlin #39005

thiagobrez · 2023-08-14T19:18:08Z

Summary:

Converts ReactPropConstantsTest to Kotlin, as requested in #38825

Changelog:

[INTERNAL] [CHANGED] - Convert ReactPropConstantsTest to Kotlin
[INTERNAL] [CHANGED] - Add null-check to ViewManagersPropertyCache.java

Test Plan:

Run ./gradlew :packages:react-native:ReactAndroid:test.
All tests should pass.

facebook-github-bot · 2023-08-14T19:18:14Z

Hi @thiagobrez!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

thiagobrez · 2023-08-14T19:24:40Z

...ative/ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewManagersPropertyCache.java

+    if(cls == null) {
+      return new HashMap<>();
+    };
+


Added null-check because cls was coming null when calling this function recursively with cls.getSuperClass() in lines 410-413, causing a NullPointerException

Why is this change needed? Was one of your test failing?

@cortinico Exactly, testNativePropsIncludeCorrectTypes was failing with a NullPointerException when instantiating the UIManagerModule:

val uiManagerModule = UIManagerModule(reactContext, viewManagers, 0)

I tracked it all the way down to ViewManagersPropertyCache.java, in getNativePropSettersForShadowNodeClass:

When this function is called recursively (in lines 410-413) with cls.getSuperclass(), the superclass might be null, causing cls.getInterfaces() (line 400) to crash.

for (Class iface : cls.getInterfaces()) { ... }

Let me know if you believe there is a better way to solve this!

facebook-github-bot · 2023-08-14T21:08:10Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

cortinico

Hi @thiagobrez
Thanks for sending this over!

Can I ask you to fix the warnings that are causing the CI to fail:

> Task :packages:react-native:ReactAndroid:compileDebugUnitTestKotlin FAILED
e: warnings found and -Werror specified
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:50:21 Parameter 'v' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:50:31 Parameter 'value' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:54:20 Parameter 'v' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:54:30 Parameter 'value' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:58:22 Parameter 'v' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:58:32 Parameter 'value' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:62:23 Parameter 'v' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:62:33 Parameter 'value' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:66:23 Parameter 'v' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:66:33 Parameter 'value' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:70:26 Parameter 'v' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:70:36 Parameter 'value' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:74:25 Parameter 'v' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:74:35 Parameter 'value' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:78:22 Parameter 'v' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:78:32 Parameter 'value' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:82:20 Parameter 'v' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:82:30 Parameter 'value' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:86:27 Parameter 'v' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:86:37 Parameter 'index' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:86:49 Parameter 'value' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:90:25 Parameter 'v' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:90:35 Parameter 'index' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:90:47 Parameter 'value' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:94:30 Parameter 'v' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:94:40 Parameter 'index' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:94:52 Parameter 'value' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:98:23 Parameter 'v' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:98:33 Parameter 'value' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:105:28 Parameter 'v' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:105:38 Parameter 'index' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:105:50 Parameter 'value' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:148:34 Unchecked cast: Any? to Map<String, *>

> Task :packages:react-native:ReactAndroid:compileReleaseUnitTestKotlin FAILED
e: warnings found and -Werror specified
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:50:21 Parameter 'v' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:50:31 Parameter 'value' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:54:20 Parameter 'v' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:54:30 Parameter 'value' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:58:22 Parameter 'v' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:58:32 Parameter 'value' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:62:23 Parameter 'v' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:62:33 Parameter 'value' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:66:23 Parameter 'v' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:66:33 Parameter 'value' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:70:26 Parameter 'v' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:70:36 Parameter 'value' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:74:25 Parameter 'v' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:74:35 Parameter 'value' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:78:22 Parameter 'v' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:78:32 Parameter 'value' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:82:20 Parameter 'v' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:82:30 Parameter 'value' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:86:27 Parameter 'v' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:86:37 Parameter 'index' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:86:49 Parameter 'value' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:90:25 Parameter 'v' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:90:35 Parameter 'index' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:90:47 Parameter 'value' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:94:30 Parameter 'v' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:94:40 Parameter 'index' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:94:52 Parameter 'value' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:98:23 Parameter 'v' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:98:33 Parameter 'value' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:105:28 Parameter 'v' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:105:38 Parameter 'index' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:105:50 Parameter 'value' is never used
w: file:///root/react-native/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt:148:34 Unchecked cast: Any? to Map<String, *>

as those are tests you'll have to @Suppress some of the warnings at the class level probably

cortinico · 2023-08-15T14:20:52Z

...act-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt

+    val viewManagers = listOf<ViewManager<*, *>>(ViewManagerUnderTest())
+    val reactContext = ReactApplicationContext(RuntimeEnvironment.getApplication())
+    val uiManagerModule = UIManagerModule(reactContext, viewManagers, 0)
+    val constants: Map<*, *>? = valueAtPath(uiManagerModule.constants, "SomeView", "NativeProps")


Suggested change

val constants: Map<*, *>? = valueAtPath(uiManagerModule.constants, "SomeView", "NativeProps")

val constants: Map<*, *> = valueAtPath(uiManagerModule.constants, "SomeView", "NativeProps")

cortinico · 2023-08-15T14:21:00Z

...act-native/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropConstantsTest.kt

+  }
+
+  companion object {
+    private fun valueAtPath(nestedMap: Map<String, *>?, vararg keyPath: String): Map<*, *>? {


Suggested change

private fun valueAtPath(nestedMap: Map<String, *>?, vararg keyPath: String): Map<*, *>? {

private fun valueAtPath(nestedMap: Map<String, *>, vararg keyPath: String): Map<*, *> {

cortinico · 2023-08-15T14:21:51Z

...ative/ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewManagersPropertyCache.java

+    if(cls == null) {
+      return new HashMap<>();
+    };
+


Why is this change needed? Was one of your test failing?

thiagobrez · 2023-08-16T08:33:12Z

@cortinico Of course, I wasn't sure if suppressing was recommended - sending fixes soon!

thiagobrez · 2023-08-16T12:01:38Z

@cortinico Hey, just sent the following fixes:

Suppressed the unused variables at the inner class level (ViewManagerUnderTest). Since we're just implementing a dummy class for the test, it seemed like a better idea than suppressing every individual function.
Implemented the suggested changes, plus rewrote a few things in valueAtPath to fix the unchecked cast in I had in value?.get(key) as? Map<String, *>. Also, replaced assertThat with require since these were runtime validations. Because of that, I could remove the error() call and get a cleaner return type for the function.

Let me know what you think :)

analysis-bot · 2023-08-16T12:24:02Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	8,947,332	-866
android	hermes	armeabi-v7a	n/a	--
android	hermes	x86	n/a	--
android	hermes	x86_64	n/a	--
android	jsc	arm64-v8a	9,540,932	-626
android	jsc	armeabi-v7a	n/a	--
android	jsc	x86	n/a	--
android	jsc	x86_64	n/a	--

Base commit: 0ba9808
Branch: main

...ative/ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewManagersPropertyCache.java

…react/uimanager/ViewManagersPropertyCache.java

cortinico

Let me know what you think :)

Changes looks great to me 👍 We can import and merge this one now

facebook-github-bot · 2023-08-17T11:18:33Z

@cortinico has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2023-08-17T18:08:21Z

@cortinico merged this pull request in 4a48fd2.

Convert ReactPropConstantsTest to Kotlin

7f889fc

thiagobrez commented Aug 14, 2023

View reviewed changes

Update with main

2e068e8

facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Aug 14, 2023

cortinico requested changes Aug 15, 2023

View reviewed changes

Suppress unused params, fix valueAtPath typings

f5f07d8

thiagobrez requested a review from cortinico August 16, 2023 12:01

cortinico reviewed Aug 17, 2023

View reviewed changes

...ative/ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewManagersPropertyCache.java Outdated Show resolved Hide resolved

Update packages/react-native/ReactAndroid/src/main/java/com/facebook/…

0046506

…react/uimanager/ViewManagersPropertyCache.java

cortinico approved these changes Aug 17, 2023

View reviewed changes

facebook-github-bot closed this in 4a48fd2 Aug 17, 2023

facebook-github-bot added the Merged This PR has been merged. label Aug 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert ReactPropConstantsTest to Kotlin #39005

Convert ReactPropConstantsTest to Kotlin #39005

thiagobrez commented Aug 14, 2023

facebook-github-bot commented Aug 14, 2023

thiagobrez Aug 14, 2023 •

edited

Loading

cortinico Aug 15, 2023

thiagobrez Aug 16, 2023

facebook-github-bot commented Aug 14, 2023

cortinico left a comment

cortinico Aug 15, 2023

cortinico Aug 15, 2023

cortinico Aug 15, 2023

thiagobrez commented Aug 16, 2023

thiagobrez commented Aug 16, 2023

analysis-bot commented Aug 16, 2023

cortinico left a comment

facebook-github-bot commented Aug 17, 2023

facebook-github-bot commented Aug 17, 2023

	val constants: Map<, >? = valueAtPath(uiManagerModule.constants, "SomeView", "NativeProps")
	val constants: Map<, > = valueAtPath(uiManagerModule.constants, "SomeView", "NativeProps")

	private fun valueAtPath(nestedMap: Map<String, >?, vararg keyPath: String): Map<, *>? {
	private fun valueAtPath(nestedMap: Map<String, >, vararg keyPath: String): Map<, *> {

Convert ReactPropConstantsTest to Kotlin #39005

Convert ReactPropConstantsTest to Kotlin #39005

Conversation

thiagobrez commented Aug 14, 2023

Summary:

Changelog:

Test Plan:

facebook-github-bot commented Aug 14, 2023

Action Required

Process

thiagobrez Aug 14, 2023 • edited Loading

Choose a reason for hiding this comment

cortinico Aug 15, 2023

Choose a reason for hiding this comment

thiagobrez Aug 16, 2023

Choose a reason for hiding this comment

facebook-github-bot commented Aug 14, 2023

cortinico left a comment

Choose a reason for hiding this comment

cortinico Aug 15, 2023

Choose a reason for hiding this comment

cortinico Aug 15, 2023

Choose a reason for hiding this comment

cortinico Aug 15, 2023

Choose a reason for hiding this comment

thiagobrez commented Aug 16, 2023

thiagobrez commented Aug 16, 2023

analysis-bot commented Aug 16, 2023

cortinico left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Aug 17, 2023

facebook-github-bot commented Aug 17, 2023

thiagobrez Aug 14, 2023 •

edited

Loading