Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature async-describe -- Analysis of required design & usage changes #4641

Closed
craigphicks opened this issue May 29, 2021 · 3 comments
Closed
Labels
duplicate been there, done that, got the t-shirt... type: feature enhancement proposal

Comments

@craigphicks
Copy link

craigphicks commented May 29, 2021

Is your feature request related to a problem or a nice-to-have?? Please describe.
Async-describe seems to already be a requested feature (see Describe block with async function behaving weirdly #2975).
This post is a discussion of what interface changes would be needed and how usage would change.

Feature async-describe -- Analysis of required design & usage changes

It is understandable that async-describe doesn't currently work because there is no explicit reference to a parent describe object passed to an it object being constructed. I assume the it object constructor accesses a mocha global current-describe variable maintained only in the context of the main fiber (although I am not sure exactly how that is done).

A design change to allow async-describe would require altering interfaces to explicitly pass the intended parent describe object, as well as a syncronization mechanism, e.g.,

const parentd = mocha.topd()
describe('description', parentd, async function(parentdx){
  ....
  await ... //resource async preproc
  it('test', parentdx, async function(parentdxx){ ... })
  ...
  describe('subdescribe', parentdx, async function(parentdxx){ ... })
  ...
  await parentdx.sync()  // optionally wait until node parentdx has no tasks waiting or executing   
  ...
  await ... //resource async postproc
  ....
}
...

describe('description', parentd, async function(parentdx){...}
...
await parentd.sync()  // optionally  wait until node parentd has no tasks waiting or executing
...   

Notice the different spellings for parentd, parentdx and parentdxx to make clear here that they are not the same object. They needn't actually be different spellings because their scopes are different.

The "optional" await parentdx.sync() should be judiciously placed by the user to prevent too many resources being allocated. It would also be convenient for each describe/it to return a promise so that

describe('description', parentd, async function(parentdx){...}
await parentd.sync()  // optionally  wait until node parentd has no tasks waiting or executing
describe('description', parentd, async function(parentdx){...}
await parentd.sync()  // optionally  wait until node parentd has no tasks waiting or executing

could be reduced to

await describe('description', parentd, async function(parentdx){...}
await describe('description', parentd, async function(parentdx){...}

Mocha doesn't know about resources, so only the user can decide that. Having more describes in play would allow more its to be psuedo-concurrently running in the pipeline (taking advantage of IO waits, etc.), but each describe may require some resources which has a memory cost - some balance is required.

However, even if await parentd.sync() or await describe... were never used,
the mocha back end would still have a describe-tree with it-leaves that it could walk to get the proper reporting order even though the temporal order of starting each describe-node/it-leaf were not the proper reporting order.

The mocha back end would still have the independent ability (as I presume it does now) to limit/throttle the total number of it leaves executing concurrently in a (hidden to the user) mocha global it task pipeline

Back compat

I think that by keying on the arguments passed to describe/it that the async-describe feature could be back compatible with existing usage - same source, although using both async and non-async together might be tricky if it were actually desired (it is not expected to be desired).

Perhaps new new function names, e.g., adescribe and ait, could be used if they are going to return promises, but it is rather ugly.

@craigphicks craigphicks added the type: feature enhancement proposal label May 29, 2021
@juergba juergba added the duplicate been there, done that, got the t-shirt... label May 29, 2021
@juergba
Copy link
Member

juergba commented May 29, 2021

@craigphicks thank you for this issue. There are already similar issues about this topic, see eg. #2116.
Indeed it would be a very use- and powerful feature.

I disagree with your analysis though. Mocha executes several runtime cycles, and the describe block runs in a separate cycle before the runner starts with its tests/hooks. You can find a cycle overview in our docs.

@craigphicks
Copy link
Author

craigphicks commented May 29, 2021

@juergba Thanks for your timely reply.

I read your referenced post and the cycle doc, however I'm not sure what you disagree with - I can't see the conflict. (Obviously my suggestion is not the existing implementation but a modified version thereof.)

I do understand the current implementation already use a tree, said tree implicitly defined by order the temporal order the nodes are read in. But it seems to me that the user-intended-order and that temporal-readin-order may diverge with async functions being passed to describe. The reason I suggested passing a tree-node (parent-suite) parent parameter to async describe and it , e.g.

describe('desc',parentSuite, async function(parentSuite){...})

is so that the user-intended-order can still be maintained. Do you think that is possible (in the general case) without passing a parent-node parameter?

I appreciate your willingness to engage fully - but if you don't have the time I understand - feel free to not reply.

@craigphicks
Copy link
Author

craigphicks commented May 29, 2021

Continuing with back compatibility, I think the cleanest approach would be conventional importing

import {describe,it,rootsuite} from 'mocha/async'
....

and over ride the existing global describe and it rather than keying by args. There is no real need to mix non-async and async usage. before* and after* would no longer be required.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate been there, done that, got the t-shirt... type: feature enhancement proposal
Projects
None yet
Development

No branches or pull requests

2 participants