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

[WIP] Pull streams #64

Merged
merged 4 commits into from
Sep 8, 2016
Merged

[WIP] Pull streams #64

merged 4 commits into from
Sep 8, 2016

Conversation

dignifiedquire
Copy link
Contributor

@dignifiedquire dignifiedquire commented Aug 21, 2016

Migrate

  • fixed-size-chunker
  • exporter
  • importer

@@ -2,7 +2,7 @@ sudo: false
language: node_js
node_js:
- 4
- 5
- stable
Copy link
Contributor

Choose a reason for hiding this comment

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

Add Node.js 5 tests too

@daviddias daviddias force-pushed the pull branch 2 times, most recently from c422ea8 to 3229064 Compare September 8, 2016 17:03
@daviddias
Copy link
Contributor

daviddias commented Sep 8, 2016

Got an error, basically due to assumed ordering


  1) core importer small file (smaller than a chunk) inside a dir:

      Uncaught AssertionError: expected [ Array(3) ] to deeply equal [ Array(3) ]
      + expected - actual

           "path": "foo/bar/200Bytes.txt"
           "size": 211
         }
         {
      +    "multihash": "Qmf5BQbTUyUAvd6Ewct83GYGnE1F6btiC3acLhR8MDxgkD"
      +    "path": "foo/bar"
      +    "size": 270
      +  }
      +  {
           "multihash": "QmQrb6KKWGo8w7zKfx2JksptY6wN7B2ysSBdKZr4xMU36d"
           "path": "foo"
           "size": 320
         }
      -  {
      -    "multihash": "Qmf5BQbTUyUAvd6Ewct83GYGnE1F6btiC3acLhR8MDxgkD"
      -    "path": "foo/bar"
      -    "size": 270
      -  }
       ]

Strange that it is not ordered for this example. I would say that ordering is not important, but this might actually be a bug. //cc @dignifiedquire

@dignifiedquire
Copy link
Contributor Author

@diasdavid what do we want to do about the ordering, just make the tests not care about it?

@daviddias
Copy link
Contributor

Also phantom fails on the last test of Importer with

PhantomJS 2.1.1 (Linux 0.0.0) WARN: 'Unhandled rejection: AssertionError: expected [ Array(5) ] to deeply equal [ Array(5) ]

@dignifiedquire
Copy link
Contributor Author

Why would this be a bug? it's just traversing/emitting in a different order

@daviddias
Copy link
Contributor

In PhantomJS

 1) IPFS data importing tests on the Browser importer nested directory (2 levels deep):
     timeout of 80000ms exceeded. Ensure the done() callback is being called in this test.


  2) IPFS data importing tests on the Browser chunker: fixed size 256 KiB chunks of non scalar filesize:
     expected [ Array(4) ] to have a length of 5 but got 4

@daviddias
Copy link
Contributor

Also first time seeing this:

[BABEL] Note: The code generator has deoptimised the styling of "/Users/david/code/js-ipfs-unixfs-engine/test/test-fixed-size-chunker.js" as it exceeds the max of "100KB".
[BABEL] Note: The code generator has deoptimised the styling of "/Users/david/code/js-ipfs-unixfs-engine/test/test-exporter.js" as it exceeds the max of "100KB".
[BABEL] Note: The code generator has deoptimised the styling of "/Users/david/code/js-ipfs-unixfs-engine/test/test-importer.js" as it exceeds the max of "100KB".

@dignifiedquire
Copy link
Contributor Author

I have no idea what phantomjs has for a problem, except that it might be too slow to do anything right :( I can't debug Phantom on my machine currently as every time I start it, it will die with

phantomjs(11121,0x7fff793ce000) malloc: *** error for object 0x7fc0898d2400: pointer being freed was not allocated
*** set a breakpoint in malloc_error_break to debug

@dignifiedquire
Copy link
Contributor Author

@diasdavid fixed the first issue

@dignifiedquire
Copy link
Contributor Author

found the reason of the second issue: https://www.npmjs.com/package/buffer#one-minor-difference and pull-block relies on this :( not sure how this worked before as the chunker in master uses buffer.slice as well.

@daviddias
Copy link
Contributor

PhantomJS exists in the pipeline to verify that the ES5 transpiled version works as expected, in this specific case it is not a transpilation issue, but an 'old browser' problem. We haven't declared what are the official minimum browser version to run js-ipfs, but it will be certainly a browser that at least requires WebRTC (which brings it to the class of modern browsers, where we won't hit this issue).

I've been wanting to be able to disable PhantomJS for a while (ipfs/aegir#48), seems that now it is the time :)

@daviddias daviddias merged commit 51fe91f into master Sep 8, 2016
@daviddias daviddias deleted the pull branch September 8, 2016 23:50
@daviddias daviddias removed the status/in-progress In progress label Sep 8, 2016
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