feat: support `concurrent` in Jest Each by Mark1626 · Pull Request #9326 · jestjs/jest

Mark1626 · 2019-12-18T15:40:29Z

Summary

Fixes #8985.

Supported .concurrent in test.each

Open Points need to discuss

Is this test.each.concurrent.only valid?
Is this test.concurrent.each valid?
There does not seem to be tests for concurrent both in jest-jasmine and jest-circus, tests in jest-circus are using execa for running the tests I can't seem to run a test which has test.concurrent

Notes on implementation

Have updated types in Global.ts to accommodate each.concurrent
Have created an additional method in jest-each bindConcurrent
Currently there is code duplication in jest-each, will remove it raising as draft PR for now to get clarifications
Have used bindConcurrent in jest-jasmine and jest-circus

Example

test.each.concurrent([1,2,3])('Name', (num)=> {
  // This function is executed concurrently with different `num`!
});

Test plan

Have added tests in jest-each, jest-jasmine, as for jest-circus I've added an open point regarding this

\ cc @SimenB

codecov-io · 2019-12-18T15:54:54Z

Codecov Report

Merging #9326 into master will decrease coverage by 0.02%.
The diff coverage is 56.52%.

@@            Coverage Diff             @@
##           master    #9326      +/-   ##
==========================================
- Coverage   64.77%   64.74%   -0.03%     
==========================================
  Files         280      280              
  Lines       11987    12002      +15     
  Branches     2956     2957       +1     
==========================================
+ Hits         7764     7771       +7     
- Misses       3591     3599       +8     
  Partials      632      632

Impacted Files	Coverage Δ
packages/jest-jasmine2/src/each.ts	`0% <0%> (ø)`	⬆️
...us/src/legacy-code-todo-rewrite/jestAdapterInit.ts	`0% <0%> (ø)`	⬆️
packages/jest-jasmine2/src/jasmineAsyncInstall.ts	`0% <0%> (ø)`	⬆️
packages/jest-each/src/index.ts	`100% <100%> (ø)`	⬆️
packages/jest-each/src/bind.ts	`100% <100%> (ø)`	⬆️

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 7f69176...2134ef4. Read the comment docs.

SimenB

Sorry about the slow response, I've been away for the holidays.

This looks great! Missing changelog, docs and integration tests, but I'm sure you're aware of that already 🙂

Only thing is that I wonder if test.concurrent.each makes more sense than test.each.concurrent?

e.g.

test.concurrent.each`
  thing  | otherThing
  ${'1'} | ${'2'}
`('%s %s')(({ thing, otherThing }) => {
  expect(thing).not.toEqual(otherThing);
});

It would look weird to add the concurrent modifier at the end here, methinks, like so

test.each`
  thing  | otherThing
  ${'1'} | ${'2'}
`('%s %s').concurrent(({ thing, otherThing }) => {
  expect(thing).not.toEqual(otherThing);
});

Is this test.each.concurrent.only valid?

Should be, yes

Is this test.concurrent.each valid?

As mentioned, I think that's the way it should be written.

There does not seem to be tests for concurrent both in jest-jasmine and jest-circus, tests in jest-circus are using execa for running the tests I can't seem to run a test which has test.concurrent

Yeah, .concurrent isn't really well tested. I'd add an integration test showing that the tests run/skip as expected. In https://github.com/facebook/jest/blob/9419034d6b575fe2c157453fe8b7d2000be66aad/e2e/__tests__/jasmineAsync.test.ts seems sensible (although maybe the jasmine part of the name is a bit misleading)

packages/jest-each/src/bind.ts

Mark1626 · 2020-01-07T17:34:43Z

@SimenB Hope you had a good vacation.

Is this test.each.concurrent.only valid?

Should be, yes

The current behavior right now in both jest-jasmine and jest-circus

test.concurrent.only - Supported
test.concurrent.skip - Supported

test.each.only - Not Supported
test.each.skip - Not Supported

Is there a reason each in jasmine and circus doesn't currently support only and skip?
jest-each seems to support them https://github.com/facebook/jest/tree/master/packages/jest-each#testonlyname-fn
Should implement each and only for test.each.concurrent?

I'll check out the test and add it

SimenB · 2020-01-07T23:38:15Z

@SimenB Hope you had a good vacation.

I had a wonderful vacation, thank you!

Is there a reason each in jasmine and circus doesn't currently support only and skip?

Not as far as I know, that seems like an oversight. We can address that in a separate PR though - if they're no supported now there's not need to add support for each either.

mattphillips · 2020-01-10T13:48:10Z

Hey all, I've just taken a look over this and the associated issue - I'm in agreement with @SimenB that the API should be:

test.concurrent.each

This is inline with what we have for test.only.each/test.skip.each and other APIs.

In addition to test.concurrent.each I think we want to add test.concurrent.only.each and test.concurrent.skip.each as the singular versions of these exist on test.concurrent already.

As for the code changes, I don't think we should duplicate bind for concurrent - I think we could update EachTestFn type to return some type parameter.

I've knocked up a proof of concept with the types here: #9390

Let me know your thoughts and happy to answer any questions 😄

Mark1626 · 2020-01-12T04:10:21Z

I was hacking around with this signature to have a single bind function, seem to be getting issues elsewhere.

export default function bind<
  T extends GlobalCallback | GlobalCallbackConcurrent
>(cb: T, supportsDone: boolean = true) {
  return (
    table: Global.EachTable,
    ...taggedTemplateData: Global.TemplateData
  ) =>
    function eachBind(
      title: string,
      test: T extends GlobalCallbackConcurrent
        ? Global.ConcurrentEachTestFn
        : Global.EachTestFn,
      timeout?: number,
    ): void {};
}

I'll explore @mattphillips 's approach

e2e/__tests__/jasmineAsync.test.ts

Mark1626 · 2020-01-12T10:04:56Z

packages/jest-jasmine2/src/jasmineAsyncInstall.ts

    return originalFn.call(env, specName, () => promise, timeout);
  };
+
+  // @ts-ignore


I'm not sure about this line.
The return of this function seems to be ItConcurrentBase and needs an each function to be present.
We bind the each after calling jasmineAsyncInstall, so I added the ts-ignore.
Or would an empty each function be better than ts-ignore

Maybe the each install should happen in here? cc/ @SimenB

(Not to familiar with this section of the codebase)

e2e/jasmine-async/__tests__/concurrent.test.js

mattphillips · 2020-01-15T18:38:32Z

packages/jest-each/src/__tests__/array.test.ts

 describe('jest-each', () => {
  [
    ['test'],
+    ['test', 'concurrent'],


Probably want to add test cases for

test.concurrent.skip and test.concurrent.only

An example on how to test skip would be great

@Mark1626 these comments are addressed, right? The tests on line 45 and 46

mattphillips · 2020-01-15T18:39:03Z

packages/jest-each/src/__tests__/template.test.ts

 describe('jest-each', () => {
  [
    ['test'],
+    ['test', 'concurrent'],


Same as above comment

Mark1626 · 2020-02-03T14:46:16Z

@mattphillips @SimenB Tests inside the each are not counted.

test.each(
  [
    [1, 1, 2],
    [1, 2, 3],
    [2, 1, 3]
  ],
  "returns the result of adding %d to %d",
  async (a, b, expected) => {
    expect(a + b).toBe(expected);
  }
);

A simple test file with this test results in Your test suite must contain at least one test.
~~Is this an expected behaviour?~~ Filing this as an issue

jimmycallin · 2020-05-14T20:23:43Z

Hi! Would very much like this PR to be merged :) What's the current status?

Mark1626 · 2020-05-18T06:30:27Z

@jimmycallin Got busy a bit with work I'll continue the work

philefstat · 2020-07-07T21:57:26Z

This is great! 🙌 Thank you

Mark1626 · 2020-07-19T18:01:48Z

I had to squash the commits as rebase was a major nightmare.

Summing up so far:

Currently supported

`jest-each`

each.test.concurrent
each.test.concurrent.only
each.test.concurrent.skip

jest-jasmine

it.concurrent.each
it.concurrent.only.each
it.concurrent.skip.each

jest-circus

it.concurrent.each
it.concurrent.only.each
it.concurrent.skip.each

Tests

jest-each has tests within the package itself
jasmine and circus have e2e

Documentation

Haven't touched it, will start when things are finalized

Note: currently circus does not have test for concurrent, have created a new e2e for concurrent.each

cc: @mattphillips @SimenB

Mark1626 · 2020-08-02T15:00:26Z

Sorry, but bump

SimenB

Thanks! I believe this is pretty much ready to go, pending some doc updates with the new APIs.

@mattphillips wanna look over again?

SimenB · 2020-08-02T17:42:30Z

packages/jest-each/src/bind.ts

-  table: Global.EachTable,
-  ...taggedTemplateData: Global.TemplateData
-) =>
+export default <A extends Global.TestCallback>(


What is A? Can you give it a more descriptive name?

Renamed it to EachCallback

CHANGELOG.md

mattphillips

Looks good to me, GJ!

- Change generic to meaningful name - Update CHANGELOG

SimenB · 2020-08-05T15:34:16Z

docs/GlobalAPI.md


 Even though the call to `test` will return right away, the test doesn't complete until the promise resolves as well.

+### `test.concurrent(name, fn, timeout)`


I had forgotten this is undocumented... Can you add something like

Note: test.concurrent is considered experimental - see https://github.com/facebook/jest/labels/Area%3A%20Concurrent for details on missing features and other issues

SimenB · 2020-08-05T15:35:11Z

Ignore the leak failures on node 14, it's a regression in node: nodejs/node#34636

Mark1626 · 2020-08-05T15:38:17Z

packages/jest-jasmine2/src/jasmineAsyncInstall.ts


    return spec;
  };
+  concurrentFn.each = () => {};


In jest-jasmine2 each is binded after the function is made concurrent, so I had it as a no-op as it will eventually be added. In the previous iterations I had to do a ts-ignore. I'm open to change it

a code comment explaining why makes sense to me

SimenB · 2020-08-06T11:56:44Z

Thanks @Mark1626!

jimmycallin · 2020-08-06T12:24:58Z

Yay! Thanks @Mark1626!

Mark1626 · 2020-08-06T12:34:22Z

@SimenB @mattphillips Thanks for the support and opportunity

nickhodaly · 2020-08-07T23:20:15Z

@SimenB Any idea on when this might be released?

SimenB · 2020-08-10T11:41:43Z

Right now https://github.com/facebook/jest/releases/tag/v26.3.0

nickhodaly · 2020-08-11T18:31:48Z

Awesome! Thanks @SimenB and @Mark1626. Going to try this out now.

nickhodaly · 2020-08-20T18:15:03Z

@SimenB @Mark1626 I have been testing out test.concurrent.each and have noticed some odd behavior, primarily when tests are failing. It seems that when I have multiple test failures in a data-driven concurrent run, the error reporting is no longer accurate. Results from a given failed test seem to be applied to other tests which actually should be passing, but instead are marked as failed with stacktraces and results that are identical to other real failed tests. In addition to this, the # of failed tests seems to fluctuate under these conditions as the same test may be reported as failed 2/3 times.

Another issue I noticed (not sure if it's related), once the run is complete and the report is printed to the console, the elapsed time (in ms) that typically show up like so ✓ Test #4 (176 ms) are missing from multiple tests, so it will instead appear as ✓ Test #4

None of this behavior is reproducible when simply switching to test.each.

Mark1626 · 2020-08-21T11:25:53Z

@nickhodaly I'll check what's the problem, do you have a repl/repo I can use to recreate this

nickhodaly · 2020-08-21T22:59:45Z

@nickhodaly I'll check what's the problem, do you have a repl/repo I can use to recreate this

Before you waste your time looking into let me double check I am not making some newbie JS related mistake. First time using JS so still grasping async/await. I will let you know what I find soon. Really appreciate your quick response though. @Mark1626

github-actions · 2021-05-11T04:06:54Z

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 Dec 18, 2019

jeysal requested review from SimenB and mattphillips December 18, 2019 15:41

SimenB requested changes Jan 4, 2020

View reviewed changes

packages/jest-each/src/bind.ts Outdated Show resolved Hide resolved

mattphillips mentioned this pull request Jan 10, 2020

Add concurrent mode capability to jest-each types #9390

Closed

Mark1626 force-pushed the concurrent-each branch from d0981c8 to 2f9de45 Compare January 12, 2020 09:54

Mark1626 commented Jan 12, 2020

View reviewed changes

e2e/__tests__/jasmineAsync.test.ts Show resolved Hide resolved

Mark1626 commented Jan 12, 2020

View reviewed changes

mattphillips reviewed Jan 15, 2020

View reviewed changes

e2e/jasmine-async/__tests__/concurrent.test.js Outdated Show resolved Hide resolved

mattphillips reviewed Jan 15, 2020

View reviewed changes

Mark1626 mentioned this pull request Feb 3, 2020

Tests marked with each are not added in total count #9506

Closed

Mark1626 force-pushed the concurrent-each branch from 2134ef4 to 2c65b21 Compare July 19, 2020 17:41

Mark1626 marked this pull request as ready for review July 19, 2020 17:41

SimenB approved these changes Aug 2, 2020

View reviewed changes

mattphillips approved these changes Aug 3, 2020

View reviewed changes

Mark1626 added 3 commits August 4, 2020 07:40

feat: Support concurrent in each

8d19287

test: Add e2e for jest-circus

6522d27

Fix typing

2fc2b8b

Mark1626 added 2 commits August 5, 2020 19:17

Review changes

4fdb1c7

- Change generic to meaningful name - Update CHANGELOG

Add API documentation in docs and jest-each

0af6632

Mark1626 force-pushed the concurrent-each branch from 47fc1c8 to 0af6632 Compare August 5, 2020 14:35

Mark1626 added 3 commits August 5, 2020 20:18

Review changes, rename generic to meaningful name

528af5b

Update documentation

b69973f

Lint Documentation

67ce352

SimenB reviewed Aug 5, 2020

View reviewed changes

Mark1626 commented Aug 5, 2020

View reviewed changes

Mark1626 added 2 commits August 5, 2020 21:10

Add note on test.concurrent experimental status

3a4f087

add comment about noop

1028e7b

SimenB changed the title ~~Support Concurrent in Jest Each~~ feat: support concurrent in Jest Each Aug 6, 2020

SimenB merged commit c95abca into jestjs:master Aug 6, 2020

Mark1626 deleted the concurrent-each branch August 7, 2020 13:43

scttcper mentioned this pull request Sep 1, 2020

.each is unusable with @jest/globals #10447

Closed

scttcper mentioned this pull request Sep 8, 2020

fix: test.each type always return a callable function #10488

Merged

This was referenced Dec 17, 2020

[Snyk] Security upgrade @jest/reporters from 25.5.1 to 26.4.1 meghasfdc/jest#44

Open

[Snyk] Security upgrade @jest/reporters from 25.5.1 to 26.4.1 tjenkinson/jest#55

Open

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


		Even though the call to `test` will return right away, the test doesn't complete until the promise resolves as well.

		### `test.concurrent(name, fn, timeout)`

Uh oh!

Conversation

Mark1626 commented Dec 18, 2019 • edited by SimenB Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Open Points need to discuss

Notes on implementation

Example

Test plan

Uh oh!

codecov-io commented Dec 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

SimenB left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Mark1626 commented Jan 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SimenB commented Jan 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattphillips commented Jan 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mark1626 commented Jan 12, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mark1626 commented Feb 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jimmycallin commented May 14, 2020

Uh oh!

Mark1626 commented May 18, 2020

Uh oh!

philefstat commented Jul 7, 2020

Uh oh!

Mark1626 commented Jul 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Currently supported

jest-each

jest-jasmine

jest-circus

Tests

Documentation

Uh oh!

Mark1626 commented Aug 2, 2020

Uh oh!

SimenB left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mattphillips left a comment

Choose a reason for hiding this comment

Uh oh!

SimenB Aug 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SimenB commented Aug 5, 2020

Mark1626 commented Dec 18, 2019 •

edited by SimenB

Loading

codecov-io commented Dec 18, 2019 •

edited

Loading

SimenB left a comment •

edited

Loading

Mark1626 commented Jan 7, 2020 •

edited

Loading

SimenB commented Jan 7, 2020 •

edited

Loading

mattphillips commented Jan 10, 2020 •

edited

Loading

Mark1626 commented Feb 3, 2020 •

edited

Loading

Mark1626 commented Jul 19, 2020 •

edited

Loading

`jest-each`

SimenB Aug 5, 2020 •

edited

Loading

nickhodaly commented Aug 20, 2020 •

edited

Loading