-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat: reworking tests with new ipfsd-ctl #1167
Conversation
test/cli/pubsub.js
Outdated
@@ -12,7 +12,7 @@ const ipfsExec = require('../utils/ipfs-exec') | |||
const IPFS = require('../../src') | |||
|
|||
const DaemonFactory = require('ipfsd-ctl') | |||
const df = DaemonFactory.create() | |||
const df = DaemonFactory.create({ type: 'js' }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this use the latest js-ipfs from this repo??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm passing the path to the latest ipfs during spawn.
test/cli/pubsub.js
Outdated
@@ -41,9 +41,8 @@ describe('pubsub', function () { | |||
this.timeout(60 * 1000) | |||
|
|||
DaemonFactory | |||
.create({ remote: false }) | |||
.create({ type: 'proc' }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PubSub on CLI tests should test with a Daemon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There were actually two instances, one spawned as a daemon and one running in process, created by daemon-factory
and instance-factory
respectively.
5d81bcb
to
fb3590b
Compare
ipfsd-ctl 0.27 has been published. Could you update the PR? |
fb3590b
to
8e63934
Compare
depends on - ipfs-inactive/interface-js-ipfs-core#186 |
@dryajov CI is failing. |
depends on ipfs-inactive/interface-js-ipfs-core#203 |
requires - ipfs/js-ipfsd-ctl#184 |
@dryajov I believe that all dependencies have been merged and released. What's missing here? |
@diasdavid there is one outstanding issue i'm troubleshooting now, which so far seems to be a race condition somewhere in libp2p. It also only seem to happen in the browser. The message says that libp2p is not started, but the tests pass, which makes me think that something is racy when updating libp2p state. I'll keep digging. |
5d3d3f4
to
084c12e
Compare
@diasdavid all good, the libp2p issue is fixed, circle and Travis are green. |
], done) | ||
}) | ||
}) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dryajov why delete the core circuit relay tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from ICR:
10:37 <@daviddias> dryajov: why delete the circuit relay tests?
10:38 moved to interop
10:39 they’re all there, but having them in js-ipfs in the first place was a workaround to get at least some of them running in the browser
10:39 now that we have the new ipfsd-ctl, thats not required anymore
10:40 I think we might need some around enabling/disabling circuit, but as far as those specific tests, they’re not part of interop
10:40 > they’re not part of interop
10:40 *now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We talked about this in IRC and agreed that we need Circuit relay tests in js-ipfs to test the interface, but they will be brought back with #1063
No description provided.