chore: remove try-catch from tests where functions are async (#2531)

achingbrain · web-flow · commit 220d1f07345c · 2019-10-15T10:52:16.000+01:00
* chore: remove try-catch from tests where functions are async We had a few instances in the tests where assertions may be missed due to functions not throwing where we thought they would so this PR refactors that code to use `.then(onResult, onReject)` instead. Follows on from #2514 (comment)
diff --git a/package.json b/package.json
@@ -173,7 +173,7 @@
     "pull-defer": "~0.2.3",
     "pull-file": "^1.1.0",
     "pull-mplex": "~0.1.1",
-    "pull-ndjson": "~0.1.1",
+    "pull-ndjson": "^0.2.0",
     "pull-pushable": "^2.2.0",
     "pull-sort": "^1.0.1",
     "pull-stream": "^3.6.14",
diff --git a/src/core/components/pubsub.js b/src/core/components/pubsub.js
@@ -33,7 +33,11 @@ module.exports = function pubsub (self) {
         return
       }
 
-      checkOnlineAndEnabled()
+      try {
+        checkOnlineAndEnabled()
+      } catch (err) {
+        return Promise.reject(err)
+      }
 
       return self.libp2p.pubsub.subscribe(topic, handler, options)
     },
@@ -50,7 +54,11 @@ module.exports = function pubsub (self) {
         return
       }
 
-      checkOnlineAndEnabled()
+      try {
+        checkOnlineAndEnabled()
+      } catch (err) {
+        return Promise.reject(err)
+      }
 
       return self.libp2p.pubsub.unsubscribe(topic, handler)
     },
diff --git a/src/http/api/resources/files-regular.js b/src/http/api/resources/files-regular.js
@@ -336,6 +336,12 @@ exports.refs = {
     const unique = request.query.unique
     const maxDepth = request.query['max-depth']
 
+    // have to do this here otherwise the validation error appears in the stream tail and
+    // this doesn't work in browsers: https://github.com/ipfs/js-ipfs/issues/2519
+    if (edges && format !== Format.default) {
+      throw Boom.badRequest('Cannot set edges to true and also specify format')
+    }
+
     const source = ipfs.refsPullStream(key, { recursive, format, edges, unique, maxDepth })
     return sendRefsReplyStream(request, h, `refs for ${key}`, source)
   }
diff --git a/test/cli/daemon.js b/test/cli/daemon.js
@@ -104,14 +104,12 @@ describe('daemon', () => {
       }
     })
 
-    try {
-      await daemon
-      throw new Error('Did not kill process')
-    } catch (err) {
-      expect(err.killed).to.be.true()
-
-      expect(stdout).to.include('Daemon is ready')
-    }
+    await expect(daemon)
+      .to.eventually.be.rejected()
+      .and.to.include({
+        killed: true
+      })
+      .and.to.have.property('stdout').that.includes('Daemon is ready')
   })
 
   it('should allow bind to multiple addresses for API and Gateway', async function () {
@@ -142,15 +140,14 @@ describe('daemon', () => {
       }
     })
 
-    try {
-      await daemon
-      throw new Error('Did not kill process')
-    } catch (err) {
-      expect(err.killed).to.be.true()
+    const err = await expect(daemon)
+      .to.eventually.be.rejected()
+      .and.to.include({
+        killed: true
+      })
 
-      apiAddrs.forEach(addr => expect(err.stdout).to.include(`API listening on ${addr.slice(0, -2)}`))
-      gatewayAddrs.forEach(addr => expect(err.stdout).to.include(`Gateway (read only) listening on ${addr.slice(0, -2)}`))
-    }
+    apiAddrs.forEach(addr => expect(err.stdout).to.include(`API listening on ${addr.slice(0, -2)}`))
+    gatewayAddrs.forEach(addr => expect(err.stdout).to.include(`Gateway (read only) listening on ${addr.slice(0, -2)}`))
   })
 
   it('should allow no bind addresses for API and Gateway', async function () {
@@ -171,15 +168,12 @@ describe('daemon', () => {
       }
     })
 
-    try {
-      await daemon
-      throw new Error('Did not kill process')
-    } catch (err) {
-      expect(err.killed).to.be.true()
-
-      expect(err.stdout).to.not.include('API listening on')
-      expect(err.stdout).to.not.include('Gateway (read only) listening on')
-    }
+    await expect(daemon)
+      .to.eventually.be.rejected()
+      .and.to.include({
+        killed: true
+      })
+      .and.have.property('stdout').that.does.not.include(/(API|Gateway \(read only\)) listening on/g)
   })
 
   skipOnWindows('should handle SIGINT gracefully', async function () {
@@ -211,32 +205,17 @@ describe('daemon', () => {
     await ipfs('init')
 
     const daemon = ipfs('daemon --silent')
-    const stop = async (err) => {
-      daemon.kill()
 
-      if (err) {
-        throw err
-      }
+    setTimeout(() => {
+      daemon.kill('SIGKILL')
+    }, 5 * 1000)
 
-      try {
-        await daemon
-      } catch (err) {
-        if (!err.killed) {
-          throw err
-        }
-      }
-    }
-
-    return new Promise((resolve, reject) => {
-      daemon.stdout.on('data', (data) => {
-        reject(new Error('Output was received ' + data.toString('utf8')))
+    await expect(daemon)
+      .to.eventually.be.rejected()
+      .and.to.include({
+        killed: true,
+        stdout: ''
       })
-
-      setTimeout(() => {
-        resolve()
-      }, 5 * 1000)
-    })
-      .then(stop, stop)
   })
 
   it('should present ipfs path help when option help is received', async function () {
@@ -262,16 +241,15 @@ describe('daemon', () => {
       }
     })
 
-    try {
-      await daemon
-      throw new Error('Did not kill process')
-    } catch (err) {
-      expect(err.killed).to.be.true()
+    const err = await expect(daemon)
+      .to.eventually.be.rejected()
+      .and.to.include({
+        killed: true
+      })
 
-      expect(err.stdout).to.include(`js-ipfs version: ${pkg.version}`)
-      expect(err.stdout).to.include(`System version: ${os.arch()}/${os.platform()}`)
-      expect(err.stdout).to.include(`Node.js version: ${process.versions.node}`)
-    }
+    expect(err.stdout).to.include(`js-ipfs version: ${pkg.version}`)
+    expect(err.stdout).to.include(`System version: ${os.arch()}/${os.platform()}`)
+    expect(err.stdout).to.include(`Node.js version: ${process.versions.node}`)
   })
 
   it('should init by default', async function () {
@@ -290,12 +268,11 @@ describe('daemon', () => {
       }
     })
 
-    try {
-      await daemon
-      throw new Error('Did not kill process')
-    } catch (err) {
-      expect(err.killed).to.be.true()
-    }
+    await expect(daemon)
+      .to.eventually.be.rejected()
+      .and.to.include({
+        killed: true
+      })
 
     expect(fs.existsSync(repoPath)).to.be.true()
   })
diff --git a/test/cli/init.js b/test/cli/init.js
@@ -83,11 +83,9 @@ describe('init', function () {
   it('profile non-existent', async function () {
     this.timeout(40 * 1000)
 
-    try {
-      await ipfs('init --profile doesnt-exist')
-    } catch (err) {
-      expect(err.stdout).includes('Could not find profile')
-    }
+    await expect(ipfs('init --profile doesnt-exist'))
+      .to.eventually.be.rejected()
+      .and.to.have.property('stdout').that.includes('Could not find profile')
   })
 
   it('should present ipfs path help when option help is received', async function () {
diff --git a/test/core/create-node.spec.js b/test/core/create-node.spec.js
@@ -171,20 +171,14 @@ describe('create node', function () {
 
     expect(ipfs.isOnline()).to.be.false()
 
-    try {
-      await ipfs.ready
-    } catch (err) {
-      expect(ipfs.isOnline()).to.be.false()
-
-      // After the error has occurred, it should still reject
-      try {
-        await ipfs.ready
-      } catch (_) {
-        return
-      }
-    }
+    await expect(ipfs.ready)
+      .to.eventually.be.rejected()
+
+    expect(ipfs.isOnline()).to.be.false()
 
-    throw new Error('ready promise did not reject')
+    // After the error has occurred, it should still reject
+    await expect(ipfs.ready)
+      .to.eventually.be.rejected()
   })
 
   it('should create a ready node with IPFS.create', async () => {
diff --git a/test/core/dht.spec.js b/test/core/dht.spec.js
@@ -74,16 +74,10 @@ describe.skip('dht', () => {
     })
 
     describe('put', () => {
-      it('should callback with error for DHT not available', async () => {
-        let res
-        try {
-          res = await ipfs.dht.put(Buffer.from('a'), Buffer.from('b'))
-        } catch (err) {
-          expect(err).to.exist()
-          expect(err.code).to.equal('ERR_DHT_DISABLED')
-        }
-
-        expect(res).to.not.exist()
+      it('should error when DHT not available', async () => {
+        await expect(ipfs.dht.put(Buffer.from('a'), Buffer.from('b')))
+          .to.eventually.be.rejected()
+          .and.to.have.property('code', 'ERR_DHT_DISABLED')
       })
     })
   })
diff --git a/test/core/gc.spec.js b/test/core/gc.spec.js
@@ -7,7 +7,6 @@ const IPFSFactory = require('ipfsd-ctl')
 const pEvent = require('p-event')
 const env = require('ipfs-utils/src/env')
 const IPFS = require('../../src/core')
-const { Errors } = require('interface-datastore')
 
 // We need to detect when a readLock or writeLock is requested for the tests
 // so we override the Mutex class to emit an event
@@ -189,11 +188,11 @@ describe('gc', function () {
       await rm1
 
       // Second rm should fail because GC has already removed that block
-      try {
-        await rm2
-      } catch (err) {
-        expect(err.code).eql(Errors.dbDeleteFailedError().code)
-      }
+      const results = await rm2
+      const result = results
+        .filter(result => result.hash === cid2.toString())
+        .pop()
+      expect(result).to.have.property('error').that.contains('block not found')
 
       // Confirm second block has been removed
       const localRefs = (await ipfs.refs.local()).map(r => r.ref)
diff --git a/test/core/libp2p.spec.js b/test/core/libp2p.spec.js
@@ -11,7 +11,6 @@ const Multiplex = require('pull-mplex')
 const SECIO = require('libp2p-secio')
 const KadDHT = require('libp2p-kad-dht')
 const Libp2p = require('libp2p')
-const isNode = require('detect-node')
 
 const libp2pComponent = require('../../src/core/components/libp2p')
 
@@ -315,40 +314,5 @@ describe('libp2p customization', function () {
         done()
       })
     })
-
-    it('select floodsub as pubsub router if node', (done) => {
-      const ipfs = {
-        _repo: {
-          datastore
-        },
-        _peerInfo: peerInfo,
-        _peerBook: peerBook,
-        // eslint-disable-next-line no-console
-        _print: console.log,
-        _options: {}
-      }
-      const customConfig = {
-        ...testConfig,
-        Pubsub: {
-          Router: 'floodsub'
-        }
-      }
-
-      try {
-        _libp2p = libp2pComponent(ipfs, customConfig)
-      } catch (err) {
-        if (!isNode) {
-          expect(err).to.exist()
-          expect(err.code).to.eql('ERR_NOT_SUPPORTED')
-          done()
-        }
-      }
-
-      _libp2p.start((err) => {
-        expect(err).to.not.exist()
-        expect(_libp2p._modules.pubsub).to.eql(require('libp2p-floodsub'))
-        done()
-      })
-    })
   })
 })
diff --git a/test/core/name-pubsub.js b/test/core/name-pubsub.js
@@ -106,8 +106,9 @@ describe('name-pubsub', function () {
     const keys = ipns.getIdKeys(fromB58String(idA.id))
     const topic = `${namespace}${base64url.encode(keys.routingKey.toBuffer())}`
 
-    await nodeB.name.resolve(idA.id)
-      .then(() => expect.fail('should not have been able to resolve idA.id'), (err) => expect(err).to.exist())
+    await expect(nodeB.name.resolve(idA.id))
+      .to.eventually.be.rejected()
+      .and.to.have.property('code', 'ERR_NO_RECORD_FOUND')
 
     await waitForPeerToSubscribe(nodeA, topic)
     await nodeB.pubsub.subscribe(topic, checkMessage)
@@ -128,11 +129,9 @@ describe('name-pubsub', function () {
     const resolvesEmpty = await nodeB.name.resolve(idB.id)
     expect(resolvesEmpty).to.be.eq(emptyDirCid)
 
-    try {
-      await nodeA.name.resolve(idB.id)
-    } catch (error) {
-      expect(error).to.exist()
-    }
+    await expect(nodeA.name.resolve(idB.id))
+      .to.eventually.be.rejected()
+      .and.to.have.property('code', 'ERR_NO_RECORD_FOUND')
 
     const publish = await nodeB.name.publish(path)
     expect(publish).to.be.eql({
diff --git a/test/core/name.spec.js b/test/core/name.spec.js
diff --git a/test/core/pubsub.spec.js b/test/core/pubsub.spec.js
diff --git a/test/core/utils.js b/test/core/utils.js
