Migrate JSPackagerClientTest to Kotlin #37747
Conversation
facebook-github-bot
added
the
CLA Signed
Base commit: 08dc0a6 |
| val client = JSPackagerClient("test_client", mSettings, createRH("methodValue", handler)) | ||
|
|
||
| client.onMessage( | ||
| "{\"version\": 2, \"method\": \"methodValue\", \"params\": \"paramsValue\"}" |
There was a problem hiding this comment.
Can you use Kotlin multiline string here (i.e. """...""", then you won't need to escape the quotes?..
|
@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.
Thanks for sending this @brunohensel
I've left some comments that needs to be addressed before we can merge this. Thanks for doing this work.
|
|
||
| @RunWith(RobolectricTestRunner::class) | ||
| class JSPackagerClientTest { | ||
| private fun createRH(action: String, handler: RequestHandler): Map<String, RequestHandler> { |
There was a problem hiding this comment.
Since we are at this, let's call this createRequestHandler?
Plus let's move the private methods at the bottom after all the tests
There was a problem hiding this comment.
Since I'm going to change some semantics here, I noticed that JSPackagerClient is always called passing in the same settings and clientId as test_client. Wdyt encoding this as default arguments to a function, passing in on the call site only the handler and connectionCallback? Like getClient(requestHandlers = handler, connectionCallback = connectionCallback)
There was a problem hiding this comment.
Yup that's also a valid point for improvement 👍
There was a problem hiding this comment.
I just pushed the changes, let me know if I have to revert something.
...ive/ReactAndroid/src/test/java/com/facebook/react/packagerconnection/JSPackagerClientTest.kt
Outdated
Show resolved
Hide resolved
...ive/ReactAndroid/src/test/java/com/facebook/react/packagerconnection/JSPackagerClientTest.kt
Show resolved
Hide resolved
| val m: MutableMap<String, RequestHandler> = HashMap() | ||
| m[action] = handler | ||
| return m |
There was a problem hiding this comment.
This method should just become:
private fun createRequestHandler(action: String, handler: RequestHandler): Map<String, RequestHandler> = mapOf(action to handler)we don't need explicit HashMap or so
...ive/ReactAndroid/src/test/java/com/facebook/react/packagerconnection/JSPackagerClientTest.kt
Outdated
Show resolved
Hide resolved
...ive/ReactAndroid/src/test/java/com/facebook/react/packagerconnection/JSPackagerClientTest.kt
Outdated
Show resolved
Hide resolved
…react/packagerconnection/JSPackagerClientTest.kt Co-authored-by: Nicola Corti <corti.nico@gmail.com>
…react/packagerconnection/JSPackagerClientTest.kt Co-authored-by: Nicola Corti <corti.nico@gmail.com>
…react/packagerconnection/JSPackagerClientTest.kt Co-authored-by: Nicola Corti <corti.nico@gmail.com>
…react/packagerconnection/JSPackagerClientTest.kt Co-authored-by: Nicola Corti <corti.nico@gmail.com>
|
@rshest has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
* main: (1824 commits) Convert FallbackJSBundleLoaderTest to Kotlin (facebook#37750) Migrate JSPackagerClientTest to Kotlin (facebook#37747) (refactor): kotlin-ify `ShareModuleTest.java` (facebook#37746) Upgrade `@react-native/codegen-typescript-test`'s Jest dependency to Jest 29 (facebook#37745) Move flow-typed definitions to repo root (facebook#37636) Convert InterpolatorTypeTest to Kotlin (facebook#37724) Update documentation of ReactHost.reload method (facebook#37691) Reduce visibility of ReactHost.destroy() method (facebook#37693) Reduce visibility in React Context (facebook#37695) Remove InstanceHandleHelper as unused (facebook#37740) Convert CompositeReactPackageTest to Kotlin (facebook#37734) Add license header to SetSpanOperation.java Convert FakeEventDispatcher to kotlin (facebook#37739) Convert FakeRCTEventEmitter to Kotlin (facebook#37733) Convert InteropModuleRegistryTest to Kotlin (facebook#37735) Bump `autorebase.yml` to `v1.8` (facebook#37584) Convert StackTraceHelperTest to Kotlin (facebook#37741) Convert BlobModuleTest class to Kotlin (facebook#37719) Update prettier to v2.8.8 (facebook#37738) Introduce BoltsFutureTask class to avoid leaking bolts.Task into ReactHost API (facebook#37744) ... # Conflicts: # BUCK # packages/react-native/React/Views/UIResponder+FirstResponder.h # packages/react-native/React/Views/UIResponder+FirstResponder.m
Summary:
Migrate JSPackagerClientTest.java to Kotlin. See #37708
Changelog:
[INTERNAL] [CHANGED] - Migrate JSPackagerClientTest to Kotlin
Test Plan:
Tests pass: ./gradlew :packages:react-native:ReactAndroid:test
Formatted with KtFmt