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,test: make message test more accurate #14580

Closed
wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Aug 1, 2017

Make a message test more accurate in what it’s testing for.
This requires not swallowing stack overflow RangeErrors in
console.log and similar methods, which I would consider a
bugfix in itself.

Fixes: nodejs/node-v8#5 (i.e. this is required for upgrading to V8 > 6.0)

/cc @targos

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

console, test

Make a message test more accurate in what it’s testing for.
This requires not swallowing stack overflow RangeErrors in
`console.log` and similar methods, which I would consider a
bugfix in itself.

Fixes: nodejs/node-v8#5
@addaleax addaleax added console Issues and PRs related to the console subsystem. dont-land-on-v4.x test Issues and PRs related to the tests. labels Aug 1, 2017
@nodejs-github-bot nodejs-github-bot added the console Issues and PRs related to the console subsystem. label Aug 1, 2017
compiledConsole = consoleDescriptor.get();
if (compiledConsole.log) {
// Using `console.log` itself might not succeed yet, but the code for it
// has been compiled.
} else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove this branch?

if (!compiledConsole.log) {
  throw e;
}

If you think it's clearer as is, no problem, ignore the comment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing it out, but yes, I think it’s clearer this way.

@targos
Copy link
Member

targos commented Aug 3, 2017

@@ -91,6 +91,10 @@ function write(ignoreErrors, stream, string, errorhandler) {

stream.write(string, errorhandler);
} catch (e) {
// console is a debugging utility, so it swalling errors is not desirable
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: swallowing

targos pushed a commit to targos/node that referenced this pull request Aug 3, 2017
Make a message test more accurate in what it’s testing for.
This requires not swallowing stack overflow RangeErrors in
`console.log` and similar methods, which I would consider a
bugfix in itself.

PR-URL: nodejs#14580
Fixes: nodejs/node-v8#5
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@targos
Copy link
Member

targos commented Aug 3, 2017

Landed in fb3d0e2 (with fixed typo)

@targos targos closed this Aug 3, 2017
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops just missed it...

const console = consoleDescriptor.get();
if (console.log) {
console.log('Hello, World!');
compiledConsole = consoleDescriptor.get();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a check that e.message === 'Maximum call stack size exceeded' (should be assert, but we can assume the stack is full)

compiledConsole = consoleDescriptor.get();
if (compiledConsole.log) {
// Using `console.log` itself might not succeed yet, but the code for it
// has been compiled.
} else {
throw e;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO you should throw a new Error, one that explicitly represents the failure to compile (could be prepared in advance if there's no room on the stack).

addaleax added a commit that referenced this pull request Aug 7, 2017
Make a message test more accurate in what it’s testing for.
This requires not swallowing stack overflow RangeErrors in
`console.log` and similar methods, which I would consider a
bugfix in itself.

PR-URL: #14580
Fixes: nodejs/node-v8#5
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
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. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Broken message test
6 participants