Skip to content
This repository was 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
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,9 @@ Options, and their defaults
// the port the app will start on
port: 8080,
// default endpoint path
endpoint: '/batch'
endpoint: '/batch',
// default close callback
getClose: () => {},
}
```

Expand Down
6 changes: 5 additions & 1 deletion src/server.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ const defaultConfig = {
plugins: [],
port: 8080,
host: '0.0.0.0',
getClose: () => {},
};

export default function hypernova(userConfig, onServer) {
Expand All @@ -31,7 +32,10 @@ export default function hypernova(userConfig, onServer) {
throw new TypeError('Hypernova requires a `getComponent` property and it must be a function');
}

logger.init(config.logger);
// don't initialize logger if it has already been initialized
if (logger.init) {
logger.init(config.logger);
}

const app = express();

Expand Down
11 changes: 9 additions & 2 deletions src/worker.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,9 @@ const attachEndpoint = (app, config, callback) => {
app.post(config.endpoint, renderBatch(config, callback));
};


const initServer = (app, config, callback) => {
let server;

function exit(code) {
return () => process.exit(code);
}
Expand Down Expand Up @@ -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?

}

function errorHandler(err, req, res, next) { // eslint-disable-line no-unused-vars
// If there is an error with body-parser and the status is set then we can safely swallow
// the error and report it.
Expand Down Expand Up @@ -91,7 +95,10 @@ const initServer = (app, config, callback) => {
// run through the initialize methods of any plugins that define them
runAppLifecycle('initialize', config.plugins, config)
.then(() => {
server = app.listen(config.port, config.host, callback);
server = app.listen(config.port, config.host, () => {
callback();
readyToClose(config.getClose);
});
return null;
})
.catch(shutDownSequence);
Expand Down
46 changes: 44 additions & 2 deletions test/server-test.js
Original file line number Diff line number Diff line change
@@ -1,13 +1,55 @@
import { assert } from 'chai';
import sinon from 'sinon';

import hypernova from '../server';

describe('Hypernova server', () => {
let sandbox;

beforeEach(() => {
sandbox = sinon.sandbox.create();
sandbox.stub(process, 'exit');
});

afterEach(() => {
sandbox.restore();
});

it('blows up if hypernova does not get getComponent', () => {
assert.throws(hypernova, TypeError);
});

it('starts up the hypernova server without blowing up', () => {
hypernova({ devMode: true, getComponent: () => {} });
it('starts up the hypernova server without blowing up', (done) => {
hypernova({
devMode: true,
getComponent: () => {},
plugins: [
{
shutDown: () => {
done();
},
},
],
getClose: (close) => {
close(null, null, 0);
},
});
});

it('starts up the hypernova server again without blowing up', (done) => {
hypernova({
devMode: true,
getComponent: () => {},
plugins: [
{
shutDown: () => {
done();
},
},
],
getClose: (close) => {
close(null, null, 0);
},
});
});
});