Async Reply functions (always emit errors) (#1596)

mastermatt · web-flow · commit 26fc08fcd331 · 2019-08-01T13:47:45.000-06:00
Functions passed to `Interceptor.reply`, either to respond with an array of `[status, body, headers]` or with just the body, can now be async/promise-returning functions.

BREAKING CHANGE: uncaught errors thrown inside of user provided reply functions, whether async or not, will no longer be caught, and will no longer generate a successful response with a status code of 500. Instead, the error will be emitted by the request just like any other unhandled error during the request processing.
diff --git a/README.md b/README.md
@@ -395,7 +395,8 @@ const scope = nock('http://www.google.com')
   })
 ```
 
-> Note: When using a callback, if you call back with an error as the first argument, that error will be sent in the response body, with a 500 HTTP response status code.
+In Nock 11 and later, if an error is passed to the callback, Nock will rethrow it as a programmer error.
+In Nock 10 and earlier, the error was sent in the response body, with a 500 HTTP response status code.
 
 You can also return the status code and body using just one function:
 
diff --git a/lib/request_overrider.js b/lib/request_overrider.js
@@ -6,6 +6,7 @@ const { IncomingMessage, ClientRequest } = require('http')
 const _ = require('lodash')
 const propagate = require('propagate')
 const timers = require('timers')
+const util = require('util')
 const zlib = require('zlib')
 
 const common = require('./common')
@@ -213,8 +214,6 @@ function RequestOverrider(req, options, interceptors, remove) {
     ended = true
     let requestBody, responseBody, responseBuffers
 
-    let continued = false
-
     //  When request body is a binary buffer we internally use in its hexadecimal representation.
     const requestBodyBuffer = Buffer.concat(requestBodyBuffers)
     const isBinaryRequestBodyBuffer = common.isUtf8Representable(
@@ -307,41 +306,31 @@ function RequestOverrider(req, options, interceptors, remove) {
     if (interceptor.replyFunction) {
       const parsedRequestBody = parseJSONRequestBody(req, requestBody)
 
-      if (interceptor.replyFunction.length === 3) {
+      let fn = interceptor.replyFunction
+      if (fn.length === 3) {
         // Handle the case of an async reply function, the third parameter being the callback.
-        interceptor.replyFunction(
-          options.path,
-          parsedRequestBody,
-          continueWithResponseBody
-        )
-        return
+        fn = util.promisify(fn)
       }
 
-      const replyResponseBody = interceptor.replyFunction(
-        options.path,
-        parsedRequestBody
-      )
-      continueWithResponseBody(null, replyResponseBody)
+      // At this point `fn` is either a synchronous function or a promise-returning function;
+      // wrapping in `Promise.resolve` makes it into a promise either way.
+      Promise.resolve(fn.call(interceptor, options.path, parsedRequestBody))
+        .then(continueWithResponseBody)
+        .catch(emitError)
       return
     }
 
     if (interceptor.fullReplyFunction) {
       const parsedRequestBody = parseJSONRequestBody(req, requestBody)
 
-      if (interceptor.fullReplyFunction.length === 3) {
-        interceptor.fullReplyFunction(
-          options.path,
-          parsedRequestBody,
-          continueWithFullResponse
-        )
-        return
+      let fn = interceptor.fullReplyFunction
+      if (fn.length === 3) {
+        fn = util.promisify(fn)
       }
 
-      const fullReplyResult = interceptor.fullReplyFunction(
-        options.path,
-        parsedRequestBody
-      )
-      continueWithFullResponse(null, fullReplyResult)
+      Promise.resolve(fn.call(interceptor, options.path, parsedRequestBody))
+        .then(continueWithFullResponse)
+        .catch(emitError)
       return
     }
 
@@ -367,7 +356,7 @@ function RequestOverrider(req, options, interceptors, remove) {
         ? interceptor.body
         : [interceptor.body]
       responseBuffers = bufferData.map(data => Buffer.from(data, 'hex'))
-      continueWithResponseBody(null, undefined)
+      continueWithResponseBody()
       return
     }
 
@@ -391,36 +380,22 @@ function RequestOverrider(req, options, interceptors, remove) {
       }
     }
 
-    return continueWithResponseBody(null, responseBody)
-
-    function continueWithFullResponse(err, fullReplyResult) {
-      if (!err) {
-        try {
-          responseBody = parseFullReplyResult(response, fullReplyResult)
-        } catch (innerErr) {
-          emitError(innerErr)
-          return
-        }
-      }
-
-      continueWithResponseBody(err, responseBody)
-    }
+    return continueWithResponseBody(responseBody)
 
-    function continueWithResponseBody(err, responseBody) {
-      if (continued) {
-        // subsequent calls from reply callbacks are ignored
+    function continueWithFullResponse(fullReplyResult) {
+      try {
+        responseBody = parseFullReplyResult(response, fullReplyResult)
+      } catch (innerErr) {
+        emitError(innerErr)
         return
       }
-      continued = true
 
-      if (err) {
-        response.statusCode = 500
-        responseBody = err.stack
-      }
+      continueWithResponseBody(responseBody)
+    }
 
+    function continueWithResponseBody(responseBody) {
       //  Transform the response body if it exists (it may not exist
       //  if we have `responseBuffers` instead)
-
       if (responseBody !== undefined) {
         debug('transform the response body')
 
diff --git a/tests/test_reply_function_async.js b/tests/test_reply_function_async.js
@@ -4,8 +4,6 @@
 // callback with the response body or an array containing the status code and
 // optional response body and headers.
 
-const assertRejects = require('assert-rejects')
-const http = require('http')
 const { test } = require('tap')
 const nock = require('..')
 const got = require('./got_client')
@@ -51,60 +49,30 @@ test('reply takes a callback for status code', async t => {
   scope.done()
 })
 
-test('reply should throw on error on the callback', t => {
-  let dataCalled = false
-
-  const scope = nock('http://example.test')
+test('reply should throw on error on the callback', async t => {
+  nock('http://example.test')
     .get('/')
     .reply(500, (path, requestBody, callback) =>
       callback(new Error('Database failed'))
     )
 
-  // TODO When this request is converted to `got`, it causes the request not
-  // to match.
-  const req = http.request(
-    {
-      host: 'example.test',
-      path: '/',
-      port: 80,
-    },
-    res => {
-      t.equal(res.statusCode, 500, 'Status code is 500')
-
-      res.on('data', data => {
-        dataCalled = true
-        t.ok(data instanceof Buffer, 'data should be buffer')
-        t.ok(
-          data.toString().indexOf('Error: Database failed') === 0,
-          'response should match'
-        )
-      })
-
-      res.on('end', () => {
-        t.ok(dataCalled, 'data handler was called')
-        scope.done()
-        t.end()
-      })
-    }
-  )
-
-  req.end()
+  await t.rejects(got('http://example.test'), {
+    name: 'RequestError',
+    message: 'Database failed',
+  })
 })
 
 test('an error passed to the callback propagates when [err, fullResponseArray] is expected', async t => {
-  const scope = nock('http://example.test')
+  nock('http://example.test')
     .get('/')
     .reply((path, requestBody, callback) => {
       callback(Error('boom'))
     })
 
-  await assertRejects(got('http://example.test/'), ({ statusCode, body }) => {
-    t.is(statusCode, 500)
-    t.matches(body, 'Error: boom')
-    return true
+  await t.rejects(got('http://example.test'), {
+    name: 'RequestError',
+    message: 'boom',
   })
-
-  scope.done()
 })
 
 test('subsequent calls to the reply callback are ignored', async t => {
@@ -115,7 +83,7 @@ test('subsequent calls to the reply callback are ignored', async t => {
     .reply(201, (path, requestBody, callback) => {
       callback(null, 'one')
       callback(null, 'two')
-      callback(null, 'three')
+      callback(new Error('three'))
       t.pass()
     })
 
@@ -125,3 +93,59 @@ test('subsequent calls to the reply callback are ignored', async t => {
   t.is(statusCode, 201)
   t.equal(body, 'one')
 })
+
+test('reply can take a status code with an 2-arg async function, and passes it the correct arguments', async t => {
+  const scope = nock('http://example.com')
+    .post('/foo')
+    .reply(201, async (path, requestBody) => {
+      t.equal(path, '/foo')
+      t.equal(requestBody, 'request-body')
+      return 'response-body'
+    })
+
+  const response = await got.post('http://example.com/foo', {
+    body: 'request-body',
+  })
+
+  t.equal(response.statusCode, 201)
+  t.equal(response.body, 'response-body')
+  scope.done()
+})
+
+test('reply can take a status code with a 0-arg async function, and passes it the correct arguments', async t => {
+  const scope = nock('http://example.com')
+    .get('/')
+    .reply(async () => [201, 'Hello World!'])
+
+  const response = await got('http://example.com/')
+
+  t.equal(response.statusCode, 201)
+  t.equal(response.body, 'Hello World!')
+  scope.done()
+})
+
+test('when reply is called with a status code and an async function that throws, it propagates the error', async t => {
+  nock('http://example.test')
+    .get('/')
+    .reply(201, async () => {
+      throw Error('oh no!')
+    })
+
+  await t.rejects(got('http://example.test'), {
+    name: 'RequestError',
+    message: 'oh no!',
+  })
+})
+
+test('when reply is called with an async function that throws, it propagates the error', async t => {
+  nock('http://example.test')
+    .get('/')
+    .reply(async () => {
+      throw Error('oh no!')
+    })
+
+  await t.rejects(got('http://example.test'), {
+    name: 'RequestError',
+    message: 'oh no!',
+  })
+})
