-
Notifications
You must be signed in to change notification settings - Fork 124
fix: add test for block rm #367
Conversation
it('should error on removing non-existent block', (done) => { | ||
const cid = new CID('QmYi5NFboBxXvdoRDSa7LaLcQvCukULCaDbZKXUXz4umPa') | ||
ipfs.block.rm(cid, (err, block) => { | ||
expect(err).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.
We should make sure the error message contains whatever we expect it to have inside of it.
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.
+1
const cid = new CID(hash) | ||
ipfs.block.rm(cid, (err) => { | ||
expect(err).to.not.exist() | ||
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.
We need to make sure the block was removed, but I'm not sure if we have any public API for checking if a block already exists locally?
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.
Can we use https://docs.ipfs.io/reference/api/http/#api-v0-refs-local for this check?
const cid = new CID(hash) | ||
ipfs.block.rm(cid, (err) => { | ||
expect(err).to.not.exist() | ||
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.
Can we use https://docs.ipfs.io/reference/api/http/#api-v0-refs-local for this check?
it('should error on removing non-existent block', (done) => { | ||
const cid = new CID('QmYi5NFboBxXvdoRDSa7LaLcQvCukULCaDbZKXUXz4umPa') | ||
ipfs.block.rm(cid, (err, block) => { | ||
expect(err).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.
+1
@vmx any chance you can fix these up? |
@alanshaw The problem is that this change has a looooong tail. IIRC things are not implemented as expected/correctly. But I'll have a another look at the test, it's worth adding it even if we need to skip it atm. |
I'm happy to add skips where appropriate as long as the tests are passing in at least one of js-ipfs or js-ipfs-api 🚀 |
block.rm was deleted from the API as it messes with Pinning for go-ipfs. Read ipfs-inactive/js-ipfs-http-client#792 |
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.
Needs #367 (comment) to be solved
Slightly more comprehensive tests were added in #487 and the implementation will ship with |
Sadly those tests can't be enabled yet as js-ipfs-api doesn't implement
block/rm
yet (see ipfs-inactive/js-ipfs-http-client#792).But I thought I make a pull request with the test though, so that work doesn't get wasted.