fix: warn if maxDataSize=0 (#744)

kjin · web-flow · commit e322b6ca0763 · 2019-08-19T10:33:43.000-07:00
Also, this is not reported in breakpoints in Node 12+, so modify
a test to reflect this.
diff --git a/src/agent/debuglet.ts b/src/agent/debuglet.ts
@@ -54,9 +54,10 @@ const NODE_VERSION_MESSAGE =
   'Node.js version not supported. Node.js 5.2.0 and ' +
   'versions older than 0.12 are not supported.';
 const NODE_10_CIRC_REF_MESSAGE =
-  'capture.maxDataSize=0 is not recommended on older versions of Node' +
-  ' 10/11. See https://github.com/googleapis/cloud-debug-nodejs/issues/516' +
-  ' for more information.';
+  'capture.maxDataSize=0 is not recommended on older versions of Node 10/11' +
+  ' and Node 12.' +
+  ' See https://github.com/googleapis/cloud-debug-nodejs/issues/516 for more' +
+  ' information.';
 const BREAKPOINT_ACTION_MESSAGE =
   'The only currently supported breakpoint actions' + ' are CAPTURE and LOG.';
 
@@ -453,7 +454,7 @@ export class Debuglet extends EventEmitter {
     if (
       this.config.capture &&
       this.config.capture.maxDataSize === 0 &&
-      utils.satisfies(process.version, '>=10 <10.15.3 || >=11 <11.7')
+      utils.satisfies(process.version, '>=10 <10.15.3 || >=11 <11.7 || >=12')
     ) {
       that.logger.warn(NODE_10_CIRC_REF_MESSAGE);
     }
diff --git a/test/test-circular.ts b/test/test-circular.ts
@@ -25,6 +25,7 @@ import * as debugapi from '../src/agent/v8/debugapi';
 import consoleLogLevel = require('console-log-level');
 import * as stackdriver from '../src/types/stackdriver';
 import {Variable} from '../src/types/stackdriver';
+import {satisfies} from '../src/agent/util/utils';
 
 const code = require('./test-circular-code.js');
 
@@ -42,7 +43,19 @@ function stateIsClean(api: debugapi.DebugApi): boolean {
   return true;
 }
 
-describe(__filename, () => {
+/**
+ * Stable object ID was reverted in V8 7.3 (Node 12), so circular objects will
+ * not work in the near future. For now, we warn if the user specifies
+ * maxDataSize = 0, as this will cause the app to crash.
+ */
+const maybeDescribe = satisfies(
+  process.version,
+  '>=10 <10.15.3 || >=11 <11.7 || >=12'
+)
+  ? describe.skip
+  : describe;
+
+maybeDescribe(__filename, () => {
   const config = Object.assign({}, defaultConfig, {
     workingDirectory: __dirname,
     forceNewAgent_: true,
@@ -76,8 +89,6 @@ describe(__filename, () => {
   afterEach(() => {
     assert(stateIsClean(api));
   });
-  // TODO(kjin): Re-enable this test after issue #742 has been addressed
-  /*
   it('Should be able to read the argument and the context', done => {
     // TODO: Have this actually implement Breakpoint
     const brk: stackdriver.Breakpoint = {
@@ -142,5 +153,4 @@ describe(__filename, () => {
       process.nextTick(code.foo.bind({}));
     });
   });
-  */
 });
diff --git a/test/test-this-context.ts b/test/test-this-context.ts
@@ -78,8 +78,6 @@ describe(__filename, () => {
   afterEach(() => {
     assert(stateIsClean(api));
   });
-  // TODO(kjin): Re-enable this test after issue #742 has been addressed
-  /*
   it('Should be able to read the argument and the context', done => {
     // TODO: Have this actually implement Breakpoint
     const brk: stackdriver.Breakpoint = {
@@ -104,7 +102,7 @@ describe(__filename, () => {
         );
         assert.deepStrictEqual(ctxMembers![0], {name: 'a', value: '10'});
         assert.strictEqual(args.length, 0, 'There should be zero arguments');
-        if (utils.satisfies(process.version, '>=11')) {
+        if (utils.satisfies(process.version, '>=11 && <12')) {
           assert.strictEqual(locals.length, 3, 'There should be three locals');
           assert.deepStrictEqual(locals[0].name, 'this');
           assert.deepStrictEqual(locals[1], {name: 'b', value: '1'});
@@ -122,7 +120,6 @@ describe(__filename, () => {
       process.nextTick(code.foo.bind({}, 1));
     });
   });
-  */
   it('Should be able to read the argument and deny the context', done => {
     // TODO: Have this actually implement Breakpoint
     const brk = {
