-
Notifications
You must be signed in to change notification settings - Fork 9
Conversation
5b3ca1d
to
e690d28
Compare
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.
I think it would be good to add a basic discovery module here and run the test suite on it, to ensure its working properly without checking other modules. Similar to https://github.com/libp2p/interface-connection/blob/refactor/async/test/compliance.spec.js. It will be super basic right now, but useful nonetheless.
it('should not fail to start the service if it is already started', async () => { | ||
await discovery.start() | ||
await discovery.start() | ||
}) |
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.
discovery.on
and discovery.removeListener
should get added as well, but those can wait until we add a proper test suite here, since they'll get tested as a part of that.
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.
Yap, I plan to add them in a next step, once we are working more closely with all the other peer discovery protocols.
Things are slightly different for each of them, and I need to have a better context on all of them to create a generic test setup for all. This way, I think we can start with this for the *-star
, and then upgrade them once we get into dht
and mdns
ones
0105e4c
to
34e18da
Compare
34e18da
to
b0b54db
Compare
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.
1 minor thing for release scripts, otherwise this looks good!
Updated API for async and added a naive suite of tests.
This test suite should be enhanced afterward.
Needed on: