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

CTRL+C does not immediately stop running tests #3569

Closed
6 tasks done
nstepien opened this issue Jun 13, 2023 · 25 comments · Fixed by #3642
Closed
6 tasks done

CTRL+C does not immediately stop running tests #3569

nstepien opened this issue Jun 13, 2023 · 25 comments · Fixed by #3642

Comments

@nstepien
Copy link
Contributor

Describe the bug

CTRL+C does not immediately stop Vitest when tests are slow or stuck.

I've seen Vitest being impossible to terminate when tests are stuck because of an issue with fake timers for example, I had to completely close the terminal.
When test are just particularly slow it's frustrating that Vitest doesn't immediately stops when pressing CTRL+C.
I think this affects bail: 1 too, not 100% sure.

Jest does not have this issue.

Reproduction

https://stackblitz.com/edit/vitest-dev-vitest-c5wvvm?file=test%2Fsuite1.test.ts,test%2Fslow.test.ts,test%2Fsuite2.test.ts,vite.config.ts,test%2Fsuite0.test.ts&initialPath=__vitest__/

  1. run npm run test:run slow
  2. press CTRL+C when the test runs
  3. Vitest hangs and does not immediately terminate
    image

System Info

System:
    OS: Windows 10 10.0.22621
    CPU: (64) x64 AMD Ryzen Threadripper 3970X 32-Core Processor
    Memory: 17.54 GB / 31.86 GB
  Binaries:
    Node: 20.2.0 - C:\Program Files\nodejs\node.EXE
    npm: 9.6.7 - C:\Program Files\nodejs\npm.CMD
  Browsers:
    Edge: Spartan (44.22621.1778.0), Chromium (114.0.1823.43)
    Internet Explorer: 11.0.22621.1
  npmPackages:
    @vitejs/plugin-react: ^4.0.0 => 4.0.0
    @vitest/coverage-v8: ^0.32.0 => 0.32.0
    vite: ^4.3.5 => 4.3.9
    vitest: ^0.32.0 => 0.32.0

Used Package Manager

npm

Validations

@stackblitz
Copy link

stackblitz bot commented Jun 13, 2023

Fix this issue in StackBlitz Codeflow Start a new pull request in StackBlitz Codeflow.

@sheremet-va
Copy link
Member

I think this is an intended change, you need to press Ctrl+C the second time to immediately stop the process:

@nstepien
Copy link
Contributor Author

Pressing CTRL+C more than once doesn't seem to help I'm afraid.

image

@AriPerkkio
Copy link
Member

@nstepien does this happen only on Stackblitz or also on your local machine?

I can reproduce this on Stackblitz but it works fine on my local machine, MacOS.

@nstepien
Copy link
Contributor Author

For this specific example, pressing CTRL+C locally works fine.

But in our project, trying to terminate vitest doesn't not immediately stop the tests no matter how many times I press CTRL+C.
We have 245 test suites, and I don't know the root cause of why it's not stopping, so unfortunately I can't provide a minimal viable repro.
Please see the gif for a demonstration (local VSCode):

Recording 2023-06-14 at 11 54 31

What's the reasoning for gracefully stopping tests?
If I want to stop vitest then I want vitest to immediately stop, I don't care if some (slow) tests are still running.
This isn't an issue with Jest.

@AriPerkkio
Copy link
Member

But in our project, trying to terminate vitest doesn't not immediately stop the tests no matter how many times I press CTRL+C.

It should exit on the second SIGINT, so this is a bug. When implementing this feature I used multiple third party projects as reference for manual testing and they all worked fine. Without being able to reproduce this issue it's hard to identify the root cause.

Could you test if your local project exits properly with vitest@0.31.1 version? Also if possible, please share your Vitest configuration.

What's the reasoning for gracefully stopping tests?

In #3373 it was requested that we print the test summary instead of exiting the process immediately. If we don't intercept the SIGINT, the Node process will exit immediately.

@nstepien
Copy link
Contributor Author

When implementing this feature I used multiple third party projects as reference for manual testing and they all worked fine. Without being able to reproduce this issue it's hard to identify the root cause.

Please try this open-source repo of ours: https://github.com/adazzle/react-data-grid
Run npm t then try to stop the tests.
Its vitest config is here: https://github.com/adazzle/react-data-grid/blob/main/vite.config.ts

Could you test if your local project exits properly withvitest@0.31.1 version?

It does terminate immediately yes, though it doesn't print the test summary.

it was requested that we print the test summary instead of exiting the process immediately.

That makes sense to me. 👍
IMHO SIGINT should immediately stop the tests, print the summary (including any errors), and exit.

@AriPerkkio
Copy link
Member

Please try this open-source repo of ours: https://github.com/adazzle/react-data-grid

I've been trying to reproduce the issue for a while now but everything seems to work fine. I've tried cancelling the tests in different phases to see if some other test is causing this but cancelling with CTRL+C works all the times.

Screen.Recording.2023-06-14.at.15.08.47.mov

@nstepien
Copy link
Contributor Author

nstepien commented Jun 14, 2023

This is what I see. Maybe this is an issue on Windows only?

Recording 2023-06-14 at 13 18 51

@AriPerkkio
Copy link
Member

@nstepien could you try other terminals as well? I guess in the video above you are using Powershell? Please test what happens with CMD and Git Bash.

@nstepien
Copy link
Contributor Author

@AriPerkkio I've now tried with both CMD and Git Bash, and the same thing happened, no difference in behavior with Powershell. I've also tried Windows Terminal instead of the terminal in VSCode, no luck there either.

@AriPerkkio
Copy link
Member

Great, thanks for testing.

I'm running out of ideas. I don't have access to Windows machine at the moment so it's hard to debug this. But I guess this is indeed a Windows related issue.

@sheremet-va
Copy link
Member

sheremet-va commented Jun 16, 2023

Great, thanks for testing.

I'm running out of ideas. I don't have access to Windows machine at the moment so it's hard to debug this. But I guess this is indeed a Windows related issue.

I don't think Windows supports process.on('SIGINT'). We should use readline when OS is Windows.

@AriPerkkio
Copy link
Member

I don't think Windows supports process.on('SIGINT')

I still wonder why changes of #3407 broke Windows though. I'm looking into setting virtual environment on my machine to be able to reproduce this.

@sheremet-va
Copy link
Member

#3407

Because process.on('SIGINT', () => ctx.cancelCurrentRun('keyboard-input')) is never called on Windows?

@AriPerkkio
Copy link
Member

Then it should work as before and exit the process. Now it seems to cause some kind of blocking. @nstepien could you again do some testing and verify whether --watch and --run modes make any difference?

@nstepien
Copy link
Contributor Author

nstepien commented Jun 16, 2023

@sheremet-va SIGINT does work on Windows:

import process from 'process';

process.once('SIGINT', () => {
  console.log('Received SIGINT');
});

setTimeout(() => {
  process.exit(0);
}, 100000);

This will terminate the process on the second CTRL+C.

@AriPerkkio My previous attempts were with vitest run.
vitest watch and vitest seem to close immediately after the second CTRL+C (without summary).
Looks like only vitest run is affected?

@AriPerkkio
Copy link
Member

Even on Windows 11 I'm unable to reproduce this issue. Below I'm pressing CTRL+C multiple times and the tests are skipped.

windows-ctrl-c.mov

@AriPerkkio AriPerkkio added help wanted Extra attention is needed windows labels Jun 17, 2023
@nstepien
Copy link
Contributor Author

@AriPerkkio Could you try with npm t colSpan? It seems like it gets stuck on this test suite specifically.

@AriPerkkio
Copy link
Member

Could you try with npm t colSpan?

Still unable to reproduce. That colSpan matches test file with 3 test cases. When first test case is running, I press CTRL+ c and the 2 other test cases are skipped.

@nstepien
Copy link
Contributor Author

@AriPerkkio

When first test case is running, I press CTRL+ c and the 2 other test cases are skipped.

That's fine, but not the issue at hand.
Pressing CTRL+C multiple times does not immediately stop vitest. Please see the gif below.
This can be very frustrating when a test hangs and vitest cannot be terminated.
If it was up to me I'd make vitest terminate immediately on CTRL+C after showing a summary of what happened.

Recording 2023-06-19 at 17 37 32

@AriPerkkio
Copy link
Member

Pressing CTRL+C multiple times does not immediately stop vitest.

It should, and that is happening on other platforms than Windows. That's why this is a bug. However I'm unable to reproduce this at all.

For example running these two tests can be cancelled when first one is running. We can cancel test run without having to wait 4-5s. This is exactly what is being discussed here, right?

test("this test takes 5000ms", async () => {
  await sleep(5000);
});
test("this test takes 4000ms", async () => {
  await sleep(4000);
});
# 1. Start tests
$ time pnpm test tests/slow.test.ts
 RUN  v0.32.2 /x/y/vitest-example-project
 ...

# 2. Wait 1-2s and press CTRL+C twice
 ❯ tests/slow.test.ts (2)
   ⠴ this test takes 5000ms
   · this test takes 4000ms
 ELIFECYCLE  Test failed. See above for more details.
real    0m1.200s
user    0m0.649s
sys     0m0.087s

# 3. Process was terminated. We did not have to wait 4-5s.
$

@AriPerkkio
Copy link
Member

I'm now able to reproduce this using the minimal example from above. This is clearly a Windows bug as on macOs the test case exits on second CTRL + c immediately.

Screen.Recording.2023-06-19.at.20.24.42.mov

@nstepien
Copy link
Contributor Author

We can cancel test run without having to wait 4-5s. This is exactly what is being discussed here, right?

Yep!

@AriPerkkio
Copy link
Member

I've been debugging this for a while and I think the root cause is identified now. In #3407 I assumed that signal-exit would be the one who handled multiple process.on('SIGINT') handlers, so that on first SIGINT it would call the one defined in Vitest and on second SIGINT it would call its own. But this does not seem to be the case. It looks like this weird SIGINT handling is coming from pnpm and does not happen on Windows. 😕

Testing following script gives different SIGINT handling depending on OS and package manager:

// index.mjs
setInterval(() => process.stdout.write("."), 500);
process.on("SIGINT", () => console.log("SIGINT received"));

// Helper to force exit after 10 seconds just to that we don't get stuck forever
setTimeout(() => {
  console.log("Forcing timeout exit");
  process.exit(0);
}, 10_000);
{
  "scripts": {
    "start": "node index.mjs"
  }
}

Running the script above and hitting CTRL + C twice, with 1s delay between:

  • Expected: When CTRL + C is pressed, the message should be printed. Process should not terminate as SIGINT is intercepted.

  • MacOS + pnpm@8.6.2: Process terminates on second CTRL + C. Handler is called twice on first CTRL + C.

  • MacOS + npm@9.3.1: Process does not terminate. Handler is called twice on each CTRL + C.

  • MacOS + yarn@1.22.19: On first CTRL + C the script moves to background and cursor is moved to terminal. The logging leaks from background process to terminal. SIGINT handler is called only once.

  • MacOS + node@18.14.0, as in node index.mjs: Handler is called only once. Process does not terminate. This works correctly. ✅

  • Windows 11 + pnpm@8.6.2: On first CTRL + C the script moves to background and cursor is moved to terminal. The logging leaks from background process to terminal. SIGINT handler is called only once.

  • Windows 11 + npm@9.5.1: Same as pnpm above.

  • Windows 11 + node©18.16.0: Same as on Mac, correct behaviour. ✅

So I guess we should not count on catching SIGINT at all. Maybe we should instead capture keys with process.stdin.on('keypress' even when in run mode, and simply call process.exit when second key press is caught.

@AriPerkkio AriPerkkio removed the help wanted Extra attention is needed label Jun 21, 2023
@AriPerkkio AriPerkkio added bug and removed windows labels Jun 21, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Jul 12, 2023
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