Skip to content
This repository has been archived by the owner on Mar 10, 2020. It is now read-only.

test: ipns over pubsub #361

Merged
merged 4 commits into from
Oct 18, 2018
Merged

test: ipns over pubsub #361

merged 4 commits into from
Oct 18, 2018

Conversation

vasco-santos
Copy link
Contributor

PR implemented taking into consideration the most recent documentation of the go-ipfs API on:

http://ipfs.io/ipfs/QmPKzWaeHumLuiKLb2Ak67s5kxZ1p6YbnjVKYYfF9hTjzn

This tests aim to be integrated on s-ipfs-api#846 and in a future PR to js-ipfs containing the IPNS over Pubsub implementation.

@ghost ghost assigned vasco-santos Aug 31, 2018
@ghost ghost added the in progress label Aug 31, 2018
@alanshaw alanshaw changed the title Feat/ipns over pubsub test: ipns over pubsub Sep 4, 2018
SPEC/NAME.md Outdated
@@ -76,6 +79,8 @@ This way, you can republish a new version of your website under the same address

`callback` must follow `function (err, name) {}` signature, where `err` is an error if the operation was not successful. `name` is a string that contains the IPFS hash.

`callback` must follow `function (err, result) {}` signature, where `err` is an error if the operation was not successful. `result` is an object that contains the resulting path, such as:
Copy link
Contributor

Choose a reason for hiding this comment

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

image

Why are there two callback definitions?

SPEC/NAME.md Outdated

```JavaScript
{
strings: ['/ipns/QmQrX8hka2BtNHa8N8arAq16TCVx5qHcb46c5yPewRycLm']
Copy link
Contributor

Choose a reason for hiding this comment

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

"strings"? Shall we just return an array here?

Copy link
Contributor Author

@vasco-santos vasco-santos Oct 17, 2018

Choose a reason for hiding this comment

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

Yes! I got confused here. Both js-ipfs-api and js-ipfs should return only the array in the end!

SPEC/NAME.md Show resolved Hide resolved
ipfs.name.publish(value, { resolve: false }, (err, res) => {
expect(err).to.not.exist()

ipfs.name.resolve(nodeId, (err) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just so I understand this correctly, when you resolve, you get automatically subscribed to updates? Is there a way to not subscribe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When you try to resolve an ipns path and the experimental flag --enable-namesys-pubsub is being used, it will get automatically subscribed. This way, when an ipns record is updated, we will have the most recent version of it stored locally.

For now, there is no way to not subscribe it (if you use the experimental flag), but since it is experimental, we may consider adding a flag for it, in case we enable it by default in the future.

expect(err).to.not.exist()
expect(res).to.exist()
expect(res).to.have.property('canceled')
expect(res.canceled).to.eql(true)
Copy link
Contributor

@alanshaw alanshaw Oct 17, 2018

Choose a reason for hiding this comment

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

Should we also check this value is no longer in our subs list?

expect(res.canceled).to.eql(true)

done()
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: async waterfall or something?


const fixture = Object.freeze({
data: loadFixture('js/test/fixtures/testfile.txt', 'interface-ipfs-core'),
cid: 'Qma4hjFTnCasJ8PVp3mZbZK5g2vGDT4LByLJ7m8ciyRFZP'
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO we should just use the CID in the result from add and not hard code it. We're not testing "add" functionality here and if the CID changes in the future (because of changes in default dag layout or cid version for instance) there's fewer things to update in the tests. If you do change this then please also change in the other test files :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will change in both

expect(err).to.not.exist()
expect(res).to.exist()
expect(res).to.have.property('strings')
expect(res.strings).to.eql(null)
Copy link
Contributor

Choose a reason for hiding this comment

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

It should always be an array.

expect(err).to.not.exist()
expect(res).to.exist()
expect(res).to.have.property('strings')
expect(res.strings).to.be.an('array').that.does.include(`/ipns/${nodeId}`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Check that it was empty initially?

expect(res.strings).to.be.an('array').that.does.include(`/ipns/${nodeId}`)

done()
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: use async

SPEC/NAME.md Outdated
@@ -74,7 +77,7 @@ This way, you can republish a new version of your website under the same address
}
```

`callback` must follow `function (err, name) {}` signature, where `err` is an error if the operation was not successful. `name` is a string that contains the IPFS hash.
`callback` must follow `function (err, result) {}` signature, where `err` is an error if the operation was not successful. `result` is an object that contains the resulting path.
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this was correct - both js-ipfs-api and js-ipfs return only the path.

@vasco-santos vasco-santos force-pushed the feat/ipns-over-pubsub branch from 1418857 to 08a5b90 Compare October 17, 2018 16:18
@alanshaw alanshaw merged commit 2d7071c into master Oct 18, 2018
@ghost ghost removed the in progress label Oct 18, 2018
@alanshaw alanshaw deleted the feat/ipns-over-pubsub branch October 18, 2018 09:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants