-
Notifications
You must be signed in to change notification settings - Fork 43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use aegir for testing js-libp2p #104
Conversation
const IS_DIALER_STR = process.env.is_dialer | ||
const IP = process.env.ip | ||
|
||
const isDialer = IS_DIALER_STR === 'true' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might force this to true for browser (I didn't do this, but makes sense to me now)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
webrtc can listen, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can it? I was under the impression that this only will be possible once we support browser-to-browser, which we do not yet, do we?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't do this yet. Hopefully soon
throw new Error("Failed to wait for listener") | ||
} | ||
console.log(`node ${node.peerId} pings: ${otherMa}`) | ||
const rtt = await node.ping(multiaddr(otherMa)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this steps fails for me on my own approach due to what I discussed in the slack message. Something with the address being invalid for the transport in the browser. Does this work for you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The browser filters out undialable addresses with: https://github.com/libp2p/js-libp2p-websockets/blob/master/src/filters.ts#L24. So you need a dns + wss multiaddr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might want to add that to the README, that and any other (aegir) info :) I know readme's aren't added a lot so far, but to me they do help and they can help any potential contributor or reader/user to understand how to use the different tests and related code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To the websockets readme? This isn't related to aegir, just when running from browsers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meant more like to a README which would be placed https://github.com/libp2p/test-plans/tree/master/multidim-interop/js/v0.41, usually I like to add readme's or other doc on how to use the code, as I know from experience that not everybody is always an expert in all the different protocols used, and than it can be helpful to see a quick example how to use/run the code. It's next to such a WS Example paragraph that one can then add quick instructions how to run this with a DNS + wss mutliaddr, in all the detail (and nothing more) that is required to be able to run this)).
If this is already well documented, you can next to an example of course just link to that doc with a 1 liner attached to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WS Example paragraph that one can then add quick instructions how to run this with a DNS + wss mutliaddr, in all the detail (and nothing more) that is required to be able to run this)).
We can't run this with DNS + WSS yet because of the SSL error. But once we're able to do it I'll make sure to document and subtleties around it.
This is ready for review. Adds one browser -> go (webtransport) test case. I'll add the websockets one in a future PR when I update |
FYI @achingbrain, for js-libp2p head testing:
See #99 for more details. I'm happy to add this to js-libp2p since I already have context here. The biggest question for me is how to handle the various transport dependencies since they exist in separate repos. Does this mean that every release will also have to manually update the dependencies in the test? |
All in all I like this code. Good for you for pursuing the Aegir approach. If there's something I can do (in parallel or taking over (again)) in related to this PR or other interop testing stuff, do let me know here or via slack. I'm at your service @MarcoPolo . |
WebSockets won't be testable by the browser until we fix hugomrdias/playwright-test#525 |
def691c
to
ad1e88b
Compare
I'll merge this tomorrow so I can continue with more browser testing. |
This also adds browser support.
This doesn't actually let the browser dial the other node with websockets because the other node doesn't have a valid SSL cert. This can be fixed by passing in this option when we create the page in
playwright-test
(this will require an upstream change).This would work with webtransport.
Code needs a bit of clean up. The main files are .aegir.js which handles the setup/tear down and the redis proxy server, and ping.spec.ts which does the ping test.