[new rule] prefer-to-throw / consistent-expect-error

[chrisblossom]

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');
        }
    });
});

[jeysal]

Matching any try-catch where the catch block contains an expect to recommend using expect.assertions/expect.hasAssertions seems doable, but I'm not sure about toThrow - I don't see a good way to avoid false positives where the assertions in the catch blocks do more than just check the error message

[chrisblossom]

I don't see a good way to avoid false positives where the assertions in the catch blocks do more than just check the error message

I don't know enough about eslint to respond to this concern, but I would accept this as a limitation and document it. I would just use a // eslint-disable-next-line consistent-expect-error if there is an actual need to use a try / catch.

This would also mean it should not be enabled in the recommended config (or it would enable to try / catch syntax, which I don't think is preferred. For example, typescript/no-explicit-any is a great rule to use, but sometimes you need any.

Matching any try-catch where the catch block contains an expect to recommend using expect.assertions/expect.hasAssertions

I think this is a great additional rule and should be enabled as part of the recommended config. This should include a .catch for a promise.

[jeysal]

I don't know enough about eslint to respond to this concern, but I would

accept this as a limitation and document it. Yeah, documenting the limitation might suffice in this case.

I think this is a great additional rule

Additional? I thought this was what your code examples are about?

…

On Thu, 11 Jul 2019, 19:30 Chris Blossom, ***@***.***> wrote: I don't see a good way to avoid false positives where the assertions in the catch blocks do more than just check the error message I don't know enough about eslint to respond to this concern, but I would accept this as a limitation and document it. I would just use a // eslint-disable-next-line consistent-expect-error if there is an actual need to use a try / catch. This would also mean it should not be enabled in the recommended config (or it would enable to try / catch syntax, which I don't think is preferred. For example, typescript/no-explicit-any <https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/no-explicit-any.md> is a great rule to use, but sometimes you need any. Matching any try-catch where the catch block contains an expect to recommend using expect.assertions/expect.hasAssertions I think this is a great additional rule and should be enabled as part of the recommended config. This should include a .catch for a promise. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#295?email_source=notifications&email_token=AD2TI55F7HD5F2VQGUI7V5LP65U33A5CNFSM4H7IC7X2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZXNJGA#issuecomment-510579864>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AD2TI53C2PEJPPNXKPSYJDLP65U33ANCNFSM4H7IC7XQ> .

[chrisblossom]

Additional? I thought this was what your code examples are about?

I guess looking back it doesn't seem to fit in the description of prefer-to-throw, or consistent-expect-error. Seems like it would be something like no-unsafe-try-catch. But I'd be happy either way.

[cartogram]

We have a similar rule to this at Shopify: https://github.com/Shopify/eslint-plugin-shopify/blob/master/docs/rules/jest/no-try-expect.md for preventing the try/catch part. I was already planning on submitting a PR for it soon. @SimenB @jeysal, would this be a rule you are interested in having as part of this repo?

@chrisblossom I think that would help prevent some of what you are looking to do.

[SimenB]

@cartogram yeah, definitely!

[chrisblossom]

eslint-plugin-shopify/jest/no-try-expect work great if jestjs/jest#8718 gets merged, but if it doesn't I'd prefer a more intelligent rule (and named accordingly) as discussed here that prefers the .toThrow syntax unless it can't be used.

Valid:

test('toThrow', () => {
	expect(() => {
		throw new Error('boom');
	}).toThrow('boom');
});

test('try/catch 2+ expect', () => {
	expect(() => {
		throw new Error('boom');
	}).toThrow('boom');

	expect.hasAssertions();
	try {
		throw new Error('boom');
	} catch (error) {
		// has multiple expects
		expect(error.message).toEqual('boom');
		expect(error.code).toEqual('NOT_FOUND');
	}
});

Invalid:

test('try/catch one expect', () => {
	// invalid, prefer .toThrow
	expect.hasAssertions();
	try {
		throw new Error('boom');
	} catch (error) {
		expect(error.message).toEqual('boom');
	}
});

test('try/catch missing hasAssertions', () => {
	// missing hasAssertions but multiple expects
	// expect.hasAssertions();
	try {
		throw new Error('boom');
	} catch (error) {
		expect(error.message).toEqual('boom');
		expect(error.code).toEqual('NOT_FOUND');
	}
});

EDIT: I'm not sure how this rule will work with aggregate errors without jestjs/jest#8718. Examples: tc39/proposal-promise-any and aggregate-error.

If jestjs/jest#8718 does land, I think we could also add a fixer to this rule(however it gets merged).

[ron23]

I too don't really understand the existing no-try-expect rule that was introduced in #331 .
How can I test more complicated logic in a catch?
Example:

it('test redux action was fired', () => {
      expect.assertions(1);
      try {
        await store.dispatch(actions.someFailingAction);
      } catch (e) {
        expect(store.getActions()[0].type).toEqual('person/CREATE_ERROR');
      }
}

I want to be able to assert on things that happened after my async invocation.
Another use case is testing multiple assertion on the error itself and I don't always like to use expect.objectContaining or alike.

I think that the warning should be removed if hasAssertions exists or offer a better text.
It also doesn't prevent me from using return ... catch and do the assertion in the catch so i'm not sure how this is different than the async-await case.

[G-Rath]

@ron23 you should be able to use expect().toThrow:

it('test redux action was fired', () => {
      expect.assertions(2);

      expect(async() => await store.dispatch(actions.someFailingAction)).toThrow();
      expect(store.getActions()[0].type).toEqual('person/CREATE_ERROR');
}

[SimenB]

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.

[ron23]

Thanks @SimenB
Can you elaborate about the difference between your suggested solution to one without invoking the async function inside the expect?
The difference between:
await expect(() => store.dispatch(actions.someFailingAction)).rejects.toThrow();
to
await expect(store.dispatch(actions.someFailingAction).rejects.toThrow();

As for not having solution for multiple assertion means I have to disable the rule :(

[G-Rath]

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 :)

[jp7837]

@G-Rath Thanks for the suggestion! One minor comment, shouldn’t the catch in getError rethrow if the try’d part throws “no error was thrown!”? Otherwise the caller would have to check this known error each invocation.

[G-Rath]

@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 :)

[G-Rath]

@chrisblossom I'm just re-reading this issue, and not sure what the action(s) for it are - we've now got no-conditional-expect that handles expect in try/catchs, and I've posted above a wrapper that can be used to catch errors for testing without violating no-conditional-expect (which I've just made a PR for adding it to the docs).

I think to keep things clean it might be good if we closed this issue, and you can open new issues if you have any bugs, questions, features, etc - is that ok with you?

[github-actions]

🎉 This issue has been resolved in version 24.4.3 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[github-actions]

🎉 This issue has been resolved in version 25.0.0-next.5 🎉

The release is available on:

• npm package (@next dist-tag)
• GitHub release

Your semantic-release bot 📦🚀