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: correct methods overriding #15

Merged
merged 1 commit into from
Nov 14, 2014
Merged

Fix: correct methods overriding #15

merged 1 commit into from
Nov 14, 2014

Conversation

j0tunn
Copy link
Contributor

@j0tunn j0tunn commented Aug 12, 2014

Current implementation replaces ProcessLauncher._start method, but instead of replacing any method dealing with kill it just adds new listener for kill event which just calls callback.
As a result:

  • BaseLauncher._done method never called and so no event done emitted and karma process hangs
  • callback called twice:
    • first time from ProcessLauncher's kill listener without any error
    • second time from WebDriverInstance's kill listener
  • nobody removes tmp dir created by ProcessLauncher's start listener

To fix this we shouldn't add any new listener but override any methods called from ProcessLauncher's start/kill listeners:

  • _start
  • _process.kill
  • _onKillTimeout

Don't add new listener for kill event, but override ProcessLauncher's methods for correct exit.
@j0tunn
Copy link
Contributor Author

j0tunn commented Aug 12, 2014

@aparkinson take a look at this PR please

@mtscout6 mtscout6 merged commit de499fa into karma-runner:master Nov 14, 2014
@j0tunn
Copy link
Contributor Author

j0tunn commented Nov 15, 2014

@mtscout6 thanks man. When we should expect new version?

@mtscout6
Copy link
Member

I'm still working on push rights to npm, as soon as that's in order I'll
push out an update.

On Fri, Nov 14, 2014, 22:24 j0tunn notifications@github.com wrote:

@mtscout6 https://github.com/mtscout6 thanks man. When we should expect
new version?


Reply to this email directly or view it on GitHub
#15 (comment)
.

@mtscout6
Copy link
Member

Pushed with version 0.3.0

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.

2 participants