jest-circus async tests #3819

aaronabramov · 2017-06-14T16:11:47Z

makes integration_tests/__tests__/jasmine_async-test.js pass.

AfterAll hook will not affect individual tests, but rather throw a global error that will fail the whole test file
BeforeAll failure results in one error being added to all dependent tests
Adds a test utility for testing jest-circus in isolation
timeouts can be passed to test/*Each/*All as a last argument
test.concurrent

aaronabramov · 2017-06-14T17:24:08Z

packages/jest-circus/src/legacy_code_todo_rewrite/jest-adapter-init.js

@@ -43,6 +43,32 @@ const initialize = ({
  global.fit = global.it.only;
  global.fdescribe = global.describe.only;

+  global.test.concurrent = (


i'm not including test.concurrent into jest-circus as a core functionality. In the current state it's never going to work properly (can't use certain matchers/hooks/proper .skip/.only/etc).
I also think we shouldn't document it either. It will be harder to reimplement it properly in the future if people start using it

we could actually make a breaking change to test.concurrent to pass the context as first argument (rather than "done") like we were planning to do, that way we could make it work properly with state.

there's too many changes that need to be made before we can do that.
it's easier to keep it as it is right now and we'll focus on redesigning it later

aaronabramov · 2017-06-14T17:25:05Z

packages/jest-circus/types.js

    |};

-export type TestStatus = 'pass' | 'fail' | 'skip';
+export type TestStatus = 'skip' | 'done';


i removed fail status, since it's a derived value that can be calculated from test.errors.length.
All we need to know is whether it was skipped or finished running

how about 'skip' | 'complete' then? Any other words instead of 'done'?

'finish' maybe?

cpojer · 2017-06-20T19:47:09Z

integration_tests/jasmine_async/__tests__/promise_beforeAll-test.js

    // failing before hook
    beforeAll(() => {
      return new Promise(resolve => setTimeout(resolve, 100));
-    }, 10);
+    }, 11);


why is it 11 now?

easier to find in the console :)

cpojer · 2017-06-20T19:48:49Z

packages/jest-circus/src/__mocks__/test_event_handler.js

+
+'use strict';
+
+const testEventHandler = (event, state) => {


This is really nice, I love how you are testing this. Could we make it a bit more generic somehow so that we don't have to keep this file in sync, though?

cpojer · 2017-06-20T19:49:29Z

packages/jest-circus/src/__mocks__/test_utils.js

+const TEST_EVENT_HANDLER_PATH = path.resolve(__dirname, './test_event_handler');
+
+const runTest = (source: string) => {
+  const tmpFilename = path.join(os.tmpdir(), 'circus-test-file.js');


Why do we write this into tmp?

this is the safest, but not the most pleasant way of writing files.
I was thinking about creating a gitignored directory (files_for_integration_tests) and write everything there, so it's easy to cd into the dir and see what's up in there

cpojer · 2017-06-20T19:52:32Z

packages/jest-circus/src/eventHandler.js

-          `"currentDescribeBlock" has to be there since we're finishing its definition.`,
-        );
-      }
+      invariant(currentDescribeBlock, `currentDescribeBlock mest to be there`);


mest? What?

i think it's something in between jest, must and has. i can't spell :'(

cpojer · 2017-06-20T19:52:54Z

packages/jest-circus/src/eventHandler.js

      if (currentDescribeBlock.parent) {
        state.currentDescribeBlock = currentDescribeBlock.parent;
      }
      break;
    }
    case 'add_hook': {
      const {currentDescribeBlock} = state;
-      const {fn, hookType: type} = event;
-      currentDescribeBlock.hooks.push({fn, type});
+      const {fn, hookType: type, timeout} = event;


I'm not sure I like the aliasing. Why do we need to rename hookType to type?

cpojer · 2017-06-20T20:03:06Z

Looks good to me as long as it matches Jasmine except for afterAll. Not able to give the full 100% proper review because I'm technically on vacation, so would love it if @rogeliog or @thymikee could take a look too.

rogeliog

It looks really good overall, just some small comments

rogeliog · 2017-06-20T20:16:41Z

packages/jest-circus/src/__mocks__/test_utils.js

@@ -0,0 +1,75 @@
+/**


This is really nice!

rogeliog · 2017-06-20T20:24:18Z

packages/jest-circus/src/eventHandler.js

+      const {type} = hook;
+
+      if (type === 'beforeAll') {
+        invariant(describeBlock, 'always present for `*All` hooks');


I think we need to add a prefix to the message? Similar to the contentBlock invariant message

rogeliog · 2017-06-20T20:25:22Z

packages/jest-circus/src/eventHandler.js

+        // too complicated, so we'll consider them to be global.
+        state.unhandledErrors.push(error);
+      } else {
+        invariant(test, 'always present for `*Each` hooks');


I think we need to add a prefix to the message? Similar to the contentBlock invariant message

rogeliog · 2017-06-20T20:26:07Z

packages/jest-circus/src/eventHandler.js

      break;
    }
    case 'test_skip': {
      event.test.status = 'skip';
      break;
    }
-    case 'test_failure': {
-      event.test.status = 'fail';


Do we not need the fail status anymore?

rogeliog · 2017-06-20T20:28:10Z

packages/jest-circus/src/index.js


-const test = (testName: TestName, fn?: TestFn) =>
-  dispatch({fn, name: 'add_test', testName});
+const test = (testName: TestName, fn?: TestFn, timeout?: number) =>


NIT: line 37 uses timeout: ?number instead of timeout?: number

that's expected though! :)
in this function timeout is optional, and on line37, it's always passed, but can be null

github-actions · 2021-05-13T10:06:40Z

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.

facebook-github-bot added the CLA Signed ✔️ label Jun 14, 2017

aaronabramov force-pushed the circus-async branch from b629332 to cbd70ce Compare June 14, 2017 16:16

jest-cirtus async tests

09a8561

aaronabramov force-pushed the circus-async branch from cbd70ce to 09a8561 Compare June 14, 2017 16:17

aaronabramov commented Jun 14, 2017

View reviewed changes

cpojer changed the title ~~jest-cirtus async tests~~ jest-circus async tests Jun 20, 2017

cpojer reviewed Jun 20, 2017

View reviewed changes

cpojer approved these changes Jun 20, 2017

View reviewed changes

rogeliog approved these changes Jun 20, 2017

View reviewed changes

aaronabramov closed this Jul 20, 2017

dalongi mentioned this pull request Aug 11, 2017

Improved flow types for Jest v20 + Flow >= 0.39 flow-typed/flow-typed#1136

Merged

aaronabramov mentioned this pull request Aug 25, 2017

jest-circus feature parity with jest-jasmine #4362

Closed

11 tasks

github-actions bot locked as resolved and limited conversation to collaborators May 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

jest-circus async tests #3819

jest-circus async tests #3819

aaronabramov commented Jun 14, 2017

aaronabramov Jun 14, 2017

cpojer Jun 20, 2017

aaronabramov Jun 20, 2017

aaronabramov Jun 14, 2017

cpojer Jun 20, 2017

thymikee Jun 20, 2017

cpojer Jun 20, 2017

aaronabramov Jun 20, 2017

cpojer Jun 20, 2017

cpojer Jun 20, 2017

aaronabramov Jun 20, 2017

cpojer Jun 20, 2017

aaronabramov Jun 20, 2017

cpojer Jun 20, 2017

cpojer commented Jun 20, 2017

rogeliog left a comment

rogeliog Jun 20, 2017

rogeliog Jun 20, 2017

rogeliog Jun 20, 2017

rogeliog Jun 20, 2017

rogeliog Jun 20, 2017

aaronabramov Jun 20, 2017

rogeliog Jun 20, 2017

github-actions bot commented May 13, 2021

jest-circus async tests #3819

jest-circus async tests #3819

Conversation

aaronabramov commented Jun 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpojer commented Jun 20, 2017

rogeliog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 13, 2021