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

Add failing get recursive test #736

Closed
wants to merge 3 commits into from
Closed

Add failing get recursive test #736

wants to merge 3 commits into from

Conversation

victorb
Copy link
Member

@victorb victorb commented Jan 29, 2017

Adding a test case for recursive .get that is failing with:

    Error: unsupported version
    at CID.get buffer [as buffer] (/Users/user/projects/ipfs/js-monorepo/packages/js-ipfs/node_modules/ipfs-unixfs-engine/node_modules/cids/src/index.js:75:15)
    at Bitswap._getStreamSingle (/Users/user/projects/ipfs/js-monorepo/packages/js-ipfs/node_modules/ipfs-bitswap/src/index.js:139:23)
    at Bitswap.getStream (/Users/user/projects/ipfs/js-monorepo/packages/js-ipfs/node_modules/ipfs-bitswap/src/index.js:121:19)
    at BlockService.getStream (/Users/user/projects/ipfs/js-monorepo/packages/js-ipfs/node_modules/ipfs-block-service/src/index.js:76:28)
    at IPLDResolver.getStream (/Users/user/projects/ipfs/js-monorepo/packages/js-ipfs/node_modules/ipld-resolver/src/index.js:146:15)
    at visitor (/Users/user/projects/ipfs/js-monorepo/packages/js-ipfs/node_modules/ipfs-unixfs-engine/src/exporter/index.js:25:20)
    at /Users/user/projects/ipfs/js-monorepo/packages/js-ipfs/node_modules/pull-traverse/index.js:62:18
    at /Users/user/projects/ipfs/js-monorepo/packages/js-ipfs/node_modules/pull-stream/throughs/flatten.js:28:16
    at /Users/user/projects/ipfs/js-monorepo/packages/js-ipfs/node_modules/pull-stream/throughs/flatten.js:28:16
    at /Users/user/projects/ipfs/js-monorepo/packages/js-ipfs/node_modules/pull-stream/sources/values.js:21:7
    at nextChunk (/Users/user/projects/ipfs/js-monorepo/packages/js-ipfs/node_modules/pull-stream/throughs/flatten.js:20:9)
    at /Users/user/projects/ipfs/js-monorepo/packages/js-ipfs/node_modules/pull-stream/throughs/flatten.js:41:11
    at drain (/Users/user/projects/ipfs/js-monorepo/packages/js-ipfs/node_modules/pull-paramap/index.js:18:11)
    at /Users/user/projects/ipfs/js-monorepo/packages/js-ipfs/node_modules/pull-paramap/index.js:44:13
    at ipldResolver.get (/Users/user/projects/ipfs/js-monorepo/packages/js-ipfs/node_modules/ipfs-unixfs-engine/src/exporter/dir.js:34:7)
    at pull.collect (/Users/user/projects/ipfs/js-monorepo/packages/js-ipfs/node_modules/ipld-resolver/src/index.js:139:9)

@victorb victorb added the status/in-progress In progress label Jan 29, 2017
@daviddias
Copy link
Member

will look into this one today. @pgte does anything catch your eye there?

@daviddias daviddias added the kind/bug A bug in existing code (including security flaws) label Jan 29, 2017
@daviddias
Copy link
Member

I've discovered that the bug is in unixfs-engine, where instead of having item.hash on https://github.com/ipfs/js-ipfs-unixfs-engine/blob/master/src/exporter/index.js#L25, we have item being:

{
  content: [Function],
  path: 'QmUhUuiTKkkK8J6JZ9zmj8iNHPuNfGYcszgRumzhHBxEEU/about',
  size: 1677
}

This escaped previous testing because we had disabled recursive tests due to js-ipfs-api + go-ipfs browser problems:

https://github.com/ipfs/interface-ipfs-core/blob/master/src/files.js#L371-L431

If those tests are enabled, we can see that now ipfs files get also fails on js-ipfs core.

@pgte can you take a look at this one? The new unixfs-engine code is fresh in your mind. I'll try to circle back to this tomorrow if we still haven't found a solution.

@daviddias
Copy link
Member

@victorbjelkholm also, you have to unlink the added folder on this CLI tests, so that the working directory stays clean.

@pgte
Copy link
Contributor

pgte commented Jan 30, 2017

@victorbjelkholm as asked by @diasdavid, added test for recursive dir using core. (will also add similar test to js-ipfs-unixfs-engine) next.
Back to you.

@pgte pgte assigned victorb and unassigned pgte Jan 30, 2017
@daviddias daviddias removed the status/ready Ready to be worked label Jan 30, 2017
@daviddias daviddias deleted the test/recursive-get branch January 30, 2017 15:21
return ipfs('files get QmUhUuiTKkkK8J6JZ9zmj8iNHPuNfGYcszgRumzhHBxEEU').then((out) => {
const directory = path.join(process.cwd(), 'QmUhUuiTKkkK8J6JZ9zmj8iNHPuNfGYcszgRumzhHBxEEU')
expect(fs.readdirSync(directory).length).to.be.eql(8)
// TODO add assertion on content of files
Copy link
Member

Choose a reason for hiding this comment

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

@victorbjelkholm your turn, tests are passing :)

(also, make sure to unlink to delete that file, like the other tests do.

@daviddias
Copy link
Member

oh, github closed this PR because of the other one being merged. weird @github

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind/bug A bug in existing code (including security flaws)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants