feat: add more RPC client tests #408
Conversation
kratico
force-pushed
the
feat/rpc-tests
branch
from
d8d292c to
da800c0
Compare
rpc/client.test.ts
Outdated
| const createMockClient = () => { | ||
| let listener: C.rpc.ProviderListener<Error, Error> | ||
| const providerMockFactory: Provider = (_discoveryValue, clientListener) => { | ||
| listener = clientListener |
There was a problem hiding this comment.
this is very coupled to how the rpc.Client calls the provider factory.
I'm open to more elegant suggestions to extract the client listener so we can emit testing messages/errors
| this.pendingSubscriptions[message.id] = (maybeError) => { | ||
| listenerBound(maybeError) | ||
| if (maybeError instanceof Error) { | ||
| stop() | ||
| } | ||
| } |
There was a problem hiding this comment.
bug fix, subscriptions should stop on errors
| this.call(message) | ||
| .then((maybeError) => { | ||
| if (maybeError instanceof Error) { | ||
| if (maybeError instanceof Error || maybeError.error) { |
There was a problem hiding this comment.
bug fix, if the subscription creation is not successful it should stop the subscription
| @@ -47,7 +47,7 @@ export class Client< | |||
| const pendingCall = this.pendingCalls[id]! | |||
| pendingCall.resolve(e) | |||
| delete this.pendingCalls[id] | |||
| this.pendingSubscriptions[id]!(e) | |||
| this.pendingSubscriptions[id]?.(e) | |||
There was a problem hiding this comment.
bug fix, on error there may not be pendingSubscriptions for the given message id
There was a problem hiding this comment.
This is a very important fix, as these errors were not getting caught, and thus crashed the process, including on the codegen server.
rpc/provider/proxy.ts
Outdated
| const { cleanUp, listeners, inner } = connection(url, listener) | ||
| let conn | ||
| try { | ||
| conn = connection(url, listener) |
There was a problem hiding this comment.
if the connection was not created in .send, this call will attempt to create it again.
And if it fails to create the connection it will throw.
There was a problem hiding this comment.
Good point... we bake in the assumption that the connection connection will resolve to an open connection. How might we want to adjust connection to resolve to an error in the case of an out-of-flow release?
There was a problem hiding this comment.
copying comment from below
I believe that it would be better to just do
conn = connections.get(url)
Then, we need to decide what release should do if there is no connection.
One option could be that if there isn't anything to release, then do nothing.
Thoughts?
kratico
force-pushed
the
feat/rpc-tests
branch
from
40dd468 to
7e9130b
Compare
rpc/provider/proxy.ts
Outdated
| const { cleanUp, listeners, inner } = connection(url, listener) | ||
| let conn | ||
| try { | ||
| conn = connection(url, listener) |
There was a problem hiding this comment.
Good point... we bake in the assumption that the connection connection will resolve to an open connection. How might we want to adjust connection to resolve to an error in the case of an out-of-flow release?
rpc/provider/smoldot.test.ts
Outdated
| stopped.resolve() | ||
| }) | ||
| // @ts-ignore make JSON.stringify to throw | ||
| provider.send(1n) |
There was a problem hiding this comment.
Same comment as above
rpc/provider/smoldot.ts
Outdated
| try { | ||
| conn = await connection(props, listener) | ||
| } catch (_error) { | ||
| return |
There was a problem hiding this comment.
Although an error would likely mean that the connection is dead / "released", this––for some reason––seems not ideal. What're your thoughts on this?
There was a problem hiding this comment.
I believe that it would be better to just do
conn = connections.get(props)
Then, we need to decide what release should do if there is no connection.
One option could be that if there isn't anything to release, then do nothing.
Thoughts?
kratico
force-pushed
the
feat/rpc-tests
branch
from
cc936fc to
d09431a
Compare
Co-authored-by: Harry Solovay <[email protected]>
Resolves #139
Description
Add error test cases for RPC client, proxyProvider, smoldotProvider and fix a few related bugs