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

test: Bitswap wantlist peerid #267

Merged
merged 6 commits into from
Jun 18, 2018
Merged

test: Bitswap wantlist peerid #267

merged 6 commits into from
Jun 18, 2018

Conversation

wraithgar
Copy link
Contributor

@wraithgar wraithgar commented May 10, 2018 •
edited
Loading

Tests for wantlist including with peerId param.

Still need to add copy to SPEC/BITSWAP.md

ETA branched from #254 and that PR should merge before this one.

@ghost ghost assigned wraithgar May 10, 2018
@ghost ghost added the in progress label May 10, 2018
@wraithgar wraithgar mentioned this pull request May 10, 2018
Merged
@wraithgar wraithgar force-pushed the bitswap_wantlist_peerid branch from 7e084ea to 92cd03c Compare May 11, 2018 18:02
daviddias
daviddias suggested changes May 12, 2018
View reviewed changes
Copy link
Contributor

@daviddias daviddias left a comment

Choose a reason for hiding this comment

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

Cool stuff! Did these tests got run against js-ipfs-api and js-ipfs?

js/src/bitswap.js Outdated
})

it('.unwant gives error if offline', () => {
expect(() => ipfs.bitswap.unwant('my key'), (err) => {
Copy link
Contributor

@daviddias daviddias May 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This will always be an invalid key. It should be tested with a valid key and ensure that the error is because it is offline.

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 changed that to use a proper key and still expect an error.

@wraithgar
Copy link
Contributor Author

@diasdavid oh yeah looks like I forgot to push and make the PR for the js-ipfs-api code this requires.

@wraithgar wraithgar force-pushed the bitswap_wantlist_peerid branch from 92cd03c to ef0dc25 Compare May 14, 2018 16:10
@wraithgar
Copy link
Contributor Author

Conflicts resolved, rebased against updated bitswap_unwant branch (PR for which should merge first)

alanshaw
alanshaw suggested changes May 15, 2018
View reviewed changes
Copy link
Contributor

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

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

Thank you! Looks great, just a few minor points to address and it'll be good to merge 🚀

SPEC/BITSWAP.md Outdated
@@ -4,7 +4,22 @@
* [bitswap.unwant](#bitswapunwant)
* [bitswap.stat](#bitswapstat)

#### `bitswap.wantlist`
#### `unwant`
Copy link
Contributor

Choose a reason for hiding this comment

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

Please could you change to bitswap.unwant so the link from the TOC works

SPEC/BITSWAP.md Outdated

##### `Go` **WIP**

### `bitswap.wantlist`
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please remove these old headers so we don't have duplicates?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, had a merge conflict go pear shaped on me. I'll get these cleaned up.

js/src/bitswap.js Outdated
ipfsB.block.get(new CID(key))
.then(() => {})
.catch(() => {})
setTimeout(cb, 500)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you explain what's going on here? Maybe also add a comment?

js/src/bitswap.js Outdated
it('.unwant', (done) => {
ipfsA.bitswap.unwant(new CID(key), (err) => {
ipfsA.bitswap.wantlist((err, list) => {
expect(list).to.be.empty()
Copy link
Contributor

Choose a reason for hiding this comment

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

Also expect(err).to.not.exist()?

js/src/bitswap.js Outdated
it('.stat', (done) => {

ipfsA.bitswap.stat((err, stats) => {
statsTests.expectIsBitswap(err, stats)
Copy link
Contributor

Choose a reason for hiding this comment

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

Also expect(err).to.not.exist()?

js/src/bitswap.js Outdated

it('.stat gives error while offline', () => {
ipfs.bitswap.stat((err, stats) => {
expect(err).to.exist()
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we assert against the error message? Just expecting it to exist could give to false positives.

@wraithgar
Copy link
Contributor Author

Cleaned up as per review comments. Thanks for the feedback.

@wraithgar wraithgar force-pushed the bitswap_wantlist_peerid branch 2 times, most recently from 76a0f0e to 4a21bad Compare May 15, 2018 16:30
alanshaw
alanshaw suggested changes May 22, 2018
View reviewed changes
Copy link
Contributor

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

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

Apologies, I noticed a couple more small issues after your changes.

SPEC/BITSWAP.md Outdated

### `bitswap.wantlist`

> Returns the wantlist, optionally limited by peerID
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be helpful to clarify if "the wantlist" when not limited by peer ID is a list that everybody shares or if it is just yours.

Also, would "filtered" be a more accurate description than "limited"?

SPEC/BITSWAP.md Outdated
```JavaScript
ipfs.bitswap.wantlist((err, list) => console.log(list))

//[ { Wantlist object }, ... ]
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you elaborate on the shape of the "Wantlist object" here? I think we usually try to in these specs.

@wraithgar wraithgar force-pushed the bitswap_wantlist_peerid branch from 4a21bad to 97334ee Compare June 8, 2018 18:43
@wraithgar wraithgar force-pushed the bitswap_wantlist_peerid branch 7 times, most recently from a459a4e to 16bbdfe Compare June 14, 2018 22:13
@alanshaw alanshaw changed the title Bitswap wantlist peerid test: Bitswap wantlist peerid Jun 18, 2018
@alanshaw alanshaw force-pushed the bitswap_wantlist_peerid branch from 16bbdfe to 9f81bcb Compare June 18, 2018 11:57
@ghost ghost assigned alanshaw Jun 18, 2018
alanshaw added 2 commits June 18, 2018 13:17
@alanshaw
fix: improve bitswap wantlist and unwant docs
7737546 
License: MIT
Signed-off-by: Alan Shaw <[email protected]>
@alanshaw
fix: linting errors
fcc834c 
License: MIT
Signed-off-by: Alan Shaw <[email protected]>
@alanshaw alanshaw merged commit bc6d3d1 into master Jun 18, 2018
@ghost ghost removed the in progress label Jun 18, 2018
@alanshaw alanshaw deleted the bitswap_wantlist_peerid branch June 18, 2018 14:49
@alanshaw alanshaw mentioned this pull request Jun 18, 2018
Closed
alanshaw added a commit that referenced this pull request Jun 25, 2018
@alanshaw
chore: re-add bitswap tests
bd5aac6 
Originally from PR #267

License: MIT
Signed-off-by: Alan Shaw <[email protected]>
alanshaw added a commit that referenced this pull request Jun 26, 2018
@alanshaw
chore: re-add bitswap tests
2c75978 
Originally from PR #267

License: MIT
Signed-off-by: Alan Shaw <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@daviddias daviddias daviddias requested changes

@alanshaw alanshaw alanshaw approved these changes

@travisperson travisperson Awaiting requested review from travisperson

@victorb victorb Awaiting requested review from victorb

Assignees

@wraithgar wraithgar

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@wraithgar @alanshaw @daviddias