Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

toThrow should return the error #8698

Open
chrisblossom opened this issue Jul 16, 2019 · 19 comments
Open

toThrow should return the error #8698

chrisblossom opened this issue Jul 16, 2019 · 19 comments

Comments

@chrisblossom
Copy link
Contributor

馃殌 Feature Proposal

.toThrow and .rejects.toThrow should return the error thrown.

Motivation

When working with a project that uses ava I noticed their .throws and .throwsAsync return the original error. It is very convenient.

This would make it possible to never need the expect.hasAssertions() + try / catch syntax.

Example

function throwSyncError() {
	const syncError = new Error('sync error');
	syncError.code = 'SYNC';
	throw syncError;
}

async function throwAsyncError() {
	const asyncError = new Error('sync error');
	asyncError.code = 'ASYNC';
	throw asyncError;
}

test('can get error message', async () => {
	const syncError = expect(() => throwSyncError()).toThrow('sync error');
	expect(syncError.code).toEqual('SYNC');

	const asyncError = await expect(throwAsyncError()).rejects.toThrow('sync error');
	expect(asyncError.code).toEqual('ASYNC');
});

Pitch

Because using .toThrow and .rejects.toThrow over try/catch it prevents tests that don't fail because they no longer reject. #3917

@jeysal
Copy link
Contributor

jeysal commented Jul 16, 2019

Sounds like a good idea to me. @SimenB @thymikee @pedrottimark wdyt?

@SimenB
Copy link
Member

SimenB commented Jul 18, 2019

While I agree the idea is sound (I love this part of Ava's API), I'm not sure we should do this. All matchers today return void (or Promise<void> in the case of resolves and rejects) - changing that is a pretty big change. I'd definitely like @cpojer's and/or @scotthovestadt's thoughts on this before we do much work on it (sorry about not responding sooner, @chrisblossom!).

Note that this change will also break things like https://github.com/mattphillips/jest-chain (/cc @mattphillips who might have thoughts on this 馃檪)


As for my own opinion - I think this is better solved by a custom matcher that takes predicate rather than returning the error.

expect(() => { throw new Error('boom'); }).toThrowWithField(err => err.code === 'MODULE_NOT_FOUND');

or something like that. Such a matcher may or may not make sense to have in core, but that's a separate discussion

@chrisblossom
Copy link
Contributor Author

As for my own opinion - I think this is better solved by a custom matcher that takes predicate rather than returning the error.

Personally I disagree. But I also think that 90% of the matchers should be removed and should just be expressed as plain javascript with toEqual. Less too learn, less to document.

I'd prefer to use the existing matcher if anything:

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

		// boolean return (true passes, false fails)
		return error.message === 'boom';
	});

	expect(errorThrown.code).toEqual('MODULE_NOT_FOUND');
});

But I think the PR as is feels a lot more natural / expected. Also I think it is best to provide the tools to never need try/catch considering how easy it is to shoot yourself in the foot with it.

@chrisblossom
Copy link
Contributor Author

All matchers today return void (or Promise in the case of resolves and rejects) - changing that is a pretty big change.

Just curious, why is this considered a big change?

@SimenB
Copy link
Member

SimenB commented Jul 18, 2019

Note that you can do this today:

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

For message you can just add the string as .toThrow('boom'), but the asymmetric expect.objectContaining allows you to check any field (at any depth). And you can create your own custom asymmetric matchers if you want as well

@chrisblossom
Copy link
Contributor Author

Another big use case for this is dealing with aggregate errors. Examples: tc39/proposal-promise-any and aggregate-error. Seems very difficult to deal with with the current matching api. With this PR you could set the error and run your own expects.

@cspotcode
Copy link
Contributor

cspotcode commented Nov 24, 2019

The type declarations say that matchers return the actual value. Is that intended to be the case, or is it a mistake?

https://github.com/facebook/jest/blob/master/packages/expect/src/types.ts#L118-L122

I'm hitting the same issue where it's annoying to assert against thrown values. I already know how to use expect(valueGoesHere).<matchers that I already understand how to use>() It seems unnecessarily inconvenient that I have to use a different style of matching for thrown values.


expect(promise).rejects.<matcher here>() can be used to extract the rejection of a promise and match against it. Can there be a sync variant? expect(fn).throws.toMatchObject({code: 'ENOENT'})?

It's still not ideal because multiple assertions means multiple function invocations, which is not the case with the promise. The assertion library should be giving the developer a straightforward way to assert that something throws and get a reference to the thrown value.

@farah
Copy link

farah commented Apr 1, 2020

@SimenB Is there any progress to this proposal? I really like it and would be greatly ease testing async functions.

@shellscape
Copy link

Upvoting this feature as well. 馃崒 that it's been sitting for a year without action while other test suites support this.

@zyf0330
Copy link

zyf0330 commented Jul 3, 2020

Note that you can do this today:

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

For message you can just add the string as .toThrow('boom'), but the asymmetric expect.objectContaining allows you to check any field (at any depth). And you can create your own custom asymmetric matchers if you want as well

@SimenB Is this doced anywhere? I canot find.

@eurostar-fennec-cooper
Copy link

Note that you can do this today:

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

For message you can just add the string as .toThrow('boom'), but the asymmetric expect.objectContaining allows you to check any field (at any depth). And you can create your own custom asymmetric matchers if you want as well

This is great, but I'd like to be able to use expect(thing()).rejects.toBeInstanceOf(SomeErrorClass) and additionally check that the thrown error has the correct data added to it (ie constructed with the right message or value).

@Janpot
Copy link

Janpot commented Oct 7, 2020

This is great, but I'd like to be able to use expect(thing()).rejects.toBeInstanceOf(SomeErrorClass) and additionally check that the thrown error has the correct data added to it (ie constructed with the right message or value).

Currently I do

const promise = thing()
await expect(promise).rejects.toThrow(MyErrorClass)
await expect(promise).rejects.toThrow('Error message')
await expect(promise).rejects.toThrow(expect.objectContaining({ code: 'MY_CODE' }));

But it only kind of works for async code if you want thing() to be evaluated only once. It's a bit clunky.

@ITenthusiasm
Copy link

As for my own opinion - I think this is better solved by a custom matcher that takes predicate rather than returning the error.

expect(() => { throw new Error('boom'); }).toThrowWithField(err => err.code === 'MODULE_NOT_FOUND');

or something like that. Such a matcher may or may not make sense to have in core, but that's a separate discussion

For the sake of keeping Jest consistent, I'm actually fine with this approach. However, I think it would be useful for toThrow to accept a callback.

expect(() => { throw new Error('boom'); }).toThrow((err: any): void => {
  expect(err.code).toBe('MODULE_NOT_FOUND');
});

It's basically just a callback that enables the user to perform their own assertions on the thrown error.

And you can create your own custom asymmetric matchers if you want as well

Ideally, users should be able to avoid making custom matchers. That's probably its own barrier to entry. I've been using Jest for years, and I've never had to make custom matchers. I imagine this is why A) jest has so many matchers out of the box to begin with and B) popular libraries often provide matchers that prevent their users from worrying about this. This is great. But for a typical user, it also makes the idea of creating matchers intimidating. I imagine such an approach is avoided for the average person.

Resorting to "Make a custom matcher" kind of defeats the point and raises a number of problems. Moreover, as chrisblossom, said, leaving the try/catch in the user's hands increases the chance of making mistakes.

The current toThrow doesn't seem to expect a function. So it could easily be overloaded and given a if (typeof input === 'function') portion, I would presume. (I haven't read the code yet.) Thoughts? @SimenB @chrisblossom And/or @cpojer and @scotthovestadt? since it seems Simen really wanted your opinions too.

@ITenthusiasm
Copy link

Welp. For anyone who's interested, I had the (bad) idea of trying out matchers for the first time. I'm not sure whether or not people will find it useful, but I'm leaving it behind as an artifact just in case... while people are waiting to hear back from the Jest devs.

My solution should solve both sync and async use cases. @Janpot could you let me know if you find this useful? You still have to write a bit of code with my solution, but hopefully the approach looks less hackish to you.

const promise = thing() // `thing` is an async function

await expect(promise).rejects.toThrowWithError((error: MyErrorClass) => {
  expect(error).toBeInstanceOf(MyErrorClass);
  expect(error.message).toBe("Error message");
  expect(error.code).toBe("MY_CODE");
});

Jest Code (TypeScript)

Note that I tested my code locally, and it works in all of the anticipated use cases. So if you use my code and it doesn't work, then either A) You have an outdated version of jest or typescript (or some other environment problem), or B) I copy-pasta'd wrong... in which case you should ping me. 馃檪

Extend jest wherever you want -- in your test file, or in a file that you import:

expect.extend({
  toThrowWithError(received, expected) {
    // Constants
    const passErrorToUser = (err: unknown): jest.CustomMatcherResult => {
      expected(err);
      return { pass: true, message: () => "" };
    };

    /* -------------------- Handle Promise Modifiers -------------------- */
    if (this.promise === "resolves")
      return {
        pass: false,
        message: () =>
          this.utils.matcherErrorMessage(
            this.utils.matcherHint("toThrowWithError", undefined, undefined, {
              promise: this.promise,
            }),
            `'${this.promise}' is an invalid modifier for 'toThrowWithError'`,
            ""
          ),
      };

    // Jest already takes care of validating `rejects`. We just need to pass the error along.
    if (this.promise === "rejects") return passErrorToUser(received);

    /* -------------------- Argument Validation -------------------- */
    if (typeof received !== "function")
      return {
        pass: false,
        message: () =>
          this.utils.matcherErrorMessage(
            this.utils.matcherHint("toThrowWithError"),
            `${this.utils.RECEIVED_COLOR("received")} value must be a function`,
            `Received has value: ${this.utils.RECEIVED_COLOR(typeof received)}`
          ),
      };

    if (typeof expected !== "function")
      return {
        pass: false,
        message: () =>
          this.utils.matcherErrorMessage(
            this.utils.matcherHint("toThrowWithError"),
            `${this.utils.EXPECTED_COLOR("expected")} value must be a function`,
            `Expected has value: ${this.utils.EXPECTED_COLOR(typeof expected)}`
          ),
      };

    /* -------------------- Matcher Logic -------------------- */
    try {
      received();
      const errorMessage = "Received function did not throw" as const; // copied from Jest's `toThrow` matcher.

      return {
        pass: false,
        message: () =>
          `${this.utils.matcherHint("toThrowWithError", undefined, undefined)}\n\n${errorMessage}`,
      };
    } catch (err) {
      return passErrorToUser(err);
    }
  }
});

For TS users, you'll still have to update the types for Jest. In a type declaration file of your choosing, apply the following:

// Add custom jest matchers
declare global {
  namespace jest {
    interface Matchers<R> {
      /**
       * Used to test that a function throws when it is called. Allows
       * assertions to be performed on the error that is generated.
       */
      // eslint-disable-next-line @typescript-eslint/no-explicit-any
      toThrowWithError(expected: (error: any) => void): R extends Promise<void> ? R : void;
    }
  }
}

(Note that I use TS ESLint's recommended settings. So I have a comment for disabling a linter warning.)

I would still love to hear from the Jest devs about updating their toThrow matcher.

@geitir
Copy link

geitir commented May 17, 2022

Being able to analyze the error in the usual jest chained functional style feels like it should be part of the standard api, especially with the discouragement of try{} catch{}

@pcattori
Copy link

pcattori commented Nov 10, 2022

Currently I do

const promise = thing()
await expect(promise).rejects.toThrow(MyErrorClass)
await expect(promise).rejects.toThrow('Error message')
await expect(promise).rejects.toThrow(expect.objectContaining({ code: 'MY_CODE' }));

But it only kind of works for async code if you want thing() to be evaluated only once. It's a bit clunky.

I noticed that jest is normalizing errors, getting rid of some fields. So for example, I'm writing a CLI that I'm testing via execa and I want to inspect the exitCode for the expected error. But await expect(promise).rejects.toThrow(expect.objectContaining({ exitCode: 'MY_CODE' })) doesn't work as jest has omitted the exitCode from the execa error!

I was able to workaround this an make a getError function that reliably returns the expected error unmodified:

Usage:

it("checks an error", async () => {
  let error = await getError(doAsyncStuff(arg1, arg2));

  // wrap in `async` function if you want to use a sync function or `await` keyword
  // let error = await getError(async () => {...})

  // typeguards for typescript to be happy, since `error` will be `unknown` just like in a `catch` clause
  // if (!expectedErrorGuard(error)) throw error;
  // for example:
  // if (!(error instanceof Error)) throw error; 

  expect(error.message).toContain("my error message")
  expect(error.exitCode).toBe(1)
})

Implementation:

class NoErrorThrownError extends Error {}
const getError = async (
  erroring: Promise<unknown> | (() => Promise<unknown>)
) => {
  try {
    let promise = typeof erroring === "function" ? erroring() : erroring;
    await promise;
    throw new NoErrorThrownError();
  } catch (error: unknown) {
    if (error instanceof NoErrorThrownError) throw error;
    return error;
  }
};

Just realized this is exactly what the lint rule recommends 馃槄 so not my idea

@skudbucket
Copy link

I wanted to add that implementing this (or the proposal to make toThrow accept a callback) would also make it easier to to deal with dynamic error messages. For example, if the error message was something like Error: This part of the error message is static but then there is ${someValueThatIDoNotWantJestToCareAbout}, I would want my test to check whether the error message contained just the static part of the error.

The only ways to do that I'm currently aware of are by using a try/catch block (which I don't want to do because it is discouraged for reasons that seem valid), or writing a custom matcher (which I don't want to do either because I would rather not have to learn how to just to accomplish something that seems like it ought to be fairly simple).

I'll probably dig into writing a custom matcher, but FWIW I think the usability of Jest would be significantly improved if it was easier to make assertions about errors other than the message exactly matching a particular string.

Copy link

This issue is stale because it has been open for 1 year with no activity. Remove stale label or comment or this will be closed in 30 days.

@github-actions github-actions bot added the Stale label May 16, 2024
@skudbucket
Copy link

This still seems like a legit issue to me (correct me if I'm wrong), so just commenting to keep the bot from killing it as stale (I couldn't remove the stale label)

@github-actions github-actions bot removed the Stale label May 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.