src: reduce redundant "error:" for inspect and findrefs

oyyd · mmarchini · commit 74414fc26a1f · 2019-07-19T10:14:39.000-07:00
When `inspect` or `findrefs` an invalid expr, llnode will print an error message prefixed with "error: error: error:". This PR will reduce one redundant "error:" for such scenario and it seems that we can't remove the other redundant "error:" by calling SB API only. PR-URL: #285 Reviewed-By: Matheus Marchini <mat@mmarchini.me>
diff --git a/src/llnode.cc b/src/llnode.cc
@@ -192,10 +192,8 @@ bool PrintCmd::DoExecute(SBDebugger d, char** cmd,
   SBExpressionOptions options;
   SBValue value = target.EvaluateExpression(full_cmd.c_str(), options);
   if (value.GetError().Fail()) {
-    SBStream desc;
-    if (value.GetError().GetDescription(desc)) {
-      result.SetError(desc.GetData());
-    }
+    SBError error = value.GetError();
+    result.SetError(error);
     result.SetStatus(eReturnStatusFailed);
     return false;
   }
diff --git a/src/llscan.cc b/src/llscan.cc
@@ -504,10 +504,8 @@ bool FindReferencesCmd::DoExecute(SBDebugger d, char** cmd,
       SBExpressionOptions options;
       SBValue value = target.EvaluateExpression(full_cmd.c_str(), options);
       if (value.GetError().Fail()) {
-        SBStream desc;
-        if (value.GetError().GetDescription(desc)) {
-          result.SetError(desc.GetData());
-        }
+        SBError error = value.GetError();
+        result.SetError(error);
         result.SetStatus(eReturnStatusFailed);
         return false;
       }
diff --git a/test/plugin/inspect-test.js b/test/plugin/inspect-test.js
@@ -546,7 +546,7 @@ function verifyHashMap(t, sess) {
     const parent = 'hashmap';
     const addresses = collectMembers(
         t, lines.join('\n'), hashMapTests, parent);
-    verifyMembers(t, sess, addresses, hashMapTests, parent, teardown);
+    verifyMembers(t, sess, addresses, hashMapTests, parent, verifyInvalidExpr);
   });
 }
 
@@ -614,6 +614,20 @@ function verifyMembers(t, sess, addresses, tests, parent, next) {
   verifyProperty(t, sess, addresses, 0);
 }
 
+function verifyInvalidExpr(t, sess) {
+  sess.send('v8 inspect invalid_expr');
+  sess.waitError(/error:/, (err, line) => {
+    if (err) {
+      return teardown(t, sess, err);
+    }
+    t.ok(
+      /error: error: use of undeclared identifier 'invalid_expr'/.test(line),
+      'invalid expression should return an error'
+    );
+    teardown(t, sess);
+  });
+}
+
 tape('v8 inspect', (t) => {
   t.timeoutAfter(15000);
 
diff --git a/test/plugin/scan-test.js b/test/plugin/scan-test.js
@@ -22,6 +22,18 @@ tape('v8 findrefs and friends', (t) => {
   }
 });
 
+function testFindrefsForInvalidExpr(t, sess, next) {
+  sess.send('v8 findrefs invalid_expr');
+  sess.waitError(/error:/, (err, line) => {
+    t.error(err);
+    t.ok(
+      /error: error: use of undeclared identifier 'invalid_expr'/.test(line),
+      'invalid expression should return an error'
+    );
+    next();
+  });
+}
+
 function test(executable, core, t) {
   const sess = common.Session.loadCore(executable, core, (err) => {
     t.error(err);
@@ -32,7 +44,6 @@ function test(executable, core, t) {
     sess.send('version');
   });
 
-
   sess.linesUntil(versionMark, (err, lines) => {
     t.error(err);
     t.ok(/\d+ Class/.test(lines.join('\n')), 'Class should be in findjsobjects');
@@ -157,7 +168,6 @@ function test(executable, core, t) {
       sess.send(`v8 findrefs ${match[1]}`);
     }
     t.ok(found, 'Zlib should be in findjsinstances');
-
     // Just a separator
     sess.send('version');
   });
@@ -181,8 +191,13 @@ function test(executable, core, t) {
       t.error(err)
       if(hasSymbol)
         t.ok(/Context\.scopedAPI/.test(lines.join('\n')), 'Should find reference #4');
-      sess.quit();
-      t.end();
+
+      // `waitError()` don't share the same `waitQueue` with `wait()` so that
+      // we add the function below to delay the event registration.
+      testFindrefsForInvalidExpr(t, sess, () => {
+        sess.quit();
+        t.end();
+      });
     });
   });
 }
