Convert BaseJavaModuleTest to Kotlin #37822
Conversation
facebook-github-bot
added
the
CLA Signed
Base commit: 03f70bf |
|
Hello @cortinico, can you have a look at this PR? |
...ages/react-native/ReactAndroid/src/test/java/com/facebook/react/bridge/BaseJavaModuleTest.kt
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| @Test(expected = NativeArgumentsParseException::class) | ||
| @Throws(Exception::class) |
There was a problem hiding this comment.
Thank you for your reviews, I would love to know why this should be removed
There was a problem hiding this comment.
The @Test(expected = NativeArgumentsParseException::class) bit should be enough to fulfill the purpose of this test, the @Throws(Exception::class) part doesn't add anything there in terms of information, neither it's required for proper compilation.
...ages/react-native/ReactAndroid/src/test/java/com/facebook/react/bridge/BaseJavaModuleTest.kt
Outdated
Show resolved
Hide resolved
...ages/react-native/ReactAndroid/src/test/java/com/facebook/react/bridge/BaseJavaModuleTest.kt
Outdated
Show resolved
Hide resolved
|
Thank you so much for taking this on! Looks good overall, aside from a few stylistic suggestions. |
|
@rshest has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
...ages/react-native/ReactAndroid/src/test/java/com/facebook/react/bridge/BaseJavaModuleTest.kt
Show resolved
Hide resolved
| } | ||
|
|
||
| @Test(expected = NativeArgumentsParseException::class) | ||
| @Throws(Exception::class) |
There was a problem hiding this comment.
The @Test(expected = NativeArgumentsParseException::class) bit should be enough to fulfill the purpose of this test, the @Throws(Exception::class) part doesn't add anything there in terms of information, neither it's required for proper compilation.
|
@rshest has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
There was a problem hiding this comment.
Hey @abdennour-jebbar-nw it seems like the CI is still red on 2 tests with an NPE. Could you check them out?
|
@cortinico I'm investigating the failing tests |
|
Hey @abdennour-jebbar-nw , what are your plans with the diff? Will you be able to follow up, or should we maybe close it? |
|
@rshest I'm still following up with this PR, I'm trying to figure out why the moduleWrapper remains null |
| private inner class GeneratedMethodsModule : NativeTestGeneratedModuleSpec() { | ||
| override fun getName(): String ="GeneratedMethods" | ||
|
|
||
| override fun generatedMethod(a: String, b: Int) {} |
There was a problem hiding this comment.
| override fun generatedMethod(a: String, b: Int) {} | |
| override fun generatedMethod(a: String?, b: Int?) {} |
| } | ||
|
|
||
| private abstract inner class NativeTestGeneratedModuleSpec : BaseJavaModule(), TurboModule { | ||
| @ReactMethod abstract fun generatedMethod(a: String, b: Int) |
There was a problem hiding this comment.
| @ReactMethod abstract fun generatedMethod(a: String, b: Int) | |
| @ReactMethod abstract fun generatedMethod(a: String?, b: Int?) |
| private class MethodsModule : BaseJavaModule() { | ||
| override fun getName(): String = "Methods" | ||
|
|
||
| @ReactMethod fun regularMethod(a: String, b: Int) {} |
There was a problem hiding this comment.
| @ReactMethod fun regularMethod(a: String, b: Int) {} | |
| @ReactMethod fun regularMethod(a: String?, b: Int?) {} |
Hey, I think I know the reason why the remaining two tests were crashing, please see my code suggestions above. Since the methods in question (and their arguments) were mocked, it's not safe to have them strictly non-nullable. This caused the crash. |
|
@rshest Thank you so much for following up with me, I'll try the changes asap, once green I'll update the PR. Your help was so much appreciated 🙌 |
|
This pull request was successfully merged by @abdennour-jebbar-nw in 8ddb334. When will my fix make it into a release? | Upcoming Releases |
@abdennour-jebbar-nw I verified internally that the tests are fixed, so your PR is merged to trunk now. Thank you very much, once again, for working on this! :) |
Summary:
As part of the effort to Kotlin-fy React Native tests, I've converted BaseJavaModuleTest to Kotlin.
Changelog:
[Internal] [Changed] - Convert BaseJavaModuleTest to Kotlin
Test Plan:
Tests pass: ./gradlew :packages:react-native:ReactAndroid:test
Formatted with KtFmt