-
Notifications
You must be signed in to change notification settings - Fork 124
testing(): add object.get tests for multihash string #139
Conversation
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.
Thanks @kenshyx ! Just a couple of comments and we should be golden. Notice that it is failing on linting.
ipfs.object.put(testObj).then((node1) => { | ||
ipfs.object.get(node1.multihash).then((node2) => { | ||
return ipfs.object.put(testObj).then((node1) => { | ||
return ipfs.object.get(node1.multihash).then((node2) => { |
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.
@kenshyx all of these tests are written to test the callback API. Check at the bottom the section to test de promises API.
We always test in depth through the Callback API.
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 don't understand, those lines are under promise API
:)
src/object.js
Outdated
// get object from ipfs multihash string | ||
return ipfs.object.get(node1.toJSON().multihash).then((node2) => { | ||
expect(node2).to.exist() | ||
}); |
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.
Make this a separate test.
Oh, I just realized I didn't read correctly where the changes were being added and in fact, you were doing the right thing! It seems that all of the other Promise API tests are finishing through calling done instead of returning a promise, which is bad because errors are not caught. I'm sorry for the confusion @kenshyx, would you like to fix all the Promise API tests? If not I can do it as my redemption :) |
Yes, I'll do it. |
Let me know if there is something else I have to update @diasdavid |
These tests will pass after ipfs/js-ipfs#849 is merged