fix(recorder): allow recording req headers when not outputting objects (

#1617) * test: add coverage for logging recorded objects There was no test that `output_objects: true`, without also setting `dont_print: true` so this flow was not covered. * fix: allow recording req headers when not outputting objects When exploring coverage gaps a few things were uncovered: - OutgoingMessage._headers has been deprecated in favor of `getHeaders()`. https://nodejs.org/api/deprecations.html#deprecations_dep0066_outgoingmessage_prototype_headers_outgoingmessage_prototype_headernames - IncommingMessage.rawHeaders is never falsy so there isn't a need to fall back to `headers` - There was a bug that kept request headers from being added to `matchHeader` lines in non-object outputs. `enable_reqheaders_recording` was only having an affect if `output_objects` was also true, but headers were never being added despite that because it was looking for headers under the wrong attribute on the request. A test was added to cover the flow of wanting the `matchHeader` as the other flow was already covered.
nock · Jul 15, 2019 · a952d9b · a952d9b
1 parent 8c582ab
commit a952d9b
Show file tree

Hide file tree

Showing 4 changed files with 112 additions and 55 deletions.
diff --git a/lib/recorder.js b/lib/recorder.js
@@ -1,9 +1,5 @@
 'use strict'
 
-// This file contains some coverage ignores marked TODO-coverage. Before
-// refactoring within this file, these lines should be cleaned up.
-// https://github.com/nock/nock/issues/1607
-
 const debug = require('debug')('nock.recorder')
 const _ = require('lodash')
 const qs = require('qs')
@@ -69,7 +65,8 @@ function generateRequestAndResponseObject(
   bodyChunks,
   options,
   res,
-  dataChunks
+  dataChunks,
+  reqheaders
 ) {
   const response = getBodyFromChunks(dataChunks, res.headers)
   options.path = req.path
@@ -81,8 +78,11 @@ function generateRequestAndResponseObject(
     body: getBodyFromChunks(bodyChunks).body,
     status: res.statusCode,
     response: response.body,
-    rawHeaders: res.rawHeaders || res.headers,
-    reqheaders: req._headers,
+    rawHeaders: res.rawHeaders,
+  }
+
+  if (reqheaders) {
+    nockDef.reqheaders = reqheaders
   }
 
   if (response.isBinary) {
@@ -92,15 +92,22 @@ function generateRequestAndResponseObject(
   return nockDef
 }
 
-function generateRequestAndResponse(req, bodyChunks, options, res, dataChunks) {
+function generateRequestAndResponse(
+  req,
+  bodyChunks,
+  options,
+  res,
+  dataChunks,
+  reqheaders
+) {
   const requestBody = getBodyFromChunks(bodyChunks).body
   const responseBody = getBodyFromChunks(dataChunks, res.headers).body
 
   // Remove any query params from options.path so they can be added in the query() function
   let { path } = options
-  let queryIndex = 0
+  const queryIndex = req.path.indexOf('?')
   let queryObj = {}
-  if ((queryIndex = req.path.indexOf('?')) !== -1) {
+  if (queryIndex !== -1) {
     // Remove the query from the path
     path = path.substring(0, queryIndex)
 
@@ -135,16 +142,12 @@ function generateRequestAndResponse(req, bodyChunks, options, res, dataChunks) {
     ret.push(JSON.stringify(requestBody))
   }
   ret.push(')\n')
-  // TODO-coverage: Try to add coverage of this case.
-  /* istanbul ignore if */
-  if (req.headers) {
-    for (const k in req.headers) {
-      ret.push(
-        `  .matchHeader(${JSON.stringify(k)}, ${JSON.stringify(
-          req.headers[k]
-        )})\n`
-      )
-    }
+
+  reqheaders = reqheaders || {}
+  for (const [fieldName, fieldValue] of Object.entries(reqheaders)) {
+    const safeName = JSON.stringify(fieldName)
+    const safeValue = JSON.stringify(fieldValue)
+    ret.push(`  .matchHeader(${safeName}, ${safeValue})\n`)
   }
 
   if (queryIndex !== -1) {
@@ -157,15 +160,8 @@ function generateRequestAndResponse(req, bodyChunks, options, res, dataChunks) {
   ret.push(res.statusCode.toString())
   ret.push(', ')
   ret.push(JSON.stringify(responseBody))
-  /* istanbul ignore else */
-  if (res.rawHeaders) {
-    ret.push(', ')
-    ret.push(inspect(res.rawHeaders))
-  } else if (res.headers) {
-    // TODO-coverage: Try to add coverage of this case.
-    ret.push(', ')
-    ret.push(inspect(res.headers))
-  }
+  ret.push(', ')
+  ret.push(inspect(res.rawHeaders))
   ret.push(');\n')
 
   return ret.join('')
@@ -240,34 +236,33 @@ function record(recOptions) {
         debug(thisRecordingId, proto, 'intercepted request ended')
 
         let out
+        let reqheaders
+
+        // Ignore request headers completely unless it was explicitly enabled by the user (see README)
+        if (enableReqHeadersRecording) {
+          // We never record user-agent headers as they are worse than useless -
+          // they actually make testing more difficult without providing any benefit (see README)
+          reqheaders = req.getHeaders()
+          common.deleteHeadersField(reqheaders, 'user-agent')
+        }
+
         if (outputObjects) {
           out = generateRequestAndResponseObject(
             req,
             bodyChunks,
             options,
             res,
-            dataChunks
+            dataChunks,
+            reqheaders
           )
-          /* istanbul ignore else */
-          // TODO-coverage: The `else` case is missing coverage. Maybe look
-          // into the enable_reqheaders_recording option.
-          if (out.reqheaders) {
-            //  We never record user-agent headers as they are worse than useless -
-            //  they actually make testing more difficult without providing any benefit (see README)
-            common.deleteHeadersField(out.reqheaders, 'user-agent')
-
-            //  Remove request headers completely unless it was explicitly enabled by the user (see README)
-            if (!enableReqHeadersRecording) {
-              delete out.reqheaders
-            }
-          }
         } else {
           out = generateRequestAndResponse(
             req,
             bodyChunks,
             options,
             res,
-            dataChunks
+            dataChunks,
+            reqheaders
           )
         }
 
@@ -290,10 +285,7 @@ function record(recOptions) {
 
         if (!dontPrint) {
           if (useSeparator) {
-            /* istanbul ignore if */
             if (typeof out !== 'string') {
-              // TODO-coverage: This is missing coverage. Could this be
-              // connected to the `output_object` option?
               out = JSON.stringify(out, null, 2)
             }
             logging(SEPARATOR + out + SEPARATOR)

diff --git a/tests/test_define.js b/tests/test_define.js
@@ -249,7 +249,7 @@ test('define() uses reqheaders', t => {
       t.equal(res.statusCode, 200)
 
       res.once('end', () => {
-        t.equivalent(res.req._headers, reqheaders)
+        t.equivalent(res.req.getHeaders(), reqheaders)
         t.end()
       })
       // Streams start in 'paused' mode and must be started.

diff --git a/tests/test_intercept.js b/tests/test_intercept.js
@@ -946,7 +946,7 @@ test('get correct filtering with scope and request headers filtering', t => {
     }
   )
 
-  t.equivalent(req._headers, { host: requestHeaders.host })
+  t.equivalent(req.getHeaders(), { host: requestHeaders.host })
 })
 
 test('different subdomain with reply callback and filtering scope', async t => {

diff --git a/tests/test_recorder.js b/tests/test_recorder.js
@@ -126,6 +126,40 @@ test('records objects', t => {
   })
 })
 
+test('logs recorded objects', async t => {
+  t.plan(3)
+
+  nock.restore()
+  nock.recorder.clear()
+  t.equal(nock.recorder.play().length, 0)
+
+  const server = http.createServer((request, response) => {
+    t.pass('server received a request')
+    response.writeHead(200)
+    response.end()
+  })
+  t.once('end', () => server.close())
+  await new Promise(resolve => server.listen(resolve))
+  const { port } = server.address()
+
+  const logging = log => {
+    t.true(
+      log.startsWith(
+        '\n<<<<<<-- cut here -->>>>>>\n{\n  "scope": "http://localhost:'
+      )
+    )
+  }
+
+  nock.recorder.rec({
+    logging,
+    output_objects: true,
+  })
+
+  await got.post(`http://localhost:${port}`)
+
+  nock.restore()
+})
+
 test('records objects and correctly stores JSON object in body', async t => {
   nock.restore()
   nock.recorder.clear()
@@ -553,7 +587,7 @@ test('records https correctly', t => {
   })
 })
 
-test('records request headers correctly', t => {
+test('records request headers correctly as an object', t => {
   const server = http.createServer((request, response) => response.end())
   t.once('end', () => server.close())
 
@@ -598,6 +632,37 @@ test('records request headers correctly', t => {
   })
 })
 
+test('records request headers correctly when not outputting objects', async t => {
+  t.plan(5)
+
+  nock.restore()
+  nock.recorder.clear()
+  t.equal(nock.recorder.play().length, 0)
+
+  const server = http.createServer((request, response) => {
+    t.pass('server received a request')
+    response.writeHead(200)
+    response.end()
+  })
+  t.once('end', () => server.close())
+  await new Promise(resolve => server.listen(resolve))
+  const { port } = server.address()
+
+  nock.recorder.rec({
+    dont_print: true,
+    enable_reqheaders_recording: true,
+  })
+
+  await got.post(`http://localhost:${port}`, { headers: { 'X-Foo': 'bar' } })
+
+  nock.restore()
+
+  const recorded = nock.recorder.play()
+  t.equal(recorded.length, 1)
+  t.type(recorded[0], 'string')
+  t.true(recorded[0].includes('  .matchHeader("x-foo", "bar")'))
+})
+
 test('records and replays gzipped nocks correctly', t => {
   const exampleText = '<html><body>example</body></html>'
 
@@ -907,7 +972,7 @@ test('includes query parameters from superagent', t => {
     superagent
       .get(`http://localhost:${server.address().port}`)
       .query({ q: 'test search' })
-      .end(res => {
+      .end(() => {
         nock.restore()
         const ret = nock.recorder.play()
         t.true(ret.length >= 1)
@@ -917,7 +982,7 @@ test('includes query parameters from superagent', t => {
   })
 })
 
-test('encodes the query parameters when not outputing objects', t => {
+test('encodes the query parameters when not outputting objects', t => {
   const server = http.createServer((request, response) => {
     response.writeHead(200)
     response.end()
@@ -937,7 +1002,7 @@ test('encodes the query parameters when not outputing objects', t => {
     superagent
       .get(`http://localhost:${server.address().port}`)
       .query({ q: 'test search++' })
-      .end(res => {
+      .end(() => {
         nock.restore()
         const recording = nock.recorder.play()
         t.true(recording.length >= 1)
@@ -1183,7 +1248,7 @@ test('records and replays binary response correctly', t => {
     '47494638396101000100800000000000ffffff21f90401000000002c000000000100010000020144003b'
   const transparentGifBuffer = Buffer.from(transparentGifHex, 'hex')
 
-  // start server that always respondes with transparent gif at available port
+  // start server that always responds with transparent gif at available port
   const server = http.createServer((request, response) => {
     response.writeHead(201, {
       'Content-Type': 'image/gif',
@@ -1220,7 +1285,7 @@ test('records and replays binary response correctly', t => {
 
         const recordedFixtures = nock.recorder.play()
 
-        // stope server, stope recording, start intercepting
+        // stop server, stop recording, start intercepting
         server.close(error => {
           t.error(error)