outmoded / discuss

The "mailing list"
99 stars 9 forks source link

Weird oversight in documentation with server.start() and server.inject() #548

Closed kswope closed 5 years ago

kswope commented 7 years ago

I've looked through books, docs, github issues and blog posts and there seems to be no consensus, or even awareness, of how to handle the combination of server.start and server.inject

The following scenario is everywhere:

in server.js

  server.start( () => {} )

and in handler.test.js

const server = require('./server')

test('dummy'), () => {
  server.inject(something)
})

Almost everybody seems oblivious that this starts a server listening on a socket when testing requests then goes through server.inject(). This cannot be good for performance, parallel tests or even sharing a port with a dev server.

I've seen what seems like a better practice, but ugly, shown below, but I've only seen it in one blog post and two github issues.

if ( ! module.parent ) {
  server.start( () => {} )
}

Somebody tell me what is the right way and then lets document it somewhere.

devinivy commented 7 years ago

Here's what I said on the original issue,

If you're going to test a server I highly suggest initializing it with server.initialize() but not starting it. That will enforce plugin dependencies and ensure the server can connect to any caches, etc, but not listen on a port. Here's some example code to initialize a server but not start it during testing: https://github.com/devinivy/boilerplate-api/blob/pal/server/index.js.

As for the documentation of this, @nlf and I have spoken about introducing a "cookbooks" section of the hapi website that this hapi folk knowledge might fit into.

AdriVanHoudt commented 7 years ago

@devinivy what is the status of the cookbooks? Maybe wait and target v17?

kswope commented 7 years ago

I'm willing to help with the cookbooks, I can offer the insight of the clueless.

@devinivy I could be very wrong about this, but because server.start and server.initialize are both non-blocking isn't any test that uses server.inject going to rely on the better outcome of a race condition?

In other words


server = require('./server')

test('one', () => {
  // is server done initializing or running here???
  server.inject(stuff)
})
devinivy commented 7 years ago

Yes, you need to wait for it to finish– I wrote a tool called labbable for this purpose.

Another simple solution that I believe now works and I'd recommend– if you server.initialize() an initialized server then it's a no-op. So feel free to call server.initialize() in your tests as well.

kswope commented 7 years ago

What about this recipe for the cookbook, does this solve the race problems?

server.js ( all setup, no running or initialization )

  const Hapi = require( 'hapi' ) 
  const server = new Hapi.Server( { } );
  server.connection( { port } ) 
  server.route( require( './routes' ) ) 
  module.exports = server

index.js ( or run.js, init.js, etc)

const server = require('./server')

server.initialize( () => {
  server.start( () => {
    console.log( `Server listening at ${server.info.uri} ${process.pid}` )
  } )
} )

something.test.js

const server = require('./server')                                                                   

beforeAll( ( done ) => {
  server.initialize( done )   // 'done', not 'done()'!!! too easy to screw up
} ) 

test( 'testing stuff', async () => { 
  response = server.inject('/path/to/stuff')
} ) 
devinivy commented 7 years ago

This would work, if you ensure that run.js is not required anywhere. Also, there's no need to call initialize() if start() is unconditionally called afterwards– start() calls initialize() itself.

kswope commented 7 years ago

because of this pattern I keep seeing


server.register({
// ...
}, (err) => {
    server.start((err) => {
      // ...
    });
});

I assumed server.initialize was taking over the async duties of server.register for calling server.start when ready.