feat: modularise tests by command, add tools to skip and only #290

alanshaw · 2018-06-02T13:37:47Z

This PR modularises tests by command and provides tools for consumers of this library to use skip/only on a command basis.

Encourages more test isolation. Many tests were expecting values from previous tests to be available, this has largely been eradicated
Allows us to test partially implemented sub-systems e.g. currently repo.version and repo.stat are implemented but repo.gc is not. This allows us to import tests for repo.version and repo.stat without failing the suite but also import and skip tests for repo.gc for visibility until the implementation is complete
Removes the need for implementation specific checks to skip tests i.e. withGo goes away because we can just run the tests we want to run on any given implementation
Removes the need for platform specific checks to skip tests i.e. isNode goes away because we can just run the tests we want to run on any given platform
Consistently expresses the test titles more descriptively as "it should..." or "it should not..." to allow for easier understanding of test purpose
Consistently labels promised versions of tests with the same name as the non promised version but with the suffix "(promised)". Also moves these tests so they are adjacent in the suite file. This allows the developer to more easily see if the issue is because of the callback/promise interface for a command or a bug within the code for the command

~~This work is backwards compatible with the previous API so no changes to dependant code needs to be made.~~ I wanted this to be backwards compatible, but quickly ran into an issue where the common object we pass to suites expects to only be used once. I started to pass it to multiple suites and the teardown function was trying to tear down nodes that were already stopped! So, unfortunately breaking change: common should now be a function that creates a common object for use with the tests. I'll submit PR's to js-ipfs (ipfs/js-ipfs#1389) and js-ipfs-api (ipfs-inactive/js-ipfs-http-client#785) to deal with this.

running tests by command

const tests = require('interface-ipfs-core')

tests.repo.version(common)

N.B. you can still run a whole sub-system suite like this:

const tests = require('interface-ipfs-core')
tests.repo(common)

skipping tests for a specific command

const tests = require('interface-ipfs-core')

tests.repo.version(common)
tests.repo.stat(common)
tests.repo.gc(common, { skip: true }) // pass an options object to skip these tests

// OR, at the subsystem level

tests.repo(common, { skip: ['gc'] })

skipping specific tests

const tests = require('interface-ipfs-core')

tests.repo.version(common)
tests.repo.stat(common)
tests.repo.gc(common, { skip: ['should do a thing'] }) // named test(s) to skip

// OR, at the subsystem level

tests.repo(common, { skip: ['should do a thing'] })

only

The idea here is that only can be used during development/bug fixing to run only problematic tests without having to npm link and find the test(s) to add .only to.

running only tests for a specific command

const tests = require('interface-ipfs-core')

tests.repo.version(common)
tests.repo.stat(common)
tests.repo.gc(common, { only: true }) // pass an options object to run only these tests

// OR, at the subsystem level

tests.repo(common, { only: ['gc'] })

running only specific tests

const tests = require('interface-ipfs-core')

tests.repo.version(common)
tests.repo.stat(common)
tests.repo.gc(common, { only: ['should do a thing'] }) // only run these named test(s)

// OR, at the subsystem level

tests.repo(common, { only: ['should do a thing'] })

travisperson · 2018-06-04T18:13:23Z

This hits a lot of points that Victor and I wanted to try and look into this quarter. I really like the way this is laid out and the removal of the withGo and other opt out variables.

I would love to help take this through to competition.

BREAKING CHANGE: Consumers of this test suite now have fine grained control over what tests are run. Tests can now be skipped and "onlyed" (run only specific tests). This can be done on a test, command and sub-system level. See the updated usage guide for instructions: https://github.com/ipfs/interface-ipfs-core/blob/master/README.md#usage. This means that tests skips depending on implementation (e.g. go/js), environment (e.g. node/browser) or platform (e.g. macOS/linux/windows) that were previously present in this suite have been removed. Consumers of this library should add their own skips based on the implementation that's being tested and the environment/platform that the tests are running on. The following other breaking changes have been made: 1. The common object passed to test suites has changed. It must now be a function that returns a common object (same shape and functions as before). 2. The `ipfs.ls` tests (not MFS `ipfs.files.ls`) is now a root level suite. You'll need to import it and use like `tests.ls(createCommon)` to have those tests run. 3. The `generic` suite (an alias to `miscellaneous`) has been removed. See #290 for more details. License: MIT Signed-off-by: Alan Shaw <alan@tableflip.io>

alanshaw · 2018-06-08T14:57:10Z

I appreciate this is a massive PR to review, but a lot of it is mechanical changes. Please read the PR description above first (a lot of this is echoed in the changes to the README). Just to highlight how things have moved around:

Each sub-system (dag, files, swarm etc.) is now a folder with an index.js file. It defines all tests available in that subsystem - so for files, the commands are add, cat, get etc..

All the tests for files.add are grouped in src/files/add.js, so src/files/index.js creates an object that includes all the tests for each command and calls a function called createSuite.

createSuite turns that object into a function that runs all of the tests (it looks like function (createCommon, options) {}) and also adds each command as a property. What's fun is that the function that createSuite returns (what you get back from src/files/index.js) looks just like the function you get from src/files/add.js so you can use them interchangeably to run ALL tests for files (by calling tests.files()) or just tests for add (by calling tests.files.add()).

BREAKING CHANGE: Consumers of this test suite now have fine grained control over what tests are run. Tests can now be skipped and "onlyed" (run only specific tests). This can be done on a test, command and sub-system level. See the updated usage guide for instructions.

This means that tests skips depending on implementation (e.g. go/js), environment (e.g. node/browser) or platform (e.g. macOS/linux/windows) that were previously present in this suite have been removed. Consumers of this library should add their own skips based on the implementation that's being tested and the environment/platform that the tests are running on.

The following other breaking changes have been made:

The common object passed to test suites has changed. It must now be a function that returns a common object (same shape and functions as before).
The ipfs.ls tests (not MFS ipfs.files.ls) is now a root level suite. You'll need to import it and use like tests.ls(createCommon) to have those tests run.
The generic suite (an alias to miscellaneous) has been removed.

travisperson

Took a first long pass. I'll double back and take another look at a bit to refresh my eyes.

travisperson · 2018-06-12T14:54:44Z

js/src/dag/put.js

+    })
+
+    // This works because dag-cbor will just treat pbNode as a regular object
+    it.skip('should not put dag-pb node with wrong multicodec', (done) => {


Should this skip be removed?

Possibly, but it is currently skipped:

https://github.com/ipfs/interface-ipfs-core/blob/510516aaf5b310a15f413622352821f1e71bad0e/js/src/dag.js#L74-L80

travisperson · 2018-06-12T15:03:35Z

js/src/dht/provide.js

+        ipfs.dht.provide(new CID(res[0].hash), (err) => {
+          expect(err).to.not.exist()
+          done()
+        })


I don't believe many of these tests will work anymore as there will never be any peers on the dht to provide too, and there isn't much of a reason to provide to self.

The dht for go-ipfs I know will error if no peers are connected when running provides [0]. Should this be valid, and an issue opened for go-ipfs?

[0] https://github.com/ipfs/go-ipfs/blob/08fb11fa896704d9e081bf64e66fac1f6d9d03dc/core/commands/dht.go#L280

I’ve run these tests against js-ipfs and js-ipfs-api (talks to go-ipfs) and they all pass, so the tests that could fail might already be skipped...I’ll check and get back to you

DHT tests are completely commented out in js-ipfs currently 😟 https://github.com/ipfs/js-ipfs/blob/master/test/core/interface/dht.js

Some tests in js-ipfs-api were already skipped, but not any provide tests. I will investigate further...

These tests are passing for me for js-ipfs-api:

Is it because they are providing to the bootstrap nodes?

Historically there were only a couple of DHT tests. The suite was increased with #288 which led me to found some issues with the http-api.

Weirdly, these tests just started failing for me. I'll fix.

travisperson · 2018-06-12T15:10:44Z

js/src/swarm/peers.js

+  const it = getIt(options)
+  const common = createCommon()
+
+  describe('.swarm', function () {


I believe this is .swarm.peers right?

travisperson · 2018-06-12T15:11:20Z

js/src/swarm/local-addrs.js

+          expect(err).to.not.exist()
+          ipfs = nodes[0]
+          done()
+        })


Have an extra node

travisperson · 2018-06-12T15:15:56Z

js/src/swarm/addrs.js

+        spawnNodes(2, factory, (err, nodes) => {
+          expect(err).to.not.exist()
+          ipfs = nodes[0]
+          done()


Have an extra node?

alanshaw · 2018-06-13T12:28:27Z

Sorry for the late adding of reviewers, I realised that this is a change that'll affect you all so I'd like you to all see it and know what is going on 🚀

jacobheun

+1 to this, it's a great step towards better tests.

daviddias · 2018-06-19T03:53:45Z

js/src/block/get.js

+const dirtyChai = require('dirty-chai')
+const multihash = require('multihashes')
+const CID = require('cids')
+const Buffer = require('safe-buffer').Buffer


No need for safe-buffer anymore.

daviddias · 2018-06-19T03:55:03Z

js/src/bootstrap/rm.js

+const { getDescribe, getIt } = require('../utils/mocha')
+
+const expect = chai.expect
+chai.use(dirtyChai)


If we are going to get describe and it through a local module, better to also pick expect from it, saving 4 lines from each module (and avoiding forgetting about it in the future).

daviddias · 2018-06-19T03:55:21Z

js/src/bootstrap/rm.js

+const expect = chai.expect
+chai.use(dirtyChai)
+
+const invalidArg = 'this/Is/So/Invalid/'


Move it inside the module.exports

daviddias · 2018-06-19T03:55:56Z

js/src/bootstrap/rm.js

+module.exports = (createCommon, options) => {
+  const describe = getDescribe(options)
+  const it = getIt(options)
+  const common = createCommon()


Why does common need to created here, if there are no options for it?

We need to create a common object for each command now that they have been split into modules since it tracks nodes that are created so that they can be destroyed when teardown is called.

// Before: tests.files(common) // -> calls: tests.files.add(common) // teardown destroys nodes created for files.add tests.files.get(common) // teardown destroys nodes created for files.add AND files.get - ruh roh! // etc. // Now: tests.files(createCommon) // -> calls: tests.files.add(createCommon) // teardown destroys nodes created for files.add tests.files.get(createCommon) // teardown destroys nodes created for files.get

The alternative would be to make the common object smarter - to remove nodes from it's nodes array when teardown is called. However I was thinking that if we were to ever run these suites concurrently (please! one day!) then things would break.

From the PR description:

~~This work is backwards compatible with the previous API so no changes to dependent code needs to be made.~~ I wanted this to be backwards compatible, but quickly ran into an issue where the common object we pass to suites expects to only be used once. I started to pass it to multiple suites and the teardown function was trying to tear down nodes that were already stopped! So, unfortunately breaking change: common should now be a function that creates a common object for use with the tests. I'll submit PR's to js-ipfs (ipfs/js-ipfs#1389) and js-ipfs-api (ipfs-inactive/js-ipfs-http-client#785) to deal with this.

daviddias · 2018-06-19T03:56:59Z

js/src/dag/put.js

+      }, done)
+    })
+
+    it('should not put dag-cbor node with wrong multicodec', function (done) {


No need to use function if no special timeout needs to be added.

daviddias · 2018-06-19T03:57:46Z

js/src/dag/tree.js

+
+const chai = require('chai')
+const dirtyChai = require('dirty-chai')
+const { series, eachSeries } = require('async')


We use require('async/series') as a good practice to avoid requiring the whole module into our codebase.

daviddias · 2018-06-19T03:58:12Z

js/src/dht/findpeer.js

+      })
+    })
+
+    it('should fail to find other peer if peer does not exist', function (done) {


If no special timeout, please use a arrow func

daviddias · 2018-06-19T03:58:36Z

js/src/dht/findpeer.js

+    it('should fail to find other peer if peer does not exist', function (done) {
+      nodeA.dht.findpeer('Qmd7qZS4T7xXtsNFdRoK1trfMs5zU94EpokQ9WFtxdPxsZ', (err, peer) => {
+        expect(err).to.not.exist()
+        expect(peer).to.be.equal(null)


nitpick s/to.be.equal(null)/to.not.exist()

daviddias · 2018-06-19T03:58:59Z

js/src/dht/findprovs.js

+  const common = createCommon()
+
+  describe('.dht.findprovs', function () {
+    this.timeout(80 * 1000)


Let's remove the global timeout in favor of by test timeout.

vasco-santos · 2018-06-19T09:43:54Z

It seems a really better approach! Thanks @alanshaw

travisperson · 2018-06-25T18:33:04Z

js/src/pubsub/peers.js

+      ], done)
+    })
+
+    describe('.peers', () => {


Did you want to drop this describe as well?

yes! thanks :D

License: MIT Signed-off-by: Alan Shaw <alan@tableflip.io>

BREAKING CHANGE: Consumers of this test suite now have fine grained control over what tests are run. Tests can now be skipped and "onlyed" (run only specific tests). This can be done on a test, command and sub-system level. See the updated usage guide for instructions: https://github.com/ipfs/interface-ipfs-core/blob/master/README.md#usage. This means that tests skips depending on implementation (e.g. go/js), environment (e.g. node/browser) or platform (e.g. macOS/linux/windows) that were previously present in this suite have been removed. Consumers of this library should add their own skips based on the implementation that's being tested and the environment/platform that the tests are running on. The following other breaking changes have been made: 1. The common object passed to test suites has changed. It must now be a function that returns a common object (same shape and functions as before). 2. The `ipfs.ls` tests (not MFS `ipfs.files.ls`) is now a root level suite. You'll need to import it and use like `tests.ls(createCommon)` to have those tests run. 3. The `generic` suite (an alias to `miscellaneous`) has been removed. See #290 for more details. License: MIT Signed-off-by: Alan Shaw <alan@tableflip.io>

License: MIT Signed-off-by: Alan Shaw <alan@tableflip.io>

Added originally in #308 License: MIT Signed-off-by: Alan Shaw <alan@tableflip.io>

Originally from PR #267 License: MIT Signed-off-by: Alan Shaw <alan@tableflip.io>

As per original PR #311 License: MIT Signed-off-by: Alan Shaw <alan@tableflip.io>

License: MIT Signed-off-by: Alan Shaw <alan@tableflip.io>

alanshaw · 2018-06-26T22:43:07Z

I think I've addressed all the feedback now, if there's no further comments then I'll merge this in tomorrow. Thank you and much ❤️ for your time and thoughts.

daviddias

Put it on a boat 🚢 \o/

travisperson

It looks like there is a lingering file js/src/bitswap.js that should be removed.

License: MIT Signed-off-by: Alan Shaw <alan@tableflip.io>

alanshaw requested review from travisperson and victorb as code owners June 2, 2018 13:37

ghost assigned alanshaw Jun 2, 2018

ghost added the in progress label Jun 2, 2018

alanshaw force-pushed the feat/modular-tests-skip-only branch from c0afce8 to c351f5e Compare June 4, 2018 14:57

alanshaw mentioned this pull request Jun 6, 2018

files.add*Stream coverage is poor #292

Closed

travisperson mentioned this pull request Jun 7, 2018

Progressive Testing ipfs-inactive/dev-team-enablement#46

Open

alanshaw requested a review from daviddias June 8, 2018 14:57

alanshaw changed the title ~~[WIP] feat: modularise tests by command, add tools to skip and only~~ feat: modularise tests by command, add tools to skip and only Jun 8, 2018

This was referenced Jun 8, 2018

feat: modular interface tests ipfs/js-ipfs#1389

Merged

feat: modular interface tests ipfs-inactive/js-ipfs-http-client#785

Merged

alanshaw requested a review from olizilla June 8, 2018 15:01

travisperson suggested changes Jun 12, 2018

View reviewed changes

alanshaw requested review from jacobheun, achingbrain and vasco-santos June 13, 2018 12:24

alanshaw requested review from dryajov and vmx June 13, 2018 19:33

jacobheun reviewed Jun 18, 2018

View reviewed changes

daviddias suggested changes Jun 19, 2018

View reviewed changes

travisperson approved these changes Jun 21, 2018

View reviewed changes

travisperson reviewed Jun 25, 2018

View reviewed changes

travisperson mentioned this pull request Jun 25, 2018

util: waitUntilConnected #310

Closed

alanshaw added 2 commits June 26, 2018 23:37

feat: modularise block, add mocha and test suite utils

8ebf438

License: MIT Signed-off-by: Alan Shaw <alan@tableflip.io>

fix: isolated block tests

1cc38ef

License: MIT Signed-off-by: Alan Shaw <alan@tableflip.io>

alanshaw added 19 commits June 26, 2018 23:38

feat(pin): modularises pin

96cf49a

License: MIT Signed-off-by: Alan Shaw <alan@tableflip.io>

chore: cleanup requires

c60ceb2

License: MIT Signed-off-by: Alan Shaw <alan@tableflip.io>

feat: modularises pubsub

bb72445

License: MIT Signed-off-by: Alan Shaw <alan@tableflip.io>

chore: fix warning for new Buffer

b12fb09

License: MIT Signed-off-by: Alan Shaw <alan@tableflip.io>

fix: import small file fixture

b969416

License: MIT Signed-off-by: Alan Shaw <alan@tableflip.io>

fix: swarm peers suite name

a8492f2

License: MIT Signed-off-by: Alan Shaw <alan@tableflip.io>

fix: spawn only as many nodes as needed

3eda55d

License: MIT Signed-off-by: Alan Shaw <alan@tableflip.io>

chore: remove safe-buffer

1615831

License: MIT Signed-off-by: Alan Shaw <alan@tableflip.io>

chore: move expect into mocha util

5567ddf

License: MIT Signed-off-by: Alan Shaw <alan@tableflip.io>

chore: move invalidArg var into exported function

0f61c26

License: MIT Signed-off-by: Alan Shaw <alan@tableflip.io>

chore: prefer arrow functions

eb9e785

License: MIT Signed-off-by: Alan Shaw <alan@tableflip.io>

test: add test for retreive empty block

965bdcb

Added originally in #308 License: MIT Signed-off-by: Alan Shaw <alan@tableflip.io>

chore: re-add bitswap tests

2c75978

Originally from PR #267 License: MIT Signed-off-by: Alan Shaw <alan@tableflip.io>

chore: do not rely on discovery for ping tests

57418b1

As per original PR #311 License: MIT Signed-off-by: Alan Shaw <alan@tableflip.io>

feat: allow skip reasons to be passed

22d9b00

License: MIT Signed-off-by: Alan Shaw <alan@tableflip.io>

fix: pin.rm suite name

a3d3f5c

License: MIT Signed-off-by: Alan Shaw <alan@tableflip.io>

chore: remove unnecessary describe block

4f9b635

License: MIT Signed-off-by: Alan Shaw <alan@tableflip.io>

fix: ensure there is another node to provide to

7a9c4d0

License: MIT Signed-off-by: Alan Shaw <alan@tableflip.io>

alanshaw force-pushed the feat/modular-tests-skip-only branch from 0d50c67 to 7a9c4d0 Compare June 26, 2018 22:39

daviddias approved these changes Jun 27, 2018

View reviewed changes

travisperson suggested changes Jun 27, 2018

View reviewed changes

fix: remove old unused bitswap tests

616a3e0

License: MIT Signed-off-by: Alan Shaw <alan@tableflip.io>

alanshaw merged commit e232d8c into master Jun 27, 2018

ghost removed the in progress label Jun 27, 2018

alanshaw deleted the feat/modular-tests-skip-only branch June 27, 2018 21:15

achingbrain mentioned this pull request Jun 29, 2018

test: backport of ipfs/interface-ipfs-core#314 #315

Merged

alanshaw mentioned this pull request Jul 2, 2018

fix: revert to serialized pubsub operations #319

Merged

travisperson mentioned this pull request Jul 2, 2018

OKR: Reduce complexity (interface-ipfs-core) ipfs-inactive/dev-team-enablement#53

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: modularise tests by command, add tools to skip and only #290

feat: modularise tests by command, add tools to skip and only #290

alanshaw commented Jun 2, 2018 •

edited

travisperson commented Jun 4, 2018

alanshaw commented Jun 8, 2018 •

edited

travisperson left a comment

travisperson Jun 12, 2018

alanshaw Jun 13, 2018

travisperson Jun 12, 2018

alanshaw Jun 12, 2018

alanshaw Jun 13, 2018

alanshaw Jun 25, 2018

daviddias Jun 25, 2018

alanshaw Jun 26, 2018

travisperson Jun 12, 2018

travisperson Jun 12, 2018

travisperson Jun 12, 2018

alanshaw commented Jun 13, 2018

jacobheun left a comment

daviddias Jun 19, 2018

daviddias Jun 19, 2018

daviddias Jun 19, 2018

daviddias Jun 19, 2018

alanshaw Jun 19, 2018

daviddias Jun 19, 2018

daviddias Jun 19, 2018

daviddias Jun 19, 2018

daviddias Jun 19, 2018

daviddias Jun 19, 2018

vasco-santos commented Jun 19, 2018

travisperson Jun 25, 2018

alanshaw Jun 26, 2018

alanshaw commented Jun 26, 2018

daviddias left a comment

travisperson left a comment

feat: modularise tests by command, add tools to skip and only #290

feat: modularise tests by command, add tools to skip and only #290

Conversation

alanshaw commented Jun 2, 2018 • edited

travisperson commented Jun 4, 2018

alanshaw commented Jun 8, 2018 • edited

travisperson 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

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

alanshaw commented Jun 13, 2018

jacobheun 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vasco-santos commented Jun 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanshaw commented Jun 26, 2018

daviddias left a comment

Choose a reason for hiding this comment

travisperson left a comment

Choose a reason for hiding this comment

alanshaw commented Jun 2, 2018 •

edited

alanshaw commented Jun 8, 2018 •

edited