Get tests passing with pin core changes

[AdamStone]

@diasdavid I ran into a few issues with the tests when trying to get the pin core interface updated to this spec:

• The default timeout is too short
• The add test is checking for a hash string result where the API indicates it should be an object with a hash property
• The recursive ls test is run after the recursive pin has been removed, but expects pinset not to be empty.
• The indirect ls test is also run after the recursive pin has been removed, but in any case the recursively pinned object has no links, so the indirect pinset is always empty.

For now I just made small adjustments to get them passing, but I think the pinned object should have at least one link, if not a more complex tree, for a nontrivial test of indirect pins. I also wonder if scenarios like 'rejects direct pin if already recursively pinned' and 'rejects recursive pin if child object is not stored' shouldn't be tested, as well as persistence of the pinset through the datastore. Probably the particular datastore mechanism will change since currently it's not consistent with go-ipfs, but maybe we should at least test that it roundtrips? Most of the complexity in the pinning code is involved with this functionality.

[AdamStone]

Hey @diasdavid , I wanted to revisit this since the datastore is available now. I see the timeout has been increased, do you have any comments on the other points I mentioned above? I assume this PR will need to be merged before any updates I make to the pinning code can be, since it won't pass the tests otherwise.

[daviddias]

The recursive ls test is run after the recursive pin has been removed, but expects pinset not to be empty.

I believe you fixed this with this change

[daviddias]

The add test is checking for a hash string result where the API indicates it should be an object with a hash property

Could you double check this? From what

but I think the pinned object should have at least one link, if not a more complex tree, for a nontrivial test of indirect pins. I also wonder if scenarios like 'rejects direct pin if already recursively pinned' and 'rejects recursive pin if child object is not stored' shouldn't be tested, as well as persistence of the pinset through the datastore. Probably the particular datastore mechanism will change since currently it's not consistent with go-ipfs, but maybe we should at least test that it roundtrips? Most of the complexity in the pinning code is involved with this functionality.

Yes yes yes :D All of that, we should make sure to catch all of the edge cases, pinning is one of the most sensitive things as it can result in data loss if it presents the wrong information to the user.

[daviddias]

@AdamStone I pulled and run your updated tests against js-ipfs-api and:

  1) .pin callback API .ls type indirect:
     Uncaught AssertionError: expected { Object (QmPZ9gcCEpqKTo6aq61g2nXGUhM4iCL3ewB6LDXZCtioEB, QmQ5vhrL7uv6tuoN9KeVBwd4PwfQkXdVVmDLUZuTNxqgvm, ...) } to be empty
      at ipfs.pin.ls (/Users/imp/code/interface-ipfs-core/src/pin.js:62:32)
      at send (src/pin/ls.js:31:7)
      at f (node_modules/once/once.js:25:25)
      at streamToValue (src/utils/stream-to-json-value.js:30:5)
      at concat (src/utils/stream-to-value.js:12:22)
      at ConcatStream.<anonymous> (node_modules/concat-stream/index.js:36:43)
      at finishMaybe (node_modules/readable-stream/lib/_stream_writable.js:607:14)
      at afterWrite (node_modules/readable-stream/lib/_stream_writable.js:470:3)
      at _combinedTickCallback (internal/process/next_tick.js:144:20)
      at process._tickCallback (internal/process/next_tick.js:180:9)

  2) .pin callback API .add:
     Uncaught AssertionError: expected undefined to equal 'Qma4hjFTnCasJ8PVp3mZbZK5g2vGDT4LByLJ7m8ciyRFZP'
      at ipfs.pin.add (/Users/imp/code/interface-ipfs-core/src/pin.js:86:40)
      at send (src/pin/add.js:19:7)
      at f (node_modules/once/once.js:25:25)
      at streamToValue (src/utils/stream-to-json-value.js:30:5)
      at concat (src/utils/stream-to-value.js:12:22)
      at ConcatStream.<anonymous> (node_modules/concat-stream/index.js:36:43)
      at finishMaybe (node_modules/readable-stream/lib/_stream_writable.js:607:14)
      at afterWrite (node_modules/readable-stream/lib/_stream_writable.js:470:3)
      at _combinedTickCallback (internal/process/next_tick.js:144:20)
      at process._tickCallback (internal/process/next_tick.js:180:9)

  3) .pin promise API .add:
     AssertionError: expected undefined to equal 'Qma4hjFTnCasJ8PVp3mZbZK5g2vGDT4LByLJ7m8ciyRFZP'
      at ipfs.pin.add.then (/Users/imp/code/interface-ipfs-core/src/pin.js:124:42)
      at <anonymous>
      at process._tickCallback (internal/process/next_tick.js:188:7)

Could you check too? Do you know how to test this yourself? You can npm link interface-ipfs-core to js-ipfs-api and then run npm run test.

# inside the interface-ipfs-core folder
> npm run link
# inside the js-ipfs-api folder and after having run npm install
> npm run link interface-ipfs-core
# now you can run tests
> npm run test

[AdamStone]

Thanks, I didn't know that trick. I'm seeing the same errors. The js-ipfs tests were passing with these changes but I didn't try js-ipfs-api. I'll try to figure out what's going on.

[AdamStone]

@diasdavid It looks like in addition to the file added in the before part of the tests, there are two other hashes being pinned automatically. QmUNLLsPACCz1vLxQVkXqqLX5R1X345qqfHbsf67hvA3Nn which seems to be an empty directory, and QmVLDAhCY3X9P2uRudKAryuQFPM5zqA3Yij1dY8FpGbL7T which is the default starting directory (about, quick-start, readme, etc). The tests are assuming the test file is the only thing pinned. I'll update the tests to accommodate the possibility of other pins.

[AdamStone]

@diasdavid I'm running into a problem if I try to make the tests more specific. Basically,

1. js-ipfs-api and js-ipfs both run against the same set of tests in interface-ipfs-core.
2. js-ipfs-api is following the http spec outlined here (add, ls, rm) for the results format.
3. The interface-ipfs-core API indicates a different results format for the core methods.

I implemented the pinning core methods to follow the interface-ipfs-core API (3 above), and my changes to the tests there reflect that but break the js-ipfs-api tests since the results format is different.

It seems like to get everything passing, one of the following should happen:

1. The pinning core implementation should return results in the same format as the http api (2 above), and the interface-ipfs-core API (3 above) should be updated to indicate this.
2. The js-ipfs-api tests shouldn't run against interface-ipfs-core since the http api uses a different result format than the core spec.
3. The interface-ipfs-core tests should be updated to accommodate both potential results formats.

There's a similar issue with the CLI tests, because the ON tests go through the http api and the OFF tests call the core methods directly. So in the handler functions, results may appear in both formats.

Can you advise which direction to go with this, before I change anything further?

[AdamStone]

I updated this PR as well as ipfs/js-ipfs#107 and ipfs-inactive/js-ipfs-http-client#602 to get the various API interfaces working together correctly while following the intended specs. I also made the tests more specific, looking for particular hashes instead of just checking whether results are empty or not. This avoids the problem where js-ipfs-api tests include additional pins due to the starter content besides those that are added during the test.

Note that the indirect pin test is commented out because the test file has no links, so there are no indirect pins associated with it, but checking that it's empty breaks the js-ipfs-api due to the starter content including indirect pins. The test seems fairly meaningless in any case unless the test file being pinned includes a link.