fix: Add tests for .block promise API #170
Conversation
ghost
assigned 
|
Needed for ipfs/js-ipfs#1085 |
src/block.js
Outdated
| // Remove bitswap from IPFS so that block.get tries to fetch the block | ||
| // from the local repo (not from the network). This is to make sure we | ||
| // get the error (as expected) instead of waiting for a timeout. | ||
| ipfs._blockService._bitswap = null |
There was a problem hiding this comment.
The tests can't use any internals otherwise it will fail with js-ipfs-api
There was a problem hiding this comment.
Oh :/ I figured that was a a bit of a workaround. Any suggestions how to avoid the block.get() trying to get the block from bitswap thus causing it "never" to return? The idea here is to get block.get() to return an error (ie. "we don't have the block anymore").
There was a problem hiding this comment.
You will just have to .skip that test until we have https://github.com/ipfs/interface-ipfs-core/issues/58, unfortunately.
There was a problem hiding this comment.
Ok. That's unfortunate. I'll update the commit and .skip it for now. Let's leave the tests there though, so that we have them ready for the future.
There was a problem hiding this comment.
Committed. I removed the line where bitswap was nulled, added .skip to both callback and promise tests for .rm() and added some notes in the code as to why.
|
@diasdavid I believe these are running fine with both now after skipping the .rm() part. Did I miss something? Is there anything we need to change to get this merged? |
|
Hi @haadcode, this PR needs a rebase :) |
|
ping @haadcode |
Add test for callback-based .rm() Add test for all promise-based functions
|
Sorry this took a while, it's rebased now! 🚀 |
| it('a buffer, using defaults', (done) => { | ||
| const expectedHash = 'QmPv52ekjS75L4JmHpXVeuJ5uX2ecSfSZo88NSyxwA3rAQ' | ||
| const blob = new Buffer('blorb') | ||
| describe('callback API', () => { |
There was a problem hiding this comment.
@haadcode I believe you missed that there is no more separation between Callback API and Promise API and through your rebase you brought it back and duplicated some come.
There was a problem hiding this comment.
I did not notice that. Will fix it.
There was a problem hiding this comment.
@diasdavid I started to look into this but I'm little confused. Can you help here to elaborate what you mean with "no more separation between Callback and Promise API"? We still have them both, right?
There was a problem hiding this comment.
We still have both, but we don't duplicate the tests anymore :)
|
Closing this one. |
This PR will add tests for the promise API of .block
I also added a test for callback block.rm() as it was missing