[no-test-callback] doesn't handle `done` being used to fail a test

[SimenB]

The done callback in Jest can be used to fail tests, either by done.fail() or passing anything into done, like this: done('fail'). Both of these cases are handled wrong by the rule now - always replacing done with resolve, which only works when it's not supposed to fail.

This will make the rule more complicated (since it has to figure out how the callback is used instead of just replacing where done comes from). When we get to this I'd also prefer to use resolve as the name of the argument instead of the current logic (which reuses the name given to the done callback).

I've added 2 failing tests:

Index: src/rules/__tests__/no-done-callback.test.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/rules/__tests__/no-done-callback.test.ts b/src/rules/__tests__/no-done-callback.test.ts
--- a/src/rules/__tests__/no-done-callback.test.ts	(revision 9c31a8db90310fd1f0ee70e954683d718fcfb6de)
+++ b/src/rules/__tests__/no-done-callback.test.ts	(date 1618102970325)
@@ -412,5 +412,41 @@
         },
       ],
     },
+    {
+      code: 'test("something", done => {done("fail");})',
+      errors: [
+        {
+          messageId: 'noDoneCallback',
+          line: 1,
+          column: 19,
+          suggestions: [
+            {
+              messageId: 'suggestWrappingInPromise',
+              data: { callback: 'done' },
+              output:
+                'test("something", () => {return new Promise((_, reject) => {reject("error");})})',
+            },
+          ],
+        },
+      ],
+    },
+    {
+      code: 'test("something", done => {done.fail("fail");})',
+      errors: [
+        {
+          messageId: 'noDoneCallback',
+          line: 1,
+          column: 19,
+          suggestions: [
+            {
+              messageId: 'suggestWrappingInPromise',
+              data: { callback: 'done' },
+              output:
+                'test("something", () => {return new Promise((_, reject) => {reject("error");})})',
+            },
+          ],
+        },
+      ],
+    },
   ],
 });

[aryehb]

Perhaps I'm missing something, or Jest's behavior has changed at some point, but when I run the example test from the documentation:

test('some test', done => {
  expect(false).toBe(true);
  done();
});

Jest catches the error as expected, and doesn't wait for the timeout. If that's correct, I don't see why this rule is needed at all anymore.

[SimenB]

Bad example, should be

test('some test', done => {
  setTimeout(() => {
    expect(false).toBe(true);
    done();
  }, 10);
});

You'd never use done in a synchronous test, as there's nothing to wait for. Should probably update our example to not be quite so simplified

[aryehb]

I just tested that as well with the same results. The error is immediately caught by Jest without waiting for the timeout.

[astorije]

@SimenB's snippet above makes sense, however @aryehb's latest comment is accurate: 10 is in milliseconds and therefore shorter than the default 5s timeout. But Jest does handle the test failure correctly even in the situation above.

The following snippet does timeout like the documentation of no-test-callback warns about:

test('some test', done => {
  setTimeout(() => {
    expect(false).toBe(true);
    done();
  }, 10000);
});

With the following output:

  ● some test

    : Timeout - Async callback was not invoked within the 5000ms timeout specified by jest.setTimeout.Timeout - Async callback was not invoked within the 5000ms timeout specified by jest.setTimeout.Error:

    > 10 | test('some test', done => {
         |      ^
      11 |   setTimeout(() => {
      12 |     expect(false).toBe(true);
      13 |     done();

      at new Spec (node_modules/jest-jasmine2/build/jasmine/Spec.js:116:22)
      at Object.<anonymous> (test/myTest.test.ts:10:6)

Howeveeeer the 2 recommendations of the doc to either use a try...catch block, or return a Promise had no effects on this output. Specifically, I tried the following

test('try...catch #1', done => {
  try {
    setTimeout(() => {
      expect(false).toBe(true);
      done();
    }, 10000);
  } catch (e) {
    done(e);
  }
});

test('try...catch #2', done => {
  setTimeout(() => {
    try {
      expect(false).toBe(true);
      done();
    } catch (e) {
      done(e);
    }
  }, 10000);
});

test('new Promise', () => { // Marking this as `async` made no difference
  return new Promise(done => {
    setTimeout(() => {
      expect(false).toBe(true);
      done();
    }, 10000);
  });
});

All those examples resulted in the exact same output.
I'm also wondering the rule for this need, and wondering if Jest didn't use to do this differently in earlier versions, which would have made this rule necessary.

[G-Rath]

@SimenB have updated your patch to match the new state of things (.js -> .ts & the rule now uses suggestions)

[G-Rath]

@SimenB looking over this, I'm not sure how reasonable it is to support, as there's a whole lot of ways the done argument could be used that we'd miss.