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

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

Open
sobolk opened this issue Nov 3, 2023 · 2 comments
Open

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

sobolk opened this issue Nov 3, 2023 · 2 comments
Labels
tech-debt Refactors, unsavory workarounds or other technical decisions that should be revisited later testing Engineering label for issues related to tests or test tooling

Comments

@sobolk
Copy link
Member

sobolk commented Nov 3, 2023

From #575 (comment) .

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 sobolk added tech-debt Refactors, unsavory workarounds or other technical decisions that should be revisited later testing Engineering label for issues related to tests or test tooling labels Nov 3, 2023
@sobolk
Copy link
Member Author

sobolk commented Nov 9, 2023

I have attempted to use node-pty instead of execa to get some TTY emulation in #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 Unable to kill pty process on Windows microsoft/node-pty#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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tech-debt Refactors, unsavory workarounds or other technical decisions that should be revisited later testing Engineering label for issues related to tests or test tooling
Projects
None yet
Development

No branches or pull requests

2 participants