open-cli-tools / concurrently

Run commands concurrently. Like `npm run watch-js & npm run watch-less` but better.
https://www.npmjs.com/package/concurrently
MIT License
7.01k stars 228 forks source link

Integration tests never complete #395

Closed paulsmithkc closed 1 year ago

paulsmithkc commented 1 year ago

Environment MacOS 13.0.1 Node 16.16.0 Typescript 4.9.4 Concurrently 7.6.0

Describe the bug We are attempting to write some integration tests for our azure functions, however the function runtime restarts instead of exiting when the test are complete, so the process never finishes.

To Reproduce Steps to reproduce the behavior:

  1. Create a new function app
  2. Create at least one http trigger
  3. Write an integration test using jest which makes a http request against the trigger
  4. Install the following packages: jest, ts-jest, concurrently, wait-on
  5. Add the following scripts to package.json
"pretest": "tsc -p tsconfig.test.json",
"test": "concurrently -k -s last \"func start\" \"wait-on tcp:7071 && jest\"",
  1. Execute the following command on the terminal: npm run test
  2. Wait for process to complete.

Expected behavior When the jest exits, concurrently should kill the azure function runtime, and terminate.

Actual behavior

  1. Jest exits successfully.
  2. Concurrently sends a kill signal to the azure function runtime.
  3. The azure function runtime restarts itself and keeps running.
  4. Integration tests never complete, because runtime is still active.

Relevant Console Output

[1] Ran all test suites.
[1] wait-on tcp:7071 && jest exited with code 0
--> Sending SIGTERM to other processes..
[0] [2022-12-08T16:06:34.042Z] Language Worker Process exited. Pid=64056.
[0] [2022-12-08T16:06:34.042Z] node exited with code 143 (0x8F). .
[0] [2022-12-08T16:06:34.280Z] Worker process started and initialized.

Proposed Fix Add a command line argument to concurrently which will send a SIGKILL signal instead of a SIGTERM signal, to force the azure runtime to exit immediately.

gustavohenke commented 1 year ago

Are you able to provide a self contained example at all? Creating an azure function is way too much trouble.

Are there any docs about the behaviour you're seeing?

NicolasBissig commented 1 year ago

I have the same problem with Linux, windows works. My repo is here: https://github.com/NicolasBissig/azure-functions-decorators/tree/feature/examples

how to recreate:

What you should see:

[jest]  PASS  __test__/function-http-query.test.ts (7.714 s)
[jest]
[jest] Test Suites: 5 passed, 5 total
[jest] Tests:       7 passed, 7 total
[jest] Snapshots:   0 total
[jest] Time:        9.641 s
[jest] Ran all test suites.
[jest] npx wait-on http-get://localhost:7071/api/health --timeout 60000 && npm run test:only exited with code 0
--> Sending SIGTERM to other processes..
[server] [2022-12-17T15:28:31.001Z] Language Worker Process exited. Pid=7117.
[server] [2022-12-17T15:28:31.001Z] node exited with code 143 (0x8F). .
[server] [2022-12-17T15:28:31.133Z] Worker process started and initialized.
NicolasBissig commented 1 year ago

While doing some further research, I found these two related issues in the core tools repo:

Hope this helps!

paulsmithkc commented 1 year ago

@gustavohenke Have been testing the proposed PR https://github.com/open-cli-tools/concurrently/pull/396 in our development environment and it does solve the issue.

This means that all of the Azure bits are irrelevant. All you need to reproduce this behavior is a simple app that listens for SIGTERM messages and ignores them. (Likely written in your favorite flavor of C.)

If we can add a flag to concurrently which changes the signal from SIGTERM to SIGKILL that would resolve the issue.

A more robust solution would be to start by sending the SIGTERM signal, and the follow that up with SIGKILL after a delay, if the app didn't shutdown in time.

gustavohenke commented 1 year ago

add a flag to concurrently which changes the signal from SIGTERM to SIGKILL

Sounds like a simple enough fix, would be happy to add a --kill-signal flag.

The SIGKILL delay idea needs a bit more thinking. It can probably evolve later from this one.

Do you think that with this you'll have what you need to improve your workflow?

paulsmithkc commented 1 year ago

@gustavohenke Sorry for the late follow up. Yes a flag to switch from SIGTERM to SIGKILL, would fully meet our needs.

paulsmithkc commented 1 year ago

@gustavohenke I have submitted a new PR to add the --kill-signal flag.

paulsmithkc commented 1 year ago

@gustavohenke bump