Interop test between Go and Js implementations #23
Conversation
…on change to the spec PR.
…t for connecting to it.
|
@ckousik I think the recent merges created conflicts for this branch |
There was a problem hiding this comment.
Did not yet go too deep into the business logic itself, as we are also missing some documentation. Next to these comments I suggest to add a README section (for developers) about the testing, how to run them, and what are some special pre-requisites (e.g. the go-libp2p setup and special things there). Also I would make errors more clear where you can. E.g. if you say "cannot run this because I need that, than you might as well explain how to provide 'that'".
Once these comments are resolved and the documentation is also improved, I could try to look into business logic if desired. I am new to libp2p and webrtc though, so that will require some serious learning on the fly :) For that reason might also be useful if you can link in the documentation the relevant specs and RFC's.
| @@ -6,5 +6,4 @@ export interface WebRTCListenerOptions extends CreateListenerOptions { | |||
| // channelOptions?: WebRTCReceiverInit | |||
| } | |||
|
|
|||
| export interface WebRTCDialOptions extends DialOptions { | |||
| } | |||
| export interface WebRTCDialOptions extends DialOptions {} | |||
There was a problem hiding this comment.
our linting rules must be pretty relaxed as normally you would get an "empty interface" complaint here, no?
| @@ -1,8 +1,12 @@ | |||
| { | |||
| "extends": "aegir/src/config/tsconfig.aegir.json", | |||
| "compilerOptions": { | |||
There was a problem hiding this comment.
might want to make the ts rules a bit more strict, still allows quiet some potential errors from what I can tell, or perhaps I am wrong. You tell me.
| if (SERVER_MULTIADDR) { | ||
| console.log('Will test connecting to', SERVER_MULTIADDR); | ||
| } else { | ||
| console.log('Will not test connecting to an external server, as we do not appear to have one.'); |
There was a problem hiding this comment.
With export var SERVER_MULTIADDR = '';, does this test never run?
There was a problem hiding this comment.
It gets rewritten through an npm script. cc @John-LittleBearLabs
There was a problem hiding this comment.
Yeah. If you're doing test and not test:interop then it doesn't get overwritten and you won't run this.
| @@ -60,14 +61,22 @@ export class WebRTCConnection implements ic.Connection { | |||
| }, | |||
| }; | |||
| this.handleIncomingStreams(); | |||
| this.peerConnection.onconnectionstatechange = (_) => { | |||
There was a problem hiding this comment.
why not just put const closed = ['closed', 'failed', 'disconnected'].includes(this.peer.connectionState);
Avoids the whole switch statement etc.
|
🎉 This PR is included in version 1.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This PR creates an interoperability test between the Go and Js implementation and aims to automate the test in CI.