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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

console.assert() prints 'Assertion failed' when called with no arguments #34500

Closed
ghost opened this issue Jul 23, 2020 · 6 comments
Closed

console.assert() prints 'Assertion failed' when called with no arguments #34500

ghost opened this issue Jul 23, 2020 · 6 comments
Labels
console Issues and PRs related to the console subsystem.

Comments

@ghost
Copy link

ghost commented Jul 23, 2020

  • Version: 14.6.0
  • Platform: Darwin Kernel Version 19.5.0 root:xnu-6153.121.2~2/RELEASE_X86_64 x86_64
  • Subsystem: console

What steps will reproduce the bug?

  1. Create a file, e.g. index.js
  2. Add the following content:
console.assert();
  1. Run the file with the following command: node index.js
  2. Observe the console output: Assertion failed

How often does it reproduce? Is there a required condition?

Always. There are no specific conditions.

What is the expected behavior?

I guess it should not output anything in this case (or at least tell the user that they should specify an expression/value).
The typings provided by the lib.dom.d.ts (VS Code) specify that the assert method has the following signature:

assert(condition?: boolean, ...data: any[]): void;

Therefore, the value to be checked is optional -> we may not have it in place. Thus, we should handle this possibility.

Even if we do not take into account the TS typings, Node.js docs state as follows:

A simple assertion test that verifies whether value is truthy. If it is not, Assertion failed is logged. 

In the mentioned case we do not have any value to perform a check on, therefore, the result of the call is not entirely correct.

So, if the first argument that would be evaluated is an optional parameter, assert should not print any messages. So, we can handle this situation or at least do nothing in such situations. It seems to be compliant with the Console Standard#assert

Or at least we can add some kind of "warning" in the docs.

What do you see instead?

Console's output: Assertion failed

Additional information

Refs:

P.S. If this should be fixed, I would love to work on that.

@addaleax addaleax added the console Issues and PRs related to the console subsystem. label Jul 23, 2020
@addaleax
Copy link
Member

I think the TypeScript definition may be the one that’s off here – as you note, the Console spec does not mark condition as an optional argument, and to me it seems to match the principle of least surprise if console.assert() does log an assertion error (because the first argument, in that case, is undefined, which is false-y).

@ghost
Copy link
Author

ghost commented Jul 23, 2020

@addaleax yes, your point is more than valid yet the issue here that it's a bit unexpected result. So, I propose either to handle this directly in the code or at least provide some documentation on this.

@addaleax
Copy link
Member

@iandrc How would we document this better on the Node.js side? https://nodejs.org/api/console.html#console_console_assert_value_message and https://developer.mozilla.org/en-US/docs/Web/API/console/assert also don’t mark the argument as optional. We could explicitly state that “Not passing arguments to console.assert() will be treated like console.assert(undefined)”, but the problem I see with that is that it’s true for every JavaScript function, and it feels a bit odd to me to add a remark about it only in one specific location.

@ghost
Copy link
Author

ghost commented Jul 23, 2020

@addaleax that's a tough one. The best idea I've come up so far is to do smth like this (I believe it could be done with a slightly better wording):

A simple assertion test that verifies whether value is truthy. If it is not or no value provided, 
Assertion failed is logged.

I know that's a bit odd proposal, yet that may confuse someone if they'll call console.assert() by mistake. Still, I'm not sure someone will do it on purpose :)

@addaleax
Copy link
Member

@iandrc Yeah, I think that’s just fine. Feel free to open a PR with that if you like :)

@ghost
Copy link
Author

ghost commented Jul 23, 2020

@addaleax will do. Thanks for the good discussion :)

codebytere pushed a commit that referenced this issue Aug 5, 2020
Add a description and an example of console.assert() call with
no arguments. If called like this, the method should output:
"Assertion failed".

Fixes: #34500
Refs: https://nodejs.org/dist/latest-v14.x/docs/api/console.html#console_console_assert_value_message
Refs: https://console.spec.whatwg.org/#assert

PR-URL: #34501
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
addaleax pushed a commit that referenced this issue Sep 22, 2020
Add a description and an example of console.assert() call with
no arguments. If called like this, the method should output:
"Assertion failed".

Fixes: #34500
Refs: https://nodejs.org/dist/latest-v14.x/docs/api/console.html#console_console_assert_value_message
Refs: https://console.spec.whatwg.org/#assert

PR-URL: #34501
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
addaleax pushed a commit that referenced this issue Sep 22, 2020
Add a description and an example of console.assert() call with
no arguments. If called like this, the method should output:
"Assertion failed".

Fixes: #34500
Refs: https://nodejs.org/dist/latest-v14.x/docs/api/console.html#console_console_assert_value_message
Refs: https://console.spec.whatwg.org/#assert

PR-URL: #34501
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
console Issues and PRs related to the console subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant