feat: add bitswap.unwant javascript spec #254
Conversation
ghost
assigned 
|
I'm very confused by the testing of this entire suite. Should the existing and new tests from js-ipfs move here? Should they be duplicated now? |
|
@travisperson compiled some good notes on how it is all set up, it is good for understanding - https://github.com/ipfs/wg-qa-testing-dx/issues/35 As noted on the README https://github.com/ipfs/interface-ipfs-core#background
This translates to: if there is a spec, we need a test to make sure |
|
There is no bitswap spec in this repo yet, but it appears that everything is implemented in go already. Does this just mean the tests were never added here? If I were to add tests here how would I make sure the go-ipfs lib passes against them? |
Basically. This project came along quite a bit after the
There isn't a project that runs these interface tests against a golang daemon as far as I'm aware, but it should be fairly trivial to run them with I've created a gist that you can clone.
To develop From You should now be able to write a simple skeleton test inside of diff --git a/js/src/bitswap.js b/js/src/bitswap.js
new file mode 100644
index 0000000..ae62fea
--- /dev/null
+++ b/js/src/bitswap.js
@@ -0,0 +1,39 @@
+
+/* eslint-env mocha */
+/* eslint max-nested-callbacks: ["error", 8] */
+
+'use strict'
+
+const chai = require('chai')
+const dirtyChai = require('dirty-chai')
+const expect = chai.expect
+chai.use(dirtyChai)
+
+module.exports = (common) => {
+ describe('.bitswap', () => {
+ let ipfs
+
+ before(function (done) {
+ // CI takes longer to instantiate the daemon, so we need to increase the
+ // timeout for the before step
+ this.timeout(60 * 1000)
+
+ common.setup((err, factory) => {
+ expect(err).to.not.exist()
+
+ factory.spawnNode((err, node) => {
+ expect(err).to.not.exist()
+ ipfs = node
+
+ done()
+ })
+ })
+ })
+
+ after((done) => common.teardown(done))
+
+ it('A test', () => {
+ expect(true).to.be.true()
+ })
+ })
+}
diff --git a/js/src/index.js b/js/src/index.js
index 9f527e1..90463eb 100644
--- a/js/src/index.js
+++ b/js/src/index.js
@@ -16,3 +16,4 @@ exports.key = require('./key')
exports.stats = require('./stats')
exports.repo = require('./repo')
exports.bootstrap = require('./bootstrap')
+exports.bitswap = require('./bitswap')Then you just need to add it to the end of (or replace) test.block(common)
test.dht(common)
+test.bitswap(common)This should get you setup to write and run tests. ResourcesBitswap tests from the An example of running tests from this project Daemon Controller (spawns daemons, js and go) |
wraithgar
force-pushed
the
bitswap_unwant
branch
from
5da55ee to
7b83894
Compare
|
I've created a bitswap spec test based on the existing cli/http/interface tests in js-ipfs. Not all of them, there is a suite of tests in js-ipfs that deals with transferring blocks that I will tackle next. Haven't tested this in go yet, just the js-ipfs (using npm link). I think this is the right direction. Had to guess on how we would want to have tests for the "error when stopped" state that js-ipfs was doing. |
|
The javascript implementation of Should the javascript implementation be changed? |
Yes, I think it should be changed. If it's not I think we are breaking one of the main goals this project is trying to enforce
I think the interface has gotten away with being broken like this primary because with the CLI, the core module is started in offline mode, and CLI is the only use of js-ipfs (in the project) that runs the core module in offline mode. This may not be true for other use cases, such as IPFS Companion, which can use both |
|
Thank you so much for your help in this issue @travisperson. I'll get the js-ipfs stuff changed so it matches go (I don't think it will effect the surface area much because as long as I change the cli and http hooks to expect that behavior they don't have to change their output). If the js cli is the only thing that deals with offline mode I am wondering if the 'should error on offline' tests don't actually belong back in the js repo with specific cli tests? It doesn't feel part of spec at that point. |
|
I could not get this to work when switching to promisified calls. There was an error that popped up that it took me till today to finally track down. Ultimately it's good cause there's a bug in I typed up my journey here https://gist.github.com/wraithgar/c05cd1980a9f73d9b36504706f3980bf TLDR: |
|
Once we figure out what to do about the |
wraithgar
force-pushed
the
bitswap_unwant
branch
2 times, most recently
from
ff1cc7e to
bd226a4
Compare
wraithgar
force-pushed
the
bitswap_unwant
branch
3 times, most recently
from
b90c42e to
d196f84
Compare
|
Update on tests: It would appear that the tests in I think the path forward is this, for adding a feature to
|
|
This PR has been merged with master, and the tests have been tweaked to pass when run when linked back into the HOWEVER: tests to not pass in that repo. It would appear that there is either a bug in the go implementation of Steps to reproduce:
You will see that unwanting a key does not remove it from the wantlist. The tests in js-ipfs-api were only checking against an already empty wantlist. |
|
@achingbrain I wanted to ping you in this issue because it relates to what we were wanting to start discussing as per Monday's js-ipfs standup. The comment I linked to in the |
|
While trying to get these tests passing in js-ipfs I came across another thing that I am going to need some help with. Here is an example response for { Keys: [
{ "/": "QmUBdnXXPyoDFXj3Hj39dNJ5VkN3QFRskXxcGaYFBB8CNR" }
] }The go implementation of cid has a I don't see this represented anywhere in |
|
The IPLD spec says merkle-links should be an object with a For a directory containing a file Go: $ ipfs dag get QmdtgNzDnLKkhyziU3kXRtf4SfAeh1Lah4T5hwVdmXh6Br | underscore print
{
"data": "CAE=",
"links": [
{ "Name": "hello.txt", "Size": 64, "Cid": { "/": "QmcFUHo1DBpydnTSeuXszKgQqnXQCJq6Wu1BUdzEfuRahM" } }
]
}JS: $ jsipfs dag get QmU5PLEGqjetW4RAmXgHpEFL7nVCL3vFnEyrCKUfRk4MSq | underscore print
{
"data": "CAE=",
"links": [
{ "name": "hello.txt", "size": 14, "multihash": "QmZULkCELmmk5XNfCgTnCyFgAVxBRBXyDHGGMVoLFLiXEN" }
],
"size": 69
}Neither seem to follow the spec. The go implementation uses sentence case for the keys and uses |
|
@wraithgar on the tests, the I think there is possibly some unnecessary overlap due to AFAIK if you want to add a feature to js-ipfs you need to:
Hopefully I'm not missing anything there. |
|
@alanshaw The interface tests in the js-ipfs repo are (mostly?) used for all three purposes (core, http, cli). ETA: It looks like in js-ipfs the http-api tests mostly use the interface tests (with some local tests too, mostly matching the core tests. The cli tests don't have any interface tests. |
Yes we can use the
It would be good to identify and remove local tests that are duplicates.
CLI tests are all here: https://github.com/ipfs/js-ipfs/tree/master/test/cli The CLI does not expose an |
|
Hope I'm helping here and not just re-iterating what you already know! |
|
@alanshaw you're helping. There is a lot of undocumented and assumed behavior in the testing suite. We've already found here that the go and js implementations aren't systematically compared against each other and in some places diverge (sometimes neither match the spec). I'm trying to shake out these assumptions, and asking these questions is the way to do this. With that, my next question is: what are we assuming the cli test are? Are they duplicates of the interface tests or are they simply testing the cli surface area (i.e. that the commands exist and can accept all the parameters we expect). |
|
I found this diagram super helpful when I started getting my head around this: https://github.com/ipfs/js-ipfs#code-architecture-and-folder-structure With the CLI tests, yes they should be similar to the Additionally (and somewhat less importantly) the CLI will often have shorthand names for flags that should also be tested! |
|
Here's what I think we've determined
If this is accurate, it gives me a clearer picture both of the current state of things, and where they should be moving. It also helps us in coming up with a path for adding or fixing things in js-ipfs that ensures that it's properly tested, and that the implementation here matches that of go. |
wraithgar
force-pushed
the
bitswap_unwant
branch
3 times, most recently
from
3f714d4 to
5d04e53
Compare
|
The tests here now pass in both js-ipfs-api and ipfs/js-ipfs#1306 Because the other two PRs I have that deal w/ adding bitswap implementations are so tightly coupled to this effort of sorting the testing out I will be rebasing them against this PR and we can likely deal with all three at once (this PR represents the bulk of the efforts, the rest is just adding the minimal code and tests for the other two features being added). |
js/src/bitswap.js
Outdated
| expect(err).to.not.exist() | ||
| withGo = id.agentVersion.startsWith('go-ipfs') | ||
| setTimeout(done, 250) //Give block.get time to take effect | ||
| }) |
There was a problem hiding this comment.
We now have a spawnNodeWithId to help reduce this code duplication around the project.
https://github.com/ipfs/interface-ipfs-core/blob/master/js/src/utils/spawn.js#L5,L14
nit: I'm not a huge fan of the timeout here. Does it really take a lot of time for the key to enter the want list?
There was a problem hiding this comment.
No, the timeout appeared to be cruft leftover from debugging other errors in the wantlist return that had to do with the disparity between go and the js implementations. It works fine without for me in both js-ipfs and js-ipfs-api.
Switched to spawnWithNodeId too.
|
Closing as this was superseded by #267 (now merged) |
For ipfs/js-ipfs#1306