search

aws-amplify / amplify-backend

Home to all tools related to Amplify's code-first DX (Gen 2) for building fullstack apps on AWS
Apache License 2.0
184 stars 62 forks source link

Make npx accept ctrl-c in e2e tests. #582

Open sobolk opened 1 year ago

sobolk commented 1 year ago

From https://github.com/aws-amplify/samsara-cli/pull/575#discussion_r1382278621 .

When we try to call npx amplify in e2e tests then signals (SIGINT) do not reach child processes (i.e. sandbox).

We should find a solution to this problem, or a workaround which will work in canary tests.

From PR ^ I was digging a bit and found this interesting article https://stackoverflow.com/questions/43788943/send-sigint-to-a-process-by-sending-ctrl-c-to-stdin . It says that TTY terminal dispatches SIGINT to foreground process group. Which might be our problem with npx and we might need a bit more than just execa for our e2e test framework.

I suggest we that this change as is and find solution for npx and SIGINT separately in a dedicated PR.

sobolk commented 1 year ago

I have attempted to use node-pty instead of execa to get some TTY emulation in https://github.com/aws-amplify/samsara-cli/pull/604 . I got stuck with e2e test job on Windows not completing within 60 min timeout.

Observations:

  1. Using node-pty does solve Ctrl+C signal propagation when using npx amplify (instead of amplify). I.e. Pseudo TTY dispatches signals to all processes in process group so they have ability to react.
  2. All e2e and live canary tests are passing.
  3. The Windows job hangs indefinitely after running all tests.
    1. I have added plenty of debug logs to monitor PredicatedActions , process spawning, process output and process exit. I also added output from tasklist and wtfnode to reveal what prevents node process from exiting.
    2. Number of processes spawned equals number of processes that reported exit
    3. Output from tasklist after all tests ran does not contain any pid that was reported in test logs
    4. Output from wtfnode seems to confirm that we're hitting this problem https://github.com/microsoft/node-pty/issues/437 , i.e. node-pty leaves undisposed handles on Windows.

Options to proceed

  1. Do nothing and wait for node-pty fix. (This limits our testing capability)
    1. While waiting consider not using npx but lookup amplify binary and use it directly in all cases.
  2. Migrate to test runner that can ignore open handles and exit after tests complete. (Jest can do it, we know that from Gen1 CLI)
  3. Explore our own mechanism to dispatch signal to "spawned process group". (this won't address demand for TTY testing if we get spinners and other things like this)
edwardfoyle commented 1 year ago

I'm starting to think migrating to jest would be best. It has some nice utility methods that I miss from node:test in addition to this issue