feat(circus): enable writing async test event handlers

[noomorph]

Summary

While jest-circus provides handleTestEvent(event, state): void interface, it is not helpful for some scenarios inherent to end-to-end testing, as in case of Detox.

I'll illustrate that by a couple of examples:

Decide the fate of a test artifact

Let's say we have those in our <setupFilesAfterEnv>.js file:

beforeEach(async () => {
  await startVideoRecording();
});

afterEach(async () => {
  await endVideoRecording();

  if (isTestPassing()) {
    await removeVideoRecordingFromDevice();
  } else {
    await pullVideoRecordingFromDeviceAndSave();
  }
});

The problem here, that inside afterEach hook, you can't know for sure whether the entire test is passing or failing, because you are still executing its part, albeit a mere and probably the last hook.

If we move our logic to handleTestEvent provided by jest-circus, then everything is fine...

class DetoxCircusEnvironment {
  async handleTestEvent(event) {
    if (event.name === 'test_start') {
      await startVideoRecording();
    }
    
    if (event.name === 'test_done') {
      await endVideoRecording();

      if (event.test.errors.length === 0) {
        await removeVideoRecordingFromDevice();
      } else {
        await pullVideoRecordingFromDeviceAndSave();
      }
    }
  }
}

... except for the fact that you can't simply make handleTestEvent asynchronous. 🙂

And this is what this pull request is trying to solve.

Take a screenshot right after an error happens

If you want to take a screenshot right after a failed expectation or any other error, theoretically, you could write something like this:

it('should fail somewhere', () => {
  try {
    await expect(element(by.id('unexisting'))).toBeVisible();
  } catch (e) {
    await device.takeScreenshot('failure');
    throw e;
  }
});

You could even write a wrapper for it, e.g. safeIt or monkey-patch it. But then, there are also beforeEach, afterEach, beforeAll and afterAll hooks, which can be renamed to safe... or monkey-patched as well. In theory, there are also test, test.each and some other functions, and one could take them into the account too.

But is it the right solution, when jest-circus provides the API for handling test failures?

class DetoxCircusEnvironment {
  async handleTestEvent(event) {
    if (event.name === 'test_fn_failure') {
      await takeScreenshot('testFailure');
    }

    if (event.name === 'hook_failure') {
      await takeScreenshot('${event.hook.type}Failure');
    }
  }
}

Again, here we hit the limitation that you can't make it actually await before proceeding.

That leaves us with a chance that another afterEach hook or next E2E test is going to run while our spawned child process is making a screenshot on iOS or Android. Having a race condition, even if a hypothetical one, is not something that we would want to have when speaking of consistent post-error screenshots. I would prefer to know for sure that we await for such artifact actions to be completed before proceeding any further.

Conclusion

Making handleTestEvent asynchronous will simplify and improve a few essential things in Detox: user setup and better post-test artifacts.

1. No need to call detox.beforeEach and detox.afterEach (or adapters) inside user code. Detox will take care of that itself via jest-circus. The setup part will be able to shrink significantly to:

jest.setTimeout(120000);

beforeAll(async () => {
  await detox.init(config);
}, 300000);

afterAll(async () => {
  await detox.cleanup();
});

2. Ubiquitous and consistent error handling as a test scenario goes, compare:

Current Jest Circus: https://jenkins-oss.wixpress.com/job/multi-detox-pr/1862/
Forked Jest Circus: https://jenkins-oss.wixpress.com/job/multi-detox-pr/1861/

In this build, I deliberately fail two tests, one in beforeEach hook, second — in it test function, and with my fork of jest-circus I become able to get distinct post-error screenshots for both those tests:

Test plan

1. I have tested the change with Detox e2e test suite, and superficially it works OK and I receive what I expect there. The test runner completes without any weird or suspicious messages, and the failures can be seen only in places where I expect them to be.

2. I have run yarn on the entire jest monorepo, and it has not yielded any errors.

Still, I think, I might need your guidance on how better to test this change. Your ideas are very welcome.

Looking forward to our collaboration,
Yaroslav.

[SimenB]

I like it! This also needs docs and tests. For tests, maybe just doing something async (e.g. storing current time, then waiting one second, asserting Date.now() is at least 1000 higher than before, and throwing if not, maybe with some console.logs just so we know the code triggered).

I'd love to hear any thoughts from @aaronabramov, @scotthovestadt and @DiZy on how this fits into their plans for circus.

[SimenB]

why not make all events async?

[noomorph]

That handful of sync events have a very specific context of happening:

1. error - called either inside NodeJS.UncaughtExceptionListener or NodeJS.UnhandledRejectionListener. Mind that both signatures are expected to return void, not a Promise<void>:

2. start_describe_definition and finish_describe_definition - they are called right as you call the global function describe(msg, fn):

You see, to make those dispatches asynchronous, I should make the global describe asynchronous first, and that seems risky to me, because:

a. We change the signature of the global describe function, which is too pretentious, IMO.
b. We push our users into getting those nasty errors from the linters that pop whenever you call an asynchronous function in the global scope (outside of an async function):

c. Last, but not least, errors thrown inside an asynchronous describe() won't have the same effect if they had been thrown in a synchronous one, right? Hence, it will break this logic, which is undesirable too:

  const asyncError = new ErrorWithStack(undefined, describeFn);
  if (blockFn === undefined) {
    asyncError.message = `Missing second argument. It must be a callback function.`;
    throw asyncError;
  }
  // ...

3. The same goes for add_hook (globals beforeEach, afterEach, ...) and add_test (globals it, test, test.todo, ...). I was not sure it was a good idea to change their signatures, so I decided to leave those events synchronous.

4. As for setup and include_test_location_in_result, it seems you are right. I am going to push a commit that makes them async and test it out.

[SimenB]

Note that we'll probably change these errors (hook, describe, test) to fail during test run, instead of during definition - see #9515. Only matters for 2.c and 3

I'm a bit afraid that an async handleTestEvent function won't behave correctly if we don't await all dispatches. Thoughts?

(as for 2.a, it's been a long standing issue that we can't have describe(async () => {}), but that should not change in this PR, I agree 🙂)

[noomorph]

I think this should be a very marginal case when somebody wants to do something really asynchronous for add_hook and add_test. It's sad we can't do everything at once here, but I see no easy way in the spirit of this PR to accomplish the all-events-async goal. I really hope we can ignore this handful of events here. I have written the warnings in the docs already.

[SimenB]

Suggested change

	export type EventHandler = (event: Event, state: State) => void | Promise<void>;
	export type EventHandler =
	| ((event: SyncEvent, state: State) => void)
	| ((event: AsyncEvent, state: State) => Promise<void>);

[noomorph]

@SimenB, the suggested signature does not work well enough, leading to errors here:

I think I understand your intention to make EventHandler type stricter, so hopefully, the version I have just committed is enough to your liking too:

export interface EventHandler {
  (event: AsyncEvent, state: State): void | Promise<void>;
  (event: SyncEvent, state: State): void;
}

I've left an ambiguous void | Promise<void> result for the AsyncEvent for the backward compatibility. See why:

If some project has already implemented an EventHandler for jest-circus returning void, IMO, this use case remains perfectly valid. We should not force people into rewriting the handlers necessarily to async functions, not to mention the radical distinction between sync and async events. Such a change would feel too breaking, to my taste.

The compromise void | Promise<void> still might affect the existing codebase of someone, but that is fixable by a light extra typing. For instance, in jest/packages/* codebase, after the change, I had to fix signatures in those two places:

diff --git a/packages/jest-circus/src/__mocks__/testEventHandler.ts b/packages/jest-circus/src/__mocks__/testEventHandler.ts
index 23dfe2bed..ad948f8d6 100644
--- a/packages/jest-circus/src/__mocks__/testEventHandler.ts
+++ b/packages/jest-circus/src/__mocks__/testEventHandler.ts
@@ -7,7 +7,10 @@
 
 import {Circus} from '@jest/types';
 
-const testEventHandler: Circus.EventHandler = (event, state) => {
+const testEventHandler: Circus.EventHandler = (
+  event: Circus.Event,
+  state: Circus.State,
+) => {
   switch (event.name) {
     case 'start_describe_definition':
     case 'finish_describe_definition': {
diff --git a/packages/jest-circus/src/eventHandler.ts b/packages/jest-circus/src/eventHandler.ts
index 7b059983d..b947bc7d9 100644
--- a/packages/jest-circus/src/eventHandler.ts
+++ b/packages/jest-circus/src/eventHandler.ts
@@ -21,7 +21,10 @@ import {
   restoreGlobalErrorHandlers,
 } from './globalErrorHandlers';
 
-const eventHandler: Circus.EventHandler = (event, state): void => {
+const eventHandler: Circus.EventHandler = (
+  event: Circus.Event,
+  state: Circus.State,
+): void => {

That way or another, I mean just to warn you that going away from the former signature of mine:

export type EventHandler = (event: Event, state: State) => void | Promise<void>;

most likely will lead to guaranteed TypeScript errors in projects that already implement some EventHandlers on top of jest-circus. Please advise if this is something critical for decision-making here.

[noomorph]

@SimenB, so, how do you think, will adding such kind of a test suffice or we need something more?

import {runTest} from '../__mocks__/testUtils';

test('test event handler should be able to be async and pause the flow', () => {
  const {stdout} = runTest(`
    describe('slowdown via testEventHandler', () => {
      let t0;
      beforeEach(() => { t0 = Date.now() });
      it(':slowdown(1000):', () => {});
      afterEach(() => expect(Date.now() - t0).toBeGreaterThan(1000));
    });

    describe('no slowdown', () => {
      let t0;
      beforeEach(() => { t0 = Date.now() });
      it('fast test', () => {});
      afterEach(() => expect(Date.now() - t0).toBeLessThan(100));
    });
  `);

  expect(stdout).toMatchSnapshot();
});

The idea is to add a couple of lines to __mocks__/testEventHandler.ts that make it sleep for extra 1000ms on test_fn_start if test.name === ':slowdown(1000):. This way we prove the hook can pause the flow due to its first-class asynchronousity.

[rotemmiz]

👋 Hey @SimenB, @jeysal, @thymikee,

We really love Jest Cirucs, and make it our default test runner internally for our Detox tests. We do want to make Detox artifact lifecycle better (for all the reasons @noomorph mentioned), and we need your help with this PR please.

[SimenB]

I've tried pinging som fb people about their opinion in this, without much luck. I find it appealing, but I also know FB has some pretty deep integrations with jest-circus internally, so I'm vary of breaking them by making such a fundamental change

[rotemmiz]

Hey again guys @SimenB, @jeysal, @thymikee, can you please help us push this forward?
Thanks!

[rickhanlonii]

I dig it

[rickhanlonii]

cc @scotthovestadt for FB implications

[SimenB]

🎉

I'd like to see a test or two plus documentation updates (look at #8344 for where to update docs) before landing this.

[noomorph]

@SimenB , how do you think are we ready now to get merged?
P.S. I have tests now, and they look stable enough.

[thernstig]

This is amazing! 🥇

[rotemmiz]

Hey guys, is there anything missing you want us to take care of? Or can this be merged?
Thanks!

[codecov-io]

Codecov Report

Merging #9397 into master will decrease coverage by 0.01%.
The diff coverage is 10.52%.

@@            Coverage Diff             @@
##           master    #9397      +/-   ##
==========================================
- Coverage   64.90%   64.88%   -0.02%     
==========================================
  Files         288      288              
  Lines       12195    12200       +5     
  Branches     3024     3022       -2     
==========================================
+ Hits         7915     7916       +1     
- Misses       3639     3643       +4     
  Partials      641      641              

Impacted Files	Coverage Δ
packages/jest-circus/src/globalErrorHandlers.ts	17.64% <0.00%> (ø)
...circus/src/legacy-code-todo-rewrite/jestAdapter.ts	0.00% <0.00%> (ø)
...us/src/legacy-code-todo-rewrite/jestAdapterInit.ts	0.00% <0.00%> (ø)
packages/jest-circus/src/run.ts	0.00% <0.00%> (ø)
packages/jest-circus/src/index.ts	68.05% <25.00%> (ø)
packages/jest-circus/src/state.ts	75.00% <50.00%> (-9.62%)	⬇️
packages/jest-circus/src/eventHandler.ts	7.05% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3249385...032fe72. Read the comment docs.

[SimenB]

Thanks for keeping this updated @noomorph, and sorry about the slow response!

I haven't really heard anything back from FB peeps on this one (except for Rick approving, which is promising 🙂), so I'm thinking this is good to go. Might wait for Jest 26 just to be safe though? Going from sync to async seems like a breaking change, but maybe not since we don't care if the callee is async, it's all on our side

/cc @cpojer @DiZy @scotthovestadt @jeysal @thymikee any objections to landing this?

[noomorph]

Okay, so, I hope I have addressed all the review comments at the moment.
@SimenB, let me know if I need to fix anything else here.

[SimenB]

Thanks, this looks great 👍

My only question now is if this would be considered a breaking change (meaning it should wait for Jest 26) or if it's safe to land.

Thoughts?

[noomorph]

@SimenB, I am not sure to whom the question is addressed, but from my side, it looks like an extension to the existing contract, which does not break anything that existed before.

The only caveat I personally see here is maybe some potential TypeScript errors (that explicit state, action typing) if somebody was using a custom environment. As we see these errors won't necessarily emerge, but we have witnessed one case like this.

[SimenB]

Right, that's my thinking as well. We're now supporting something we didn't support before, so it shouldn't break anyone.

Allright, then I'm thinking I'll merge and release this tomorrow unless somebody thinks we should not 🙂

(Last ping, I swear) @cpojer @DiZy @scotthovestadt

[SimenB]

Thanks for the great PR and patience @noomorph!

[SimenB]

@noomorph available in https://github.com/facebook/jest/releases/tag/v25.3.0, looking forward to seeing what you build with it. 🙂

[noomorph]

Thrilled to see the release, thanks much! Now the ball is in my court, right. Stay tuned :-)

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.