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 not throwing with v22.4.0 #5634

Closed
tkrotoff opened this issue Feb 21, 2018 · 13 comments
Closed

console.assert not throwing with v22.4.0 #5634

tkrotoff opened this issue Feb 21, 2018 · 13 comments

Comments

@tkrotoff
Copy link
Contributor

tkrotoff commented Feb 21, 2018

According to NodeJS v8.x doc:

a falsy assertion will cause an AssertionError to be thrown

console.assert(false) does not throw with v22.4.0 (was working at least with v22.1.4):

// Does not work anymore
expect(() => console.assert(false, 'message')).toThrow('message');
@SimenB
Copy link
Member

SimenB commented Feb 21, 2018

/cc @ranyitz

@ranyitz
Copy link
Contributor

ranyitz commented Feb 21, 2018

We had a discussion regarding the subject on #5576.

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.

Functionality approximating that implemented by browsers can be implemented by extending Node.js' console and overriding the console.assert() method.

That's what we're doing and I think that it's better when console.assert does not throw, like in the browser. If we'll decide differently though - we'll need to detect jsdom/node in the console or create a different console for each.

/cc @thymikee

@SimenB
Copy link
Member

SimenB commented Feb 21, 2018

Might be worth a flag? Not sure

@thymikee
Copy link
Collaborator

Yep that was our decision. Maybe faulty? I don't have strong feelings about it. @mjesun @cpojer what do you think?

@tkrotoff
Copy link
Contributor Author

A testing environment (Jest) being more strict than the final running environment (browser) is not a bad thing and probably wanted.

The browser has a different goal: make the end user happy VS a testing environment validates code is rock solid.

Do we want a unit test to succeed even if the code tested contains an assert that fails?

Did you have developers complaining about console.assert() throwing in Jest and not behaving exactly like the browser?


My use case:
In the context of a React library, I use console.assert() as "guards" for internal errors (kind of "constraint programming"), not for the user (for this case there is invariant, throw, PropTypes...).
And I like to test the "guards" are here and working thx to Jest.

function removeItem(itemName) {
  console.assert(list[itemName] !== undefined, `Unknown item '${itemName}'`);
  delete this[itemName];
}

test('removeItem() with unknown item', () => {
  expect(() => removeItem('unknown')).toThrow("Unknown item 'unknown'");
});

These "guards" should never fail in production and can/should be stripped away from the production code.

@mjesun
Copy link
Contributor

mjesun commented Feb 22, 2018

My two cents on that:

  • I assume that the console.assert behavior was changed because there were some mismatches or complains before; which means that going all-in into one or another approach will not make either side happy.

  • Jest is node based, so I would expect that the default behavior matches the one in Node. So by default, the console.assert implementation should throw, as it does in Node. Also, jest-environment-node is the default environment, which makes it consistent with this behavior.

  • The jest-environment-jsdom, which emulates a browser, should also emulate the console.assert behavior in a browser, to be compliant with what browsers do. Something simple like console.assert = (...args) => console.error(...args) should be enough to mimic it.

@thymikee
Copy link
Collaborator

@mjesun Actually the default env is jest-environment-jsdom :D https://github.com/facebook/jest/blob/497be7627ef851c947da830d4a8e21046f847a78/packages/jest-config/src/defaults.js#L63
But I agree with the rest. Anybody willing to send a PR with a fix and a test?

@mjesun
Copy link
Contributor

mjesun commented Feb 22, 2018

😆 I'm too used to my internal stuff

@rickhanlonii
Copy link
Member

Agree with @mjesun and @thymikee, by default the semantics of assert should match the environment being emulated. Sounds like we broke that in node but it's working as expected in jsdom

@tkrotoff to get the assert semantics you're looking for in a jsdom environment, you can override console.assert to the behavior you want. To do that you can add this code to a setupFile:

console.assert = (statement, message) => {
  if (!statement) throw new Error(message);
};

That will fail tests like this:

image

@tkrotoff
Copy link
Contributor Author

tkrotoff commented Feb 22, 2018

@rickhanlonii

This is what I do by redirecting console.assert to Node' assert:

// File jest.setup.ts
import assert from 'assert';
console.assert = assert;
// File jest.config.js
const config = {
  setupFiles: ['./jest.setup.ts']
};
module.exports = config;

tkrotoff added a commit to tkrotoff/react-form-with-constraints that referenced this issue Feb 23, 2018
See console.assert not throwing with v22.4.0 jestjs/jest#5634
tkrotoff added a commit to tkrotoff/react-form-with-constraints that referenced this issue Feb 23, 2018
See console.assert not throwing with v22.4.0 jestjs/jest#5634
tkrotoff added a commit to tkrotoff/react-form-with-constraints that referenced this issue Apr 22, 2018
See console.assert not throwing with v22.4.0 jestjs/jest#5634
tkrotoff added a commit to tkrotoff/react-form-with-constraints that referenced this issue Apr 23, 2018
See console.assert not throwing with v22.4.0 jestjs/jest#5634
tkrotoff added a commit to tkrotoff/react-form-with-constraints that referenced this issue Apr 23, 2018
See console.assert not throwing with v22.4.0 jestjs/jest#5634
tkrotoff added a commit to tkrotoff/react-form-with-constraints that referenced this issue Apr 23, 2018
See console.assert not throwing with v22.4.0 jestjs/jest#5634
@jeysal
Copy link
Contributor

jeysal commented Jun 14, 2018

console.assert has been changed to not throw in Node 10 (nodejs/node#17706), so all environments now comply with the console spec.
Users of earlier Node versions can use the "polyfill" mentioned earlier.
I think this can be closed.

@SimenB
Copy link
Member

SimenB commented Jun 14, 2018

I agree, thanks for following up!

@SimenB SimenB closed this as completed Jun 14, 2018
webkit-commit-queue pushed a commit to WebKit/WebKit that referenced this issue Feb 12, 2021
…nding of completion emails regardless of test name

https://bugs.webkit.org/show_bug.cgi?id=221712

Patch by Roy Reapor <rreapor@apple.com> on 2021-02-12
Reviewed by Dewei Zhu.

Rule `platforms` and `tests` can be undefined, an empty array, or an array of strings. Undefined will match everything.
Empty array will match nothing. Array of strings will match items in the array.

Rule will not match if either `tests` or `platforms` is an empty array.

* tools/js/analysis-results-notifier.js:
(AnalysisResultsNotifier._validateRules.isUnderfinedOrEmptyArrayOrArrayOfStrings):
- `platforms` and `tests` can now be undefined or an empty array officially
(AnalysisResultsNotifier._validateRules):
- switched to `assert.ok()`. `console.assert()` no longer throws since node v10 (jestjs/jest#5634)
- both rule `platforms` and `tests` must pass `isUnderfinedOrEmptyArrayOrArrayOfStrings()`. previously, rules like `{tests: [1, 3], platforms: ['speedometer']}` passes validation.
(AnalysisResultsNotifier._applyUpdate):
- switched to `assert.ok()`. `console.assert()` no longer throws since node v10 (jestjs/jest#5634)
(AnalysisResultsNotifier._validateRules.isNonemptyArrayOfStrings): Deleted.
* unit-tests/analysis-results-notifier-tests.js:
- added a bunch of unittests
- specify the exact regex match for `assert.throws()` and `assert.doesNotThrow()` argument.

Canonical link: https://commits.webkit.org/234049@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@272813 268f45cc-cd09-0410-ab3c-d52691b4dbfc
philn pushed a commit to philn/old-webkit that referenced this issue Feb 21, 2021
…nding of completion emails regardless of test name

https://bugs.webkit.org/show_bug.cgi?id=221712

Patch by Roy Reapor <rreapor@apple.com> on 2021-02-12
Reviewed by Dewei Zhu.

Rule `platforms` and `tests` can be undefined, an empty array, or an array of strings. Undefined will match everything.
Empty array will match nothing. Array of strings will match items in the array.

Rule will not match if either `tests` or `platforms` is an empty array.

* tools/js/analysis-results-notifier.js:
(AnalysisResultsNotifier._validateRules.isUnderfinedOrEmptyArrayOrArrayOfStrings):
- `platforms` and `tests` can now be undefined or an empty array officially
(AnalysisResultsNotifier._validateRules):
- switched to `assert.ok()`. `console.assert()` no longer throws since node v10 (jestjs/jest#5634)
- both rule `platforms` and `tests` must pass `isUnderfinedOrEmptyArrayOrArrayOfStrings()`. previously, rules like `{tests: [1, 3], platforms: ['speedometer']}` passes validation.
(AnalysisResultsNotifier._applyUpdate):
- switched to `assert.ok()`. `console.assert()` no longer throws since node v10 (jestjs/jest#5634)
(AnalysisResultsNotifier._validateRules.isNonemptyArrayOfStrings): Deleted.
* unit-tests/analysis-results-notifier-tests.js:
- added a bunch of unittests
- specify the exact regex match for `assert.throws()` and `assert.doesNotThrow()` argument.

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@272813 268f45cc-cd09-0410-ab3c-d52691b4dbfc
@github-actions
Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants