fixed fastify spec builders, added unit tests for code as well #933
Conversation
| @@ -63,16 +64,24 @@ let testCount = 0 | |||
| const testUri = (uri, agent, test, port) => { | |||
| agent.on('transactionFinished', (transaction) => { | |||
| testCount++ | |||
| test.equals( | |||
| test.equal( | |||
There was a problem hiding this comment.
Do we have any tests that verify we are not going to end up with any raw route parameters or query string items in the naming?
I'm assuming this works but not sure if we have any coverage. These seem to just be hitting simple defined routes VS say '/person/111222333/' and ensuring we end up with '/person/:id' as the name (or whatever matches fastify-style).
If we don't have any coverage, perhaps should have a story for that.
There was a problem hiding this comment.
we don't but i'll add to this #928 which was to flesh out our integration tests for fastify
lib/instrumentation/fastify.js
Outdated
| @@ -47,9 +47,12 @@ const setupMiddlewareHandlers = (shim, fastify) => { | |||
| return function wrappedFastifyUser() { | |||
| const args = shim.argsToArray.apply(shim, arguments) | |||
| const middlewareFunction = args[0] | |||
There was a problem hiding this comment.
You seem to be using destructuring everywhere else so just mentioning you could do that here too (but I don't care if you do or don't).
Proposed Release Notes
Links
Closes #926
Details
There is overlap with this PR and https://github.com/newrelic/node-newrelic/pull/931/files. The tests will need to be reconciled. We also will be adding more test cases scenarios when this MMF is close to being done in #928