chore: convert tests async await #547
Conversation
package.json
Outdated
| "pump": "^3.0.0", | ||
| "readable-stream": "^3.1.1", | ||
| "streaming-iterables": "^4.1.0", | ||
| "through2": "^3.0.0" | ||
| }, | ||
| "devDependencies": { | ||
| "aegir": "^20.0.0" | ||
| "aegir": "^20.3.2", | ||
| "ipfsd-ctl": "ipfs/js-ipfsd-ctl#feat/interface-tests" |
There was a problem hiding this comment.
This will need to be merged & released before this PR can go in.
| const cid = await ipfs.object.patch.addLink(testNodeCid, node1b.Links[0]) | ||
| expect(node1bCid).to.eql(cid) | ||
|
|
||
| /* TODO: revisit this assertions. |
There was a problem hiding this comment.
Can this be uncommented?
There was a problem hiding this comment.
I'm having a hard time trying to figure several things out:
/* TODO: revisit this assertions.
// note: make sure we can link js plain objects
const content = Buffer.from(JSON.stringify({
title: 'serialized object'
}, null, 0))
const result = await ipfs.add(content)
expect(result).to.exist()
expect(result).to.have.lengthOf(1)
const object = result.pop()
const node3 = {
name: object.hash,
multihash: object.hash,
size: object.size
}
const node = await ipfs.object.patch.addLink(testNodeWithLinkMultihash, node3)
expect(node).to.exist()
testNodeWithLinkMultihash = node.multihash
testLinkPlainObject = node3
*/This seems to be a mix between code and pseudo-code. I don't know if I did correctly understand the goal of this test but I did try to do this: const node = await ipfs.object.patch.addLink(node1bCid, node3) where node1bCid is the cid of the node with link (as testNodeWithLinkMultihash indicates). However, I'm getting this error: Error: invalid path "": path does not begin with '/'.
Furthermore, this seems to be a different test (like a second test, testing plain objects this time) and thus, I think it should have its own test, separately of should add a link to an existing node test.
So, we can leave it as it currently is (commented), delete this test, or if you really want to keep it can you please help me? @achingbrain
| @@ -120,33 +44,26 @@ module.exports = (createCommon, options) => { | |||
|
|
|||
| expect(withoutChildCid).to.not.deep.equal(parentCid) | |||
| expect(withoutChildCid).to.deep.equal(nodeCid) | |||
|
|
|||
| /* TODO: revisit this assertions. | |||
There was a problem hiding this comment.
Can this be uncommented?
There was a problem hiding this comment.
Same here: #547 (comment)
There was a problem hiding this comment.
Please address the other feedback that @achingbrain and @hugomrdias have and lets get this merged.
I'm ok with adding pull-to-promise for now. Pull streams will disappear completely soon. Lets not spend any more time refactoring this again.
|
@PedroMiguelSS I really would like to get this merged soon! There's been no activity for 10 days now - is there something you're blocked on? |
|
No, I'm not blocked. There's been no activity for 6 days 😜 I was trying to finish adding |
You're right - my apologies |
|
Do not merge this yet. I'm still testing it. |
|
I did not forget about this, I'm just waiting for @hugomrdias to solve a dependencies related problem on his branch ipfs/js-ipfsd-ctl#feat/interface-tests, which my PR is depending on. |
PedroMiguelSS
force-pushed
the
chore/convert-tests-async-await
branch
from
b05a77b to
cdf6864
Compare
|
My changes are all done. @hugomrdias, I did run these tests on ipfs/js-ipfs-http-client#fix/update-promise-setup and somehow the Btw, this PR will be merged from |
PedroMiguelSS
force-pushed
the
chore/convert-tests-async-await
branch
from
cdf6864 to
8bd5541
Compare
|
I have no idea when |
|
This PR must be closed in favor of #562. |
This PR converts tests to use async/await syntax.