Provide test context to test.each()

[JesusTheHun]

Describe the bug

In the same vein as #4585, fixtures are unavailable when using test.each().

Reproduction

https://stackblitz.com/edit/vitest-dev-vitest-el13eq?file=test%2Fbasic.test.ts

System Info

System:
    OS: macOS 14.1.2
    CPU: (12) arm64 Apple M2 Max
    Memory: 29.42 GB / 64.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 18.16.1 - ~/.nvm/versions/node/v18.16.1/bin/node
    Yarn: 1.22.19 - ~/.nvm/versions/node/v18.16.1/bin/yarn
    npm: 9.5.1 - ~/.nvm/versions/node/v18.16.1/bin/npm
    pnpm: 7.29.1 - ~/.nvm/versions/node/v18.16.1/bin/pnpm
    bun: 1.0.14 - ~/.bun/bin/bun
  Browsers:
    Chrome: 119.0.6045.199
    Edge: 119.0.2151.97
    Safari: 17.1.2
  npmPackages:
    vitest: 1.0.0-beta.6 => 1.0.0-beta.6

Used Package Manager

npm

Validations

• Follow our Code of Conduct
• Read the Contributing Guidelines.
• Read the docs.
• Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
• Check that this is a concrete bug. For Q&A open a GitHub Discussion or join our Discord Chat Server.
• The provided reproduction is a minimal reproducible example of the bug.

[sheremet-va]

This is kind of expected. No context is passed down there at the moment.

What is your expectation? How do you expect to access context in test.each?

[JesusTheHun]

Yes, I expect each case to behave like a regular test.
Is there a particular reason it should not be able to access it ?

[sheremet-va]

How would you access it? Give me code example. In a regular test function the argument is the context, but in test.each the argument is whatever it is you passed down to each function.

[JesusTheHun]

It's on the stackblitz : just add a second argument.

const cases = [
  ['config0', 'expected0'],
  ['config1', 'expected1'],
];

myTest.each(cases)(
  'fixtures should be available as part of the second argument',
  ([config, expected], { myFixture }) => {
    expect(myFixture.doSomething(config)).toEqual(expected);
});

[sheremet-va]

It's on the stackblitz : just add a second argument.

This is not how it works. You control the second argument - with your example expected is the second argument

[JesusTheHun]

1.0.0 is still in beta right ? The API can still be changed. We agree this is a design flaw, right ?

[sheremet-va]

1.0.0 is still in beta right ? The API can still be changed. We agree this is a design flaw, right ?

What is a design flaw? Can you just give an example of what you expect without breaking changes? We definitely will not make it so the first argument in .each is an array item. This breaks compatibility with Jest and existing Vitest code without any gain.

[JesusTheHun]

The design flaw is that one cannot use test.each like any other test.
I don’t expect this to be possible without breaking changes, at least in the form I’ve written.
There is a gain, but I understand you don’t want to break the compatibility with Jest.

The alternate solution would then be to expose a static function that returns the fixtures of the current test.

edit: or add a new function other than « each », specific to vitest.

[sheremet-va]

The design flaw is that one cannot use test.each like any other test.

This is not a flaw, this is literally a design of this method - it's intentionally different. If you don't like it, you can always use forEach or for of and call a test function inside.

Your proposed solution does not benefit existing users. But what we can do is add a context as the last argument instead of the second, so it doesn't break existing code and makes sense with the proposal.

[JesusTheHun]

If you add the context as the last parameter it’s a breaking change because one could use a rest argument or simply iterate through the array.

Yes indeed, I could simply use a loop.
Do you know why ˋeach` was designed to be different ? For what purpose/benefit.

[sheremet-va]

If you add the context as the last parameter it’s a breaking change because one could use a rest argument or simply iterate through the array.

By that logic any usage of any API is a breaking change, so any new feature requires a major which to me is absurd.

Yes indeed, I could simply use a loop.
Do you know why ˋeach` was designed to be different ? For what purpose/benefit.

Well, it's in the name, is it? It needs to pass down data somehow. It was also designed before fixtures were introduced.

[JesusTheHun]

If you change the value of the argument received, it is a breaking change because it will break existing code.
A new feature cannot break existing code.

it was before the fixtures, ok.
In the cases expressed as objects, and therefore passed down as an object, we could inject fixtures in that object as non-enumerable properties. This way, it won’t break existing code.
We could actually do that for the array too now I think about it. It is dark magic though.

[JesusTheHun]

Now that I'm rested I see your point. You would only have the extra value in the array if you opt-in for fixtures, so it would not be a breaking change indeed.

[magnaar]

I guess it could work if you add an optional options parameter:

const cases = [
  ['config0', 'expected0'],
  ['config1', 'expected1'],
];

myTest.each(cases, { withFixtures: true })(
  'fixtures should be available as part of the second argument',
  ({ myFixture }, config, expected) => {
    expect(myFixture.doSomething(config)).toEqual(expected);
});

But yeah, even if the loop would do the trick for the moment, as a user, it seems quite off that test.each doesn't behave like a test with parameter. :/
(but we understand your issues to pass the fixtures to test.each)

[sheremet-va]

I guess it could work if you add an optional options parameter:

I do like this solution. I think we can just pass down true/false as the second argument. It doesn't break compatibility and it's quite easy to adopt. Maybe in the future, we can switch the flag to be false instead.

But I would make a small change - move fixtures to be the latest argument instead of the first one.

[JesusTheHun]

If the fixtures are the last parameter, then you cannot use a rest parameter for the values of the case.

[jakebailey]

#4963 was closed as a dupe of this one, as it also comes from the fact that the test context is not passed down into each (but one can write test.concurrent.each and it "work").

My suggestion there was that when .concurrent was used in the chain, that the each callback instead take an object like TestContext & { args: T } (where T are the args), however, I don't think this works for the fixture case as there's no static indicator that it needs the context (versus the chain could theoretically use some TS type trickery to make it happen).

Maybe the above can be done with that second parameter, like:

test.concurrent.each([
	// ...
])('someName %o', { args, expect } => {
	// ...
}, { withContext: true }); 

Additionally, can these cases be turned into runtime errors today? E.g. if concurrent, throw with a better error? Or if fixures in use, error? It's not clear that one can write a useful test today with .each, no?

(Also, if this is the place to collect "I need the context in test.each", should the title make that clear?)

[sheremet-va]

Additionally, can these cases be turned into runtime errors today? E.g. if concurrent, throw with a better error? Or if fixures in use, error? It's not clear that one can write a useful test today with .each, no?

I don't understand this request. Why can't you use them both? You don't have to use context/fixtures to run tests.

[jakebailey]

My thinking is that a test without expect is probably not a useful test, and you can't access expect for .concurrent without the context, so that situation is neccesarily not going to work correctly (and in my case, just causes spurious failures due to expect being called "outside" of a test).

[sheremet-va]

My thinking is that a test without expect is probably not a useful test, and you can't access expect for .concurrent without the context, so that situation is neccesarily not going to work correctly (and in my case, just causes spurious failures due to expect being called "outside" of a test).

You can (and should) use global expect in concurrent tests as a rule. The only exception is toMatchSnapshot because the name of the snapshot relies on the current test name.

We already throw an error for toMatchInlineSnapshot - we can throw toMatchSnapshot if it's called inside .each without using the local expect, but I don't see any reason to throw an error for .concurrent.each.

[jakebailey]

Thanks, I didn't realize that it was only snapshots and not the entire thing.

[JesusTheHun]

@sheremet-va

You can (and should) use global expect in concurrent tests as a rule.

Slightly off topic but you poked my curiosity. Is there any downside at using a local expect instead of the global one ?

[hi-ogawa]

You can (and should) use global expect in concurrent tests as a rule. The only exception is toMatchSnapshot because the name of the snapshot relies on the current test name.

I think currently there are few more expect API which is not concurrent safe. Internally, it comes down to the usage of either expect.setState or utils.flag(this, 'vitest-test').

For example, vitest-test state is relevant for toMatchSnapshot (e.g. vitest-test can become empty in concurrent tests with global expect and it could lead to puzzling Snapshot cannot be used outside of test message). I haven't tested but I can see vitest-test is also used for expect.soft, expect(...).resolves, expect(...).rejects API, so I suppose these are likely to cause some bad behavior as well.

expect.setState is used to implement expect.assertions and expect.hasAssertions, so they are not meant to be used inside concurrent with global expect, which is already mentioned in the doc https://vitest.dev/api/expect.html#expect-assertions

(also I made an example of such usage https://stackblitz.com/edit/vitest-dev-vitest-gyudct?file=test%2Frepro.test.ts)

[hi-ogawa]

(EDIT: Ah, sorry, I just realized this doesn't make inner tests to run in parallel...)

Though it's ugly (both API and also test logs), I think wrapping with describe.each would provide a proper local context for inner test.

https://stackblitz.com/edit/vitest-dev-vitest-mnmqgv?file=test%2Frepro.test.ts

[sheremet-va]

I think currently there are few more expect API which is not concurrent safe. Internally, it comes down to the usage of either expect.setState or utils.flag(this, 'vitest-test').

I guess you are right. Over time the usage of this increased.

Generally speaking, assertions are just functions that throw an error - you don’t really need to create a local expect (which is a bit expensive to do for every test) just to check if one value is equal to another.

[hi-ogawa]

Thanks for all the suggestions! I made some prototype on my PR #4989 for potential test.each APIs. I wrote Pros/Cons of each approach in terms of API and also implementation. If anyone is interested, I would appreciate feedback.

[JesusTheHun]

@hi-ogawa thanks for pulling this out. I'm not sure what I like more API-wise, but Jest compatibility will continue to be important for the next 3-5 years I would say, so I would rule out my own proposition of having it as the first argument.
Is it possible to consider other options ? Each solution have a downside...

// No arrow function
test.each(cases)('many cases', function (...caseArgs) => {
  const { myFixture } = this.getFixtures(); // Or maybe vi.getFixtures() ? But then it doesn't work in concurrent mode :/
});

// No array case
test.each(cases)('fixtures are available though a non-enumerable symbol', function (caseObj) => {
  const { myFixture } = vi.getFixtures(caseObj);
});

[jakebailey]

If the goal is specifically jest compat, then from reading the API examples in the PR, only each5/each6 with a required context: true options param seems to be possible without potentially breaking someone, no?

• each2 allows the test function to return another test function which is the actual test. This might be okay, if nobody ever wrote a test which previously returned something?
• each3 just passes in an object { expect, args } as the first param, but this seems directly backwards incompatible.
• each4 adds the arg as the final arg after all other args from the cases list. Maybe this works but if someone had previously written (...args) => { ... } as their test, now the context will appear there.
• each5 requires passing { context: true } after the cases, giving you the context after the args. This seems the most compatible, and is not dissimilar to APIs like Node's fs.readFileSync where an encoding option can cause the overload to change.
• each6 is the same as each5 but with the context first. This seems like the best option because one can write ({ expect }, ...args) => {} while with each5, you cannot. (This probably also makes it simpler to write types for.)

[hi-ogawa]

Hey, thanks for the quick feedback. Yeah, from the candidate so far, it's very tricky to think about API for jest compatibility while test context/fixture is vitest-only features. Also template string usage test.each`${"arg1"} | ${"arg2"}` is yet another problem, which I don't see the solution right now.

I just wanted to share what I saw while prototyping it, but I'll still keep looking as we might be still missing some ideas.

---

@JesusTheHun Thanks for the idea. About "No arrow function" API approach const { myFixture } = this.getFixtures(), I think this might be actually fine in terms of supporting "concurrent" use case since we can use testFn.bind to separate context just before test function execution. But the problem is that it might be difficult to do "fixture extraction", which currently relies on manual string parsing of testFn.toString() to look for exact string like ({ myFixture }) => { ... }.

[JesusTheHun]

@hi-ogawa so you parse the file to detect fixture usage 🧐
I thought you were using a Proxy to track usage/destructuring.
The only difference would be that the fixture would be created on its first use, instead of before the test, so some timeout gym would be required!

[sheremet-va]

so you parse the file to detect fixture usage 🧐

Not the file, only the function body. This is how it works in Playwright as far as I know. A proxy would trigger too late (when the test starts), we need to prepare all fixtures before the test has started.

[JesusTheHun]

we need to prepare all fixtures before the test has started

Can you elaborate on why it must be done before the test starts ?

@hi-ogawa we can require strict rules on how to use it, like import.meta.* constants replacement. This will simplify things a lot !

[hi-ogawa]

I thought you were using a Proxy to track usage/destructuring.

I'm not sure but technically proxy approach might be possible to employ in some way, but API would be probably affected by this. For example, const { myFixture } = this.getFixtures() would not be possible because myFixture evaluation is expected to be async. Even if we require await this.getFixtures(), I don't think fixture evaluation can be implemented with such destructuring, so it would require await this.getFixtures().myFixture, but then API might not be appealing anymore at this point.

[hi-ogawa]

I came back to this feature and it looks like I missed the one more suggestion by @jakebailey #4642 (comment) about where to put context: boolean flag.

This one actually supports template literal usages and typescript inference still works alright. I added it to test.each7 in the draft PR #4989 (also removed some bad candidates from PR)

myTest.concurrent.each7`
  a          | b
  ${'hello'} | ${2}
  ${'hi'}    | ${3}
`(
  'myTest.each7-context-template-true $a $b',
  async (arg, { expect, myFixture }) => {
   // arg = { a: "hello", b: 2 }
   // arg = { a: "hi", b: 3 }
    expect({ arg, myFixture }).toMatchSnapshot()
  },
  { context: true },
)

---

While adding more tests, I found a tricky case where args varies its length. This is probably not possible to support, but I guess it's okay since context: true will be opt-in (and maybe forever opt-in).

https://stackblitz.com/edit/vitest-dev-vitest-vnzqwz?file=test%2Frepro.test.ts

import { expect, test } from 'vitest';

test.each([
  [0, 1],
  [2, 3, 4],
])('each %s %s', (...args) => {
  // args = [0, 1]
  // args = [2, 3, 4]
  expect(args).toMatchSnapshot();
});

[sheremet-va]

I came back to this feature and it looks like I missed the one more suggestion by @jakebailey #4642 (comment) about where to put context: boolean flag.

We are deprecating the option in the last argument of test function in #5142, so it would look more like this:

myTest.concurrent.each7`
  a          | b
  ${'hello'} | ${2}
  ${'hi'}    | ${3}
`(
  'myTest.each7-context-template-true $a $b',
  { context: true },
  async (arg, { expect, myFixture }) => {
   // arg = { a: "hello", b: 2 }
   // arg = { a: "hi", b: 3 }
    expect({ arg, myFixture }).toMatchSnapshot()
  },
)

[sheremet-va]

We discussed this issue within the team, and we think it might be nicer to introduce a separate function for this, so what about having a separate API for this feature? Like test.for(/* array */):

test.concurrent.for([
  [1, 2, 3]
])('example', (first, second, third, context) => {
  context.task.name // example
})

[hi-ogawa]

If we can introduce a new API (and thus no need to worry about Jest compat or breaking change), then maybe we should consider supporting this exotic (but currently valid) usages:

// it's still not possible to type-inference nor extract fixture at the last argument
myTest.for([
  [0, 1],
  [2, 3, 4],
])('each %s %s', (...args) => {
  // args = [0, 1, { expect, someFixture }]
  // args = [2, 3, 4, { expect, someFixture }]
});

There are already some suggestions to support this:

• #issuecomment-1837525748

// regardless of each case is array or not, put each case as fixed first argument

myTest.for([
  [0, 1],
  [2, 3, 4],
])('each %s %s', (arg, { expect, someFixture }) => {
  // arg = [0, 1]
  // arg = [2, 3, 4]
});

myTest.for([
  { x: 0 }
  { x: 1 },
])('each %s %s', (arg, { expect, someFixture }) => {
  // arg = { x: 0 }
  // arg = { x: 1 }
});

// still easy to destruct array case
myTest.for([
  [0, 1],
  [2, 3],
])('each %s %s', ([a, b], { expect, someFixture }) => {
  // [a, b] = [0, 1]
  // [a, b] = [2, 3]
});

• #issuecomment-1894155135

// provide each case as a part of context in the first argument.
// not sure if it should be named `arg`, `args`, or something else...

myTest.for([
  [0, 1],
  [2, 3, 4],
])('each %s %s', ({ arg, expect, someFixture }) => {
  // arg = [0, 1]
  // arg = [2, 3, 4]
});

myTest.for([
  { x: 0 }
  { x: 1 },
])('each %s %s', ({ arg, expect, someFixture }) => {
  // arg = { x: 0 }
  // arg = { x: 1 }
});

Personally, the first suggestion #issuecomment-1837525748 looks okay to me. In terms of implementation, I think both typescript overload and fixture extraction becomes simpler with either of the approach.

[timsavage]

I found that the context is provided if a name is not supplied eg:

myTest.each([
  [0, 1],
  [2, 3, 4],
])((arg, { someFixture }) => {
  // arg = [0, 1]
  // arg = [2, 3, 4]
})

Is not ideal as the results list the tests as <anonymous>, but it does at least expose the context (and fixtures).

I'm not familiar with Jest so this might be expected.

[JCherryhomes]

Is not ideal as the results list the tests as <anonymous>, but it does at least expose the context (and fixtures).

I'm not familiar with Jest so this might be expected.

When I try this it appears that the tests never actually run. So while there is no error about the wrapper not existing it will pass regardless of what you put in the body of the test.

myTest.each([
  [0, 1],
  [2, 3, 4],
])((arg, { someFixture }) => {
    expect(true).toBe(false)
  })

shows all of the tests passing. If you try to console.log something in the test it never gets called.

[matuszek]

I was able to resolve the above (test.each not running at all) by upgrading:

-    "@vitejs/plugin-react": "4.0.0",
+    "@vitejs/plugin-react": "4.2.1",
-    "vitest": "0.31.3"
+    "vitest": "1.6.0"

Update: More precisely, upgrading gave me a new error message:

Error: Calling the test function inside another test function is not allowed. 
Please put it inside "describe" or "suite" so it can be properly collected.

and fixing that mistake by moving test.each up a level to be inside describe fixed the issue — even, as it turns out, on the older version of vitest.

[sheremet-va]

There are already some suggestions to support this:

I like this second approach.