Skip to content
This repository has been archived by the owner on Sep 30, 2023. It is now read-only.

refactor: update deps, add tests, make more lightweight #11

Merged
merged 1 commit into from
May 11, 2020

Conversation

aphelionz
Copy link
Contributor

@aphelionz aphelionz commented Apr 19, 2020

This PR updates:

  • go-ipfs-dep to ~0.5.0
  • ipfs to ~0.43.0
  • ipfs-repo to ^1.0.1
  • ipfsd-ctl to ^3.1.0

Also, updating ipfsd-ctl allowed me to (at least attempt to) clean up this a bit. I removed startIpfs and stopIpfs created a factory object with preferred configuration from ipfsd-ctl. I extended the prototype to create two new functions to the factory:

  1. spawnApi to deal with the simple boilerplate of:
const ipfsd = factory.spawn()
const ipfs = ipfsd.api
  1. localCluster, which is where the real magic [can] happen, which takes an array of ipfs types like js, go, proc, etc, connects them all via permutation, and then returns the array of connected nodes.

It will take refactoring the downstream tests a but but I believe this gives us a few advantages:

  1. We can spawn as many daemons, in whatever configuration we want, as we want during our tests, including js and go nodes in the same tests, while also still being able to loop through ['js', 'go'] in our tests like we usually do, if we want. Note that we can also still use waitForPeers, and connectPeers.
  2. We can use the factory.clean() method in the after methods in our mocha tests to properly clean up all the spawned daemons and not run into any more issues where the process doesn't close.
  3. Less code overall,

Tradeoffs

  • The ipfsd-cts exports some ipfsd object, where the object we really want it ipfsd.api. Just an extra thing but I couldn't find a place to really return that properly to keep things consistent. taken care of 🎉
  • There's still the implementations business to be dealt with, see my self-review comments below.

TODO if/when approved

  • Figure out process.env.TEST support
  • Update docs
  • squash commits
  • update to 0.9.0
  • Add CI moved to another PR

npm install time on master (Thinkpad W540, 8 cores, 24GB ram)

real 1m32.161s
user 1m26.220s
sys 0m11.440s

Code coverage:

------------------------|---------|----------|---------|---------|-------------------
File                    | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
------------------------|---------|----------|---------|---------|-------------------
All files               |   94.44 |    74.19 |      88 |   96.55 |
 orbit-db-test-utils    |    97.2 |    82.61 |     100 |   98.02 |
  connect-peers.js      |     100 |      100 |     100 |     100 |
  get-ipfs-peer-id.js   |     100 |      100 |     100 |     100 |
  index.js              |     100 |      100 |     100 |     100 |
  mem-store.js          |      96 |       75 |     100 |      96 | 9
  start-ipfs.js         |      90 |      100 |     100 |      90 | 21
  stop-ipfs.js          |     100 |      100 |     100 |     100 |
  swarm.js              |   96.15 |     87.5 |     100 |     100 | 33
  wait-for-peers.js     |     100 |       50 |     100 |     100 | 11
 ...b-test-utils/config |    87.5 |       50 |     100 |     100 |
  factory.js            |     100 |      100 |     100 |     100 |
  index.js              |   83.33 |       50 |     100 |     100 | 4-5
  node.js               |     100 |      100 |     100 |     100 |
 ...ils/implementations |   72.73 |       50 |       0 |   77.78 |
  index.js              |   83.33 |       50 |     100 |     100 | 4-5
  node.js               |      60 |      100 |       0 |      60 | 51-55
------------------------|---------|----------|---------|---------|-------------------

.gitignore Show resolved Hide resolved
config/factory.js Show resolved Hide resolved
index.js Show resolved Hide resolved
mem-store.js Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@aphelionz aphelionz marked this pull request as ready for review April 19, 2020 15:32
@aphelionz aphelionz requested review from shamb0t and haadcode April 19, 2020 15:37
@oed
Copy link

oed commented Apr 19, 2020

You are a hero 🙌

@aphelionz
Copy link
Contributor Author

aphelionz commented Apr 22, 2020

Opted not to re-export Ctl but instead create a factory with the settings we'd want, and then extended that factory's prototype to include two functions:

  1. spawnApi to deal with the simple boilerplate of:
const ipfsd = factory.spawn()
const ipfs = ipfsd.api
  1. localCluster, which is where the real magic [can] happen, which takes an array of ipfs types like js, go, proc, etc, connects them all via permutation, and then returns the array of connected nodes.

I'm only testing with proc right now. I think this approach will be promising and I'll be playing with using it in ipfs-log tomorrow.

I also may put back startIpfs and stopIpfs just so I'm not needlessly breaking the previously written tests.

@aphelionz
Copy link
Contributor Author

startIpfs, stopIpfs, and testApis were restored to not unnecessarily break existing tests, and a deprecation warning was added.

config/factory.js Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
stop-ipfs.js Outdated Show resolved Hide resolved
test-apis.js Show resolved Hide resolved
package.json Show resolved Hide resolved
@aphelionz
Copy link
Contributor Author

Ran into an error after rm -rf node_modules package-lock.json and npm install:

     Error: invalid block
      at Object.put (node_modules/ipfs-repo/src/blockstore.js:76:15)
      at BlockService.put (node_modules/ipfs-block-service/src/index.js:63:32)
      at IPLDResolver.put (node_modules/ipld/src/index.js:196:21)
      at async Object.put (node_modules/ipfs/src/core/components/dag/put.js:48:1
9)
      at async Proxy.init (node_modules/ipfs/src/core/components/init.js:128:27)
      at async InProc.init (node_modules/ipfsd-ctl/src/ipfsd-in-proc.js:93:5)
      at async Factory.spawn (node_modules/ipfsd-ctl/src/factory.js:141:7)
      at async Promise.all (index 5)
      at async Factory.factory.constructor.localSwarm (index.js:5:692)
      at async Context.<anonymous> (test/workflow.spec.js:91:21)

Co-authored-by: tabcat <tabcat@users.noreply.github.com>
@aphelionz aphelionz force-pushed the refactor-update-deps branch from bdf3560 to 9324b42 Compare May 11, 2020 00:58
@aphelionz aphelionz merged commit 84aa574 into master May 11, 2020
@aphelionz aphelionz deleted the refactor-update-deps branch May 11, 2020 01:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants