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

Simplify Testing docs #719

Merged
merged 4 commits into from Jan 31, 2018

Conversation

Projects
None yet
6 participants
@nwoltman
Copy link
Member

commented Jan 29, 2018

Rewrite the documentation to explain the 2 methods of testing (http injection or with a running server) using minimal examples.

Suggested in #715 (comment).

Checklist

  • run npm run test and npm run benchmark
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message and code follows Code of conduct
Simplify Testing docs
Rewrite the documentation to explain the 2 methods of testing (http injection or with a running server) using minimal examples.
@delvedor
Copy link
Member

left a comment

This is not the best way to test Fastify, you are exporting an already initialized instance, this means that your test will not be isolated.
You should exports a builder function that returns a new Fastify instance (as was in the past example).

@jsumners

This comment has been minimized.

Copy link
Member

commented Jan 29, 2018

I think this is a better approach than exposing the core server:

lib/routes/foo.js

module.exports = function (fastify, opts, next) {
  fastify.get('/foo', function (request, reply) {
    reply.send({foo: 'foo'})
  })
  next()
}

test/lib/routes/foo.test.js

const tap = require('tap').test
const Fastify = require('fastify')
const routePlugin = require('../../../lib/routes/foo')

test('foo', (t) => {
  t.plan(2)
  const fastify = Fastify()
  fastify.register(routePlugin)
  fastify.listen(0, (err) => {
    fastify.server.unref()
    if (err) t.threw(err)
    fastify.inject({
      method: 'GET',
      url: '/foo'
    }, (err, res) => {
      if (err) t.threw(err)
      t.is(res.statusCode, 200)
      t.strictDeepEqual(JSON.parse(res.payload), {
        foo: 'foo'
      })
    })
  })
})
@mcollina

This comment has been minimized.

Copy link
Member

commented Jan 29, 2018

I agree with @jsumners

@delvedor

This comment has been minimized.

Copy link
Member

commented Jan 29, 2018

@jsumners @mcollina why?
In that way you must register every time all the routes/plugins and so on.
With a builder function it is done automatically, you just need to run the server.

const Fastify = require('fastify')

function build (opts) {
  const fastify = Fastify(opts)
  
  fastify
   .register(require('plugin'), opts)
   .register(require('./lib'), opts)

  return fastify
}

module.exports = { build }
const { build } = require('./server')

test('basic', async => {
  const fastify = build(opts)
  await fastify.ready()
  // tests...
})
@mcollina

This comment has been minimized.

Copy link
Member

commented Jan 29, 2018

Oh I'm 👍 with a builder function too.
I'm just 👎 on exposing the main object.

Both approaches makes 100% depending on the project itself.

@jsumners

This comment has been minimized.

Copy link
Member

commented Jan 29, 2018

Additionally, it is true isolation of testing the thing that needs to be tested.

@allevo

This comment has been minimized.

Copy link
Member

commented Jan 29, 2018

In fastify-example-twitter I have used the approach suggested by @jsumners

@delvedor
Copy link
Member

left a comment

LGTM

}
// note that now we are also exposing the fastify instance
module.exports = { start, fastify }
module.exports = { buildFastify }

This comment has been minimized.

Copy link
@mcollina

mcollina Jan 29, 2018

Member

I prefer this exposed a single function. It’s more idiomatic.

t.strictEqual(response.statusCode, 200)
t.strictEqual(response.headers['content-type'], 'application/json')
t.deepEqual(JSON.parse(body), { hello: 'world' })
fastify.close()

This comment has been minimized.

Copy link
@mcollina

mcollina Jan 29, 2018

Member

we should be using t.teardown(fastify.close.bind(fastify))

This comment has been minimized.

Copy link
@nwoltman

nwoltman Jan 29, 2018

Author Member

Even though it's slower, I used t.tearDown(() => fastify.close()) because it's shorter and more readable.

t.deepEqual(JSON.parse(response.payload), { hello: 'world' })
// Even if the server is not running (inject does not run the server),
// at the end of your tests it is highly recommended to call `.close()`
// to ensure that all connections to external services get closed.
fastify.close()

This comment has been minimized.

Copy link
@mcollina

mcollina Jan 29, 2018

Member

close should be wrapped in a teardown at the top of the file

@mcollina
Copy link
Member

left a comment

LGTM, with a nit.

const fastify = buildFastify()
// Even if the server is not running (inject does not run the server),

This comment has been minimized.

Copy link
@mcollina

mcollina Jan 30, 2018

Member

this is false. inject have fastify run the full "boot" cycle, so it is running, in some lateral sense.

@nwoltman

This comment has been minimized.

Copy link
Member Author

commented Jan 31, 2018

Any more feedback or is this ready to be merged?

@jsumners

This comment has been minimized.

Copy link
Member

commented Jan 31, 2018

I think you're good to merge.

@allevo

allevo approved these changes Jan 31, 2018

Copy link
Member

left a comment

LGTM

@nwoltman nwoltman merged commit eb1904d into fastify:master Jan 31, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@nwoltman nwoltman deleted the nwoltman:docs-rewrite-testing branch Jan 31, 2018

@cyberwombat

This comment has been minimized.

Copy link
Contributor

commented Jan 31, 2018

I think the testing docs got a bit oversimplified. The app.js should still check if its loaded as the main module as in previous version of the doc. As documented right now I don't see how you can call the app.js from node app.js as it will not launch the server.

@nwoltman

This comment has been minimized.

Copy link
Member Author

commented Jan 31, 2018

@cyberwombat I think that depends on what tools you're using for testing your app. The main docs provide generic examples that work for everybody and are a good starting point. More specific use cases that use certain tools should be provided in example repos.

@cyberwombat

This comment has been minimized.

Copy link
Contributor

commented Jan 31, 2018

I agree but the base example should also work with an existing app.js don't you think? The previous testng docs outlined a method to have one app.js that worked for testing and production which, imo, is a common requirement regardless of tooling. So adding something such as

if (require.main === module) {
  buildFastify(...)
  ...

would be useful. This is contrived but my point is to make a documented example that works for live/test.

@nwoltman

This comment has been minimized.

Copy link
Member Author

commented Jan 31, 2018

Yes it is important to have an app.js that works for both testing and production, but how you set that up really depends on your particular environment and tools. Having something like if (require.main === module) may be helpful for some people, but for others it may be confusing and get in the way of them learning what they need to use Fastify. That's why I think it would be great to have a list of links to some example repos so people can check out whichever example works best for their setup.

@jsumners

This comment has been minimized.

Copy link
Member

commented Jan 31, 2018

Ugh, I really need to get my JSCAS rewrite finished up. It would be a decent example. Stuck on needing a frontend designer :(

@allevo

This comment has been minimized.

Copy link
Member

commented Jan 31, 2018

Another way is using fastify-cli

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.