dougmoscrop / serverless-http

Use your existing middleware framework (e.g. Express, Koa) in AWS Lambda 🎉
Other
1.74k stars 167 forks source link

Return the promise to simplify testing of handler #32

Closed pjarts closed 6 years ago

pjarts commented 6 years ago

When testing with AVA it's nice to have the handler return a promise since it will automatically fail the test if the promise rejects (unless you catch it yourself). And even if you don't test it that way I guess it doesn't hurt to return it.

pjarts commented 6 years ago

But it wont work since it's already been catched of course..

dougmoscrop commented 6 years ago

We sometimes do:

module.exports = app
module.exports.handler = serverless(app)

that way we can write the bulk of our tests just using supertest(-as-promised) against the exported app, and you could return that.

dougmoscrop commented 6 years ago

it's also worth pointing out that in most cases, the .catch path should not be called. even something like a 500 error, using this library, should be callback(null, { status: 500 }) so the invoke was 'successful' in that sense, i.e. you use koa-error or some other error-handling-stack-trace-sanitizing middleware.

I'm fine with returning the promise if you like, because you can still do things like

const callback = sinon.spy();

return handler({}, {}, callback)
  .then(() => {
    t.true(callback.called) // better assertions of course
  )};
pjarts commented 6 years ago

I just transferred from the non-proxy lambda integration so my brain is still expecting the callback to receive an error whenever the response is something other than ~200. As you pointed out, if you end up in the catch, something has gone terribly wrong and I guess in that case your test framework would complain about no executed assertions. Maybe it's a good idea to return the response data as well so that you can chain on the handler and make assertions on the returned data?

.then(res => {
  process.nextTick(() => {
    const statusCode = res.statusCode;
    const headers = sanitizeHeaders(res._headers);
    const isBase64Encoded = isBinary(headers, options);
    const body = getBody(res, isBase64Encoded);
    const data = {
      isBase64Encoded,
      statusCode,
      headers,
      body
    };

    callback(null, data);
    return data;
  });
})

Edit. the example above will obviously not work. Just noticed it's wrapped in process.nextTick

pjarts commented 6 years ago

I guess you would do something like

.then(res => {
  const statusCode = res.statusCode;
  const headers = sanitizeHeaders(res._headers);
  const isBase64Encoded = isBinary(headers, options);
  const body = getBody(res, isBase64Encoded);
  const data = {
    isBase64Encoded,
    statusCode,
    headers,
    body
  };
  process.nextTick(callback, null, data);
  return data;
})

which enables you to test your handler response:

return handler(evt, ctx, cb)
  .then(res => {
    t.is(res.statusCode, 200)
  })