[DebuggerV2] Highlight stack frame being shown in SourceCodeComponent #3550

caisq · 2020-04-25T19:10:59Z

Motivation for features / changes
- Make it clearer in the StackFrameComponent which frame (if any) is being highlighted in the SourceCodeComponent.
Technical description of changes
- In StackFrameContainer's internal selector, add a boolean field to the return value called focused. It is used in the template ng html to apply a CSS class (.focused-stack-frame) to the focused stack frame and only the focused stack frame.
- While adding new unit test for the new feature, simplify existing ones by using overrideSelector.
- To conform to style guide, replace toEqual() with toBe() where applicable in the touched unit tests.
Screenshots of UI changes
Detailed steps to verify changes work correctly (as executed by you)
- Unit test added
- Existing unit tests for StackFrameContainer are simplified by replacing setState() with overrideSelector().

psybuzz

Looks good! Just some minor comments

psybuzz · 2020-04-27T18:23:11Z

tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/debugger_container_test.ts

-      expect(functionElements[2].nativeElement.innerText).toEqual(
-        stackFrame2[3]
+      expect(focusedFilePathElement.nativeElement.innerText).toBe(
+        stackFrame1[1].slice(stackFrame1[1].lastIndexOf('/') + 1)


This same slicing logic is on L928, 932, 936. Perhaps create a helper function?

Since this is a test, I'll opt not to add a helper function now. It's my preference to keep test logic explicit even when it's sometimes a little more verbose.

psybuzz · 2020-04-27T18:57:08Z

tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/debugger_container_test.ts

-      expect(filePathElements[0].nativeElement.innerText).toEqual(
+      expect(filePathElements.length).toBe(3);
+      expect(filePathElements[0].nativeElement.innerText).toBe(
        stackFrame0[1].slice(stackFrame0[1].lastIndexOf('/') + 1)


While reading this test, I found myself going back to lookup which indices in the stackFrame array correspond to which fields. I think it might be helpful for readability if createTestStackFrame returned an object with named properties like 'filePath'. Although since that was outside of this PR, it's fine to not change in this PR.

I add a TODO item to debugger_types.ts. The background here is this is how Python's traceback.extract_tb returns tuples like (file_path, lineno, function_name). So this "leaks" into the data types in debugger v2 a little.

Awesome, thanks for the todo+context.

psybuzz · 2020-04-27T18:57:56Z

tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/debugger_container_test.ts

+      expect(filePathElements[1].nativeElement.innerText).toBe(
        stackFrame1[1].slice(stackFrame1[1].lastIndexOf('/') + 1)
      );
      expect(filePathElements[1].nativeElement.title).toEqual(stackFrame1[1]);


Can this also use toBe?

psybuzz · 2020-04-27T18:59:03Z

tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/debugger_container_test.ts

      );
      expect(stackFrameContainers.length).toEqual(0);
    });



Use toBe on L1028?

caisq

Thanks for the review!

caisq · 2020-04-27T20:32:13Z

tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/debugger_container_test.ts

+      expect(filePathElements[1].nativeElement.innerText).toBe(
        stackFrame1[1].slice(stackFrame1[1].lastIndexOf('/') + 1)
      );
      expect(filePathElements[1].nativeElement.title).toEqual(stackFrame1[1]);


caisq · 2020-04-27T20:34:36Z

tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/debugger_container_test.ts

-      expect(functionElements[2].nativeElement.innerText).toEqual(
-        stackFrame2[3]
+      expect(focusedFilePathElement.nativeElement.innerText).toBe(
+        stackFrame1[1].slice(stackFrame1[1].lastIndexOf('/') + 1)


Since this is a test, I'll opt not to add a helper function now. It's my preference to keep test logic explicit even when it's sometimes a little more verbose.

caisq · 2020-04-27T20:35:16Z

tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/debugger_container_test.ts

      );
      expect(stackFrameContainers.length).toEqual(0);
    });



caisq · 2020-04-27T20:38:53Z

tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/debugger_container_test.ts

-      expect(filePathElements[0].nativeElement.innerText).toEqual(
+      expect(filePathElements.length).toBe(3);
+      expect(filePathElements[0].nativeElement.innerText).toBe(
        stackFrame0[1].slice(stackFrame0[1].lastIndexOf('/') + 1)


I add a TODO item to debugger_types.ts. The background here is this is how Python's traceback.extract_tb returns tuples like (file_path, lineno, function_name). So this "leaks" into the data types in debugger v2 a little.

…tensorflow#3550) * Motivation for features / changes * Make it clearer in the StackFrameComponent which frame (if any) is being highlighted in the SourceCodeComponent. * Technical description of changes * In StackFrameContainer's internal selector, add a boolean field to the return value called `focused`. It is used in the template ng html to apply a CSS class (`.focused-stack-frame`) to the focused stack frame and only the focused stack frame. * While adding new unit test for the new feature, simplify existing ones by using `overrideSelector`. * To conform to style guide, replace `toEqual()` with `toBe()` where applicable in the touched unit tests. * Screenshots of UI changes * ![image](https://user-images.githubusercontent.com/16824702/80288736-ed102300-8707-11ea-9638-7aa197a23ff6.png) * Detailed steps to verify changes work correctly (as executed by you) * Unit test added * Existing unit tests for `StackFrameContainer` are simplified by replacing `setState()` with `overrideSelector()`.

…#3550) * Motivation for features / changes * Make it clearer in the StackFrameComponent which frame (if any) is being highlighted in the SourceCodeComponent. * Technical description of changes * In StackFrameContainer's internal selector, add a boolean field to the return value called `focused`. It is used in the template ng html to apply a CSS class (`.focused-stack-frame`) to the focused stack frame and only the focused stack frame. * While adding new unit test for the new feature, simplify existing ones by using `overrideSelector`. * To conform to style guide, replace `toEqual()` with `toBe()` where applicable in the touched unit tests. * Screenshots of UI changes * ![image](https://user-images.githubusercontent.com/16824702/80288736-ed102300-8707-11ea-9638-7aa197a23ff6.png) * Detailed steps to verify changes work correctly (as executed by you) * Unit test added * Existing unit tests for `StackFrameContainer` are simplified by replacing `setState()` with `overrideSelector()`.

caisq added 2 commits April 25, 2020 14:37

[DebuggerV2] Add highlighting of focused frame in StackTraceComponent

605ce5c

Add unit test for focused stack frame; simplify existing tests

01839d2

googlebot added the cla: yes label Apr 25, 2020

caisq changed the title ~~Dbg2 highlight stack frame~~ [DebuggerV2] Highlight stack frame being shown in SourceCodeComponent Apr 25, 2020

caisq marked this pull request as ready for review April 25, 2020 19:13

Add test; adjust background-color

291050e

caisq requested a review from psybuzz April 25, 2020 19:18

psybuzz approved these changes Apr 27, 2020

View reviewed changes

Address review comments

7e0fe4e

caisq commented Apr 27, 2020

View reviewed changes

caisq merged commit b929fae into tensorflow:master Apr 27, 2020

[DebuggerV2] Highlight stack frame being shown in SourceCodeComponent #3550

[DebuggerV2] Highlight stack frame being shown in SourceCodeComponent #3550

Uh oh!

Conversation

caisq commented Apr 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

psybuzz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caisq left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

caisq commented Apr 25, 2020 •

edited

Loading