From cfe3a9be83f2b9365ee0ddf89a23bcf8ceca39a9 Mon Sep 17 00:00:00 2001 From: James McTavish Date: Tue, 10 May 2022 09:54:05 -0400 Subject: [PATCH 1/3] Fix and re-enable previously broken tests --- src/agent/debuglet.ts | 10 ++++++++-- test/test-debuglet.ts | 24 +++++++++++++----------- test/test-firebase-controller.ts | 4 ++-- 3 files changed, 23 insertions(+), 15 deletions(-) diff --git a/src/agent/debuglet.ts b/src/agent/debuglet.ts index 072d8d91..789f43f3 100644 --- a/src/agent/debuglet.ts +++ b/src/agent/debuglet.ts @@ -765,8 +765,8 @@ export class Debuglet extends EventEmitter { } ).debuggee.id; // TODO: Handle the case when `result` is undefined. - that.emit('registered', (result as {debuggee: Debuggee}).debuggee.id); // FIXME: Do we need this? - that.debuggeeRegistered.resolve(); // FIXME: Do we need this? + that.emit('registered', (result as {debuggee: Debuggee}).debuggee.id); + that.debuggeeRegistered.resolve(); if (!that.fetcherActive) { that.startListeningForBreakpoints_(); } @@ -788,9 +788,15 @@ export class Debuglet extends EventEmitter { err.name === 'RegistrationExpiredError' ? 0 : this.config.internal.registerDelayOnFetcherErrorSec; + this.updatePromise(); this.scheduleRegistration_(delay); } + this.breakpointFetchedTimestamp = Date.now(); + if (this.breakpointFetched) { + this.breakpointFetched.resolve(); + this.breakpointFetched = null; + } this.updateActiveBreakpoints_(breakpoints); } ); diff --git a/test/test-debuglet.ts b/test/test-debuglet.ts index d3932648..3d907873 100644 --- a/test/test-debuglet.ts +++ b/test/test-debuglet.ts @@ -1169,8 +1169,7 @@ describe('Debuglet', () => { debuglet.start(); }); - /*it('should have breakpoints fetched when promise is resolved', function (done) { - // FIXME: Broken. + it('should have breakpoints fetched when promise is resolved', function (done) { this.timeout(2000); const breakpoint: stackdriver.Breakpoint = { id: 'test1', @@ -1189,7 +1188,6 @@ describe('Debuglet', () => { .post(REGISTER_PATH) .reply(200, {debuggee: {id: DEBUGGEE_ID}}) .get(BPS_PATH + '?successOnTimeout=true') - .twice() .reply(200, {breakpoints: [breakpoint]}); const debugPromise = debuglet.isReadyManager.isReady(); debuglet.once('registered', () => { @@ -1212,7 +1210,6 @@ describe('Debuglet', () => { }); it('should resolve breakpointFetched promise when registration expires', function (done) { - // FIXME: Broken. this.timeout(2000); const debug = new Debug( {projectId: 'fake-project', credentials: fakeCredentials}, @@ -1237,7 +1234,7 @@ describe('Debuglet', () => { }); debuglet.start(); - });*/ + }); it('should reject breakpoints with conditions when allowExpressions=false', function (done) { this.timeout(2000); @@ -1425,8 +1422,7 @@ describe('Debuglet', () => { // The test expires a breakpoint and then has the api respond with // the breakpoint listed as active. It validates that the breakpoint // is only expired with the server once. - /*it('should not update expired breakpoints', function (done) { - // FIXME: Broken + it('should not update expired breakpoints', function (done) { const debug = new Debug( {projectId: 'fake-project', credentials: fakeCredentials}, packageInfo @@ -1440,6 +1436,7 @@ describe('Debuglet', () => { forceNewAgent_: true, }); const debuglet = new Debuglet(debug, config); + let unexpectedUpdate = false; const scope = nock(config.apiUrl) .post(REGISTER_PATH) .reply(200, {debuggee: {id: DEBUGGEE_ID}}) @@ -1459,15 +1456,20 @@ describe('Debuglet', () => { .times(4) .reply(200, {breakpoints: [bp]}); + // Get ready to fail the test if any additional updates come through. + nock.emitter.on('no match', (req) => { + if (req.path.startsWith(BPS_PATH) && req.method === 'PUT') { + unexpectedUpdate = true; + } + }); + debuglet.once('registered', (id: string) => { assert.strictEqual(id, DEBUGGEE_ID); setTimeout(() => { assert.deepStrictEqual(debuglet.activeBreakpointMap.test, bp); setTimeout(() => { assert(!debuglet.activeBreakpointMap.test); - // Fetcher disables if we re-update since endpoint isn't mocked - // twice - assert(debuglet.fetcherActive); + assert(!unexpectedUpdate, 'unexpected update request'); debuglet.stop(); scope.done(); done(); @@ -1476,7 +1478,7 @@ describe('Debuglet', () => { }); debuglet.start(); - });*/ + }); }); describe('map subtract', () => { diff --git a/test/test-firebase-controller.ts b/test/test-firebase-controller.ts index 5543175b..5e1b29d7 100644 --- a/test/test-firebase-controller.ts +++ b/test/test-firebase-controller.ts @@ -57,7 +57,7 @@ class MockReference { if (onComplete) { onComplete(null); } - return new Promise(resolve => resolve(null)); + return Promise.resolve(); } getOrAdd(key: string): MockReference { @@ -101,7 +101,7 @@ class MockReference { if (creating && this.parentRef) { this.parentRef.childAdded(this.key, value); } - return new Promise(resolve => resolve(null)); + return Promise.resolve(); } on( From f9e7507e1f0826615d4beace12cd3ebd910c742d Mon Sep 17 00:00:00 2001 From: James McTavish Date: Tue, 10 May 2022 09:56:00 -0400 Subject: [PATCH 2/3] fix: Fix linter warnings --- test/test-debuglet.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/test-debuglet.ts b/test/test-debuglet.ts index 3d907873..7a02ce2e 100644 --- a/test/test-debuglet.ts +++ b/test/test-debuglet.ts @@ -1457,7 +1457,7 @@ describe('Debuglet', () => { .reply(200, {breakpoints: [bp]}); // Get ready to fail the test if any additional updates come through. - nock.emitter.on('no match', (req) => { + nock.emitter.on('no match', req => { if (req.path.startsWith(BPS_PATH) && req.method === 'PUT') { unexpectedUpdate = true; } From 756c8a60d639c09c94956af53a65b5f2ab5c65be Mon Sep 17 00:00:00 2001 From: James McTavish Date: Wed, 11 May 2022 09:33:51 -0400 Subject: [PATCH 3/3] Add some additional comments. --- src/agent/debuglet.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/agent/debuglet.ts b/src/agent/debuglet.ts index 789f43f3..90ff2760 100644 --- a/src/agent/debuglet.ts +++ b/src/agent/debuglet.ts @@ -192,6 +192,8 @@ export class Debuglet extends EventEmitter { private controller: Controller | null; private completedBreakpointMap: {[key: string]: boolean}; + // The following four variables are used for the "isReady" functionality. + // breakpointFetchedTimestamp represents the last timestamp when // breakpointFetched was resolved, which means breakpoint update was // successful. @@ -788,6 +790,7 @@ export class Debuglet extends EventEmitter { err.name === 'RegistrationExpiredError' ? 0 : this.config.internal.registerDelayOnFetcherErrorSec; + // The debuglet is no longer ready and the promises are stale. this.updatePromise(); this.scheduleRegistration_(delay); } @@ -803,7 +806,7 @@ export class Debuglet extends EventEmitter { } /** - * updatePromise_ is called when debuggee is expired. debuggeeRegistered + * updatePromise is called when debuggee is expired. debuggeeRegistered * CachedPromise will be refreshed. Also, breakpointFetched CachedPromise will * be resolved so that uses (such as GCF users) will not hang forever to wait * non-fetchable breakpoints.