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

test(dag): add test to verify put API overrides hash algorithm #323

Merged
merged 1 commit into from
Jul 5, 2018

Conversation

0x-r4bbit
Copy link
Contributor

As discussed in ipfs/js-ipfs#1419 (comment)
at the time of this commit, dag.put() basically ignores the hashAlg
option, as it passes it down to ipld.put(), which won't honor it until
ipld/js-ipld#133 is merged.

Once ipld/js-ipld#133 is merged, this test verifies
that e.g.

dag.put(cborNode, {
  format: 'dag-cbor',
  hashAlg: 'sha3-512'
}, (err, cid) => {
  ...
})

Actually results in a CID instance that decodes to sha3-512 and not
the sha2-256 default.

License: MIT
Signed-off-by: Pascal Precht [email protected]

@0x-r4bbit
Copy link
Contributor Author

@alanshaw let me know if this is what you've been looking for.

I can also rebase this on top of ipld/js-ipld#133 so we don't have to introduce this test and skip it at the same time.

Copy link
Contributor

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

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

@PascalPrecht 🙌 yes! One thing though, the skip should be removed from here and added to js-ipfs here with some appropriate reason text.

There shouldn't be a need to skip it for js-ipfs-api as these tests run against go-ipfs - hopefully they'll pass 😆

@0x-r4bbit
Copy link
Contributor Author

@alanshaw sure I can move it over. Just for clarification, in ipfs/js-ipfs#1419 (comment) you said

Would you be willing to add a test to interface-ipfs-core for using a hash algorithm other than the default?

So that was simply confused with js-ipfs, or do you still want to have anything added to interface-ipfs-core?

@alanshaw
Copy link
Contributor

alanshaw commented Jul 4, 2018

The test should stay here, just the instruction to skip should be moved to js-ipfs e.g. https://github.com/ipfs/js-ipfs/blob/master/test/core/interface.spec.js#L27-L30. My point was just that js-ipfs and js-ipfs-api use this same module and we should only need to skip this test in js-ipfs.

@0x-r4bbit
Copy link
Contributor Author

Ah now I get it. Phew.. this orchestration of 3+ repo dependencies can be quite confusing 😅

As discussed in ipfs/js-ipfs#1419 (comment)
at the time of this commit, `dag.put()` basically ignores the `hashAlg`
option, as it passes it down to `ipld.put()`, which won't honor it until
ipld/js-ipld#133 is merged.

Once ipld/js-ipld#133 is merged, this test verifies
that e.g.

```
dag.put(cborNode, {
  format: 'dag-cbor',
  hashAlg: 'sha3-512'
}, (err, cid) => {
  ...
})
```

Actually results in a `CID` instance that decodes to `sha3-512` and not
the `sha2-256` default.

License: MIT
Signed-off-by: Pascal Precht [email protected]
0x-r4bbit added a commit to 0x-r4bbit/js-ipfs that referenced this pull request Jul 4, 2018
Over at ipfs-inactive/interface-js-ipfs-core#323 we introduce a
test spec that ensures `dag.put()` honors the `hashAlg` option, which at
the time of this commit is ignored by the underlying `ipld.put()` API
(more info: ipfs@1a36375).

This commit skips that spec in `js-ipfs` as ipld/js-ipld#133 has to
land first to fulfill the scenario.

License: MIT
Signed-off-by: Pascal Precht <[email protected]>
@0x-r4bbit
Copy link
Contributor Author

@alanshaw updated this PR and moved the skip over to ipfs/js-ipfs#1421

Copy link
Contributor

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

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

LGTM - did you run the js-ipfs-api tests with this to check that it works on go-ipfs?

0x-r4bbit added a commit to 0x-r4bbit/js-ipfs that referenced this pull request Jul 5, 2018
Over at ipfs-inactive/interface-js-ipfs-core#323 we introduce a
test spec that ensures `dag.put()` honors the `hashAlg` option, which at
the time of this commit is ignored by the underlying `ipld.put()` API
(more info: ipfs@1a36375).

This commit skips that spec in `js-ipfs` as ipld/js-ipld#133 has to
land first to fulfill the scenario.

License: MIT
Signed-off-by: Pascal Precht <[email protected]>
@0x-r4bbit
Copy link
Contributor Author

Double-checking in this very moment.

@0x-r4bbit
Copy link
Contributor Author

0x-r4bbit commented Jul 5, 2018

screen shot 2018-07-05 at 10 33 38

So it turns out that all tests related to making options optional are failing in js-ipfs-api. The above is the output of running test:node on feat/modular-interface-tests with this PR's branch npm linked.

Investigating...

@alanshaw
Copy link
Contributor

alanshaw commented Jul 5, 2018

Looks like you might not have the latest js-ipfs-api?

@0x-r4bbit
Copy link
Contributor Author

Ah sorry. feat/modular-interface-tests have evolved in the meantime. Tests are fine on latest.

@alanshaw
Copy link
Contributor

alanshaw commented Jul 5, 2018

Oh, please checkout master on js-ipfs-api and git pull...that should fix you up.

@0x-r4bbit
Copy link
Contributor Author

screen shot 2018-07-05 at 10 37 28

And as an FYI: the test that is skipped in js-ipfs is not skipped in js-ipfs-api

@alanshaw
Copy link
Contributor

alanshaw commented Jul 5, 2018

hmm, can we unskip it in js-ipfs or is it still failing?

@alanshaw alanshaw merged commit 1c6c9f0 into ipfs-inactive:master Jul 5, 2018
@0x-r4bbit 0x-r4bbit deleted the test/dag-put-api branch July 5, 2018 08:41
@0x-r4bbit
Copy link
Contributor Author

0x-r4bbit commented Jul 5, 2018

This is still failing in js-ipfs due to ipld/js-ipld#133 not landed yet

Also notice that the test that's supposed to be skipped in js-ipfs isn't skipped in latest master yet as it's still a pending PR: ipfs/js-ipfs#1421

alanshaw pushed a commit to ipfs/js-ipfs that referenced this pull request Jul 5, 2018
Over at ipfs-inactive/interface-js-ipfs-core#323 we introduce a
test spec that ensures `dag.put()` honors the `hashAlg` option, which at
the time of this commit is ignored by the underlying `ipld.put()` API
(more info: 1a36375).

This commit skips that spec in `js-ipfs` as ipld/js-ipld#133 has to
land first to fulfill the scenario.

License: MIT
Signed-off-by: Pascal Precht <[email protected]>
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.

2 participants