concurrent.each executes iterations serially instead of concurrently

[jgoux]

Describe the bug

It seems that describe.concurrent.each is implemented as a shortcut for setting { concurrency: true } in each describe block so the tests within it are run in concurrently.

However, this is not how it works in Jest according to this PR: jestjs/jest#9326

describe.concurrent.each should not alter the describe block options but execute each describe block concurrently.

Here are my expectations:

const cases = [1, 2, 3];

// full concurrency, both the describe blocks and the tests within them are run concurrently
describe.concurrent.each(cases)(
  `case: %i`,
  { concurrent: true },
  (num) => {
    test("foo", {});
    test("bar", {});
  }
);

// only the describe blocks are executed concurrently, the tests within them are executed serially
describe.concurrent.each(cases)(
  `case: %i`,
  (num) => {
    test("foo", {});
    test("bar", {});
  }
);

// the describe blocks are executed serially, the tests within them are executed concurrently
describe.each(cases)(
  `case: %i`,
  { concurrent: true },
  (num) => {
    test("foo", {});
    test("bar", {});
  }
);

Reproduction

I was not able to install vitest correctly in StackBlitz:

❯ npm install
npm ERR! Failed to fetch dependencies:
npm ERR! - [email protected]
npm ERR! - [email protected]

npm ERR! A complete log of this run can be found in: /home/.npm/_logs/2024-03-29T17_44_21_075Z-debug-0.log

But here is the link illustrating the issue: https://stackblitz.com/edit/vitest-dev-vitest-fwwzec?file=test%2Fbasic.test.ts

System Info

System:
    OS: macOS 14.4.1
    CPU: (10) arm64 Apple M1 Pro
    Memory: 88.14 MB / 16.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 20.11.1 - /nix/store/xjgg53kjjhwcx4p3dmywjmqjbv82xhyn-nodejs-20.11.1/bin/node
    npm: 10.2.4 - /nix/store/xjgg53kjjhwcx4p3dmywjmqjbv82xhyn-nodejs-20.11.1/bin/npm
    pnpm: 8.15.5 - /nix/store/ih6qr7ga6i467b91kf52lkb6qhiib4s8-corepack-enable/bin/pnpm
  Browsers:
    Chrome: 123.0.6312.87
    Safari: 17.4.1
  npmPackages:
    vitest: 1.4.0 => 1.4.0

Used Package Manager

pnpm

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.

[jgoux]

@sheremet-va told me on Discord:

concurrent is only applied to tests
what's inside a describe block is executed during the collection phase

If I understand correctly, the collection phase executes each describe block and collects tests to be executed.

What I don't get is why for 2 given describe blocks, the concurrent tests aren't "flatten" together and executed concurrently in one batch. Currently, tests are still grouped per describe blocks and executed one group after the other (at least when using each to generate the describe blocks).

Executing all the tests concurrently, no matter their describe block would significantly speed up concurrent test suites.

Consider this test file:

import { test, describe } from 'vitest';

const cases = [1, 2, 3];

describe.concurrent.each(cases)('case %i', () => {
  test('first', async () => {
    await new Promise((resolve) => setTimeout(resolve, 10_000));
  });

  test('second', async () => {
    await new Promise((resolve) => setTimeout(resolve, 10_000));
  });
});

With the current implementation, running all the tests would take ~30 seconds (3 describe blocks of 10 seconds each ran one after the other).

Ideally, I would expect all 6 generated tests (3 describe blocks x 2 tests) to be all run concurrently, which would reduce the total time to ~10s.

I guess the user-land solution is to ban describe.each and instead use for loops + test.concurrent for max concurrency:

import { test, describe } from 'vitest';

const cases = [1, 2, 3];

for (const case of cases) {
  test(`case ${case} > first`, async () => {
    await new Promise((resolve) => setTimeout(resolve, 10_000));
  });

  test(`case ${case} > second`, async () => {
    await new Promise((resolve) => setTimeout(resolve, 10_000));
  });
}

10 seconds total for these to run. 👆

[hi-ogawa]

I think this is not necessary about each, but describes don't run in parallel currently like you might expect (and I also actually expected so at some point):

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

// takes 2 sec
describe.concurrent("not ok", () => {
  describe("1st suite", () => {
    test("inner", async () => {
      await sleep(1000)
    })
  });

  describe("2nd suite", () => {
    test("inner", async () => {
      await sleep(1000)
    })
  });
});

// takes 1 sec
describe.concurrent("ok", () => {
  test("1st case", async () => {
    await sleep(1000)
  })

  test("2nd case", async () => {
    await sleep(1000)
  })
});

I think, currently describe.concurrent is more like a syntax sugar of rewriting all test inside with test.concurrent:

// takes 2 sec
describe("not ok", () => {
  describe("1st suite", () => {
    test.concurrent("inner", async () => {
      await sleep(1000)
    })
  });

  describe("2nd suite", () => {
    test.concurrent("inner", async () => {
      await sleep(1000)
    })
  });
});

Also, considering describe.each is mostly a syntax sugar of simple loop, so it would be like:

describe.concurrent.each([1, 2])('case %i', () => {
  test('first', async () => {
    await new Promise((resolve) => setTimeout(resolve, 10_000));
  });
  test('second', async () => {
    await new Promise((resolve) => setTimeout(resolve, 10_000));
  });
});

// ⇓

describe('case 1', () => {
  test.concurrent('first', async () => {
    await new Promise((resolve) => setTimeout(resolve, 10_000));
  });
  test.concurrent('second', async () => {
    await new Promise((resolve) => setTimeout(resolve, 10_000));
  });
});

describe('case 2', () => {
  test.concurrent('first', async () => {
    await new Promise((resolve) => setTimeout(resolve, 10_000));
  });
  test.concurrent('second', async () => {
    await new Promise((resolve) => setTimeout(resolve, 10_000));
  });
});

[jgoux]

@hi-ogawa Sorry I just edited my post while you were writing yours lol

So in the end for max concurrency, describe blocks should be avoided and one should use a regular for loop right?

import { test, describe } from 'vitest';

const cases = [1, 2, 3];

for (const case of cases) {
  test(`case ${case} > first`, async () => {
    await new Promise((resolve) => setTimeout(resolve, 10_000));
  });

  test(`case ${case} > second`, async () => {
    await new Promise((resolve) => setTimeout(resolve, 10_000));
  });
}

So it's solvable in user-land, but I still feel it's a "miss" from the test framework because describe comes with useful little shortcuts (.only, .skip, .concurrent) for organizing related tests together.

[hi-ogawa]

Actually, I took a look of the current implementation and from my native thinking, it looks possible to run multiple describe in parallel since runSuiteChild here could be also describe and not only test

vitest/packages/runner/src/run.ts

Lines 312 to 316 in fee7d8b

	for (let tasksGroup of partitionSuiteChildren(suite)) {
	if (tasksGroup[0].concurrent === true) {
	const mutex = limit(runner.config.maxConcurrency)
	await Promise.all(tasksGroup.map(c => mutex(() => runSuiteChild(c, runner))))
	}

but the semantics of describe.concurrent needs to be changed or maybe introduce "describe.concurrrentSuite" since currently describe.concurrent affects in a deep manner against all test inside as I wrote in my comment above.

Also, even if what I'm imagining is implemented, for describe.each to work, it needs to be written with odd wrapper like this:

describe.concurrentSuite("odd wrapper", () => {
  describe.concurrent.each([1, 2])('case %i', () => {
    test('first', async () => {
      await new Promise((resolve) => setTimeout(resolve, 10_000));
    });
    test('second', async () => {
      await new Promise((resolve) => setTimeout(resolve, 10_000));
    });
  });
})

So in the end for max concurrency, describe blocks should be avoided and one should use a regular for loop right?

Yeah, regarding possible solution, your manual for loop should be totally fine. But, as you wrote on discord, vscode extension doesn't support it unfortunately.

[jgoux]

Yeah, regarding possible solution, your manual for loop should be totally fine. But, as you wrote on discord, vscode extension doesn't support it unfortunately.

The tests are picked up if you only use test and no describe blocks in the for loop. 👍

So that's not that bad. 😄

Would something like this work?

describe.concurrentSuite.concurrent.each([1, 2])('case %i', () => {
  test('first', async () => {
    await new Promise((resolve) => setTimeout(resolve, 10_000));
  });
  test('second', async () => {
    await new Promise((resolve) => setTimeout(resolve, 10_000));
  });
});

[hi-ogawa]

I don't know, it' just my preliminary tinkering, so I'm not even sure if it would actually work.
Btw, you mentioned about Jest, but do you have some reference about how they do describe.concurrent.each? I don't think it's mentioned in their PR there.

[jgoux]

I don't know, it' just my preliminary tinkering, so I'm not even sure if it would actually work. Btw, you mentioned about Jest, but do you have some reference about how they do describe.concurrent.each? I don't think it's mentioned in their PR there.

So, after testing with jest, it seems like describe.concurrent doesn't exist on their side, but their describe is concurrent and not serial.

Here is a small repo showing the difference between vitest and jest for the same describe block: https://github.com/jgoux/jest-concurrent-describe

Personally, jest behaviour is what I would expect from describe.

[hi-ogawa]

It looks like Jest simply runs all test.concurrent with first non concurrent test regardless of whether test.concurrent is inside the deeply nested describe. Actually, they say test.concurrent is experimental https://jestjs.io/docs/api#testconcurrentname-fn-timeout and indeed they have some bugs related to before/after hooks.

I setup this example to investigate:

• https://github.com/hi-ogawa/reproductions/tree/main/jest-concurrent
• https://stackblitz.com/edit/github-bugjry?file=basic.test.js

Though I don't think Jest is a good example to look up, I think Vitest's own way of suite level concurrency probably makes sense, so I'll continue on that on my PR.