Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

0.4.14-rcX fails js-ipfs-api test suite #4818

Closed
Kubuxu opened this issue Mar 15, 2018 · 16 comments
Closed

0.4.14-rcX fails js-ipfs-api test suite #4818

Kubuxu opened this issue Mar 15, 2018 · 16 comments
Assignees
Labels
kind/bug A bug in existing code (including security flaws) regression topic/api Topic api

Comments

@Kubuxu
Copy link
Member

Kubuxu commented Mar 15, 2018

Reference ipfs-inactive/js-ipfs-http-client#486 (comment)

Test output: https://friendpaste.com/72TbssZSoSxmdOpKxT67je

@Kubuxu Kubuxu added kind/bug A bug in existing code (including security flaws) topic/api Topic api regression labels Mar 15, 2018
@Kubuxu Kubuxu added this to the go-ipfs 0.4.14 milestone Mar 15, 2018
@magik6k
Copy link
Member

magik6k commented Mar 15, 2018

@Kubuxu
Copy link
Member Author

Kubuxu commented Mar 15, 2018

@victorbjelkholm @diasdavid could we get some help interpreting these.

I see that there are some that are false positive lookalike. Like the [] vs [null].

@magik6k
Copy link
Member

magik6k commented Mar 15, 2018

Changes I see:

{
  "Hash": "Qm...",
  "Size": 1088,
  "CumulativeSize": 1099,
  "Blocks": 0,
  "Type": "file"
}

and with --with-local:

{
  "Hash": "Qm...",
  "Size": 1088,
  "CumulativeSize": 1099,
  "Blocks": 0,
  "Type": "file",
  "WithLocality": true,
  "Local": true,
  "SizeLocal": 1099
}
  • Not sure about DHT fails, it happened on 0.4.13 too
  • pubsub/sub returned an empty object ({}) after request, this has been fixed in 0.4.14-rc2
  • The other errors seem to be related to the fact that some tests failed.

@daviddias
Copy link
Member

daviddias commented Mar 15, 2018

Try with https://github.com/ipfs/js-ipfs-api/releases/tag/v18.1.2 test suite. Currently js-ipfs-api Master is in flux due to many Windows + other fixes that were happening in parallel and complicating things, next week it will be golden again.

@magik6k
Copy link
Member

magik6k commented Mar 15, 2018

Tried with js-ipfs-api 18.1.2 + go-ipfs 0.4.14-rc2, fails mostly in the same ways as the master: https://hastebin.com/gigiforudu.vbs

@whyrusleeping
Copy link
Member

@victorbjelkholm can we get some automated CI for running js-ipfs-api tests?

@magik6k
Copy link
Member

magik6k commented Mar 15, 2018

For 0.4.13 / js-api 18.1.2 there is only a single test failing, prorbaly a random fail: https://hastebin.com/eticutoyal.vbs

@magik6k
Copy link
Member

magik6k commented Mar 15, 2018

I tried to find a difference between files/stat on 0.4.13 and 0.4.14-rc2:

0.4.14-rc2:

HTTP/1.1 200 OK
Access-Control-Allow-Headers: X-Stream-Output, X-Chunked-Output, X-Content-Length
Access-Control-Expose-Headers: X-Stream-Output, X-Chunked-Output, X-Content-Length
Content-Type: application/json
Server: go-ipfs/0.4.14-rc2
Trailer: X-Stream-Error
Vary: Origin
X-Chunked-Output: 1
Date: Thu, 15 Mar 2018 18:58:30 GMT
Connection: close
Transfer-Encoding: chunked

71
{"Hash":"Qma4hjFTnCasJ8PVp3mZbZK5g2vGDT4LByLJ7m8ciyRFZP","Size":12,"CumulativeSize":20,"Blocks":0,"Type":"file"}

0

go-ipfs/0.4.13:

HTTP/1.1 200 OK
Access-Control-Allow-Headers: X-Stream-Output, X-Chunked-Output, X-Content-Length
Access-Control-Expose-Headers: X-Stream-Output, X-Chunked-Output, X-Content-Length
Content-Type: application/json
Server: go-ipfs/0.4.13
Trailer: X-Stream-Error
Vary: Origin
Date: Thu, 15 Mar 2018 19:23:02 GMT
Transfer-Encoding: chunked

75
{"Hash":"QmcPGG45AEWEG1PCeufXDDsGWb3QE1MoCdzuH1AwN3u5Ft","Size":1088,"CumulativeSize":1099,"Blocks":0,"Type":"file"}

0

The only difference is the removed Connection: close, so one explanation I have for it is that it may cause problems with keep-alive on js-ipfs-api side.

@victorb victorb self-assigned this Mar 15, 2018
@victorb
Copy link
Member

victorb commented Mar 16, 2018

Looked into the test failures from js-ipfs-api v18.1.2 with go-ipfs 0.4.14-rc2

  • bitswap wantlist changes: from Keys: null to Keys: [], easily fixed and would be on js-ipfs-api side. I recommend to note this as a breaking change in the release notes in case libs are assuming null instead of [].
  • files.stat somehow weird values
    • go-ipfs cli works fine
    • go-ipfs http api works fine
    • only difference is X-Chunked-Output: 1 which is introduced with 0.4.14
    • probably bug regarding parsing response in js-ipfs-api
  • .files mfs (interface-core-ipfs) are screwed with as well
    • probably because parsed the same way as files.stat
  • pubsub is generally broken (~40% of tests passes) it seems, haven't looking into on go-ipfs or js-ipfs-api side yet.

@Kubuxu
Copy link
Member Author

Kubuxu commented Mar 16, 2018

Seems like X-Chunked-Output: 1 is a new header for some commands.

It is introduced in go-ipfs-cmds here:
https://github.com/ipfs/go-ipfs-cmds/blob/13acd1ab00ea043c3b202e1eb0fca2dec1a34a34/http/responseemitter.go#L207-L212

It is controlled by https://github.com/ipfs/go-ipfs-cmds/blob/master/responseemitter.go#L10-L14 and our mistake is not using EmitOnce or Emit(Single{var}) for emitting single values.

From another side, sending even chunked response with single chunk should not break the js-ipfs-api.

I am fixing this but please harden js-ipfs-api parsing.

Kubuxu added a commit that referenced this issue Mar 16, 2018
Part of #4818

License: MIT
Signed-off-by: Jakub Sztandera <[email protected]>
@victorb
Copy link
Member

victorb commented Mar 16, 2018

@Kubuxu thanks, I can confirm that 5917805 does indeed fix the files.stat calls in js-ipfs-api and got the tests to pass with that commit applied.

Will continue looking into the pubsub issues now.

@victorb
Copy link
Member

victorb commented Mar 16, 2018

Regarding pubsub, this is what I see when testing pubsub ls with 0.4.14-rc2-59178057a so I'm guessing the test failures are expected.

$ ipfs daemon --enable-pubsub-experiment # in one terminal

$ ipfs pubsub sub test # in another terminal

$ ipfs pubsub ls # in yet another terminal, but gives output:
Error: not of type error

@Kubuxu
Copy link
Member Author

Kubuxu commented Mar 16, 2018

Fixed pubsub ls in 42d1aa5
It was broken by cmds refactor when the output format was changed.

@victorb
Copy link
Member

victorb commented Mar 16, 2018

So with @Kubuxu's fixes, I've been able to get green tests for node tests on js-ipfs-api. This is the only change required on js-ipfs-api's side: ipfs-inactive/js-ipfs-http-client#718

Currently running browser+webworker tests, but should be fine.

$ ./node_modules/go-ipfs-dep/go-ipfs/ipfs version --all
go-ipfs version: 0.4.14-rc2-90017e9da
Repo version: 6
System version: amd64/linux
Golang version: go1.10

@victorb
Copy link
Member

victorb commented Mar 16, 2018

So, seems like the tests are all passing with 90017e9 and ipfs-inactive/js-ipfs-http-client#718, even in the browser and webworker.

Great work @Kubuxu for quick fixes and @magik6k for debugging awesomeness! 👍

AFDudley pushed a commit to vulcanize/go-ipfs that referenced this issue Mar 22, 2018
@whyrusleeping
Copy link
Member

This is resolved now

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

No branches or pull requests

7 participants