[DD-140] Introduce one more abstraction layer on top of helpers with mocha logic (hooks, timeouts) #85
Conversation
| @@ -32,6 +32,8 @@ async function spawnAndPromisifyIpfs() { | |||
|
|
|||
| ipfsd.stop = util.promisify(ipfsd.stop).bind(ipfsd); | |||
| ipfsd.cleanup = util.promisify(ipfsd.cleanup).bind(ipfsd); | |||
| ipfsd.api.clean = ipfsd.cleanup; | |||
| ipfsd.api.remove = ipfsd.stop; | |||
There was a problem hiding this comment.
I think we should review IPFS helper to follow the same interface as the other ones. Probably we should have an IPFSInstance.
Should we create an issue for this?
There was a problem hiding this comment.
Don't like the idea to mutate API object. Let's use a plain object for instance temporary?
return {
getApi() {
return ipfsd.api;
},
remove: ipfsd.stop,
clean: ipfsd.cleanup,
};There was a problem hiding this comment.
Yeah, I also prefer this approach. I didn't follow it at first because I don't want to modify the clients since we are returning ipfsd.api.
| clean: async function clean() { | ||
| await callInParallel(instance, 'clean'); | ||
| }, | ||
| remove: async function clean() { |
There was a problem hiding this comment.
This one needs a review too. We should follow the same interface (clean, remove) for all the startServiceInstance.
Right now this works, so, I'd say create a new issue.
| @@ -32,6 +32,8 @@ async function spawnAndPromisifyIpfs() { | |||
|
|
|||
| ipfsd.stop = util.promisify(ipfsd.stop).bind(ipfsd); | |||
| ipfsd.cleanup = util.promisify(ipfsd.cleanup).bind(ipfsd); | |||
| ipfsd.api.clean = ipfsd.cleanup; | |||
| ipfsd.api.remove = ipfsd.stop; | |||
There was a problem hiding this comment.
Don't like the idea to mutate API object. Let's use a plain object for instance temporary?
return {
getApi() {
return ipfsd.api;
},
remove: ipfsd.stop,
clean: ipfsd.cleanup,
};| * @returns {Promise<DockerInstance>} | ||
| */ | ||
| async clean() { | ||
| return this; |
There was a problem hiding this comment.
Do you want to support chained interface for all class methods or just for this? Do we really need it?
There was a problem hiding this comment.
Well, in reality, we just want a clean method for all DockerInstance. It could be an empty method to avoid breaking startHelperWithMochaHooksFactory.
There was a problem hiding this comment.
Yeah... let's keep it empty for a while.
| before(async function before() { | ||
| this.timeout(25000); | ||
| const ipfsApi = await startIPFSInstance(); | ||
| startIPFSInstance().then((ipfsApi) => { |
There was a problem hiding this comment.
describe('StateTransitionHeader', async () => {
...
const ipfsApi = await startIPFSInstance();
const addSTPacket = addSTPacketFactory(ipfsApi);There was a problem hiding this comment.
As far as I know, describe does not support async/await.
There was a problem hiding this comment.
:(( Promise-based syntax here looks ugly. Could we call it in before? I think we can call before in before and it should work without downsides.
There was a problem hiding this comment.
That doesn't work either. Keep in mind that our solution has some trade-offs. Dealing with timeouts and hooks outside test suite make the code less explicit.
There was a problem hiding this comment.
Yeah, ok. Let's keep this as is.
| @@ -0,0 +1,7 @@ | |||
| /* eslint-disable global-require */ | |||
| describe('mocha', () => { | |||
|
|
||
| describe('Three instances', () => { | ||
| let instances; | ||
| startDashCoreInstance.many(3).then((_instances) => { |
https://dashpay.atlassian.net/browse/DD-140