std assertThrows is acting like assertThrowsError

[mathiasrw]

I just started building everything in Deno - even when node is the target (after a bundle and some creative imports). Absolutely love it.

I found something that seems odd to me in std / assert. I would like to make a PR for it, but first I would like to hear if there is a (good) reason it is currently implemented like this.

The assertThrows as described in https://deno.land/manual/testing/assertions is documented as if it asserts that "something" is being thrown. Yet

Deno.test('Naming is hard',()=>{
	assertThrows(()=>{
		throw "msg"
	})
})

returns AssertionError: A non-Error object was thrown.. You will have to return a proper Error to get a pass

Deno.test('Naming is hard',()=>{
	assertThrows(()=>{
		throw Error("msg")
	})
})

On MDN the throw statement is defined as

It seems odd to demand a thrown exception is of a specific type (Error) when the name assertThrows indicates that it is testing if something is thrown.

I suggest that the current behaviour gets renamed to assertThrowsError and assertThrows is changed to disregard what type is thrown (also it's async sibling).

Let me know if there is some hidden knowledge or good reasons for the current behaviour. And if that is the case I'm happy to make a PR to the docs repo to explain it better to the next dev coming along this path of the world of Deno.

I do understand that it is a breaking change, but better to sort out consistency early than carrying confusion into the future... right? If not, maybe a way forward would be to add assertThrowsException to verify that an exception was thrown but disregard the type.

[nayeemrmn]

We assert that it's an error because it's ergonomic for the callback overload to be able to assume that: https://deno.land/[email protected]/testing/asserts.ts#L615. Names don't need to be 100% precise, assertThrows() is fine. Also std is maintained at https://github.com/denoland/deno_std.

[kt3k]

I think it's ok to allow non-Error thing thrown from the function as it's allowed by the language.

I'm not sure we need assertThrowsError. We can assert that with the call assertThrows(fn, Error).

We assert that it's an error because it's ergonomic for the callback overload to be able to assume that:

Maybe we can check that only when errorCallback is specified, or we can change the errorCallback signature to (e: unknown) => unknown

[mathiasrw]

We can assert that with the call assertThrows(fn, Error).

That is a much better solution. We should be able to provide null as second parameter so we can reach the 3rd parameter to set msgIncludes.

[mathiasrw]

Im diving in.

After cloning and running deno test --unstable --allow-all I get 39 failed tests.

Not sure what is going on. The only thing I can think about is: Could this be caused by using a M1 CPU?

failures:

	[std/datetime] dayOfYear
	std_env_args.wasm
	std_env_vars.wasm
	std_fs_create_dir.wasm
	std_fs_file_create.wasm
	std_fs_file_metadata.wasm
	std_fs_file_seek.wasm
	std_fs_file_set_len.wasm
	std_fs_file_sync_all.wasm
	std_fs_file_sync_data.wasm
	std_fs_hard_link.wasm
	std_fs_metadata.wasm
	std_fs_read.wasm
	std_fs_read_dir.wasm
	std_fs_remove_dir_all.wasm
	std_fs_rename.wasm
	std_fs_symlink_metadata.wasm
	std_fs_write.wasm
	std_io_stderr.wasm
	std_io_stdin.wasm
	std_io_stdout.wasm
	std_process_exit.wasm
	wasi_clock_res_get.wasm
	wasi_clock_time_get.wasm
	wasi_fd_fdstat_get.wasm
	wasi_fd_fdstat_get.wasm
	wasi_fd_fdstat_get.wasm
	wasi_fd_renumber.wasm
	wasi_fd_tell_file.wasm
	wasi_fd_write_file.wasm
	wasi_fd_write_stderr.wasm
	wasi_fd_write_stdout.wasm
	wasi_path_open.wasm
	wasi_proc_exit.wasm
	wasi_random_get.wasm
	wasi_sched_yield.wasm
	std_io_stdin.wasm with stdin as file
	std_io_stdout.wasm with stdout as file
	std_io_stderr.wasm with stderr as file

All except the first one fails with NotFound: No such file or directory (os error 2). The first error is a result that is off by one compared to the expected

[kt3k]

@mathiasrw Have you initialized git-submodules?

git submodule update --init

[KyleJune]

How about changing assertThrows and assertRejects to only assert that their was an exception thrown or rejected? If those functions were changed to take an optional expectation, these functions could be used for asserting specific exceptions or that their was an exception. Having it return the unknown exception would allow us to make additional assertions on the exception if they are more complex than a simple comparison.

export function assertThrows(
  fn: () => unknown,
  expected?: unknown,
): unknown;

export function assertRejects(
  fn: () => unknown,
  expected?: unknown,
): unknown;

Then to keep the ability for checking errors, we could add assertError as a separate function since it's common for people to want to make assertions about the errors that were thrown or rejected.

// with variable
let error = assertThrows(() => func());
assertError(error, CustomError, "Fail");
// one liner
assertError(assertThrows(() => func()), CustomError, "Fail");

An example of what assertError could look like can be found here: #1376

By decoupling assertError from assertThrows and assertRejects, it would also make it easy for people to be able to make assertions about errors that were already caught. Having assertThrows and assertRejects return the exception would also get rid of the need to have an errorCallback option since users could just make additional assertions on the returned value rather than in a callback.

Another example I thought of where it would be useful to have assertError as a separate function would be where an error has a cause. MDN shows that you can add an error onto a new error as a cause here.

let error = assertThrows(() => func());
assertError(error, CustomError, "Fail");
assertError(error.cause, AnotherCustomError, "Fail");

[kt3k]

@KyleJune

How about changing assertThrows and assertRejects to only assert that their was an exception thrown or rejected? If those functions were changed to take an optional expectation, these functions could be used for asserting specific exceptions or that their was an exception.

I'm in favor of this change

Having it return the unknown exception would allow us to make additional assertions on the exception if they are more complex than a simple comparison.

We had discussion about this, and decided we don't make assert functions return values. ref: #1219 (comment) ref: #1052 (comment)

So I think we should keep the return type void here

[KyleJune]

@KyleJune

How about changing assertThrows and assertRejects to only assert that their was an exception thrown or rejected? If those functions were changed to take an optional expectation, these functions could be used for asserting specific exceptions or that their was an exception.

I'm in favor of this change

Having it return the unknown exception would allow us to make additional assertions on the exception if they are more complex than a simple comparison.

We had discussion about this, and decided we don't make assert functions return values. ref: #1219 (comment) ref: #1052 (comment)

So I think we should keep the return type void here

In that case and for backwards compatibility, I think we should keep the current call signature so that their is a shorthand way to make assertions about exceptions being errors(common case), but if user provides a callback instead use it instead of assertError so that users can make their own assertions about the exception.

Currently when a callback is passed it asserts the exception is an error and optionally asserts that error has a msg before calling the callback. Since my suggestion is not doing error assertions when passing an assertion callback, I believe the last optional msg arg should be deprecated.

I believe assertError should be exposed for if users want to use the callback signature of assertThrows or assertRejects but still want to make the error assertions on it or on error.cause. I would also remove the optional callback signature from my current assertError implementation since the error would be a variable where it would be used.

[kt3k]

I would also remove the optional callback signature from my current assertError implementation since the error would be a variable where it would be used.

That sounds reasonable to me. Maybe Let's first only implement assertError? And we can revisit the change to assertThrows/Rejects after that.

[mathiasrw]

Ok. Jumping out of this. Would love to see the change, but not sure I can lift the adjustments suggested.

Update Just realised its being solved as part of #1376 - perfect! :)

[KyleJune]

Ok. Jumping out of this. Would love to see the change, but not sure I can lift the adjustments suggested.

Update Just realised its being solved as part of #1376 - perfect! :)

My PR doesn't solve this, it will just make this change easier. assertThrows and assertRejects will still do error assertions by calling assertError. If it gets merged, all you would have to do is make assertThrows and assertRejects not call assertError if typeof errorCallback == "function". I think it would make sense to have assertThrows and assertRejects also not call assertError when the only argument is fn.

I think if both those changes were made, assertThrows(fn) could be used to just check if anything was thrown, assertThrows(fn, Error, msg) could be shorthand for calling assertError on the value that is thrown, then with a callback like assertThrows(fn, (e) => assertEquals(e, "Fail")) could be used to make custom assertions on the value that was thrown.

[mathiasrw]

@KyleJune Ahh, got it. Ill keep an eye on your PR and have a go at this one after the PR has been merged.

[bartlomieju]

I think #1376 solves the issue presented here in the limit. I'm going to close the issue for now, please feel free to comment if the solution is not satisfactory.

[kt3k]

I think this issue is still valid. From my understanding, what this issue suggests is removing the below 3 lines from assertThrows:

https://github.com/denoland/deno_std/blob/6212e9021f7533f106558721909315cf8639de0a/testing/asserts.ts#L682-L684