fix ping tests #1685

daviddias · 2018-10-31T09:57:53Z

daviddias · 2018-10-31T09:58:31Z

test/http-api/inject/pubsub.js

-          expect(res.result.Strings).to.be.eql([])
-          done()
-        })
-      })


This test was no longer valid. I've checked go-ipfs and now it is indeed supposed for IPFS to return the list of peers we are pubsub'ing with when no topic is provided.

daviddias · 2018-10-31T10:35:17Z

Seems there are still some more issues on ping :(

daviddias · 2018-10-31T10:37:51Z

daviddias · 2018-10-31T13:20:55Z

src/core/components/ping-pull-stream.js

@@ -27,7 +27,8 @@ module.exports = function pingPullStream (self) {
      if (err) {
        log.error(err)
        source.push(getPacket({ success: false, text: err.toString() }))
-        source.end(err)
+        source.push(err)
+        source.end()


This made the tests pass but I'm not 100% convinced this is exactly what we want.

We need to differentiate between a real failure (invalid peer ID) and unavailable.

I've separated the getPeer from the runPing steps

daviddias · 2018-10-31T13:33:50Z

After debugging a bit more, I really think the problem is with the tests themselves. https://github.com/ipfs/interface-ipfs-core/blob/master/js/src/ping/ping.js#L58-L64 seems to be an antipattern to me. We can't expect to have error and results all at the same time.

daviddias · 2018-10-31T13:40:54Z

Now I also understand why people were reporting that Ping was not working because DHT is not available:

AssertionError: expected [Error: Error: DHT is not available] to not exist

It should instead say, that it couldn't find the Peer.

Error from go-ipfs

Error: Peer lookup error: routing: not found]

daviddias · 2018-10-31T16:50:00Z

Almost there. last mile is figuring out while the error doesn't propagate to the tests when using core directly (ipfs-api over http-api is fine)

Test Node.js


  cli
    ping
      ✓ ping host (955ms)
      ✓ ping host with --n option (791ms)
      ✓ ping host with --count option (908ms)

  HTTP API
Swarm listening on /ip4/127.0.0.1/tcp/0/ws/ipfs/QmQ2zigjQikYnyYUSXZydNXrDRhBut2mubwJBaLXobMt3A
Swarm listening on /ip4/127.0.0.1/tcp/62841/ipfs/QmQ2zigjQikYnyYUSXZydNXrDRhBut2mubwJBaLXobMt3A
API listening on /ip4/127.0.0.1/tcp/62842
Gateway (read only) listening on /ip4/127.0.0.1/tcp/62843
Web UI available at http://127.0.0.1:62842/webui
    ## http-api spec tests
      /ping
        ✓ returns 400 if both n and count are provided
        ✓ returns 400 if arg is not provided
        ✓ returns 200 and the response stream with the ping result

  interface-ipfs-core over ipfs-api tests
    .ping
      ✓ should send the specified number of packets
      ✓ should fail when pinging a peer that is not available
      ✓ should fail when pinging an invalid peer Id
    .pingPullStream
      ✓ should send the specified number of packets over pull stream
      ✓ should fail when pinging an unknown peer over pull stream
      ✓ should fail when pinging an invalid peer id over pull stream
    .pingReadableStream
      ✓ should send the specified number of packets over readable stream
      ✓ should fail when pinging peer that is not available over readable stream
      ✓ should fail when pinging an invalid peer id over readable stream

  interface-ipfs-core tests
    .ping
Swarm listening on /ip4/127.0.0.1/tcp/62915/ipfs/QmeSfaoj3aAEJUH5T7hJ7ULu1evo8j1TewyQhzvPESfaR3
Swarm listening on /ip4/127.0.0.1/tcp/62917/ipfs/QmX98kZpNKipM1EifTj6wqsYS6mmAQX37oYNmVpWPz8dv3
      ✓ should send the specified number of packets
      1) should fail when pinging a peer that is not available
      2) should fail when pinging an invalid peer Id
    .pingPullStream
Swarm listening on /ip4/127.0.0.1/tcp/62924/ipfs/QmY31MzrqTECyDz1MBHWtdMc4DyDPAMvoTXfZSDtYvhQ7K
Swarm listening on /ip4/127.0.0.1/tcp/62928/ipfs/QmTttTG2nVztU7uw4Xq76qwZEQrQxvG5tnmR8Cw5bBQGXG
      ✓ should send the specified number of packets over pull stream
      3) should fail when pinging an unknown peer over pull stream
      4) should fail when pinging an invalid peer id over pull stream
    .pingReadableStream
Swarm listening on /ip4/127.0.0.1/tcp/62935/ipfs/QmVZaeQC65aPEpkrwEnp5SUsvdxXEph4168tfij6i3aFG2
Swarm listening on /ip4/127.0.0.1/tcp/62939/ipfs/QmPLxuVK8hqzdTV3RzZP8hdrChuykNXGuNqdoSFChg4iqs
      ✓ should send the specified number of packets over readable stream
      5) should fail when pinging peer that is not available over readable stream
      6) should fail when pinging an invalid peer id over readable stream

daviddias · 2018-10-31T18:00:06Z

src/http/api/resources/ping.js

@@ -23,9 +24,19 @@ exports.get = {
  handler: (request, reply) => {


@alanshaw @hugomrdias @achingbrain et al, can I get your eyes to go through this handler. This stream tagliatelle is a nightmare came true.

Stream tagliatelle - I’m going to use that one

alanshaw

Buffering isn’t ideal but it’ll do for now!

ghost assigned daviddias Oct 31, 2018

ghost added the status/in-progress In progress label Oct 31, 2018

daviddias commented Oct 31, 2018

View reviewed changes

daviddias mentioned this pull request Oct 31, 2018

ping tests are failing #1684

Closed

daviddias force-pushed the fix-ping branch from 05b0c5e to 9e1a811 Compare October 31, 2018 13:19

daviddias commented Oct 31, 2018

View reviewed changes

daviddias requested a review from alanshaw October 31, 2018 13:21

This was referenced Oct 31, 2018

fix: remove antipattern from ping tests ipfs-inactive/interface-js-ipfs-core#386

Merged

fix: remove accidentally committed code #1686

Merged

daviddias force-pushed the fix-ping branch from 77a8f8f to ae8fd30 Compare October 31, 2018 17:58

daviddias commented Oct 31, 2018

View reviewed changes

alanshaw approved these changes Nov 1, 2018

View reviewed changes

daviddias added 2 commits November 1, 2018 09:26

chore: add missing dep, pump

e9e369c

feat: get Ping to work properly

02783fe

daviddias force-pushed the fix-ping branch from 68b099c to 02783fe Compare November 1, 2018 09:27

daviddias merged commit 27d5a57 into master Nov 1, 2018

ghost removed the status/in-progress In progress label Nov 1, 2018

daviddias deleted the fix-ping branch November 1, 2018 09:38

baby636 mentioned this pull request Jun 21, 2023

[Snyk] Fix for 1 vulnerabilities baby636/js-ipfs#584

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix ping tests #1685

fix ping tests #1685

daviddias commented Oct 31, 2018

daviddias Oct 31, 2018

daviddias commented Oct 31, 2018

daviddias commented Oct 31, 2018

daviddias Oct 31, 2018

alanshaw Oct 31, 2018

daviddias Oct 31, 2018

daviddias commented Oct 31, 2018

daviddias commented Oct 31, 2018 •

edited

Loading

daviddias commented Oct 31, 2018

daviddias Oct 31, 2018

achingbrain Nov 1, 2018

alanshaw left a comment

		@@ -23,9 +24,19 @@ exports.get = {
		handler: (request, reply) => {

fix ping tests #1685

fix ping tests #1685

Conversation

daviddias commented Oct 31, 2018

daviddias Oct 31, 2018

Choose a reason for hiding this comment

daviddias commented Oct 31, 2018

daviddias commented Oct 31, 2018

daviddias Oct 31, 2018

Choose a reason for hiding this comment

alanshaw Oct 31, 2018

Choose a reason for hiding this comment

daviddias Oct 31, 2018

Choose a reason for hiding this comment

daviddias commented Oct 31, 2018

daviddias commented Oct 31, 2018 • edited Loading

daviddias commented Oct 31, 2018

daviddias Oct 31, 2018

Choose a reason for hiding this comment

achingbrain Nov 1, 2018

Choose a reason for hiding this comment

alanshaw left a comment

Choose a reason for hiding this comment

daviddias commented Oct 31, 2018 •

edited

Loading