Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

Expose shutDownSequence for better end-to-end testing #81

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jeffrey-xiao
Copy link

For context, bullet point 2 in this comment: #66 (comment)

@@ -54,6 +54,10 @@ const initServer = (app, config, callback) => {
.catch(exit(code));
}

function readyToClose(getClose) {
getClose(shutDownSequence);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we want to delegate the responsibility of closing down the server to a callback. We should close and then fire off a callback when it's done.

Copy link
Author

@jeffrey-xiao jeffrey-xiao Jul 20, 2017

Choose a reason for hiding this comment

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

Thanks for the feedback!

Would be better to expose close or shutDownSequence in worker to hypernova?

One of the problems I had writing tests when exposing those was that sometimes I was trying to close the http server when it hasn't been initialised yet.

Also isn't a callback when the server is shutdown necessary when you can create a plugin for the shutdown lifecycle event?

@goatslacker @ljharb

Copy link
Contributor

@magicmark magicmark Jul 26, 2017

Choose a reason for hiding this comment

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

@goatslacker the intent here is to be able to close down the hypernova server (and transitively the express & http server) from the consuming application on demand.

This is to set us up to have better end to end testing - currently we can only spin up one instance of hypernova per process (i.e. per test run). (more context linked in the body of the PR)

Is there a better way than a callback in the hypernova object to expose this functionality?

Copy link
Author

Choose a reason for hiding this comment

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

@goatslacker @ljharb
Any more thoughts on this?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants