test: add dht interop tests #36

vasco-santos · 2018-09-17T16:20:56Z

This PR needs the following changes:

jhiesey

Looks good, just a few small comments. Do these tests pass at the moment?

Is it deliberate that there aren't tests that go over multiple hops (other than through the bootstrap node) anymore?

I think the way it is is fine, but have you considered a templated version of this test that just lets you specify the types of node0 through node3 and then call a function to run the test? I'm not very familiar with mocha so I don't know if that's tricky or not.

jhiesey · 2018-09-18T01:08:35Z

test/kad-dht.js

+  }, callback)
+}
+
+const getNodeId = (node, callback) => {


Maybe call this getBootstrapAddr?

Better naming yes, I will change it!

test/kad-dht.js

vasco-santos · 2018-09-18T08:50:55Z

Hey @jhiesey

Thanks for your help.

Looks good, just a few small comments. Do these tests pass at the moment?

The tests pass locally if you change the following here (node_modules):

start (callback) {
    this._running = true
    this.network.start((err) => {
      if (err) {
        return callback(err)
      }
      this.randomWalk.start(1, 100)

      callback()
    })
  }

Attention, this is not an appropriate change, as we should use the randomWalk defaults, but for that we have to change the way it is implemented with setInterval. I have started working on it yesterday, and hopefully today I will have a change to propose.

Is it deliberate that there aren't tests that go over multiple hops (other than through the bootstrap node) anymore?

I would like to create the tests with multiple hops as they were before. However, I don't know how to create a test in that format. The previous implementation of that format connected all the peers in a sequence. That way, the test was running successfully with the DHT disabled, so I believe that it was not a valid test. If you have any network suggestion to achieve that, I would like to add it!

I think the way it is is fine, but have you considered a templated version of this test that just lets you specify the types of node0 through node3 and then call a function to run the test? I'm not very familiar with mocha so I don't know if that's tricky or not.

I will have a refactor on this later, this is just a WIP for now, and before getting it ready for merging, I will try to reuse the most of the duplicated code.

jacobheun · 2018-10-30T13:44:32Z

DHT interop tests look good. We need to get ci green before this is good. If there are flakey, unrelated tests we should get a PR to resolve those. Right now the job history for this PR has no successes https://ci.ipfs.team/blue/organizations/jenkins/IPFS%2Finterop/activity?branch=PR-36

vasco-santos · 2018-10-30T13:59:49Z

@jacobheun in the last time that I ran the tests all were ok for the DHT tests, and the ones that failed were due timeouts:

https://ci.ipfs.team/blue/organizations/jenkins/IPFS%2Finterop/detail/PR-36/9/pipeline/361

This way, I wanted to have feedback about the PR before trying to have the all CI green.

test/browser.js

test/kad-dht.js

daviddias · 2019-02-13T09:54:37Z

@vasco-santos mind rebasing master onto this PR and test it with js-ipfs master?

alanshaw · 2019-04-04T15:39:11Z

@vasco-santos what's the status of this? Can we get CI green and merge?

vasco-santos · 2019-07-03T13:46:43Z

Now we are able to also add the libp2p options through the daemon.spawn 🎉

I will work on getting this updated with green CI

vasco-santos · 2019-07-19T09:24:40Z

@alanshaw this should be ready to go. What do you think?

daviddias · 2019-09-02T05:37:45Z

@vasco-santos mind rebasing master to confirm that CI is still green for clean merge?

vasco-santos · 2019-09-02T10:06:20Z

ready to go @daviddias

daviddias · 2019-09-02T12:39:48Z

test/browser.js

@@ -3,7 +3,7 @@

 // require('./exchange-files')
 // require('./pubsub')
-require('./kad-dht')
+// require('./kad-dht')


This means it is not being tested. Is that intended?

Yes, more info: #36 (review)

I can have a look after to that issue with id(), but I think that it should not be a blocker for this. I can create an issue to track it, as I think it is better to have these tests in now, than have nothing

#36 (comment)

daviddias · 2019-09-02T12:42:12Z

test/kad-dht.js

+
+    callback(null, res.addresses[0])
+  })
+}


This function is a tad misleading. All it does is extract the first listing address of a node. Perhaps rename to getNodeAddr?

Yeah, agree! I will change that!

daviddias · 2019-09-02T12:43:08Z

test/kad-dht.js

+            done()
+          })
+        })
+      })


It only tests the bootstrap with a JS Daemon, should be tested symmetrically (i.e. with go being the bootstrapper)

I am not sure if I understand your comment. But, I think I have those tests in the suite tests below

I see now below that the inverse happens

daviddias

I don't see any tests for situations where it is expected to fail, for example: two disjoint networks exist, seeking in one side succeeds and in the other fails, then, when both connect, seeking in both side succeeds

daviddias · 2019-09-02T12:44:14Z

test/kad-dht.js

+      before(function (done) {
+        this.timeout(70 * 1000)
+        const peersToSpawn = 3
+        const bootstrap = bootstrapAddr ? [bootstrapAddr] : []


In which case will bootstrapAddr not be available?

daviddias · 2019-09-02T12:47:00Z

test/kad-dht.js

+          spawnJsDaemon(bootstrap, (err, node) => {
+            expect(err).to.not.exist()
+            nodes.push(node)
+            next(err, node)


Why pass err here if it won't exist? Also, why pass node if you don't use the results out of the times call?

daviddias · 2019-09-02T12:48:30Z

test/kad-dht.js

+              }
+              cb(null, peers.length === peersToSpawn)
+            })
+          }


This function seems to only be testing if the first node is connected to all the other peers. It means that either:

The name testDhtReady is incorrect

It is creating a full mesh which will then defeat a bit of the purpose of testing the DHT

daviddias · 2019-09-02T12:51:49Z

test/kad-dht.js

+            done()
+          })
+        })
+      })


I see now below that the inverse happens

daviddias · 2019-09-02T12:55:16Z

test/kad-dht.js

+      it('should get from the network after being added', function (done) {
+        this.timeout(60 * 1000)
+
+        addFileAndCat(nodes[1], [nodes[2], nodes[3]], data, done)
      })
    })
  })


@vasco-santos I believe the tests above would be better if the tests were described through which nodes play which role (who is the node providing the content, who is the node fetching the content, who is the node bootstrapping the network, who are the other nodes) and make it just a simple config rather than hard wiring things by hand in every test.

What do you think?

daviddias · 2019-09-02T12:55:34Z

test/kad-dht.js

      })
    })
  })

-  describe('a Go node in the land of JS', () => {})
-  describe('hybrid', () => {})
+  describe('kad-dht with connections per peer', () => {


What does it mean "with connections per peer"?

daviddias · 2019-09-02T12:56:57Z

test/kad-dht.js

+        addFileAndCat(nodes[2], [nodes[3]], data, done)
+      })
+    })
+  })


Again, this batch of tests could be more clean if instead of c&p the tests, they were only written once and then called multiple times with the different node configurations

Adds additional tests from #36 as well as some extras requested in the PR comments. Refs: ipfs/js-ipfs#3905

achingbrain · 2021-10-01T17:04:51Z

Superseded by #376

ghost assigned vasco-santos Sep 17, 2018

ghost added the status/in-progress In progress label Sep 17, 2018

jhiesey reviewed Sep 18, 2018

View reviewed changes

vasco-santos mentioned this pull request Sep 19, 2018

2018 Q4 OKRs Planning libp2p/js-libp2p#247

Merged

vasco-santos force-pushed the add-dht-interop-tests branch from 41cf41d to 62600d8 Compare October 3, 2018 16:04

vasco-santos force-pushed the add-dht-interop-tests branch 5 times, most recently from 38730b9 to 18ae27b Compare October 29, 2018 12:16

vasco-santos changed the title ~~[WIP] chore: add dht interop tests~~ chore: add dht interop tests Oct 29, 2018

vasco-santos added the status/blocked Unable to be worked further until needs are met label Oct 29, 2018

vasco-santos mentioned this pull request Oct 29, 2018

Awesome Endeavour: DHT Part II ipfs/js-ipfs#856

Merged

35 tasks

daviddias changed the title ~~chore: add dht interop tests~~ test: add dht interop tests Oct 29, 2018

vasco-santos requested review from alanshaw, daviddias and jacobheun October 29, 2018 12:51

vasco-santos force-pushed the add-dht-interop-tests branch 3 times, most recently from 0a37a7e to 36285ce Compare November 11, 2018 14:05

alanshaw suggested changes Jan 2, 2019

View reviewed changes

vasco-santos force-pushed the add-dht-interop-tests branch 2 times, most recently from 1fe3d4c to 2b576c5 Compare January 10, 2019 11:06

daviddias changed the title ~~test: add dht interop tests~~ [WIP] test: add dht interop tests Feb 13, 2019

vasco-santos force-pushed the add-dht-interop-tests branch from 2b576c5 to edebc11 Compare April 5, 2019 14:38

vasco-santos mentioned this pull request Jul 3, 2019

IPNS Pubsub tests failing on master for linux #71

Closed

vasco-santos force-pushed the add-dht-interop-tests branch 7 times, most recently from 1e93916 to afc2267 Compare July 18, 2019 12:30

vasco-santos requested a review from alanshaw July 19, 2019 09:24

vasco-santos removed the status/blocked Unable to be worked further until needs are met label Aug 4, 2019

vasco-santos added 4 commits September 2, 2019 09:46

chore: add dht interop tests

2981d6a

chore: refactored interop tests

f3d5db1

fix: code review

6c6ad27

chore: use libp2p options on ctl

a6d7b31

vasco-santos force-pushed the add-dht-interop-tests branch from afc2267 to a6d7b31 Compare September 2, 2019 08:30

daviddias reviewed Sep 2, 2019

View reviewed changes

chore: address review

bdcd8fc

daviddias suggested changes Sep 2, 2019

View reviewed changes

vasco-santos mentioned this pull request Jan 17, 2020

DHT should be part of go-libp2p instead of go-ipfs libp2p/go-libp2p#773

Closed

achingbrain mentioned this pull request Oct 1, 2021

Track: enabling the DHT by default ipfs/js-ipfs#3905

Closed

13 tasks

achingbrain added a commit that referenced this pull request Oct 1, 2021

feat: expand dht tests

95430a2

Adds additional tests from #36 as well as some extras requested in the PR comments. Refs: ipfs/js-ipfs#3905

achingbrain mentioned this pull request Oct 1, 2021

feat: expand dht tests #376

Closed

achingbrain closed this Oct 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: add dht interop tests #36

test: add dht interop tests #36

vasco-santos commented Sep 17, 2018 •

edited

Loading

jhiesey left a comment

jhiesey Sep 18, 2018

vasco-santos Sep 18, 2018

vasco-santos commented Sep 18, 2018

jacobheun commented Oct 30, 2018

vasco-santos commented Oct 30, 2018

daviddias commented Feb 13, 2019

alanshaw commented Apr 4, 2019

vasco-santos commented Jul 3, 2019

vasco-santos commented Jul 19, 2019

daviddias commented Sep 2, 2019

vasco-santos commented Sep 2, 2019

daviddias Sep 2, 2019 •

edited

Loading

vasco-santos Sep 2, 2019

vasco-santos Sep 2, 2019

daviddias Sep 2, 2019

vasco-santos Sep 2, 2019

daviddias Sep 2, 2019

vasco-santos Sep 2, 2019

daviddias Sep 2, 2019

daviddias left a comment

daviddias Sep 2, 2019

daviddias Sep 2, 2019

daviddias Sep 2, 2019

daviddias Sep 2, 2019

daviddias Sep 2, 2019

daviddias Sep 2, 2019

daviddias Sep 2, 2019

achingbrain commented Oct 1, 2021

test: add dht interop tests #36

test: add dht interop tests #36

Conversation

vasco-santos commented Sep 17, 2018 • edited Loading

jhiesey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vasco-santos commented Sep 18, 2018

jacobheun commented Oct 30, 2018

vasco-santos commented Oct 30, 2018

daviddias commented Feb 13, 2019

alanshaw commented Apr 4, 2019

vasco-santos commented Jul 3, 2019

vasco-santos commented Jul 19, 2019

daviddias commented Sep 2, 2019

vasco-santos commented Sep 2, 2019

daviddias Sep 2, 2019 • 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

daviddias left a 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

Choose a reason for hiding this comment

achingbrain commented Oct 1, 2021

vasco-santos commented Sep 17, 2018 •

edited

Loading

daviddias Sep 2, 2019 •

edited

Loading