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

jest-each does not serialize values properly #6321

Closed
aaronabramov opened this issue May 27, 2018 · 11 comments · Fixed by #6357
Closed

jest-each does not serialize values properly #6321

aaronabramov opened this issue May 27, 2018 · 11 comments · Fixed by #6357

Comments

@aaronabramov
Copy link
Contributor

i was trying to migrate some of our tests to use jest-each, but for a lot of cases it won't work because of serialization that jest-each uses:
screen shot 2018-05-27 at 11 17 52 pm
screen shot 2018-05-27 at 11 17 09 pm

is it possible to use pretty-format for this serialization? (but collapsing everything in one line)

@SimenB
Copy link
Member

SimenB commented May 27, 2018

@mattphillips ^

@SimenB
Copy link
Member

SimenB commented May 27, 2018

Have you tried value: %i etc? It's just util.format: https://nodejs.org/api/util.html#util_util_format_format_args

@mattphillips
Copy link
Contributor

@aaronabramov @SimenB I was looking into this during the summit, I’ve got some code stashed locally to try to improve the behaviour. @SimenB perhaps vsprintf does offer slightly better serialisation than Nodes util format?

@aaronabramov
Copy link
Contributor Author

since the nature of this feature is feeding a bunch of different values into test, i don't think util.format would work great for this case :(

what if we just use generic placeholders like:

test.each([stuff, stuff, stuff])("it works with {}", => {});

also we can't serialize two values into the same string, because it'll result in two tests with the same name/id and their snapshots names will conflict.

for example this will have a lot of strings that are the same, but their values are different:

const a = {x: 1};
const b = {x: 1};

test.each([
  [a, {x: 1}], 
  [b, {x: 1}], 
  [a, b],
  [{o: a}, {o: b}]
  [{o: a}, {o: a}]
])('{} is not {}', (v1, v2) => {
  expect(v1 === v2).toBeFalse();
});

should we do something like:

<{x: 1}>#1 is not <{x: 1}>#2
<{x: 1}>#3 is not <{x: 1}>#4
<{x: 1}>#5 is not <{x: 1}>#6

if they serialize into the same string?

@aaronabramov
Copy link
Contributor Author

when i was writing tests for expect the hardest thing was actually serializing the values :)
i even had to use a special stringify function to make it work https://github.com/facebook/jest/blob/master/packages/expect/src/__tests__/matchers.test.js#L172

i think it'd be super nice if test.each just made it work without you thinking about how to make it print the right thing

@SimenB
Copy link
Member

SimenB commented May 29, 2018

Good point. I'd be down with JSON.stringify or something as the serialization, and just keep doing %s

@aaronabramov
Copy link
Contributor Author

JSON.stringify will blow up on the first circular reference :P
i think pretty-format is the best tool here

@mattphillips
Copy link
Contributor

I see this as being two separate issues:

  1. Serialising values into test titles

I'd be hesitant to add generic placeholders as printf is ubiquitous and people will recognise the pattern from other languages.

We could do JSON.stringify or pretty-format on %s but this would be a break in change as anyone using %d or whatever else would break.

I think I'd be open to maybe adding support to printf with %p for pretty-format?

@aaronabramov for your example you could use %o or %O to serialise all of the different data types, I think though that it is an edge case that your test will have different data types as parameterised data.

  1. Identical dynamic test titles

This seems like a valid problem with a normal test where multiple tests can have the same name. I'd say that we should solve that problem and it will also fix the dynamic test title seeing as test.each is just sugar for multiple tests.

@aaronabramov
Copy link
Contributor Author

i think %p for pretty-format is prefect!
and for similar test name, yeah we should probably fix it on jest's side and just blow up if something like this happens (or maybe handle the case in some smart way!)

@SimenB
Copy link
Member

SimenB commented May 29, 2018

There's a lint rule in the plugin for duplicate test names.

Should probably expand it to support yelling at dupes resulting from each

@github-actions
Copy link

This issue 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 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants