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

Fix exit code #60

Closed
wants to merge 2 commits into from
Closed

Fix exit code #60

wants to merge 2 commits into from

Conversation

joshaber
Copy link
Contributor

If we destroy the only window, Electron seems to quit with an exit code of 0, so mocha’s success/failure won’t be communicated properly.

We don’t really need to worry about destroying it anyways, since we’re about to exit.

If we destroy the only window, Electron seems to quit with an exit code
of 0, so mocha’s success/failure won’t be communicated properly.

We don’t really need to worry about destroying it anyways, since we’re
about to exit.
@joshaber joshaber mentioned this pull request May 11, 2016
@jprichardson
Copy link
Owner

This seems reasonable. @inukshuk what are your thoughts?

@joshaber
Copy link
Contributor Author

joshaber commented May 11, 2016

Huh, looks like the window destruction was just added recently: #56. I guess I'll need to rethink this.

@joshaber
Copy link
Contributor Author

joshaber commented May 11, 2016

Hrmph. I'm not sure I can make this and #56 play together nicely. But I'd argue that this is more important, since test reporting is currently broken for CI use cases.

@joshaber
Copy link
Contributor Author

joshaber commented May 11, 2016

Alright, so this works. But note that as of #56, we're no longer cleaning up browserDataPath at all since the call to win.destroy() exits. So it might be worthwhile to simply revert that PR and delete this one.

joshaber added a commit to desktop/desktop that referenced this pull request May 11, 2016
…actually fail.

jprichardson/electron-mocha#60 is the real fix,
but in the mean time we can keep moving.
@inukshuk
Copy link
Collaborator

I think @joshaber is spot on here. Reverting #56 would take care of the bigger issue for now, though I'm curious as to why clean up is being skipped at all. I haven't looked at it yet, but the first thought I had was that Electron just exists cleanly when all windows have been closed (I assume) so perhaps we'd just have to handle the all windows closed event and exit from there (though clean up may have to be a sync function not sure)? I'll take a look at it later today!

inukshuk added a commit that referenced this pull request May 12, 2016
Calling app.exit() instead of process.exist() should force close
all open windows therefore making #56 superfluous.

As discussed in #60 closing all windows lead to Electron shutting
down with exit code 0 thus breaking CI tests etc. This could be
worked around by listening for the all-windows-closed event, but
I think this is the better solution.
@inukshuk inukshuk mentioned this pull request May 12, 2016
@inukshuk
Copy link
Collaborator

I think that was it: if I just listen for all-windows-closed then the clean up takes place. I think the overall cleaner solution is to use app.exit instead of process.exit -- according to the docs this should force close all open windows, so I would hope that the issues addressed by #56 are taken care of this way as well. This way we can move clean up to app.on('quit') too. See #62 -- what do you think?

@jprichardson
Copy link
Owner

LGTM

@joshaber
Copy link
Contributor Author

Awesome, thanks @inukshuk!

@joshaber joshaber closed this May 12, 2016
@joshaber joshaber deleted the dont-destroy-the-window branch May 12, 2016 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants