chore: fix rpc test not collected#137
Conversation
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
This PR migrates the RPC test from a deprecated global test handler to a test method on the Default(WorkerEntrypoint) class, and fixes assert_equal to accept dict subclasses.
Issues
- Medium: Two test cases silently dropped. The old code tested that passing a
lambdaand a nameddef my_functoenv.PythonRpc.identity()raisesTypeError. These were removed without explanation. If they were dropped intentionally (e.g. thedefinsidewithblock was problematic), it would be good to note why — otherwise they should be kept.
| with assertRaises(TypeError): | ||
| await env.PythonRpc.identity(js.RegExp.new("ab+c", "i")) | ||
| with assertRaises(TypeError): | ||
| await env.PythonRpc.identity({"test": (1, 2, 3)}) |
There was a problem hiding this comment.
Two test cases from the original code are missing here. The old version also verified that passing a lambda and a named function raises TypeError:
| await env.PythonRpc.identity({"test": (1, 2, 3)}) | |
| with assertRaises(TypeError): | |
| await env.PythonRpc.identity({"test": (1, 2, 3)}) | |
| with assertRaises(TypeError): | |
| await env.PythonRpc.identity(lambda x: x + x) |
The old code also had a def my_func case, but defining a function inside a with block is awkward. The lambda test alone covers this — though if the omission was intentional, a comment explaining why would be helpful.
|
I'm Bonk, and I've done a quick review of your PR. This PR migrates the RPC test from a deprecated global I found one actionable issue and posted a review comment on the PR:
The rest of the changes (structural migration to class method, |
There was a problem hiding this comment.
Please use Hide Whitespace when reviewing. The diff is actually very small but looks big because of the whitespace.
| "Expected the returned value from RPC to be a Python type." | ||
| assert isinstance(js_map, dict) | ||
| # js.Map ==> dict ==> js.Object | ||
| assert js_map["1"] == "one" |
There was a problem hiding this comment.
This was a breaking change we introduced in #112 (js.Map ==> js.Object)
| # TODO: Support RegExp. | ||
| with assertRaises(TypeError): | ||
| await env.PythonRpc.identity(js.RegExp.new("ab+c", "i")) | ||
| with assertRaises(TypeError): |
There was a problem hiding this comment.
We now support passing functions to rpc boundary
1 participant
I noticed that
python-rpc/worker.pytests were not running at all because it had a globaltestfunction which we deprecated.This migrates it to
testmethod under theWorkerEntrypointclass.