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

feat(circus): enable writing async test event handlers #9397

Merged
merged 15 commits into from Apr 8, 2020
Merged

feat(circus): enable writing async test event handlers #9397

merged 15 commits into from Apr 8, 2020

Conversation

noomorph
Copy link
Contributor

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();
});
  1. 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:

Screenshot from 2020-01-10 18-28-42

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.

@codecov-io

This comment has been minimized.

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

packages/jest-circus/src/run.ts Show resolved Hide resolved
packages/jest-circus/src/run.ts Outdated Show resolved Hide resolved

export type Event =
export type Event = SyncEvent | AsyncEvent;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not make all events async?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>:

globalErrorHandlers

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

Screenshot from 2020-01-13 13-33-48

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):

promise-then
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;
  }
  // ...
  1. 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.

  2. 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.

Copy link
Member

@SimenB SimenB Feb 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🙂)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -31,9 +31,17 @@ export type Hook = {
timeout: number | undefined | null;
};

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>);

Copy link
Contributor Author

@noomorph noomorph Jan 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Screenshot from 2020-01-13 14-15-04

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
Copy link
Contributor Author

@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.

@noomorph noomorph requested a review from SimenB January 15, 2020 08:31
@rotemmiz
Copy link

👋 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
Copy link
Member

SimenB commented Jan 20, 2020

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
Copy link

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

Copy link
Member

@rickhanlonii rickhanlonii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dig it

@rickhanlonii
Copy link
Member

cc @scotthovestadt for FB implications

@SimenB
Copy link
Member

SimenB commented Feb 5, 2020

🎉

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

@drew-gross

This comment has been minimized.

@codecov-io

This comment has been minimized.

@drew-gross

This comment has been minimized.

@noomorph
Copy link
Contributor Author

noomorph commented Mar 4, 2020

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

@thernstig
Copy link
Contributor

This is amazing! 🥇

@rotemmiz
Copy link

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

@codecov-io
Copy link

codecov-io commented Mar 25, 2020

Codecov Report

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

Impacted file tree graph

@@            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.

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

packages/jest-circus/README.md Show resolved Hide resolved
packages/jest-circus/src/__mocks__/testEventHandler.ts Outdated Show resolved Hide resolved
packages/jest-circus/src/eventHandler.ts Show resolved Hide resolved
e2e/__tests__/testEnvironmentCircus.test.ts Outdated Show resolved Hide resolved
e2e/test-environment-circus/testEnvironment.js Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
noomorph and others added 2 commits April 3, 2020 14:44
…ent.js

Co-Authored-By: Simen Bekkhus <sbekkhus91@gmail.com>
…ent.js

Co-Authored-By: Simen Bekkhus <sbekkhus91@gmail.com>
@noomorph
Copy link
Contributor Author

noomorph commented Apr 4, 2020

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.

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

noomorph commented Apr 6, 2020

@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
Copy link
Member

SimenB commented Apr 6, 2020

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 SimenB merged commit 222565a into jestjs:master Apr 8, 2020
@SimenB
Copy link
Member

SimenB commented Apr 8, 2020

Thanks for the great PR and patience @noomorph!

@SimenB
Copy link
Member

SimenB commented Apr 8, 2020

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

@noomorph
Copy link
Contributor Author

noomorph commented Apr 8, 2020

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

jeysal added a commit to mmkal/jest that referenced this pull request Apr 10, 2020
…pshots

* upstream/master: (225 commits)
  docs: add CLA link to contributing docs (jestjs#9789)
  chore: roll new version of docs
  v25.3.0
  chore: update changelog for release
  chore(jest-types): correct type testRegex for ProjectConfig (jestjs#9780)
  feat(circus): enable writing async test event handlers (jestjs#9397)
  feat: enable all babel syntax plugins (jestjs#9774)
  chore: add helper for getting Jest's config in e2e tests (jestjs#9770)
  feat: pass ESM options to transformers (jestjs#9597)
  chore: replace `any`s with `unknown`s (jestjs#9626)
  feat: pass ESM options to Babel (jestjs#9766)
  chore(website): add copy button the code blocks (jestjs#9750)
  chore: bump istanbul-reports for new uncovered lines design (jestjs#9758)
  chore: correct CHANGELOG.md (jestjs#9763)
  chore(jest-types): expose type `CacheKeyOptions` for `getCacheK… (jestjs#9762)
  docs: Fix simple typo, seperated -> separated (jestjs#9760)
  v25.2.7
  chore: update changelog for release
  fix: drop getters and setters when diffing objects for error (jestjs#9757)
  chore(jest-types): correct return type of shouldRunTestSuite fo… (jestjs#9753)
  ...
@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants