fix console & buffered console assert behaviour#5576
Conversation
| LogTimers, | ||
| } from 'types/Console'; | ||
|
|
||
| import assert from 'assert'; |
There was a problem hiding this comment.
This is correct in node env, not in jsdom. Unsure if we care about that differentiation?
There was a problem hiding this comment.
The differences are small, but I'm not sure it should affect this fix
There was a problem hiding this comment.
Node:
Chrome:
The output is pretty different
There was a problem hiding this comment.
@SimenB If I understand correctly, jsdom forwards its output to the Node.js console by default.
I'm not sure that we should print different messages (AssertionError [ERR_ASSERTION]/Assertion failed) depends on the environment.
What do you suggest?
Codecov Report
@@ Coverage Diff @@
## master #5576 +/- ##
==========================================
+ Coverage 60.65% 60.83% +0.18%
==========================================
Files 214 214
Lines 7314 7316 +2
Branches 4 3 -1
==========================================
+ Hits 4436 4451 +15
+ Misses 2877 2864 -13
Partials 1 1
Continue to review full report at Codecov.
|
ranyitz
force-pushed
the
fix-console-assert
branch
from
8fa6f29 to
d27417c
Compare
| try { | ||
| assert(!!args[0], ...args.slice(1)); | ||
| } catch (error) { | ||
| this._log('assert', error.toString()); |
There was a problem hiding this comment.
https://nodejs.org/api/console.html#console_console_assert_value_message_args
Note: The console.assert() method is implemented differently in Node.js than the console.assert() method available in browsers.
Specifically, in browsers, calling console.assert() with a falsy assertion will cause the message to be printed to the console without interrupting execution of subsequent code. In Node.js, however, a falsy assertion will cause an AssertionError to be thrown.
Should we respect that? We'd have to pass environment, or detect jsdom/node otherwise. Personally I'd like console.assert errors to always be caught, like in browsers.
There was a problem hiding this comment.
Yes, I agree with you about it, because otherwise, the assertion error would fail the test.
| if (args[0]) { | ||
| this._log('assert', format(...args.slice(1))); | ||
| try { | ||
| assert(!!args[0], ...args.slice(1)); |
There was a problem hiding this comment.
let's just use assert(...args) since extra arguments are irrelevant to it. Same for buffered console.
ranyitz
force-pushed
the
fix-console-assert
branch
from
2769e7a to
db08ab1
Compare
ranyitz
force-pushed
the
fix-console-assert
branch
from
db08ab1 to
b51559d
Compare
|
@thymikee Is there anything left to do? |
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
6 participants
Summary
This is a fix for #5574
There was a mistake in #5514, where the
console.assertbehaviour was accidenitly implemented wrong.Here there is a fix for the tests and the behaviour to use
assertmodule and log when there is an assertion error.