Extend callbackToPromise tests

[rmeritz]

• Add 100% coverage
• Test promise reject
• Test callbackArgIndex argument usage

[jugglinmike]

Testing Promise resolution values is unavoidably asynchronous. Since the tests don't return a Promise, Jest will consider them synchronous and move on before they're truly complete. If the Promises reject, Jest won't consider that a test failure.

For example, this change:

  it('lets a function return a promise that can reject', function() {
      var wrapped = callbackToPromise(rejectValue, 1);
-     expect(wrapped(2)).rejects.toThrow(/reject this value/);
+     expect(wrapped(2)).rejects.toThrow(/reject this kangaroo/);
  });

Here's the output from running the test suite

> airtable@0.8.1 test-unit /home/mike/projects/bocoup/airtable/airtable.js
> jest --env node

(node:2036) UnhandledPromiseRejectionWarning: Error: expect(received).rejects.toThrow(expected)

Expected pattern: /reject this kangaroo/
Received message: "I reject this value"

       9 | 
      10 |     function rejectValue(value, callback) {
    > 11 |         callback(new Error('I reject this value'));
         |                  ^
      12 |     }
      13 | 
      14 |     function sum() {

      at Number.apply (test/callback_to_promise.test.js:11:18)
      at lib/callback_to_promise.js:40:20
      at wrapped (lib/callback_to_promise.js:32:20)
      at Object.<anonymous> (test/callback_to_promise.test.js:30:16)
(node:2036) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 2)
(node:2036) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
PASS test/callback_to_promise.test.js
PASS test/update.test.js
PASS test/delete.test.js
PASS test/base.test.js
PASS test/create.test.js
PASS test/airtable.test.js
PASS test/record.test.js
PASS test/object_to_query_param_string.test.js
PASS test/has.test.js
PASS test/airtable_error.test.js
PASS test/node_version.test.js
PASS test/browser_build.test.js

Test Suites: 12 passed, 12 total
Tests:       106 passed, 106 total
Snapshots:   0 total
Time:        2.285s
Ran all test suites.

That looks like a test failure, but really, it's just Node.js reporting an unhandled rejection. When Jest finishes, it reports "12 passed, 12 total." More importantly, the process's exit code is still 0, so it won't be detected in continuous integration.

This is a problem with the existing tests, so it's your call if you want to fix those in this pull request or in a follow-up.

[rmeritz]

Testing Promise resolution values is unavoidably asynchronous. Since the tests don't return a Promise, Jest will consider them synchronous and move on before they're truly complete. If the Promises reject, Jest won't consider that a test failure.

I added return and tested the tests actually fail with bad assertions. So I think my tests and the existing tests a fixed now. Nice catch.

[kasrak]

s/explicately/explicitly

[rmeritz]

Done.

[kasrak]

I think it might be worth making this test the case where "if an explicit callbackArgIndex is set, but the function is called with too few arguments, we want to push undefined onto args so that our constructed callback ends up at the right index."

[rmeritz]

Added more unit testing for all cases of callbackIndex being set

[kasrak]

Thanks!