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: support concurrent in Jest Each #9326

Merged
merged 10 commits into from Aug 6, 2020
Merged

Conversation

Mark1626
Copy link
Contributor

@Mark1626 Mark1626 commented Dec 18, 2019

Summary

Fixes #8985.

Supported .concurrent in test.each

Open Points need to discuss

  1. Is this test.each.concurrent.only valid?
  2. Is this test.concurrent.each valid?
  3. 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
Copy link

codecov-io commented Dec 18, 2019

Codecov Report

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

Impacted file tree graph

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

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.

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 Outdated Show resolved Hide resolved
@Mark1626
Copy link
Contributor Author

Mark1626 commented Jan 7, 2020

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

SimenB commented Jan 7, 2020

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

mattphillips commented Jan 10, 2020

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

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

@@ -170,6 +174,9 @@ function makeConcurrent(

return originalFn.call(env, specName, () => promise, timeout);
};

// @ts-ignore
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'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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

(Not to familiar with this section of the codebase)

return globals;
};

describe('jest-each', () => {
[
['test'],
['test', 'concurrent'],
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably want to add test cases for

test.concurrent.skip and test.concurrent.only

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An example on how to test skip would be great

Copy link
Member

Choose a reason for hiding this comment

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

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

return globals;
};

describe('jest-each', () => {
[
['test'],
['test', 'concurrent'],
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above comment

@Mark1626
Copy link
Contributor Author

Mark1626 commented Feb 3, 2020

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

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

@Mark1626
Copy link
Contributor Author

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

@philefstat
Copy link

This is great! 🙌 Thank you

@Mark1626 Mark1626 marked this pull request as ready for review July 19, 2020 17:41
@Mark1626
Copy link
Contributor Author

Mark1626 commented Jul 19, 2020

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

Mark1626 commented Aug 2, 2020

Sorry, but bump

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! I believe this is pretty much ready to go, pending some doc updates with the new APIs.

@mattphillips wanna look over again?

table: Global.EachTable,
...taggedTemplateData: Global.TemplateData
) =>
export default <A extends Global.TestCallback>(
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed it to EachCallback

CHANGELOG.md Show resolved Hide resolved
Copy link
Contributor

@mattphillips mattphillips left a comment

Choose a reason for hiding this comment

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

Looks good to me, GJ!

- Change generic to meaningful name
- Update CHANGELOG
@@ -465,6 +465,170 @@ test('has lemon in it', () => {

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)`
Copy link
Member

@SimenB SimenB Aug 5, 2020

Choose a reason for hiding this comment

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

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

SimenB commented Aug 5, 2020

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

@@ -187,6 +191,8 @@ function makeConcurrent(

return spec;
};
concurrentFn.each = () => {};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

a code comment explaining why makes sense to me

@SimenB SimenB changed the title Support Concurrent in Jest Each feat: support concurrent in Jest Each Aug 6, 2020
@SimenB SimenB merged commit c95abca into jestjs:master Aug 6, 2020
@SimenB
Copy link
Member

SimenB commented Aug 6, 2020

Thanks @Mark1626!

@jimmycallin
Copy link

Yay! Thanks @Mark1626!

@Mark1626
Copy link
Contributor Author

Mark1626 commented Aug 6, 2020

@SimenB @mattphillips Thanks for the support and opportunity

@Mark1626 Mark1626 deleted the concurrent-each branch August 7, 2020 13:43
@nickhodaly
Copy link

@SimenB Any idea on when this might be released?

@SimenB
Copy link
Member

SimenB commented Aug 10, 2020

@nickhodaly
Copy link

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

@nickhodaly
Copy link

nickhodaly commented Aug 20, 2020

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

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

@nickhodaly
Copy link

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

concurrent test execution support for test.each
8 participants