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

concurrent.each executes iterations serially instead of concurrently #5458

Open
6 tasks done
jgoux opened this issue Mar 29, 2024 · 8 comments · May be fixed by #5491
Open
6 tasks done

concurrent.each executes iterations serially instead of concurrently #5458

jgoux opened this issue Mar 29, 2024 · 8 comments · May be fixed by #5491
Labels
p2-to-be-discussed Enhancement under consideration (priority)

Comments

@jgoux
Copy link
Contributor

jgoux commented Mar 29, 2024

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! - yocto-queue@1.0.0
npm ERR! - queue-microtask@1.2.3

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

@jgoux
Copy link
Contributor Author

jgoux commented Mar 30, 2024

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

hi-ogawa commented Mar 30, 2024

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

jgoux commented Mar 30, 2024

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

hi-ogawa commented Mar 30, 2024

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

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

jgoux commented Mar 30, 2024

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

hi-ogawa commented Mar 30, 2024

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

jgoux commented Apr 1, 2024

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.

@sheremet-va sheremet-va added p2-to-be-discussed Enhancement under consideration (priority) and removed pending triage labels Apr 2, 2024
@hi-ogawa hi-ogawa linked a pull request Apr 5, 2024 that will close this issue
6 tasks
@hi-ogawa
Copy link
Contributor

hi-ogawa commented Apr 30, 2024

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:

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-to-be-discussed Enhancement under consideration (priority)
Projects
Status: Has plan
Development

Successfully merging a pull request may close this issue.

3 participants