add callback to pubsub.unsubscribe and test (#300) #302

noot · 2018-12-20T16:52:41Z

As per the issue I opened: #300

Upon pubsub.unsubscribe, the node was successfully unsubscribing, however the callback was never being called, so there was no explicit way to confirm success.

I have changed unsubscribe as to call the callback, so it now complies with the interface-ipfs-core spec. I have also added a test.

Let me know if any changes are needed, thanks!

noot · 2018-12-20T18:16:11Z

Not sure how to fix the remaining failing tests, sorry.

jacobheun

Don't worry about the commitlint ci failure, I can resolve that during the merge. We use the commits to generate changelogs so they are prefixed with certain values to denote what should be updated in the changelog, such as feat:, fix:, test:. Added a few notes on changes.

jacobheun · 2018-12-20T18:25:29Z

src/pubsub.js

@@ -43,6 +43,8 @@ module.exports = (node) => {
      if (floodSub.listenerCount(topic) === 0) {
        floodSub.unsubscribe(topic)
      }
+
+      setImmediate(() => callback())


Since the callback is being added, even though it should have already exists, we should check that it is actually a function before invoking it.

makes sense, I added a check

jacobheun · 2018-12-20T18:30:58Z

test/pubsub.node.js

+              expect(err).to.not.exist()
+              console.log('tunsubscribed!')
+              done()
+            }), 600)


We should get rid of the timeouts in this test. Subscribing -> publishing -> unsubscribing should be able to be executed in serial successfully without a timeout.

…t from tests

jacobheun · 2018-12-20T21:57:59Z

test/pubsub.node.js

+            nodes[0].pubsub.unsubscribe('pubsub', handler, (err) => {
+              expect(err).to.not.exist()
+              console.log('\tunsubscribed!')
+              done()


It's possible done could be called before we run the expect in the publish handler. It would be good to include chai-checkmark in this file and mark each of the expects. That way we can do expect(4).checks(done) so we can ensure everything is verified before finishing the test.

Remove the console.log as well and I think this is good to go. Thank you for the PR!

Sounds good! I've added chai-checkmark and the check to make sure all the expects happen, as well as moved done and got rid of the console.log.

done() is still being called here, so the test is finishing before expect check happens. Should just need to delete done on line 72.

When I remove done() at line 72, I get Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. If we place done() at line 77, at the end of the waterfall, it works. I'm not sure if this correctly allows for the expect check to happen though.

noot · 2019-01-03T17:28:30Z

Hey, any updates on this?

jacobheun · 2019-01-03T17:32:20Z

Sorry for the delay, I've been away on holiday but am back now. The test done logic is still not happening correctly. Done is being called and is passed as a callback to waterfall, this isn't correct. The timeout you were hitting is probably because things aren't firing in the right order, or are being triggered early. I can take a deeper look at what the issue is later today.

noot · 2019-01-03T17:51:12Z

@jacobheun No worries! Sounds good, thanks. Let me know what issues you find.

jacobheun · 2019-01-04T18:16:29Z

There were some errors in the test, primarily some of the function arguments and .mark() not being added for chai-checkmark. The delay after the subscribe is also important, otherwise publish will likely be called before the peer is aware of the subscriber.

I think it was a bit hard to follow so I made some changes along with the fixes that hopefully make things a bit easier to read.

    it('start two nodes and send one message, then unsubscribe', (done) => {
      // Check the final series error, and the publish handler
      expect(2).checks(done)

      let nodes
      const data = Buffer.from('test')
      const handler = (msg) => {
        // verify the data is correct and mark the expect
        expect(msg.data).to.eql(data).mark()
      }

      series([
        // Start the nodes
        (cb) => startTwo((err, _nodes) => {
          nodes = _nodes
          cb(err)
        }),
        // subscribe on the first
        (cb) => nodes[0].pubsub.subscribe('pubsub', handler, cb),
        // Wait a moment before publishing
        (cb) => setTimeout(cb, 500),
        // publish on the second
        (cb) => nodes[1].pubsub.publish('pubsub', data, cb),
        // unsubscribe on the first
        (cb) => nodes[0].pubsub.unsubscribe('pubsub', handler, cb),
        // Stop both nodes
        (cb) => stopTwo(nodes, cb)
      ], (err) => {
        // Verify there was no error, and mark the expect
        expect(err).to.not.exist().mark()
      })
    })

If that looks good and makes sense to you can you push the update?

noot · 2019-01-04T19:23:58Z

@jacobheun That's a lot easier to follow, I think it makes sense. I'll push the changes now

jacobheun · 2019-01-04T22:38:39Z

I'm going to look into the windows ci failure. It's possible it's just flakey, but it needs to get fixed either way.

noot · 2019-01-05T17:46:10Z

okay cool, let me know if I need to make any changes.

noot · 2019-01-19T04:40:57Z

@jacobheun hey, any updates or changes needed?

jacobheun · 2019-01-22T14:50:38Z

@noot not yet, I was away last week, hoping to take a look at it this week.

jacobheun · 2019-02-01T16:23:47Z

test/pubsub.node.js

+        // Wait a moment before publishing
+        (cb) => setTimeout(cb, 500),
+        // publish on the second
+        (cb) => nodes[1].pubsub.publish('pubsub', data, cb),


@noot sorry for the delay, it looks like the tests were just failing because windows needs some time before unsubscribing. This should fix it:

Suggested change

(cb) => nodes[1].pubsub.publish('pubsub', data, cb),

(cb) => nodes[1].pubsub.publish('pubsub', data, cb),

// Wait a moment before unsubscribing

(cb) => setTimeout(cb, 500),

I've made the changes!

jacobheun · 2019-02-01T18:46:05Z

@noot thank you for the PR! I've squashed and merged this to master via 679d446 to fix the code and commitlint.

Co-authored-by: Alex Potsides <[email protected]>

## [1.0.13](libp2p/js-libp2p-crypto@v1.0.12...v1.0.13) (2023-03-10) ### Trivial Changes * Update .github/workflows/semantic-pull-request.yml [skip ci] ([b66007c](libp2p/js-libp2p-crypto@b66007c)) * Update .github/workflows/semantic-pull-request.yml [skip ci] ([110063c](libp2p/js-libp2p-crypto@110063c)) * Update .github/workflows/semantic-pull-request.yml [skip ci] ([c789b0c](libp2p/js-libp2p-crypto@c789b0c)) ### Dependencies * **dev:** upgrade aegir to `38.1.2` ([libp2p#302](libp2p/js-libp2p-crypto#302)) ([9d60e39](libp2p/js-libp2p-crypto@9d60e39))

noot added 2 commits December 20, 2018 11:48

add callback to pubsub.unsubscribe and test (libp2p#300)

473eaf4

fix linting issues

2d724d7

jacobheun suggested changes Dec 20, 2018

View reviewed changes

pubsub.unsubscribe: check that callback is a function; removed timeou…

e937f73

…t from tests

jacobheun reviewed Dec 20, 2018

View reviewed changes

noot added 2 commits December 20, 2018 17:05

remove console.log; add chai-checkmark

e0d715b

move done to end of waterfall

b7eb218

noot added 2 commits January 4, 2019 14:24

update pubsub test

e4a45c6

fix lint; remove unneeded waterfall

e446e7f

jacobheun reviewed Feb 1, 2019

View reviewed changes

add delay before unsubscribe

2dce1ba

jacobheun closed this Feb 1, 2019

jacobheun mentioned this pull request Feb 1, 2019

pubsub.unsubscribe should comply with interface-ipfs-core spec, since it links to it in the docs #300

Closed

maschad added a commit to maschad/js-libp2p that referenced this pull request Jun 21, 2023

deps(dev): upgrade aegir to 38.1.2 (libp2p#302)

9d60e39

Co-authored-by: Alex Potsides <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add callback to pubsub.unsubscribe and test (#300) #302

add callback to pubsub.unsubscribe and test (#300) #302

noot commented Dec 20, 2018

noot commented Dec 20, 2018

jacobheun left a comment •

edited

Loading

jacobheun Dec 20, 2018

noot Dec 20, 2018

jacobheun Dec 20, 2018

noot Dec 20, 2018

jacobheun Dec 20, 2018

noot Dec 20, 2018

jacobheun Dec 20, 2018

noot Dec 20, 2018 •

edited

Loading

noot commented Jan 3, 2019

jacobheun commented Jan 3, 2019

noot commented Jan 3, 2019

jacobheun commented Jan 4, 2019

noot commented Jan 4, 2019

jacobheun commented Jan 4, 2019

noot commented Jan 5, 2019

noot commented Jan 19, 2019

jacobheun commented Jan 22, 2019

jacobheun Feb 1, 2019

noot Feb 1, 2019

jacobheun commented Feb 1, 2019

add callback to pubsub.unsubscribe and test (#300) #302

add callback to pubsub.unsubscribe and test (#300) #302

Conversation

noot commented Dec 20, 2018

noot commented Dec 20, 2018

jacobheun left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noot Dec 20, 2018 • edited Loading

Choose a reason for hiding this comment

noot commented Jan 3, 2019

jacobheun commented Jan 3, 2019

noot commented Jan 3, 2019

jacobheun commented Jan 4, 2019

noot commented Jan 4, 2019

jacobheun commented Jan 4, 2019

noot commented Jan 5, 2019

noot commented Jan 19, 2019

jacobheun commented Jan 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobheun commented Feb 1, 2019

jacobheun left a comment •

edited

Loading

noot Dec 20, 2018 •

edited

Loading