eslint-plugin-jest: [new rule] prefer-to-throw / consistent-expect-error

I would like to see a new rule named prefer-to-throw, or consistent-expect-error that would (preferably) default to the .toThrow syntax.

If consistent-expect-error is set to use the try/catch syntax, an error should be thrown if expect.hasAssertions (or a variation of) has not been set.

const throwSyncError = () => {
    throw new Error('sync example');
};

const throwAsyncError = async () => {
    throw new Error('async example');
};

describe('prefer-to-throw - sync', () => {
    test('eslint pass', () => {
        expect(() => throwSyncError()).toThrow('async example');
    });

    test('eslint error', () => {
        expect.hasAssertions();
        try {
            throwSyncError();
        } catch (error) {
            expect(error.message).toEqual('sync example');
        }
    });
});

describe('prefer-to-throw - async', () => {
    test('eslint pass', async () => {
        await expect(throwAsyncError()).rejects.toThrow('async example');
    });

    test('eslint error', async () => {
        expect.hasAssertions();
        try {
            await throwAsyncError();
        } catch (error) {
            expect(error.message).toEqual('async example');
        }
    });
});

About this issue

  • Original URL
  • State: closed
  • Created 5 years ago
  • Comments: 17 (8 by maintainers)

Most upvoted comments

Can you elaborate about the difference between your suggested solution to one without invoking the async function inside the expect?

re my response: I had just woken up and have not retained the context for this issue since it’s 6 months old, so I freeballed it to push you in the right direction 😂

I expected it to be “mostly right”, but should have mentioned “something like [code example]” to make that clearer.

@SimenB’s code example is actually also “almost” - it actually needs to be:

const promiseMeSomething = () => new Promise((rs, rj) => rj('hello world'));

test('it works', async () => {
  await expect(promiseMeSomething()).rejects.toThrow('hello world');
});

So to break down whats going on here, and why:

  • rejects is required because toThrow is sync, and promises don’t “throw” in the try/catch sense
  • rejects requires a promise, which is why we call promiseMeSomething directly instead of in a function
    • This is where @SimenB was “almost” - his example would have been rejected with Matcher error: received value must be a promise.
    • This also answers your question I believe about the difference between the two code lines you provided
  • Finally, we need await because otherwise the test will “pass” with a promise rejection error that jest won’t see.

Overall I think at this point (thats again without having the context from 6 months ago), I would personally recommend using a utility function of some kind if you want to test things on the actual error:

const getError = async (call: Function): Error => {
  try {
    await call();

    throw new Error('no error was thrown!');
  } catch(error) {
    return error;
  }
}

The benefit to me is that the getError utility function is a clear easy to recognize pattern that solely “inverts” errors & returns by returning any errors that are thrown, and throwing if no errors are.

That way you don’t duplicate that across your tests, where it’s possible make a typo that could still pass in your testbase. Granted we have tools to help reduce the likelihood of this, but it’s still possible.

(This is one of the reasons why no-try-expect was introduced).

In addition, we’re balancing usefulness & time-saving-potential against complexity and maintainability:

I think that the warning should be removed if hasAssertions exists or offer a better text.

Detecting if hasAssertions exists isn’t just a one-line function call - it requires parsing the whole function body and then some; that’s not including things other ways you can add expect.hasAssertions, such as as a hook for every test.

However, improving the text is definitely something we could do easily; I’ve not looked into the message myself, but more than happy to discuss that 😃

almost - need to be await expect(() => store.dispatch(actions.someFailingAction)).rejects.toThrow();.

We don’t have a really clean way if you wanna make multiple assertions on the error, though.

@jp7837 (it’s been another 6 months, so this is all iirc)

Otherwise the caller would have to check this known error each invocation.

Yes, that’s the point. The idea is that you’re wanting to perform checks against the error, which will always fail against our 'no error thrown!' error unless they were just testing that it’s an instance of Error (or that the title was a string?).

If you throw, then you’ll still get told the test failed, but not in a nice way because there’s a difference between an error being thrown and an assertion failing 😃

So, with the above helper these would all fail as expected:

it('matches', () => {
  const error = getError(doSomething);

  expect(error instanceof MyCustomError).toBe(true);
  expect(error.message).toMatch(/this is my error/iu);
  expect(error).toHaveProperty('theReasonWeThrowAnError');
});

The above will would all assert gracefully if no error was actually thrown 😃

@cartogram yeah, definitely!