From b929faea0eb43b1fc61e8b30398423191a40ec73 Mon Sep 17 00:00:00 2001 From: Shanqing Cai Date: Mon, 27 Apr 2020 17:08:31 -0400 Subject: [PATCH] [DebuggerV2] Highlight stack frame being shown in SourceCodeComponent (#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()`. --- .../debugger_container_test.ts | 187 +++++++----------- .../store/debugger_types.ts | 2 + .../stack_trace/stack_trace_component.css | 5 + .../stack_trace/stack_trace_component.ng.html | 5 +- .../stack_trace/stack_trace_component.ts | 3 + .../stack_trace/stack_trace_container.ts | 14 +- 6 files changed, 92 insertions(+), 124 deletions(-) diff --git a/tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/debugger_container_test.ts b/tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/debugger_container_test.ts index 5ab71389d3..002e915afa 100644 --- a/tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/debugger_container_test.ts +++ b/tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/debugger_container_test.ts @@ -37,6 +37,10 @@ import { AlertType, TensorDebugMode, } from './store/debugger_types'; +import { + getFocusedExecutionStackFrames, + getFocusedSourceLineSpec, +} from './store'; import { createAlertsState, createDebuggerState, @@ -559,184 +563,125 @@ describe('Debugger Container', () => { }); describe('Stack Trace module', () => { - it('Shows non-empty stack frames correctly', () => { + it('Shows non-empty stack frames; highlights focused frame', () => { const fixture = TestBed.createComponent(StackTraceContainer); - fixture.detectChanges(); - const stackFrame0 = createTestStackFrame(); const stackFrame1 = createTestStackFrame(); const stackFrame2 = createTestStackFrame(); - store.setState( - createState( - createDebuggerState({ - executions: { - numExecutionsLoaded: { - state: DataLoadState.LOADED, - lastLoadedTimeInMs: 111, - }, - executionDigestsLoaded: { - state: DataLoadState.LOADED, - lastLoadedTimeInMs: 222, - pageLoadedSizes: {0: 100}, - numExecutions: 1000, - }, - executionDigests: {}, - pageSize: 100, - displayCount: 50, - scrollBeginIndex: 90, - focusIndex: 98, - executionData: { - 98: createTestExecutionData({ - stack_frame_ids: ['a0', 'a1', 'a2'], - }), - }, - }, - stackFrames: { - a0: stackFrame0, - a1: stackFrame1, - a2: stackFrame2, - }, - }) - ) - ); + store.overrideSelector(getFocusedExecutionStackFrames, [ + stackFrame0, + stackFrame1, + stackFrame2, + ]); + store.overrideSelector(getFocusedSourceLineSpec, { + host_name: stackFrame1[0], + file_path: stackFrame1[1], + lineno: stackFrame1[2], + }); fixture.detectChanges(); const hostNameElement = fixture.debugElement.query( By.css('.stack-trace-host-name') ); - expect(hostNameElement.nativeElement.innerText).toEqual('(on localhost)'); + expect(hostNameElement.nativeElement.innerText).toBe('(on localhost)'); const stackFrameContainers = fixture.debugElement.queryAll( By.css('.stack-frame-container') ); - expect(stackFrameContainers.length).toEqual(3); + expect(stackFrameContainers.length).toBe(3); const filePathElements = fixture.debugElement.queryAll( By.css('.stack-frame-file-path') ); - expect(filePathElements.length).toEqual(3); - expect(filePathElements[0].nativeElement.innerText).toEqual( + expect(filePathElements.length).toBe(3); + expect(filePathElements[0].nativeElement.innerText).toBe( stackFrame0[1].slice(stackFrame0[1].lastIndexOf('/') + 1) ); - expect(filePathElements[0].nativeElement.title).toEqual(stackFrame0[1]); - expect(filePathElements[1].nativeElement.innerText).toEqual( + expect(filePathElements[0].nativeElement.title).toBe(stackFrame0[1]); + expect(filePathElements[1].nativeElement.innerText).toBe( stackFrame1[1].slice(stackFrame1[1].lastIndexOf('/') + 1) ); - expect(filePathElements[1].nativeElement.title).toEqual(stackFrame1[1]); - expect(filePathElements[2].nativeElement.innerText).toEqual( + expect(filePathElements[1].nativeElement.title).toBe(stackFrame1[1]); + expect(filePathElements[2].nativeElement.innerText).toBe( stackFrame2[1].slice(stackFrame2[1].lastIndexOf('/') + 1) ); - expect(filePathElements[2].nativeElement.title).toEqual(stackFrame2[1]); + expect(filePathElements[2].nativeElement.title).toBe(stackFrame2[1]); const linenoElements = fixture.debugElement.queryAll( By.css('.stack-frame-lineno') ); - expect(linenoElements.length).toEqual(3); - expect(linenoElements[0].nativeElement.innerText).toEqual( + expect(linenoElements.length).toBe(3); + expect(linenoElements[0].nativeElement.innerText).toBe( `Line ${stackFrame0[2]}` ); - expect(linenoElements[1].nativeElement.innerText).toEqual( + expect(linenoElements[1].nativeElement.innerText).toBe( `Line ${stackFrame1[2]}` ); - expect(linenoElements[2].nativeElement.innerText).toEqual( + expect(linenoElements[2].nativeElement.innerText).toBe( `Line ${stackFrame2[2]}` ); const functionElements = fixture.debugElement.queryAll( By.css('.stack-frame-function') ); - expect(functionElements.length).toEqual(3); - expect(functionElements[0].nativeElement.innerText).toEqual( - stackFrame0[3] + expect(functionElements.length).toBe(3); + expect(functionElements[0].nativeElement.innerText).toBe(stackFrame0[3]); + expect(functionElements[1].nativeElement.innerText).toBe(stackFrame1[3]); + expect(functionElements[2].nativeElement.innerText).toBe(stackFrame2[3]); + + // Check the focused stack frame has been highlighted by CSS class. + const focusedElements = fixture.debugElement.queryAll( + By.css('.focused-stack-frame') ); - expect(functionElements[1].nativeElement.innerText).toEqual( - stackFrame1[3] + expect(focusedElements.length).toBe(1); + const focusedFilePathElement = focusedElements[0].query( + By.css('.stack-frame-file-path') ); - expect(functionElements[2].nativeElement.innerText).toEqual( - stackFrame2[3] + expect(focusedFilePathElement.nativeElement.innerText).toBe( + stackFrame1[1].slice(stackFrame1[1].lastIndexOf('/') + 1) ); }); - it('Shows loading state when stack-trace data is unavailable', () => { + it('does not highlight any frame when there is no frame focus', () => { const fixture = TestBed.createComponent(StackTraceContainer); + const stackFrame0 = createTestStackFrame(); + const stackFrame1 = createTestStackFrame(); + const stackFrame2 = createTestStackFrame(); + store.overrideSelector(getFocusedExecutionStackFrames, [ + stackFrame0, + stackFrame1, + stackFrame2, + ]); + store.overrideSelector(getFocusedSourceLineSpec, null); fixture.detectChanges(); - store.setState( - createState( - createDebuggerState({ - executions: { - numExecutionsLoaded: { - state: DataLoadState.LOADED, - lastLoadedTimeInMs: 111, - }, - executionDigestsLoaded: { - state: DataLoadState.LOADED, - lastLoadedTimeInMs: 222, - pageLoadedSizes: {0: 100}, - numExecutions: 1000, - }, - executionDigests: {}, - pageSize: 100, - displayCount: 50, - scrollBeginIndex: 90, - focusIndex: 98, - executionData: { - 98: createTestExecutionData({ - stack_frame_ids: ['a0', 'a1', 'a2'], - }), - }, - }, - stackFrames: {}, // Note the empty stackFrames field. - }) - ) + // Check that no stack frame has been highlighted by CSS class. + const focusedElements = fixture.debugElement.queryAll( + By.css('.focused-stack-frame') ); + expect(focusedElements.length).toBe(0); + }); + + it('Shows loading state when stack-trace data is unavailable', () => { + const fixture = TestBed.createComponent(StackTraceContainer); + store.overrideSelector(getFocusedExecutionStackFrames, []); fixture.detectChanges(); const stackFrameContainers = fixture.debugElement.queryAll( By.css('.stack-frame-container') ); - expect(stackFrameContainers.length).toEqual(0); + expect(stackFrameContainers.length).toBe(0); }); it('Emits sourceLineFocused when line number is clicked', () => { const fixture = TestBed.createComponent(StackTraceContainer); - fixture.detectChanges(); - const stackFrame0 = createTestStackFrame(); const stackFrame1 = createTestStackFrame(); const stackFrame2 = createTestStackFrame(); - store.setState( - createState( - createDebuggerState({ - executions: { - numExecutionsLoaded: { - state: DataLoadState.LOADED, - lastLoadedTimeInMs: 111, - }, - executionDigestsLoaded: { - state: DataLoadState.LOADED, - lastLoadedTimeInMs: 222, - pageLoadedSizes: {0: 100}, - numExecutions: 1000, - }, - executionDigests: {}, - pageSize: 100, - displayCount: 50, - scrollBeginIndex: 90, - focusIndex: 98, - executionData: { - 98: createTestExecutionData({ - stack_frame_ids: ['a0', 'a1', 'a2'], - }), - }, - }, - stackFrames: { - a0: stackFrame0, - a1: stackFrame1, - a2: stackFrame2, - }, - }) - ) - ); + store.overrideSelector(getFocusedExecutionStackFrames, [ + stackFrame0, + stackFrame1, + stackFrame2, + ]); fixture.detectChanges(); const linenoElements = fixture.debugElement.queryAll( diff --git a/tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/store/debugger_types.ts b/tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/store/debugger_types.ts index dac2076c4f..8af75d1678 100644 --- a/tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/store/debugger_types.ts +++ b/tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/store/debugger_types.ts @@ -44,6 +44,8 @@ export interface DebuggerRunListing { } // Each item is [host_name, file_path, lineno, function]. +// TODO(cais): Consider making this an object with meaningful +// keys instead. export type StackFrame = [string, string, number, string]; export type StackFramesById = {[id: string]: StackFrame}; diff --git a/tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/views/stack_trace/stack_trace_component.css b/tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/views/stack_trace/stack_trace_component.css index 1a4fce8e9d..cef55e97cd 100644 --- a/tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/views/stack_trace/stack_trace_component.css +++ b/tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/views/stack_trace/stack_trace_component.css @@ -13,6 +13,11 @@ See the License for the specific language governing permissions and limitations under the License. ==============================================================================*/ +.focused-stack-frame { + background-color: rgba(255, 111, 0, 0.3); + font-weight: bold; +} + .stack-frame-array { height: 360px; overflow-x: auto; diff --git a/tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/views/stack_trace/stack_trace_component.ng.html b/tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/views/stack_trace/stack_trace_component.ng.html index 424c049e1c..a565d7ced1 100644 --- a/tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/views/stack_trace/stack_trace_component.ng.html +++ b/tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/views/stack_trace/stack_trace_component.ng.html @@ -35,7 +35,10 @@
-
+
{ + getFocusedSourceLineSpec, + (stackFrames, focusedSourceLineSpec) => { if (stackFrames === null) { return null; } @@ -46,12 +50,18 @@ export class StackTraceContainer { const [host_name, file_path, lineno, function_name] = stackFrame; const pathItems = file_path.split('/'); const concise_file_path = pathItems[pathItems.length - 1]; + const focused = + focusedSourceLineSpec !== null && + host_name === focusedSourceLineSpec.host_name && + file_path === focusedSourceLineSpec.file_path && + lineno === focusedSourceLineSpec.lineno; output.push({ host_name, file_path, concise_file_path, lineno, function_name, + focused, }); } return output;