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

Exit gracefully on linux #12139

Merged
merged 6 commits into from Mar 7, 2018

Conversation

Projects
None yet
3 participants
@ckerr
Member

ckerr commented Mar 6, 2018

  1. Fixes #12050 which was caused by a timing issue. The singleton fixture was reporting its readiness before app had emitted a 'ready' signal. At times, this could cause the parent test harness to run its tests before the child Electron process had finished starting its event loop.

  2. Rewrites the exit_gracefully_on_macos() test to run on other platforms too.

This code is ready for review but is a WIP in the sense that I need confirmation from CI about the portability of the rewritten test.

ckerr added some commits Mar 6, 2018

Fix timing issue in singleton fixture.
Singleton now sends the "we've started" message out only after it's
received a `'ready'` event from `app`. Previously it sent the message
out immediately, resulting in the parent test trying to manipulate it
before Singleton's event loop was fully bootstrapped.
Check for graceful exits on Linux, too.
Rewrite the "exits gracefully on macos" spec to run on Linux too.

@ckerr ckerr requested a review from electron/reviewers as a code owner Mar 6, 2018

ckerr added some commits Mar 6, 2018

Better error logging in api-app-spec.js. (#12122)
In the 'exits gracefully' test for app.exit(exitCode),
print the relevant error information if the test fails.
@codebytere

codebytere approved these changes Mar 6, 2018 edited

🚀 ty for this, solves such a key flake

@codebytere

This comment has been minimized.

Member

codebytere commented Mar 6, 2018

@ckerr it appears it's successful on mac and linux but that it's failing on windows. After 👀-ing some documentation, it looks like

Windows does not support sending signals, but Node.js offers some emulation with process.kill(), and subprocess.kill(). Sending signal 0 can be used to test for the existence of a process. Sending SIGINT, SIGTERM, and SIGKILL cause the unconditional termination of the target process.

I think this is still workable cross-platform but we'll have to include a separate handler for win32 processes in the test for CI to go

Run the exit-gracefully test on macOS and Linux.
Windows does not support sending signals, but Node.js offers some
emulation with process.kill(), and subprocess.kill(). Sending signal 0
can be used to test for the existence of a process. Sending SIGINT,
SIGTERM, and SIGKILL cause the unconditional termination of the target
process.

So, we'll need a different approach if we want to test this in win32.
@ckerr

This comment has been minimized.

Member

ckerr commented Mar 6, 2018

@codebytere, thank you for doing the win32 research on this!

I agree we'll need a different approach for win32. Simulating SIGTERM is a good Node feature, but forcing an unconditional exit doesn't give us a meaningful test when we're looking for a graceful shutdown. So let's limit this implementation of the test to macOS and Linux.

@codebytere

This comment has been minimized.

Member

codebytere commented Mar 6, 2018

in that case, i'll merge when CI goes

@codebytere codebytere merged commit 65ee977 into master Mar 7, 2018

9 checks passed

WIP ready for review
Details
ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
jenkins: macOS/pr-head This commit looks good
Details

@codebytere codebytere deleted the exit-gracefully-on-linux branch Mar 7, 2018

@alexeykuzmin

This comment has been minimized.

Contributor

alexeykuzmin commented Mar 7, 2018

I would vote for backporting the change to 2-0-x. Flaky tests is pain.

@ckerr, @codebytere

codebytere added a commit that referenced this pull request Mar 7, 2018

Exit gracefully on linux (#12139)
* Fix timing issue in singleton fixture.

Singleton now sends the "we've started" message out only after it's
received a `'ready'` event from `app`. Previously it sent the message
out immediately, resulting in the parent test trying to manipulate it
before Singleton's event loop was fully bootstrapped.

* Check for graceful exits on Linux, too.

Rewrite the "exits gracefully on macos" spec to run on Linux too.

* Check for graceful exits everywhere.

* Tweak comment

* Better error logging in api-app-spec.js. (#12122)

In the 'exits gracefully' test for app.exit(exitCode),
print the relevant error information if the test fails.

* Run the exit-gracefully test on macOS and Linux.

Windows does not support sending signals, but Node.js offers some
emulation with process.kill(), and subprocess.kill(). Sending signal 0
can be used to test for the existence of a process. Sending SIGINT,
SIGTERM, and SIGKILL cause the unconditional termination of the target
process.

So, we'll need a different approach if we want to test this in win32.
@codebytere

This comment has been minimized.

Member

codebytere commented Mar 7, 2018

done, pr up

sethlu added a commit to sethlu/electron that referenced this pull request May 3, 2018

Exit gracefully on linux (electron#12139)
* Fix timing issue in singleton fixture.

Singleton now sends the "we've started" message out only after it's
received a `'ready'` event from `app`. Previously it sent the message
out immediately, resulting in the parent test trying to manipulate it
before Singleton's event loop was fully bootstrapped.

* Check for graceful exits on Linux, too.

Rewrite the "exits gracefully on macos" spec to run on Linux too.

* Check for graceful exits everywhere.

* Tweak comment

* Better error logging in api-app-spec.js. (electron#12122)

In the 'exits gracefully' test for app.exit(exitCode),
print the relevant error information if the test fails.

* Run the exit-gracefully test on macOS and Linux.

Windows does not support sending signals, but Node.js offers some
emulation with process.kill(), and subprocess.kill(). Sending signal 0
can be used to test for the existence of a process. Sending SIGINT,
SIGTERM, and SIGKILL cause the unconditional termination of the target
process.

So, we'll need a different approach if we want to test this in win32.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment