Fix system test break by gaofanmichael · Pull Request #333 · googleapis/cloud-debug-nodejs

gaofanmichael · 2017-09-26T22:52:34Z

Fixed the e2e test when logpoint and breakpoint was set in one place.

src/agent/inspectordebugapi.ts

      public id: string, public active: boolean,
      public apiBreakpoint: apiTypes.Breakpoint,
-      public parsedCondition: estree.Node,
+      public parsedCondition: estree.Node, public locationStr: string,


src/agent/inspectordebugapi.ts

-  scriptmapper: {[id: string]: any} = {};
-
  listeners: {[id: string]: utils.Listener} = {};
+  scriptmapper: {[id: string]: any} = {};


src/agent/inspectordebugapi.ts

    const listener =
        this.onBreakpointHit.bind(this, breakpoint, (err: Error) => {
-          this.listeners[bp.id].enabled = false;
+          this.listeners[breakpoint.id as string].enabled = false;


src/agent/inspectordebugapi.ts

-          return result.error ?
-              result.error :
-              (result.object as inspector.Runtime.RemoteObject).value();
+          if (result.error)


src/agent/v8inspector.ts

-      includeCommandLineAPI?: boolean, silent?: boolean,
-      returnByValue?: boolean, generatePreview?: boolean,
-      throwOnSideEffect?: boolean) {
+      callFrameId: string, expression: string, returnByValue?: boolean,


src/agent/inspectordebugapi.ts

+    const bpId = breakpointData.id;
+
+    // delete breakpoint from locationmapper and breakpointmapper.
+    this.deleteBreakpointInArray(


kjin

My concern with landing this as-is is the combination of (1) extensive overloading of the term "breakpoint", which can mean either "v8/inspector breakpoint" or "stackdriver breakpoint", and (2) the potential difficulty in maintaining this code due to its complexity.

Given the existence of now five members (breakpoints, listeners, scriptmapper, locationmapper, breakpointmapper) of InspectorDebugApi that map various types of information from different origins to one another, as well as a BreakpointData class that holds additional associations, I think it would be worthwhile to consider writing a data structure that can encapsulate this complexity in a simpler and more easily reasoned-through format.

In particular, I think the the information held in breakpoints, listeners, locationmapper, and breakpointmapper can be put together in a more cohesive way, since they are all mappings indexed on some breakpoint property.

googlebot · 2017-09-27T22:53:44Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

gaofanmichael · 2017-09-27T23:01:33Z

I signed it!

googlebot · 2017-09-27T23:01:35Z

CLAs look good, thanks!

ofrobots · 2017-09-27T23:19:31Z

Hmm.. what happened with the CLA here? Do you somehow have a commit with a different address set in your git config?

gaofanmichael · 2017-09-27T23:23:38Z

I have not modified my git config. I added my corp email address to CLA list and it worked. I have no ideas why googlebot checked cla:no only this time.

src/agent/inspectordebugapi.ts

+  // undefined.
  listeners: {[id: string]: utils.Listener} = {};
+  // scriptmapper maps scriptId to actual script path.
+  scriptmapper: {[id: string]: any} = {};


src/agent/inspectordebugapi.ts

    return Object.keys(this.listeners).length;
  }

+  private deleteBreakpointInArray(breakpointArray: string[], bp: string) {


src/agent/inspectordebugapi.ts

+      {v8BreakpointId: inspector.Debugger.BreakpointId,
+       locationStr: string}|null {
+    let condition;
+    if (breakpoint.condition) condition = breakpoint.condition;


src/agent/inspectordebugapi.ts

+    if (!this.locationmapper[locationStr]) {
+      // The first time when a breakpoint was set to this location.
+      let res = this.v8Inspector.setBreakpointByUrl(
+          line - 1, matchingScript, undefined, column - 1, condition);


src/agent/inspectordebugapi.ts

+    // location Str will be a JSON string of Stackdriver breakpoint location.
+    // It will be used as key at locationmapper to ensure there will be no
+    // duplicate breakpoints at the same location.
+    let locationStr = JSON.stringify(breakpoint.location);


src/agent/inspectordebugapi.ts

+    this.locationmapper[locationStr].push(breakpoint.id as string);
+    this.breakpointmapper[v8BreakpointId].push(breakpoint.id as string);
+
+    return {v8BreakpointId: v8BreakpointId, locationStr: locationStr};


src/agent/inspectordebugapi.ts

      callFrames: inspector.Debugger.CallFrame[]): void {
-    // TODO: Address the situation where `breakpoint.id` is `null`.
-    const bp = this.breakpoints[breakpoint.id as string];
-    if (!bp.active) {


src/agent/inspectordebugapi.ts

        const frame = callFrames[0];
        const evaluatedExpressions = breakpoint.expressions.map(function(exp) {
-          const result = state.evaluate(exp, frame, that.v8Inspector);
+          const result = state.evaluate(exp, frame, that.v8Inspector, true);


src/agent/v8inspector.ts

-      includeCommandLineAPI?: boolean, silent?: boolean,
-      returnByValue?: boolean, generatePreview?: boolean,
-      throwOnSideEffect?: boolean) {
+      callFrameId: string, expression: string, returnByValue?: boolean,


src/types/api-types.ts

  // TODO: The `controller.ts` file assumes `id` is not null or undefined.
  //       Verify it it should be optional.
-  id?: string;
+  id?: BreakpointId;


kjin

LGTM from my end w/ nit.

src/agent/utils.ts

  }
 }
+
+export function removeElementInArray(array: string[], element: string) {


src/agent/inspectordebugapi.ts

@@ -33,9 +33,9 @@ import {V8Inspector} from './v8inspector';



src/agent/v8debugapi.ts

@@ -187,7 +187,7 @@ export class V8DebugApi implements debugapi.DebugApi {
      shouldStop: () => boolean): void {
    const that = this;
    // TODO: Address the case whree `breakpoint.id` is `null`.


src/agent/v8debugapi.ts

@@ -338,7 +338,7 @@ export class V8DebugApi implements debugapi.DebugApi {
      this.v8.setListener(this.handleDebugEvents);
    }
    // TODO: Address the case whree `breakpoint.id` is `null`.


src/agent/v8debugapi.ts

      execState: v8Types.ExecutionState): void {
    // TODO: Address the situation where `breakpoint.id` is `null`.
-    const v8bp = this.breakpoints[breakpoint.id as string].v8Breakpoint;
+    const v8bp = this.breakpoints[breakpoint.id].v8Breakpoint;


src/agent/v8debugapi.ts

@@ -422,15 +422,14 @@ export class V8DebugApi implements debugapi.DebugApi {
      execState: v8Types.ExecutionState): void {
    const expressionErrors: Array<apiTypes.Variable|null> = [];
    // TODO: Address the case where `breakpoint.id` is `null`.


fixed test

8dadf58

gaofanmichael requested review from DominicKramer, kjin and ofrobots September 26, 2017 22:52

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Sep 26, 2017

DominicKramer reviewed Sep 26, 2017

View reviewed changes

kjin suggested changes Sep 27, 2017

View reviewed changes

gaofanmichael added 2 commits September 27, 2017 14:15

readability fix

a954b92

encapsulation

706b899

googlebot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Sep 27, 2017

googlebot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Sep 27, 2017

remove active

bba215a

kjin suggested changes Sep 27, 2017

View reviewed changes

resolve comments

646edc2

kjin approved these changes Sep 28, 2017

View reviewed changes

src/agent/utils.ts Outdated

}

}

export function removeElementInArray(array: string[], element: string) {

This comment was marked as spam.

Sign in to view

rename function

c7b6472

ofrobots reviewed Sep 28, 2017

View reviewed changes

remove todo

886fb46

gaofanmichael merged commit fa41721 into googleapis:master Sep 28, 2017

gaofanmichael deleted the e2etest branch September 28, 2017 16:54

		@@ -33,9 +33,9 @@ import {V8Inspector} from './v8inspector';

Conversation

gaofanmichael commented Sep 26, 2017

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

kjin left a comment

Choose a reason for hiding this comment

Uh oh!

googlebot commented Sep 27, 2017

Uh oh!

gaofanmichael commented Sep 27, 2017

Uh oh!

googlebot commented Sep 27, 2017

Uh oh!

ofrobots commented Sep 27, 2017

Uh oh!

gaofanmichael commented Sep 27, 2017

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

kjin left a comment

Choose a reason for hiding this comment