-
-
Notifications
You must be signed in to change notification settings - Fork 34
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
Provide example using database with 100% test coverage #18
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this needs a few improvements.
- use fastify-postgres instead of manually instantiating a pool.
- add 'use strict' everywhere
- have tests that actually test the database queries
I updated the code to be like this, but now it will be hard to mock and return error since I must create whole fastify object with
|
fastify-postgres/server.js
Outdated
reply.send(books.rows); | ||
const client = await app.pg.connect(); | ||
const { rows } = await client.query("SELECT * FROM books"); | ||
client.release(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would recommend the following:
fastify.post('/user/:username', (req, reply) => {
// will return a promise, fastify will send the result automatically
return fastify.pg.transact(async client => {
// will resolve to an id, or reject with an error
const id = await client.query('INSERT INTO users(username) VALUES($1) RETURNING id', [req.params.username])
// potentially do something with id
return id
})
})
and remove the try/catch. Note that in case of an error the client is never released
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it okay that the client never released when error happen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work! Almost there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Hi, fastify are great framework (Thanks for everything ❤️ ) I use it a lot ~ however this is my first time writing test for my HTTP API haha. I didn't seems to found any example on the internet on how to achieve 100% coverage in integration test. Because sometimes error are harder to replicate, so it need mock. With this following example :
Lest say the
create()
function will only throw error if you lost your connection to a database,catch(error)
block will never be covered on test. Thanks to one of the fastify maintainer that answering my question on SO I now able to test achieve 100% coverage! 💯Therefore I want to place the example here, so that everyone search it can found it easily.
Checklist
npm run test
andnpm run benchmark
and the Code of conduct