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

parallel/test-repl-pretty-stack & parallel/test-repl-pretty-custom-stack assert the exact error message coming from V8 #43676

Closed
marjakh opened this issue Jul 4, 2022 · 4 comments
Labels
errors Issues and PRs related to JavaScript errors originated in Node.js core. test Issues and PRs related to the tests. v8 engine Issues and PRs related to the V8 dependency.

Comments

@marjakh
Copy link
Contributor

marjakh commented Jul 4, 2022

Version

No response

Platform

No response

Subsystem

No response

What steps will reproduce the bug?

I'm enhancing the "Unexpected identifier" error message here https://chromium-review.googlesource.com/c/v8/v8/+/3726147 ; these tests would need to be updated once that change rolls into Node, or already in preparation for the change (by relaxing the assertion so that it passes both before and after the change).

Unfortunately, tests which assert the exact error message make improving the error messages more cumbersome than it has to be. :/ I don't have a good solution there; in this particular case it would've helped if the test asserted that there's "Unexpected identifier" somewhere in the error message (but allows more content than that).

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

No response

What is the expected behavior?

No response

What do you see instead?

.

Additional information

No response

@BridgeAR
Copy link
Member

BridgeAR commented Jul 4, 2022

@marjakh it should be fine to update the tests to only match for the Unexpected identifier:

--- a/test/parallel/test-repl-pretty-stack.js
+++ b/test/parallel/test-repl-pretty-stack.js
@@ -26,10 +26,17 @@ function run({ command, expected, ...extraREPLOptions }, i) {
 
   r.write(`${command}\n`);
   console.log(i);
-  assert.strictEqual(
-    accum.replace(stackRegExp, '$1*:*'),
-    expected.replace(stackRegExp, '$1*:*')
-  );
+  if (typeof expected === 'string') {
+    assert.strictEqual(
+      accum.replace(stackRegExp, '$1*:*'),
+      expected.replace(stackRegExp, '$1*:*')
+    );
+  } else {
+    assert.match(
+      accum.replace(stackRegExp, '$1*:*'),
+      expected
+    );
+  }
   r.close();
 }
 
@@ -43,8 +50,7 @@ const tests = [
   },
   {
     command: 'let x y;',
-    expected: 'let x y;\n      ^\n\n' +
-              'Uncaught SyntaxError: Unexpected identifier\n'
+    expected: /^let x y;\n {6}\^\n\nUncaught SyntaxError: .*Unexpected identifier\n/
   },
   {
     command: 'throw new Error(\'Whoops!\')',

@targos targos added errors Issues and PRs related to JavaScript errors originated in Node.js core. test Issues and PRs related to the tests. v8 engine Issues and PRs related to the V8 dependency. labels Jul 4, 2022
@targos
Copy link
Member

targos commented Jul 18, 2022

@BridgeAR would you like to prepare a PR? Otherwise I can start from the diff you posted and do it. There's an example of the test failures with the new V8 in https://ci.nodejs.org/job/node-test-commit-linuxone/33075/

@targos
Copy link
Member

targos commented Jul 25, 2022

Fixed on canary-base. Thanks for reporting!

@targos targos closed this as completed Jul 25, 2022
@BridgeAR
Copy link
Member

@targos thanks for doing that! I missed your comment before.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
errors Issues and PRs related to JavaScript errors originated in Node.js core. test Issues and PRs related to the tests. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

No branches or pull requests

3 participants