Data driven tests

[mattphillips]

Summary

Add Data Driven Testing from jest-each as discussed in #6082

Test plan

I've added tests using the new bindings. I've only added one test for the skip and only bindings as otherwise they will result in the runner appearing to ignore multiple tests.

jest-each is also 100% unit tested under the hood too.

New API	jest-each equivalent
test.each	each.test
xtest.each	each.xtest
it.each	each.it
describe.each	each.describe
fit.each	each.fit
xdescribe.each	each.xdescribe
fdescribe.each	each.fdescribe
xit.each	each.xit
test.only.each	each.test.only
test.skip.each	each.skip.only
it.only.each	each.it.only
it.skip.each	each.it.skip
describe.only.each	each.describe.only
describe.skip.each	each.describe.skip

TODO

• Add docs to website

/cc @SimenB

[codecov-io]

Codecov Report

Merging #6102 into master will increase coverage by 0.14%.
The diff coverage is 95.23%.

@@            Coverage Diff             @@
##           master    #6102      +/-   ##
==========================================
+ Coverage   64.21%   64.36%   +0.14%     
==========================================
  Files         217      218       +1     
  Lines        8333     8393      +60     
  Branches        4        3       -1     
==========================================
+ Hits         5351     5402      +51     
- Misses       2981     2990       +9     
  Partials        1        1

Impacted Files	Coverage Δ
packages/jest-jasmine2/src/index.js	16.51% <0%> (+1.41%)	⬆️
packages/jest-jasmine2/src/each.js	85.96% <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 da60c78...9983616. Read the comment docs.

[SimenB]

This looks awesome, thanks so much for doing this!

@cpojer thoughts on the dependency over importing the source code?

If we do keep it as a dep, I think we should move it to jest-community.

I've added tests using the new bindings. I've only added one test for the skip and only bindings as otherwise they will result in the runner appearing to ignore multiple tests.

I think we should add some integrations tests, both to assert on the output, but also to see that skip, only and failures looks correct.

[cpojer]

There is only a finite set of these. It's more to type out, but can we be less smart and just manually inline this, like:

environment.global.test.each = …
environment.global.xtest.each = …

etc.?

If you look at line 67
https://github.com/facebook/jest/pull/6102/files#diff-e4399d1682ddb42f8a43f1a917d42d87R67

You'll also see that a lot of these are just aliases for each other, so you don't need to assign .each to all of them, just about half of them! :)

[mattphillips]

Good shout, I've just updated :)

[cpojer]

This is awesome. I'm happy for this to be a part of Jest but the code can be optimized a bit. Let's not be smart with string keys and iteration, we can just inline the things we want to do.

[cpojer]

If this is gonna be a core part of Jest, it's got to be inside of this repo, assuming @mattphillips is happy to contribute jest-each to this repo and update it to the standards of this repo.

[SimenB]

To easier roll this in long-lived jest releases, it would be awesome to release 1.0.0 before merging this 🙂

See Christoph's comment about the module living in this repo

[mattphillips]

My bad I misunderstood the comments in the issue I thought the idea was to import jest-each as a dep. I'm happy to merge it into core, I suspect by doing that jest-each's code can be simplified, when I made it I didn't want to pollute Jest's prototype, so jest-each acts as a proxy to existing APIs.

If it is part of core it will be fine to add these additional properties without the need to proxy too 😄.

What is the process for moving jest-each into core?

@SimenB @cpojer

[SimenB]

If you are fine with not supporting jest@<23 moving forward, I'd say move it into jest-jasmine2 and extend as needed by adding the properties directly.
If you want to keep supporting jest<23, then jest-each as is (pretty much, it needs to be typed using flow etc) should be made a separate package in this repo.

My only worry is that it will make jest-circus drift further from jest-jasmine2, but it shouldn't be too much of a bother fixing that later.

EDIT: One advantage of just moving it into jest-jasmine2 without making it a separate package is that you can keep maintaining jest-each as a package for older versions of jest. That might actually be the best solution, come to think of it 🙂

[mattphillips]

@SimenB Thats a good point about jest@<23 so maybe it is best to just bring it as is. I think if we encapsulate it in its own module namespace then hopefully jest-circus can just depend on it too and when the time comes it won't be too much work to migrate 🤞

I'm happy adding the flowtypes 😄

How do I add a new package to Jest's packages directory. Is it as simple as mkdir or is there a lerna specific command?

[SimenB]

You can do lerna import, but we don't really need the commit history. Just creating the directory should be fine.

But I added an edit to my comment, thoughts on that?

[mattphillips]

One advantage of just moving it into jest-jasmine2 without making it a separate package is that you can keep maintaining jest-each as a package for older versions of jest. That might actually be the best solution, come to think of it 🙂

I'm open to either solution 😄

I think it could lead to a situation where people raise issues in the wrong place potentially? Why is .each not working? in core and then we have to send them over to jest-each. Although that could be a moot point as we'd still just tell them to download jest-each even though its hosted in core 🤷‍♂️

Hmmm actually the biggest issue I see is keeping them in sync, bug fixes would generally apply to both the jest-jasmine2 implementation and jest-each.

I'm now leaning towards bring the existing lib into core 😆

[cpojer]

I think directly embedding into jest-jasmine2 makes sense. We'll deal with circus separately.

[SimenB]

do we need this when we have https://nodejs.org/api/util.html#util_util_format_format_args?

[mattphillips]

Nice I didn't know about util.format just updated :)

[SimenB]

instead of doing this, I wonder if it would be better to create actual integration tests (in https://github.com/facebook/jest/tree/master/integration-tests) and assert on the output there?

While this works, it seems sorta weird to invoke something passed to a mock function, and making asserts on how that behaves.

Might just be me, though!

[mattphillips]

I agree it would be nice to have an integration test too, I used TDD when building this thats why it seems strange but it allowed me to just write unit tests for the behaviour 😃

I'll add some tests in /integration-tests too (I didn't know about that directory)

[SimenB]

This should have the full license header, methinks.

/**
 * Copyright (c) 2014-present, Facebook, Inc. All rights reserved.
 *
 * This source code is licensed under the MIT license found in the
 * LICENSE file in the root directory of this source tree.
 *
 * @flow
 */

[SimenB]

not sure about that one, though...

[mattphillips]

Yeah I wasn't sure either - I can add it to be on the safe side

[mattphillips]

Done in 30ebfb5

[SimenB]

you'll also need to execute them (and assert on the output). See e.g. https://github.com/facebook/jest/blob/master/integration-tests/__tests__/failures.test.js

[mattphillips]

🤦‍♂️ I was wondering how they were ran 🤣

[mattphillips]

Added in 6607413

[mattphillips]

@SimenB @cpojer I think this is at a good point now, if you guys could take another look 😄

Only outstanding thing is to write up some docs for the website.

[SimenB]

Seems like one of the new tests are failing CI

[SimenB]

maybe tweak this a bit.

[SimenB]

I wonder if this message could be better (it should also have a stack pointing back to the user code.)

[SimenB]

maybe using pretty-format for the Received: true,true,true part, and explicitly saying "expected length 2, got 3" somehow

[mattphillips]

👍

[SimenB]

I added some suggestions in a second comment, but the error could contain the line that is wrong as well?

[SimenB]

is 4 correct? maybe not possible to make better

[mattphillips]

Yeah there is 8 tests, 4 with .only so 4 are skipped

Same for the .skip test but just inversed

[SimenB]

Do you need the split etc? It's a special case of failures

[mattphillips]

Nope removed

[SimenB]

it would be great if this was not needed. if it is, no biggie

[mattphillips]

I can't see why it wouldn't work on windows so I'll remove it and see what happens :)

[SimenB]

it'd be great to not trim, so the output in the snapshot matches stdout

[mattphillips]

Cool removed it

[SimenB]

This is really shaping up! I think it mainly just misses docs and perhaps some improvements for the error messages to help the user understand what failed. Tweaks like that can come later though, when people play around with it 🙂

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