FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix: address TODOs · googleapis/cloud-debug-nodejs@fd773c8 · GitHub

This repository was archived by the owner on Apr 3, 2024. It is now read-only.
/ cloud-debug-nodejs Public archive

Commit fd773c8

Browse files
kjin
committed
fix: address TODOs
1 parent 246c7c2 commit fd773c8

3 files changed

Lines changed: 61 additions & 113 deletions

File tree

‎src/agent/debuglet.ts‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -49,8 +49,8 @@ const NODE_VERSION_MESSAGE =
4949
'Node.js version not supported. Node.js 5.2.0 and ' +
5050
'versions older than 0.12 are not supported.';
5151
const NODE_10_CIRC_REF_MESSAGE =
52-
'capture.maxDataSize=0 is not recommended on Node 10+. See ' +
53-
'https://github.com/googleapis/cloud-debug-nodejs/issues/516' +
52+
'capture.maxDataSize=0 is not recommended on older versions of Node' +
53+
' 10/11. See https://github.com/googleapis/cloud-debug-nodejs/issues/516' +
5454
' for more information.';
5555
const BREAKPOINT_ACTION_MESSAGE =
5656
'The only currently supported breakpoint actions' +
@@ -407,9 +407,7 @@ export class Debuglet extends EventEmitter {
407407
// This is ignorable.
408408
}
409409

410-
// TODO(kjin): When stableObjectId is added to Node 10, narrow the range
411-
// of Node versions for which this warning applies.
412-
if (utils.satisfies(process.version, '>=10')) {
410+
if (utils.satisfies(process.version, '>=10 <10.15.2 || >=11 <11.7')) {
413411
if (this.config.capture && this.config.capture.maxDataSize === 0) {
414412
that.logger.warn(NODE_10_CIRC_REF_MESSAGE);
415413
}

‎test/test-circular.ts‎

Lines changed: 58 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@ import {defaultConfig} from '../src/agent/config';
2020
import {Debuglet} from '../src/agent/debuglet';
2121
import * as scanner from '../src/agent/io/scanner';
2222
import * as SourceMapper from '../src/agent/io/sourcemapper';
23-
import * as utils from '../src/agent/util/utils';
2423
import * as debugapi from '../src/agent/v8/debugapi';
2524

2625
import consoleLogLevel = require('console-log-level');
@@ -29,11 +28,6 @@ import {Variable} from '../src/types/stackdriver';
2928

3029
const code = require('./test-circular-code.js');
3130

32-
// TODO(kjin): When stableObjectId is added to Node 10, remove this check.
33-
// the inspector protocol is only used on Node >= 10 and thus isn't
34-
// tested on earlier versions
35-
const skipIfInspector = utils.satisfies(process.version, '>=10') ? it.skip : it;
36-
3731
function stateIsClean(api: debugapi.DebugApi): boolean {
3832
assert.strictEqual(
3933
api.numBreakpoints_(), 0, 'there should be no breakpoints active');
@@ -70,67 +64,64 @@ describe(__filename, () => {
7064
afterEach(() => {
7165
assert(stateIsClean(api));
7266
});
73-
skipIfInspector(
74-
'Should be able to read the argument and the context', (done) => {
75-
// TODO: Have this actually implement Breakpoint
76-
const brk: stackdriver.Breakpoint = {
77-
id: 'fake-id-123',
78-
location: {path: 'test-circular-code.js', line: 9}
79-
} as stackdriver.Breakpoint;
80-
api.set(brk, (err1) => {
81-
assert.ifError(err1);
82-
api.wait(brk, (err2) => {
83-
assert.ifError(err2);
84-
assert.ok(brk.stackFrames.length >= 1);
85-
const locals = [...brk.stackFrames[0].locals].sort(
86-
(a, b) => a.name!.localeCompare(b.name!));
87-
const nonStatusVars =
88-
(brk.variableTable.filter(entry => entry && !!entry.members) as
89-
Variable[]);
90-
const statusVarOffset =
91-
brk.variableTable.length - nonStatusVars.length;
92-
assert.ok(locals.length >= 3);
93-
// At least three locals: a, b, and context (alias for this).
94-
// In newer versions of inspector, this appears both as this and
95-
// as context.
96-
const aLocal = locals[0];
97-
const bLocal = locals[1];
98-
const contextLocal = locals[2];
99-
const thisLocal = locals[3]; // Maybe non-existent
100-
assert.ok(aLocal && bLocal && contextLocal);
101-
assert.ok(
102-
!thisLocal ||
103-
thisLocal.varTableIndex === contextLocal.varTableIndex);
104-
// All three non-status entries in the varTable correspond to each
105-
// of the locals, respectively.
106-
assert.strictEqual(nonStatusVars.length, 3);
107-
// Every entry has a truthy members field.
108-
assert.ok(!nonStatusVars.some(e => !e.members));
109-
const aVar = nonStatusVars[aLocal.varTableIndex! - statusVarOffset];
110-
const bVar = nonStatusVars[bLocal.varTableIndex! - statusVarOffset];
111-
const thisVar =
112-
nonStatusVars[contextLocal.varTableIndex! - statusVarOffset];
113-
assert.strictEqual(aVar.members!.length, 1); // a
114-
assert.deepStrictEqual(aVar.members![0], bLocal); // a.b
115-
assert.strictEqual(bVar.members!.length, 2); // b
116-
assert.deepStrictEqual(bVar.members![0], aLocal); // b.a
117-
assert.deepStrictEqual(
118-
bVar.members![1],
119-
{name: 'c', varTableIndex: contextLocal.varTableIndex});
120-
assert.strictEqual(thisVar.members!.length, 2); // this
121-
assert.deepStrictEqual(thisVar.members![0], {
122-
name: 'x',
123-
varTableIndex: contextLocal.varTableIndex
124-
}); // this.x
125-
assert.deepStrictEqual(
126-
thisVar.members![1],
127-
{name: 'y', varTableIndex: aLocal.varTableIndex}); // this.y
128-
api.clear(brk, (err3) => {
129-
assert.ifError(err3);
130-
done();
131-
});
132-
});
133-
process.nextTick(code.foo.bind({}));
67+
it('Should be able to read the argument and the context', (done) => {
68+
// TODO: Have this actually implement Breakpoint
69+
const brk: stackdriver.Breakpoint = {
70+
id: 'fake-id-123',
71+
location: {path: 'test-circular-code.js', line: 9}
72+
} as stackdriver.Breakpoint;
73+
api.set(brk, (err1) => {
74+
assert.ifError(err1);
75+
api.wait(brk, (err2) => {
76+
assert.ifError(err2);
77+
assert.ok(brk.stackFrames.length >= 1);
78+
const locals = [...brk.stackFrames[0].locals].sort(
79+
(a, b) => a.name!.localeCompare(b.name!));
80+
const nonStatusVars =
81+
(brk.variableTable.filter(entry => entry && !!entry.members) as
82+
Variable[]);
83+
const statusVarOffset = brk.variableTable.length - nonStatusVars.length;
84+
assert.ok(locals.length >= 3);
85+
// At least three locals: a, b, and context (alias for this).
86+
// In newer versions of inspector, this appears both as this and
87+
// as context.
88+
const aLocal = locals[0];
89+
const bLocal = locals[1];
90+
const contextLocal = locals[2];
91+
const thisLocal = locals[3]; // Maybe non-existent
92+
assert.ok(aLocal && bLocal && contextLocal);
93+
assert.ok(
94+
!thisLocal ||
95+
thisLocal.varTableIndex === contextLocal.varTableIndex);
96+
// All three non-status entries in the varTable correspond to each
97+
// of the locals, respectively.
98+
assert.strictEqual(nonStatusVars.length, 3);
99+
// Every entry has a truthy members field.
100+
assert.ok(!nonStatusVars.some(e => !e.members));
101+
const aVar = nonStatusVars[aLocal.varTableIndex! - statusVarOffset];
102+
const bVar = nonStatusVars[bLocal.varTableIndex! - statusVarOffset];
103+
const thisVar =
104+
nonStatusVars[contextLocal.varTableIndex! - statusVarOffset];
105+
assert.strictEqual(aVar.members!.length, 1); // a
106+
assert.deepStrictEqual(aVar.members![0], bLocal); // a.b
107+
assert.strictEqual(bVar.members!.length, 2); // b
108+
assert.deepStrictEqual(bVar.members![0], aLocal); // b.a
109+
assert.deepStrictEqual(
110+
bVar.members![1],
111+
{name: 'c', varTableIndex: contextLocal.varTableIndex});
112+
assert.strictEqual(thisVar.members!.length, 2); // this
113+
assert.deepStrictEqual(
114+
thisVar.members![0],
115+
{name: 'x', varTableIndex: contextLocal.varTableIndex}); // this.x
116+
assert.deepStrictEqual(
117+
thisVar.members![1],
118+
{name: 'y', varTableIndex: aLocal.varTableIndex}); // this.y
119+
api.clear(brk, (err3) => {
120+
assert.ifError(err3);
121+
done();
134122
});
135123
});
124+
process.nextTick(code.foo.bind({}));
125+
});
126+
});
136127
});

‎test/test-debuglet.ts‎

Lines changed: 0 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -309,47 +309,6 @@ describe('Debuglet', () => {
309309
debuglet.start();
310310
});
311311

312-
it('should emit a warning iff maxDataSize = 0 on Node 10', async () => {
313-
// TODO(kjin): When stableObjectId is added to Node 10, remove this test.
314-
const useInspector = utils.satisfies(process.version, '>=10');
315-
const maxDataSizeZeroConfig = Object.assign({}, defaultConfig, {
316-
capture: Object.assign({}, defaultConfig.capture, {maxDataSize: 0})
317-
});
318-
const checkMaxDataSizeWarningForConfig =
319-
(config: ResolvedDebugAgentConfig,
320-
shouldShowMessage: boolean) => new Promise((resolve, reject) => {
321-
const debug = new Debug(
322-
{projectId: 'fake-project', credentials: fakeCredentials},
323-
packageInfo);
324-
325-
const debuglet = new Debuglet(debug, config);
326-
let text = '';
327-
debuglet.logger.warn = (s: string) => {
328-
text += s;
329-
};
330-
331-
debuglet.once('initError', reject);
332-
333-
debuglet.once('started', () => {
334-
assert.strictEqual(
335-
text.includes(
336-
'capture.maxDataSize=0 is not recommended on Node 10+'),
337-
shouldShowMessage);
338-
debuglet.stop();
339-
resolve();
340-
});
341-
342-
debuglet.start();
343-
});
344-
// Run the actual test cases.
345-
// Check what happens if maxDataSize = 0, regardless of Node version.
346-
await checkMaxDataSizeWarningForConfig(
347-
maxDataSizeZeroConfig, useInspector);
348-
// Additionally, check that a warning doesn't get emitted if
349-
// maxDataSize != 0.
350-
await checkMaxDataSizeWarningForConfig(defaultConfig, false);
351-
});
352-
353312
describe('environment variables', () => {
354313
let env: NodeJS.ProcessEnv;
355314
beforeEach(() => {

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL