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

Add possibility to stop a karma server #1854

Merged

Conversation

budde377
Copy link
Member

This is a fix for issue #136. This:

  • Add detached mode using the karma start --detached command.
  • Add middleware for stopping a server (detached or not).
  • Described the detached option.

Any pointers on testing this or describing the new command (karma stop)?

@dignifiedquire
Copy link
Member

Thanks for this. I would really like to see an e2e test making sure this works as expected.

@dignifiedquire
Copy link
Member

Any pointers on testing this or describing the new command (karma stop)?

Take a look at the support files for the e2e tests, they are relatively straightforward javascript functions https://github.com/karma-runner/karma/blob/master/test/e2e/steps/core_steps.js#L43
We would need to add one for stop to test this. For docs on cucumberjs see here: https://github.com/cucumber/cucumber-js

@budde377
Copy link
Member Author

budde377 commented Feb 5, 2016

Cheers, I'll have a look at this ASAP.

On Thu, Feb 4, 2016 at 9:24 PM Friedel Ziegelmayer [email protected]
wrote:

Any pointers on testing this or describing the new command (karma stop)?

Take a look at the support files for the e2e tests, they are relatively
straightforward javascript functions
https://github.com/karma-runner/karma/blob/master/test/e2e/steps/core_steps.js#L43
We would need to add one for stop to test this. For docs on cucumberjs
see here: https://github.com/cucumber/cucumber-js


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

@budde377 budde377 force-pushed the feature-stop-karma-server branch from 9342a53 to 4c8f6e0 Compare February 8, 2016 16:31
@budde377
Copy link
Member Author

budde377 commented Feb 8, 2016

Now testing:

  • Stopping a server that isn't running
  • Stopping a server that is running
    • Succesfull termination
    • Informative error message

@budde377 budde377 force-pushed the feature-stop-karma-server branch from 4c8f6e0 to db79170 Compare February 8, 2016 19:04
@@ -0,0 +1,42 @@
Feature: Stop karma
TODO write this
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing todo ;)

@dignifiedquire
Copy link
Member

This is pretty sweet, one thing that would be nice is to have the stopper be also available on the public api (http://karma-runner.github.io/0.13/dev/public-api.html) and have some docs on how to use it. This could be used then in things like task runners to make it easier to spawn long running processes.

@budde377
Copy link
Member Author

Yeah, I can see that! I'll add it.

On Wed, Feb 10, 2016 at 9:47 PM Friedel Ziegelmayer <
[email protected]> wrote:

This is pretty sweet, one thing that would be nice is to have the stopper
be also available on the public api (
http://karma-runner.github.io/0.13/dev/public-api.html) and have some
docs on how to use it. This could be used then in things like task runners
to make it easier to spawn long running processes.


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

@budde377 budde377 force-pushed the feature-stop-karma-server branch from db79170 to 1fe10fc Compare February 11, 2016 18:39
@budde377
Copy link
Member Author

@dignifiedquire Here you go.


### **stopper.stop(options, [callback=process.exit])**

This function will signal a running server to stop. Equivalent of `karma stop`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this work if detached=false?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The detached setting is currently only working when running the server from CLI, since it's basically just calling itself in a new detached process.

So yes and no: The stop command will stop the server regardless of the detached setting and the server will not run detached when starting it programically. If this is a priority I'll certainly look into it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are doing sth very similar here: https://github.com/karma-runner/grunt-karma/blob/master/tasks/grunt-karma.js#L114-L134 so it would be nice if we could replace that with this new option, rather than duplication that effort

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, I'll look into it during the weekend.

@budde377
Copy link
Member Author

@dignifiedquire I have added the detached feature. In order to do so, I had to write the configuration as a file, hence the tmp dependency.

@dignifiedquire
Copy link
Member

@budde377 I think the e2e tests are not passing anymore :(

Add detached mode using the `karma start --detached` command.
Add middleware for stopping a server (detached or not).
Described the detached option.
Add the `stopper`  as a command to the public API allowing for enabling users to
programically terminate a running server.
When setting `detached=true` in config, the server will start
detached.
Default config to empty Object if undefined.
Add a setup-function for setting up the log-level and colors from config.
@budde377 budde377 force-pushed the feature-stop-karma-server branch from 3784109 to d14bd62 Compare February 21, 2016 09:57
@budde377
Copy link
Member Author

@dignifiedquire, This should do the trick.

@budde377
Copy link
Member Author

A bit too fast there, I'll take a look at the failing node 0.10.

Change the timing on tests, allowing Node 0.10 to catch-up.
@budde377
Copy link
Member Author

@dignifiedquire Done

@dignifiedquire
Copy link
Member

Thanks @budde377 merging this. Let's hope this time around things go smoother :)

dignifiedquire added a commit that referenced this pull request Feb 22, 2016
Add possibility to stop a karma server
@dignifiedquire dignifiedquire merged commit 6fe5a77 into karma-runner:master Feb 22, 2016
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