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

test(files): add test to ensure meaningful error is thrown in cat() #330

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

0x-r4bbit
Copy link
Contributor

To verify changes discussed in https://github.com/ipfs/js-ipfs-api/issues/799#issuecomment-402086401, this commit
adds a test that checks files.cat() throws a meaningful error when a key or ipfs path with
unsupported encoding is given.

License: MIT
Signed-off-by: Pascal Precht pascal.precht@gmail.com

0x-r4bbit added a commit to 0x-r4bbit/js-ipfs-api that referenced this pull request Jul 18, 2018
… `cat()`

A test to verify this is available at ipfs-inactive/interface-js-ipfs-core#330 and needs to be merged first.

Fixes #799

License: MIT
Signed-off-by: Pascal Precht pascal.precht@gmail.com
0x-r4bbit added a commit to 0x-r4bbit/js-ipfs-api that referenced this pull request Jul 18, 2018
… `cat()`

A test to verify this is available at ipfs-inactive/interface-js-ipfs-core#330 and needs to be merged first.

Fixes #799

License: MIT
Signed-off-by: Pascal Precht pascal.precht@gmail.com
To verify changes discussed in https://github.com/ipfs/js-ipfs-api/issues/799#issuecomment-402086401, this commit
adds a test that checks `files.cat()` throws a meaningful error when a key or ipfs path with
unsupported encoding is given.

License: MIT
Signed-off-by: Pascal Precht pascal.precht@gmail.com
0x-r4bbit added a commit to 0x-r4bbit/js-ipfs-api that referenced this pull request Jul 18, 2018
A test to verify this is available at ipfs-inactive/interface-js-ipfs-core#330 and needs to be merged first.

Fixes #799

License: MIT
Signed-off-by: Pascal Precht pascal.precht@gmail.com
@alanshaw
Copy link
Contributor

alanshaw commented Jul 23, 2018

Hey @PascalPrecht did you run this against js-ipfs as well as js-ipfs-api to check it passes for go-ipfs as well as js-ipfs?

@0x-r4bbit
Copy link
Contributor Author

Ha good point @alanshaw !

I've only tested this against js-ipfs-api (where it passes). Running this against js-ipfs gives me:

  1) interface-ipfs-core tests
       .files.cat
         should error on key/path with invalid encoding:
     AssertionError: expected 'Error: Non-base58 character' to include 'Error: selected encoding not supported'
      at ipfs.files.cat.catch (/Users/pascalprecht/projects/ipfs/interface-ipfs-core/js/src/files/cat.js:139:37)

So js-ipfs seems to behave differently to go-ipfs here as well. At least they throw different errors (while both seem legit). What should do you think should be done? Shall I change the test to just expecting some error?

I think though, it'd be better to specify what error we're expecting...

Copy link
Contributor

@victorb victorb left a comment

Choose a reason for hiding this comment

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

Let's say that ipfs.files.cat didn't throw an error in this test, wouldn't the test pass? Should probably add a .then(() => throw new Error('Shouldnt be here')) so in case it's valid, the test fails

@0x-r4bbit
Copy link
Contributor Author

@victorbjelkholm Hi and thank you for getting back to me here!

Sure, I can add that.

Do you also have some thoughts on

What should do you think should be done? Shall I change the test to just expecting some error?

By any chance?

@daviddias
Copy link
Contributor

@PascalPrecht can you rebase master onto this branch?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants