Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

fix(bootstrap:add): prevent duplicate inserts #893

Merged
merged 4 commits into from
Jul 3, 2017
+25 −2
Merged

fix(bootstrap:add): prevent duplicate inserts #893

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
42741c8
fix(bootstrap:add): prevent duplicate inserts
kenshyx Jun 30, 2017
3ab1ace
chore():
kenshyx Jun 30, 2017
e995910
chore(): remove ununsed var
kenshyx Jun 30, 2017
b2bedf0
chore(): add test for peer list length
kenshyx Jun 30, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/core/components/bootstrap.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ module.exports = function bootstrap (self) {
}
if (args.default) {
config.Bootstrap = defaultNodes
} else if (multiaddr) {
} else if (multiaddr && config.Bootstrap.indexOf(multiaddr) === -1) {
config.Bootstrap.push(multiaddr)
}
self._repo.config.set(config, (err) => {
Expand Down
1 change: 0 additions & 1 deletion src/core/components/stop.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
'use strict'

const series = require('async/series')
const setImmediate = require('async/setImmediate')

module.exports = (self) => {
return (callback) => {
Expand Down
21 changes: 21 additions & 0 deletions test/http-api/over-ipfs-api/bootstrap.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,27 @@ module.exports = (ctl) => {
})
})

it('prevents duplicate inserts of bootstrap peers', () => {
return ctl
.bootstrap
.add(validIp4)
.then(res => {
expect(res).to.be.eql({ Peers: [validIp4] })
return ctl.bootstrap.add(validIp4)
})
.then((res) => {
expect(res).to.be.eql({ Peers: [validIp4] })
return ctl.bootstrap.list()
})
.then((res) => {
expect(res).to.exist()
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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a expectation about that the bootstrap list should now only have one element in it as well.

Copy link
Contributor Author

@kenshyx kenshyx Jun 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@victorb victorb Jun 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That way, this test would also be protected against future changes (additions/removal of peers) to the default bootstrap list.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, how do I set the bootstrap list to be empty without calling rm for the default bootstrap list?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not getting your question @victorbjelkholm, these test nodes have their bootstrap lists empty by default:

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that was what I thought... @kenshyx then you can remove the .rm call and just add the two addresses.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kenshyx is doing rm because the previous tests add up to the bootstrap peers pool

})
})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


it('returns a list of bootstrap peers when called with the default option', (done) => {
ctl.bootstrap.add({ default: true }, (err, res) => {
expect(err).to.not.exist()
Expand Down