-
Notifications
You must be signed in to change notification settings - Fork 124
Conversation
it('add a nested dir as array with progress', (done) => { | ||
// Needs https://github.com/ipfs/js-ipfs-api/issues/339 to be fixed | ||
// for js-ipfs-api + go-ipfs | ||
if (!isNode) { return done() } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, why only non-node environment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, good question - this is basically a copy of the above tests which carries this flag.
Edit: I wonder if its valid for that test as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@victorbjelkholm check the issue linked in the comments. go-ipfs started error'ing in v0.4.2
@dryajov mind trying running the dir tests (by removing the if clause) against latest go-ipfs and see if this is still an issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 @dryajov did you get to try this one too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did, it seems to still be an issue with the latest master.
src/files.js
Outdated
expect(file.hash).to.equal(expectedMultihash) | ||
expect(file.path).to.equal(file.hash) | ||
expect(progCount).to.equal(58) | ||
expect(progress).to.equal(57792) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the actual file size? go-ipfs tends to "add more than needed" in the progress bar, often it shows 10N%
added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran all the tests and all have passed, I think this is the actual size. I'll double check tho.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should test for it. .to.equal(sizeOfTheFile)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 @dryajov this is now one of the last things to do in order to get the whole thing merged, mind checking for the right size?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to switch attention to - ipfs/js-ipfs#1036, since that would be a major missing part of the whole effort. I'll resume this as soon as I get that one squared out.
src/files.js
Outdated
expect(root.path).to.equal('test-folder') | ||
expect(root.hash).to.equal(expectedRootMultihash) | ||
expect(progCount).to.equal(8) | ||
expect(progress).to.equal(5) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has to be wrong. How can a nested dir full of files have a progress
of 5 bytes only?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is indeed wrong, a fix is coming.
This also needs for the docs to be updated, ref: ipfs-inactive/js-ipfs-http-client#604 (comment) |
This is currently being held by - ipfs/js-ipfs#1036. Through we can probably merge this on its own and it would cover js-ipfs-api interaction with the go-ipfs progress functionality, it is missing from the js-ipfs http api which the above issue is addressing. |
docs have been updated. |
No description provided.