Skip to content
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

Merged
merged 12 commits into from
Jan 24, 2023
Merged

Use aegir for testing js-libp2p #104

merged 12 commits into from
Jan 24, 2023

Conversation

MarcoPolo
Copy link
Contributor

@MarcoPolo MarcoPolo commented Jan 14, 2023

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.

const IS_DIALER_STR = process.env.is_dialer
const IP = process.env.ip

const isDialer = IS_DIALER_STR === 'true'
Copy link
Contributor

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

webrtc can listen, no?

Copy link
Contributor

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?

Copy link
Contributor Author

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))
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

@MarcoPolo
Copy link
Contributor Author

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 playwright-test.

@MarcoPolo MarcoPolo marked this pull request as ready for review January 17, 2023 20:06
@MarcoPolo MarcoPolo requested a review from achingbrain January 17, 2023 20:06
@MarcoPolo
Copy link
Contributor Author

FYI @achingbrain, for js-libp2p head testing:

  1. We should have an implementation like this (or exactly this) in the js-libp2p repo.
  2. Have js-libp2p call this workflow from its own workflow.

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?

@GlenDC
Copy link
Contributor

GlenDC commented Jan 17, 2023

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 .

@MarcoPolo
Copy link
Contributor Author

WebSockets won't be testable by the browser until we fix hugomrdias/playwright-test#525

@MarcoPolo
Copy link
Contributor Author

I'll merge this tomorrow so I can continue with more browser testing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants