fix(bootstrap:add): prevent duplicate inserts #893
Conversation
| const insertPosition = res.Peers.indexOf(validIp4) | ||
| expect(insertPosition).to.not.equal(-1) | ||
| const duplicatePosition = res.Peers.indexOf(validIp4, insertPosition + 1) | ||
| expect(duplicatePosition).to.equal(-1) |
There was a problem hiding this comment.
Add a expectation about that the bootstrap list should now only have one element in it as well.
There was a problem hiding this comment.
Line 46 verifies if the newly added peer was inserted. The bootstrap list contains the default peer list from https://github.com/ipfs/js-ipfs/tree/master/src/init-files + the ones added manually so it won't have only one element.
There was a problem hiding this comment.
As far as I can tell, you're asserting that validIp4 is not missing from the list of peers, but never asserting that the amount of peers are correct in the test-case. Ideal, minimal test-case would start with a empty bootstrap list, add the same address twice and assert that the list only contain once element.
There was a problem hiding this comment.
That way, this test would also be protected against future changes (additions/removal of peers) to the default bootstrap list.
There was a problem hiding this comment.
Ok, how do I set the bootstrap list to be empty without calling rm for the default bootstrap list?
There was a problem hiding this comment.
I'm not getting your question @victorbjelkholm, these test nodes have their bootstrap lists empty by default:
There was a problem hiding this comment.
Ah, that was what I thought... @kenshyx then you can remove the .rm call and just add the two addresses.
There was a problem hiding this comment.
@kenshyx is doing rm because the previous tests add up to the bootstrap peers pool
| expect(insertPosition).to.not.equal(-1) | ||
| expect(res.Peers.length).to.equal(1) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Please write tests for core too. The ideal would be getting interface-ipfs-core tests for bootstrap - https://github.com/ipfs/interface-ipfs-core/tree/master/src so that we match expectations on js-ipfs-api and js-ipfs.
|
I'll go ahead and merge this. Let's not forget about the standardization of API and tests though - ipfs-inactive/interface-js-ipfs-core#97 |
License: MIT Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
Closes #892