fix: request.end accepted arguments (#1591)

* Fix typo about wrapping request.end.

I'm making this change it's own commit so I have a place to comment the
findings.

Digging through Node git history, I found the change that created the
breaking change in nock (ref nock PR 929).
nodejs/node@a10bdb5#diff-286202fdbdd74ede6f5f5334b6176b5cL779

Before Node v8, `OutgoingMessage`, which is extended by `ClientRequest`,
would literally do what it says in the docs if data was provided. It
would call `this.write(data, encoding)`. This meant that nock could wrap
only the `write` method when recording and gather all the chunks even if
the last chunk was sent to `end`. But, the above changed that to call an
internal function dual used by `end` and `write`.

* fix: request.end accepted arguments.

Fixes #1549

The method now correctly accepts all the permutations allowed.
request.end(data, encoding, callback)
request.end(data, callback)
request.end(data, encoding)
request.end(data)
request.end(callback)
request.end()

And a few tests were added to ensure all cases are explicitly covered.

lib/recorder.js

@@ -378,39 +378,41 @@ function record(recOptions) {
       }
     })
 
+    const recordChunk = (chunk, encoding) => {
+      debug(thisRecordingId, 'new', proto, 'body chunk')
+      if (!Buffer.isBuffer(chunk)) {
+        chunk = Buffer.from(chunk, encoding)
+      }
+      bodyChunks.push(chunk)
+    }
+
     const oldWrite = req.write
-    req.write = function(data, encoding) {
-      if (typeof data !== 'undefined') {
-        debug(thisRecordingId, 'new', proto, 'body chunk')
-        if (!Buffer.isBuffer(data)) {
-          data = Buffer.from(data, encoding)
-        }
-        bodyChunks.push(data)
+    req.write = function(chunk, encoding) {
+      if (typeof chunk !== 'undefined') {
+        recordChunk(chunk, encoding)
         oldWrite.apply(req, arguments)
       } else {
         throw new Error('Data was undefined.')
       }
     }
 
-    // Starting in Node 8, `res.end()` does not call `res.write()` directly.
-    // TODO: This is `req.end()`; is that a typo? ^^
+    // Starting in Node 8, `OutgoingMessage.end()` directly calls an internal `write_` function instead
+    // of proxying to the public `OutgoingMessage.write()` method, so we have to wrap `end` too.
     const oldEnd = req.end
-    req.end = function(data, encoding, callback) {
-      // TODO Shuffle the arguments for parity with the real `req.end()`.
-      // https://github.com/nock/nock/issues/1549
-      if (_.isFunction(data) && arguments.length === 1) {
-        callback = data
-        data = null
+    req.end = function(chunk, encoding, callback) {
+      debug('req.end')
+      if (typeof chunk === 'function') {
+        callback = chunk
+        chunk = null
+      } else if (typeof encoding === 'function') {
+        callback = encoding
+        encoding = null
       }
-      if (data) {
-        debug(thisRecordingId, 'new', proto, 'body chunk')
-        if (!Buffer.isBuffer(data)) {
-          // TODO-coverage: Add a test.
-          data = Buffer.from(data, encoding)
-        }
-        bodyChunks.push(data)
+
+      if (chunk) {
+        recordChunk(chunk, encoding)
       }
-      oldEnd.apply(req, arguments)
+      oldEnd.call(req, chunk, encoding, callback)
     }
 
     return req

lib/request_overrider.js

@@ -129,16 +129,18 @@ function RequestOverrider(req, options, interceptors, remove, cb) {
     return false
   }
 
-  req.end = function(data, encoding, callback) {
+  req.end = function(chunk, encoding, callback) {
     debug('req.end')
-    // TODO Shuffle the arguments for parity with the real `req.end()`.
-    // https://github.com/nock/nock/issues/1549
-    if (_.isFunction(data) && arguments.length === 1) {
-      callback = data
-      data = null
+    if (typeof chunk === 'function') {
+      callback = chunk
+      chunk = null
+    } else if (typeof encoding === 'function') {
+      callback = encoding
+      encoding = null
     }
+
     if (!req.aborted && !ended) {
-      req.write(data, encoding, function() {
+      req.write(chunk, encoding, () => {
         if (typeof callback === 'function') {
           callback()
         }

tests/test_recorder.js

@@ -444,7 +444,45 @@ test('records nonstandard ports', t => {
     })
 })
 
-test('rec() throws when reenvoked with already recorder requests', t => {
+test('req.end accepts and calls a callback when recording', t => {
+  const server = http.createServer((request, response) => {
+    response.writeHead(200)
+    response.end()
+  })
+  t.once('end', () => server.close())
+
+  nock.restore()
+  nock.recorder.clear()
+  t.equal(nock.recorder.play().length, 0)
+
+  server.listen(() => {
+    nock.recorder.rec({ dont_print: true })
+
+    let callbackCalled = false
+    const req = http.request(
+      {
+        hostname: 'localhost',
+        port: server.address().port,
+        path: '/',
+        method: 'GET',
+      },
+      res => {
+        t.true(callbackCalled)
+        t.equal(res.statusCode, 200)
+        res.on('end', () => {
+          t.end()
+        })
+        res.resume()
+      }
+    )
+
+    req.end(() => {
+      callbackCalled = true
+    })
+  })
+})
+
+test('rec() throws when reinvoked with already recorder requests', t => {
   nock.restore()
   nock.recorder.clear()
   t.equal(nock.recorder.play().length, 0)

tests/test_request_overrider.js

@@ -135,6 +135,85 @@ test('end callback called when end has callback, but no buffer', t => {
   })
 })
 
+test('request.end called with all three arguments', t => {
+  const scope = nock('http://example.test')
+    .post('/', 'foobar')
+    .reply()
+
+  let callbackCalled = false
+  const req = http.request(
+    {
+      host: 'example.test',
+      method: 'POST',
+      path: '/',
+    },
+    res => {
+      t.true(callbackCalled)
+      res.on('end', () => {
+        scope.done()
+        t.end()
+      })
+      res.resume()
+    }
+  )
+
+  // hex(foobar) == 666F6F626172
+  req.end('666F6F626172', 'hex', () => {
+    callbackCalled = true
+  })
+})
+
+test('request.end called with only data and encoding', t => {
+  const scope = nock('http://example.test')
+    .post('/', 'foobar')
+    .reply()
+
+  const req = http.request(
+    {
+      host: 'example.test',
+      method: 'POST',
+      path: '/',
+    },
+    res => {
+      res.on('end', () => {
+        scope.done()
+        t.end()
+      })
+      res.resume()
+    }
+  )
+
+  // hex(foobar) == 666F6F626172
+  req.end('666F6F626172', 'hex')
+})
+
+test('request.end called with only data and a callback', t => {
+  const scope = nock('http://example.test')
+    .post('/', 'foobar')
+    .reply()
+
+  let callbackCalled = false
+  const req = http.request(
+    {
+      host: 'example.test',
+      method: 'POST',
+      path: '/',
+    },
+    res => {
+      t.true(callbackCalled)
+      res.on('end', () => {
+        scope.done()
+        t.end()
+      })
+      res.resume()
+    }
+  )
+
+  req.end('foobar', () => {
+    callbackCalled = true
+  })
+})
+
 // http://github.com/nock/nock/issues/139
 test('finish event fired before end event', t => {
   const scope = nock('http://example.test')