Unit testing WebRTCTransport #19
Conversation
| @@ -39,7 +39,7 @@ export class WebRTCTransport implements Transport, Initializable { | |||
| } | |||
|
|
|||
| filter(multiaddrs: Multiaddr[]): Multiaddr[] { | |||
| return []; | |||
| return multiaddrs.filter(validMa); | |||
There was a problem hiding this comment.
@ckousik note that this is hiding in here.
test/transport.browser.spec.ts
Outdated
|
|
||
| it('filter gets rid of some invalids and returns a valid', async () => { | ||
| let mas: Multiaddr[] = [ | ||
| '/ip4/1.2.3.4/udp/1234/webrtc/certhash/uEiAUqV7kzvM1wI5DYDc1RbcekYVmXli_Qprlw3IkiEg6tQ', |
There was a problem hiding this comment.
It may be of interest to note, I did attempt to test filter in the case where a certhash is present but missing a character.
But filter takes as an argument a Multiaddr, not a string. And the constructor will throw an Error on that input, so you don't even get to call filter in the first place.
package.json
Outdated
| @@ -8,8 +8,8 @@ | |||
| "type": "module", | |||
| "scripts": { | |||
| "build": "aegir build", | |||
| "test": "aegir test", | |||
| "test:browser": "aegir test --target browser", | |||
| "clean": "aegir clean ; rm -rf node_modules/ ; rm -f package-lock.json yarn.lock", | |||
There was a problem hiding this comment.
Curious as to why there's a need to remove the lock files?
There was a problem hiding this comment.
Dumb paranoia on my part. I'm fine with getting rid of that script - don't think anyone else is running it anyhow.
src/sdp.ts
Outdated
| .replace('%s', ufrag) | ||
| .replace('%s', ufrag) | ||
| .replace('%s', certhashToFingerprint(ma)); | ||
| .replaceAll('{IP}', ip(ma)) |
There was a problem hiding this comment.
You could avoid this replacing by using builtin string interpolation and/or tagged templates: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals
There was a problem hiding this comment.
Yeah this would be a good use of that feature I was unaware of at the time I wrote this ;)
src/transport.ts
Outdated
| setTimeout(() => { | ||
| log.error('Data channel never opened. State was: %s', handshakeDataChannel.readyState.toString()); | ||
| dataChannelOpenPromise.reject() | ||
| }, 10000); |
There was a problem hiding this comment.
Possible magic number. Can you assign it to a variable? Also, can you comment on the significance of the number chose?
There was a problem hiding this comment.
It's a completely arbitrary number. I believe Chinmay used it when he was demonstrating the technique, and I stole it.
10 seconds does work as a timeout in that it's considerably longer than this process should ever take. But also, yes, it would be completely reasonable to name this variable.
| @@ -21,11 +23,33 @@ a=max-message-size:100000 | |||
|
|
|||
| describe('SDP creation', () => { | |||
| it('handles simple blue sky easily enough', async () => { | |||
| return; | |||
|
@John-LittleBearLabs just one little comment on a magic number, can merge after that. |
|
🎉 This PR is included in version 1.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Unit tests for Transport class
Also an impl for filter