assertRejects does not fail when an error is thrown, but a promise was not rejected

[dsherret]

Describe the bug

import { assertRejects } from "https://raw.githubusercontent.com/denoland/deno_std/main/testing/asserts.ts";
await assertRejects(function () { throw new Error("test") });

Expected behavior Should throw because no promise was rejected.

Node's assertRejects behaviour:

> assert.rejects(function() { throw new Error("test"); }).catch(err => console.log(err));
> Error: test
    at REPL16:1:35
    at waitForActual (assert.js:786:21)
    at Function.rejects (assert.js:921:31)
    at REPL16:1:8
    at Script.runInThisContext (vm.js:134:12)
    at REPLServer.defaultEval (repl.js:486:29)
    at bound (domain.js:416:15)
    at REPLServer.runBound [as eval] (domain.js:427:12)
    at REPLServer.onLine (repl.js:819:10)
    at REPLServer.emit (events.js:387:35)

This might just be an issue with the renaming from assertThrowsAsync -> assertRejects and perhaps we should keep assertThrowsAsync then change assertRejects to assert that a promise is rejected?

[kt3k]

hmm I'm not sure there's enough needs of the suggested version of assertRejects. I personally usually don't distinguish these 2 behaviors when writing async functions..

It feels a little too pedantic to me if we have both assertReject and assertThrowsAsync with the above difference.

cc @caspervonb What do you think about this suggestion?

[caspervonb]

It does what it says on the tin:

Executes a function which returns a promise, expecting it to throw or reject.

• Definitively not a bug, does what it is documented to do.
• assertThrowsAsync should never make a return imho, terrible name and the differences are too small.

await assertRejects(function () { throw new Error("test") });

Thinking about this, I'm actually a bit surprised that this isn't a type-error.

I think maybe we want this to assert (both at compile time and runtime) that a promise was actually returned, you can always do Promise.resolve turn a value into a promise. Seems that is the real source of this category of errors, if the thing is a promise then everything is fine. But in this case it isn't and you can't really assert rejection on a non promise.

[dsherret]

Here's showing the critical difference between the two and something that will pass type checking:

function getFunc(throwsSync: boolean) {
  return () => {
    if (throwsSync) {
      throw new Error();
    }

    return Promise.reject(new Error());
  };
}

await assertRejects(getFunc(false)); // pass, ok
await assertRejects(getFunc(true)); // pass, not ok

// why this matters
myFunctionToTest(false).catch(err => { /* ok */ });
myFunctionToTest(false).catch(err => {}); // function throws even though assertRejects tests passed

A more common scenario of using this might be along the lines of:

const myInstance = new Something();

await assertRejects(() => myInstance.doAction());

I think we should...

1. Change assertRejects to only pass when the function executed returns a promise that rejects.
2. If the function throws and doesn't return a promise that rejects then in addition to failing we should give a bit of an explanatory message to the user.
3. In addition to a function, allow a promise as a parameter for assertRejects.

[KyleJune]

I think we should...

1. Change assertRejects to only pass when the function executed returns a promise that rejects.

2. If the function throws and doesn't return a promise that rejects then in addition to failing we should give a bit of an explanatory message to the user.

3. In addition to a function, allow a promise as a parameter for assertRejects.

I agree with @dsherret and was about to make an issue before I found this one. Based on the name, users would expect the assertion to only pass if the function rejected. If you depend on a function correctly rejecting errors instead of throwing them, this could result in bugs making it past tests. In the example he made, they may have test coverage showing that they can catch if it has an error, but then when people actually use it that way, it fails to behave the way it's expected to.

If someone has a use case where they expect a function to throw an error instead of rejecting, they should use assertThrows instead of assertRejects to make that assertion.

[KyleJune]

Here is another example demonstrating why it matters. In it, all 3 example functions are expected to return a promise. The exampleC function doesn't return a promise and instead throws. All of the assertRejects calls pass, making users think they can catch any errors as a rejection. From the results of running this script, you can see it doesn't log that exampleC() rejects and the error goes uncaught.

import { assertRejects } from "https://deno.land/[email protected]/testing/asserts.ts";

type AsyncFunc = () => Promise<boolean>;

const exampleA: AsyncFunc = async () => {
  const fail = true;
  if (fail) {
    throw new Error("fail");
  }
  return await Promise.resolve(true);
}

const exampleB: AsyncFunc = () => {
  const fail = true;
  if (fail) {
    return Promise.reject(new Error("fail"));
  }
  return Promise.resolve(true);
}

const exampleC: AsyncFunc = () => {
  const fail = true;
  if (fail) {
    throw new Error("fail");
  }
  return Promise.resolve(true);
}

console.log('before assertRejects calls');
await assertRejects(exampleA);
await assertRejects(exampleB);
await assertRejects(exampleC);
console.log('after assertRejects calls');

await exampleA().catch(() => console.log("exampleA() rejects"));
console.log("after exampleA()");
await exampleB().catch(() => console.log("exampleB() rejects"));
console.log("after exampleB()");
await exampleC().catch(() => console.log("exampleC() rejects"));
console.log("after exampleC()");

deno run example.ts
Check file:///example.ts
before assertRejects calls
after assertRejects calls
exampleA() rejects
after exampleA()
exampleB() rejects
after exampleB()
error: Uncaught Error: fail
    throw new Error("fail");
          ^
    at exampleC (file:///example.ts:24:11)
    at file:///example.ts:39:7

[bartlomieju]

@dsherret @KyleJune I think your arguments are valid and I'm in favor of changing the behavior of assertRejects.

[Sorikairox]

I was working on it, but I got the following problem :

With the change, how do we assert async functions that throw instead of returning a rejecting Promise? I wanted to test the new assertRejects implementation, but it is an async function that throws 😓

assertThrows is not asynchronous, it only takes a non-async function as a parameter. I can make it async, but that would be another breaking change on top of this issue breaking change.

I am in favor of changing assertThrows so it can take an async function as well so we can do :

await assertThrows(myAsyncFunctionThatThrows());

Otherwise, we could do something like this but it isn't as suitable as using assertThrows IMO:

try {
    await myAsyncFunctionThatThrows();
    unreachable();
  } catch (e) {
    assertIsError(e, WhatEverErrorIsThrown);
  }

[dsherret]

With the change, how do we assert async functions that throw instead of returning a rejecting Promise?

I think we can leave assertThrows as-is and people could use it for this. Does it not work as-is?

[Sorikairox]

It sadly doesn't work as-is. assertThrows cannot take async functions/Promise as parameters.

Deno.test("testingAssertRejectsWithThrowFailAsItExpectARejection", async () => {
  assertThrows(assertRejects(() => {
    throw new Error();
  }));
});

Gives the following error :

Argument of type 'Promise<void>' is not assignable to parameter of type '() => unknown'.

[dsherret]

@Sorikairox they can use assertThrows for functions that throw before returning a promise. async functions return a promise that rejects:

async function asyncThrow() {
  throw new Error();

  return 5;
}

asyncThrow().catch(err => {}); // does not throw synchronously

function syncThrow() {
  throw new Error();

  return Promise.resolve(5);
}

assertThrows(syncThrow); // passes

[Sorikairox]

@dsherret That is my point. You can use assertThrows with syncThrow but not with asyncThrow. We cannot ask people to rewrite their async function to be used with assertThrows as they won't be able to use them with assertRejects (to check that they throw) anymore after the behavior change brought up by this issue 👀

[KyleJune]

With the change, how do we assert async functions that throw instead of returning a rejecting Promise? I wanted to test the new assertRejects implementation, but it is an async function that throws sweat

Async functions capture thrown errors and turn them into promise rejections. If the function wasn't actually async and just returns a promise normally and it were to throw instead, the assertRejects assertion should fail because the function doesn't reject, it throws.

assertThrows is not asynchronous, it only takes a non-async function as a parameter. I can make it async, but that would be another breaking change on top of this issue breaking change.

I think if assertThrows returns a rejected promise, it would fail because the function didn't throw. I think that would be desired behavior.

Having this behavior would make it easier to catch and fix issues like errors being thrown incorrectly instead of being rejected and vice versa.

[dsherret]

We cannot ask people to rewrite their async function to be used with assertThrows as they won't be able to use them with assertRejects (to check that they throw) anymore after the behavior change brought up by this issue

I think I'm not understanding what you mean. People would continue using assertRejects for async functions after this change to my understanding.

[Sorikairox]

Async functions capture thrown errors and turn them into promise rejections. If the function wasn't actually async and just returns a promise normally and it were to throw instead, the assertRejects assertion should fail because the function doesn't reject, it throws.

It is the behavior I implemented 😄

But, as I was writing tests for assertRejects new behavior, I could not assert with assertThrows that assertRejects throws an error because assertRejects is an async function and assertThrows do not take async functions as parameter.

[Sorikairox]

I think I'm not understanding what you mean. People would continue using assertRejects for async functions after this change to my understanding.

My bad, I might not be as clear as I think I am.

Before changes :

• assert that a function throws => use assertThrows
• assert that an async function rejects => use assertRejects
• assert that an async function throws => use assertRejects

After changes:

• assert that a function throws => use assertThrows
• assert that an async function rejects => use assertRejects
• assert than an async function throws => cannot use assertThrows (do no take async function as parameter) nor assertRejects (that now fail because tested function throws instead of rejects), what do we do then

[dsherret]

View async functions as only rejecting a promise because that's what they do.

Before changes :

• assert that a function throws => use assertThrows
• assert that an async function rejects/throws => use assertRejects
• assert that a function that returns a promise throws synchronously => use assertThrows, but right now assertRejects incorrectly passes

After changes:

• assert that a function throws => use assertThrows
• assert that an async function rejects/throws => use assertRejects
• assert that a function that returns a promise throws synchronously => use assertThrows and assertRejects should fail because it throws synchronously

[Sorikairox]

Okay my bad, I totally missed the point of synchronicity, which was the subject of this issue, and went in an insanely wrong direction. Doesn't feel good to be dumb 😆

Sorry for (unintentionally) wasting your time.

[dsherret]

@Sorikairox that's ok! It's a very particular behaviour 😄

No, not a time waste. We're on the same page now and thanks for contributing this!

[kamilogorek]

Fixed in https://github.com/denoland/deno_std/pull/2234