Repository navigation
Add React Native support to the TDK and environment scripts (2/4) - #607
jayant-dhingra-cb wants to merge 3 commits into
Conversation
168452c to
1057559
Compare
| await ws_conn.send_str(json.dumps(data)) | ||
| resp = await future | ||
| # Only native WebSocket test servers (ws:// URLs, e.g. React Native) get a | ||
| # per-request timeout; other WebSocket transports keep waiting as before. |
There was a problem hiding this comment.
What does "other websocket transports" mean here?
borrrden
left a comment
There was a problem hiding this comment.
There are a few things here that concern me. I am flagging them first.
| site = web.SockSite(self.__runner, sock) | ||
| chosen_port = sock.getsockname()[1] | ||
| await site.start() | ||
| print(f"[cbltest] WebSocket router listening on port {chosen_port}", flush=True) |
There was a problem hiding this comment.
Prefer just having cbl_info, no print
| # --------------------------------------------------------------------------- | ||
|
|
||
|
|
||
| class ReactNativeAndroidBridge(PlatformBridge): |
There was a problem hiding this comment.
Is this needed? For example, .NET just uses the regular Android bridge class since at the end of the day the artifact is an APK.
| # React Native test servers connect back to the pytest WebSocket | ||
| # server, so there is no HTTP endpoint to poll here. | ||
| click.echo(f"Launched {test_server_input.platform} on {test_server_input.location}") | ||
| continue |
There was a problem hiding this comment.
Is this check meant to be similar to the one below for js? Can they be combined?
| if ts.platform.startswith("reactnative"): | ||
| # React Native test servers connect back to the pytest WebSocket server | ||
| ts_definition = { | ||
| "url": "ws://localhost:8765", |
There was a problem hiding this comment.
I don't understand how this is correct. every server is going to end up with this address.
| "--device", | ||
| location, | ||
| "--pid", | ||
| "0", |
There was a problem hiding this comment.
Why pid 0? This doesn't look like it actually stops anything.
| "-derivedDataPath", | ||
| str(working / "ios" / "build"), | ||
| "-allowProvisioningUpdates", | ||
| "DEVELOPMENT_TEAM=N2Q372V7W2", |
There was a problem hiding this comment.
I'm not wild about this, why is it needed here but not anywhere else?
| def _get_host_ip_for_android(adb: Path, location: str) -> str: | ||
| """Determine the host IP reachable from an Android device. | ||
|
|
||
| For emulators the host loopback is 10.0.2.2. For physical USB devices |
There was a problem hiding this comment.
We never use emulators with the TDK anyway, so this is redundant
- Register reactnative_android and reactnative_ios test server platforms (build, download, install and launch via adb / devicectl). - Recognise cbl-reactnative as ServerVariant.REACTNATIVE. - WebSocketRouter: bind the port from a ws:// test server URL, relaunch the native app via CBL_NATIVE_WS_RELAUNCH_SCRIPT once the port is bound, and reconnect when the connection drops or a request times out. - Native ws:// test servers get a 90 s per-request timeout; other WebSocket transports (JS) keep their current behaviour. - start_backend / setup_topology: point React Native test servers at the pytest WebSocket server and skip the HTTP health check for them; add relaunch_test_servers(). HTTP test servers are unaffected. For the JS test server (the other WebSocket user) normal operation is unchanged; the only difference is that a dropped connection now fails in-flight requests instead of leaving them waiting forever.
…me for RN test servers - latestbuilds_path now goes through TestServer.artifact_path(), so release versions (no build number) resolve under releases/mobile instead of raising IndexError. - Product is now couchbase-lite-react-native, matching the other couchbase-lite-* products, the latestbuilds folder for SDK builds, and the product name ProGet's get_version API accepts (cbl-reactnative is rejected as an invalid product). - Chain the WebSocket timeout ConnectionErrors with 'from None' (B904). - Mock WebSocketRouter.ensure_connected in the transport bad-response test added on main, since send() now awaits it.
- Give each React Native test server its own TDK config URL and device ID. The pytest WebSocket router identifies the Nth WebSocket test server by "ws<N>", so the URL carries that ID and the bridge passes it to the app at launch. Previously every RN server got ws://localhost:8765 and ws0, so only one RN server could run at a time. The router now numbers its test servers instead of giving them all ws0, and binds the port of the first ws:// test server. - Launch RN Android through the shared AndroidBridge. It gains optional launch extras, adb reverse ports, and a restart-on-run mode (wake the device, force-stop before launching); other platforms run unchanged. - RN iOS: stop the running app by its PID before relaunching instead of terminating PID 0, which did nothing. - Drop the DEVELOPMENT_TEAM override from the RN iOS build; the Xcode project already sets it, as for the other iOS test servers. - Share one "connects back over WebSocket" check for JS and RN when skipping the HTTP readiness poll. JS now continues to the next test server instead of returning early. - Explain the RN-only reconnect on request timeout in the WebSocket transport. - Log through cbl_info/cbl_warning/cbl_error instead of print. - Remove the unused Android host-IP helper (emulators are not used).
1057559 to
2aff64d
Compare
borrrden
left a comment
There was a problem hiding this comment.
I don't think i can sanely review this without more discussion on how it works.
| self.__adb_location = None | ||
| self.__needs_permissions = needs_permissions | ||
| self.__launch_extras = dict(launch_extras or {}) | ||
| self.__reverse_ports = list(reverse_ports) |
There was a problem hiding this comment.
what is this used for? The comments point out a use case that I don't think exists. There should be no reason that we need to use adb reverse.
| # The app connects back to the pytest WebSocket router through an adb reverse | ||
| # tunnel, so it reaches the host on 127.0.0.1 regardless of the device's network. | ||
| device_id = kwargs.get("device_id") | ||
| launch_extras = {"deviceID": device_id, "wsURL": f"ws://127.0.0.1:{WS_PORT}"} if device_id else None |
There was a problem hiding this comment.
Why bother with a reverse tunnel here? You could just as easily pass in the host machine's URL here
| # --------------------------------------------------------------------------- | ||
|
|
||
|
|
||
| class ReactNativeIOSBridge(PlatformBridge): |
There was a problem hiding this comment.
I meant for you to look at this one too. I don't want to have such specialized bridges here. The existing bridges should be the conduit to the OS, and not have anything specific to a CBL platform
|
|
||
| The app is installed and launched once during setup, but it can give up | ||
| reconnecting before pytest binds its WebSocket port. The WebSocket router | ||
| calls this (via CBL_NATIVE_WS_RELAUNCH_SCRIPT) once the port is bound. |
There was a problem hiding this comment.
Why does this happen? I really don't like this whole process of adding in this hard to understand bandage ("this script is called by a mystery script defined by an environment variable". Where is this script?). Isn't there a better solution than this?
Summary
Teaches the Python TDK and the AWS environment scripts about the React Native test server. Part 2 of 4 (see PR 1).
What's in it
reactnative_register.py(new): registers thereactnative_androidandreactnative_iosplatforms (build, download, install and launch viaadb/devicectl).responses.py: recognisescbl-reactnativeasServerVariant.REACTNATIVE.websocket_router.py:ws://test server URL.CBL_NATIVE_WS_RELAUNCH_SCRIPTonce that port is bound.requests_transport.py: nativews://test servers get a 90 s per-request timeout. Other WebSocket transports (JS) keep waiting as before.start_backend.py,setup_topology.py: point React Native test servers at the pytest WebSocket server, skip the HTTP health check for them, and addrelaunch_test_servers(). These are additions only; no existing lines were changed.topology_schema.json: adds the two platform names.Impact on existing pipelines
Testing
pre-commit run --all-filespasses (ruff, ty, pyupgrade and the rest).client/tests: 307 passed. The 9 edge-server-manager failures are the same onmain.