From 5b687e6963faae8968f84dfcde87b8da87628a33 Mon Sep 17 00:00:00 2001 From: Tim Klever Date: Thu, 13 Aug 2020 01:04:35 -0700 Subject: [PATCH 1/2] add additional tests around existing TimelineViewingLayer functionality In addition to rounding out the test suite, this commit also removes some of the "Math.random" calls from the test suite. These randoms were causing some code coverage jitter in the test suite by randomly executing certain code paths. Removing these randomizations will keep coverage reporting consistent and avoid random "coverage losses" based on chance. Signed-off-by: Tim Klever --- .../TimelineViewingLayer.test.js | 39 ++++++++++++++++++- .../TimelineViewingLayer.tsx | 12 +++--- 2 files changed, 44 insertions(+), 7 deletions(-) diff --git a/packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/TimelineHeaderRow/TimelineViewingLayer.test.js b/packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/TimelineHeaderRow/TimelineViewingLayer.test.js index 9b14704197..2ba24a0fce 100644 --- a/packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/TimelineHeaderRow/TimelineViewingLayer.test.js +++ b/packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/TimelineHeaderRow/TimelineViewingLayer.test.js @@ -101,7 +101,7 @@ describe('', () => { it('handles drag move via _draggerReframe._onDragMove', () => { const anchor = 0.25; - const viewRangeTime = { ...props.viewRangeTime, reframe: { anchor, shift: Math.random() } }; + const viewRangeTime = { ...props.viewRangeTime, reframe: { anchor, shift: 0.5 } }; const value = 0.5; const shift = mapFromSubRange(viewStart, viewEnd, value); // make sure `anchor` is already present on the props @@ -118,12 +118,29 @@ describe('', () => { const value = 0.5; const shift = mapFromSubRange(viewStart, viewEnd, value); const anchor = 0.25; - const viewRangeTime = { ...props.viewRangeTime, reframe: { anchor, shift: Math.random() } }; + const viewRangeTime = { ...props.viewRangeTime, reframe: { anchor, shift } }; wrapper.setProps({ viewRangeTime }); instance._draggerReframe._onDragEnd({ manager, value }); expect(manager.resetBounds.mock.calls).toEqual([[]]); expect(props.updateViewRangeTime.mock.calls).toEqual([[anchor, shift, 'timeline-header']]); }); + + it('_draggerReframe._onDragEnd sorts anchor and shift', () => { + const manager = { resetBounds: jest.fn() }; + const value = 0.5; + const shift = mapFromSubRange(viewStart, viewEnd, value); + const anchor = 0.75; + const viewRangeTime = { ...props.viewRangeTime, reframe: { anchor, shift } }; + wrapper.setProps({ viewRangeTime }); + instance._draggerReframe._onDragEnd({ manager, value }); + expect(props.updateViewRangeTime.mock.calls).toEqual([[shift, anchor, 'timeline-header']]); + }); + + it('resets draggable bounds on boundsInvalidator update', () => { + const spy = jest.spyOn(instance._draggerReframe, 'resetBounds'); + wrapper.setProps({ boundsInvalidator: 'SOMETHING-NEW' }); + expect(spy).toHaveBeenCalledTimes(1); + }); }); describe('render()', () => { @@ -155,6 +172,24 @@ describe('', () => { expect(wrapper.find('.isDraggingRight.isReframeDrag').length).toBe(1); }); + it('renders the reframe dragging normalized left', () => { + const viewRangeTime = { ...props.viewRangeTime, reframe: { anchor: -0.25, shift: viewEnd } }; + wrapper.setProps({ viewRangeTime }); + expect(wrapper.find('.isDraggingRight.isReframeDrag').length).toBe(1); + }); + + it('renders the reframe dragging normalized right', () => { + const viewRangeTime = { ...props.viewRangeTime, reframe: { anchor: viewStart, shift: 1.25 } }; + wrapper.setProps({ viewRangeTime }); + expect(wrapper.find('.isDraggingRight.isReframeDrag').length).toBe(1); + }); + + it('does not render the reframe on out of bounds', () => { + const viewRangeTime = { ...props.viewRangeTime, reframe: { anchor: 1.5, shift: 1.75 } }; + wrapper.setProps({ viewRangeTime }); + expect(wrapper.find('.isReframeDrag').length).toBe(0); + }); + it('renders the shiftStart dragging', () => { const viewRangeTime = { ...props.viewRangeTime, shiftStart: viewEnd }; wrapper.setProps({ viewRangeTime }); diff --git a/packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/TimelineHeaderRow/TimelineViewingLayer.tsx b/packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/TimelineHeaderRow/TimelineViewingLayer.tsx index 0edb6d3ed5..3249592c9b 100644 --- a/packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/TimelineHeaderRow/TimelineViewingLayer.tsx +++ b/packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/TimelineHeaderRow/TimelineViewingLayer.tsx @@ -171,20 +171,22 @@ export default class TimelineViewingLayer extends React.PureComponent { + _getAnchorAndShift = (value: number) => { const { current, reframe } = this.props.viewRangeTime; const [viewStart, viewEnd] = current; const shift = mapFromViewSubRange(viewStart, viewEnd, value); const anchor = reframe ? reframe.anchor : shift; + return { anchor, shift }; + }; + + _handleReframeDragUpdate = ({ value }: DraggingUpdate) => { + const { anchor, shift } = this._getAnchorAndShift(value); const update = { reframe: { anchor, shift } }; this.props.updateNextViewRangeTime(update); }; _handleReframeDragEnd = ({ manager, value }: DraggingUpdate) => { - const { current, reframe } = this.props.viewRangeTime; - const [viewStart, viewEnd] = current; - const shift = mapFromViewSubRange(viewStart, viewEnd, value); - const anchor = reframe ? reframe.anchor : shift; + const { anchor, shift } = this._getAnchorAndShift(value); const [start, end] = shift < anchor ? [shift, anchor] : [anchor, shift]; manager.resetBounds(); this.props.updateViewRangeTime(start, end, 'timeline-header'); From 0c8c73cab7d8a9a796215997ff0e3832ff9dce6d Mon Sep 17 00:00:00 2001 From: Tim Klever Date: Thu, 13 Aug 2020 01:14:26 -0700 Subject: [PATCH 2/2] migrate from callback ref to CreateRef in TimelineViewingLayer CreateRef API was introduced in React 16.3 Signed-off-by: Tim Klever --- .../TimelineViewingLayer.test.js | 11 +++++++++-- .../TimelineHeaderRow/TimelineViewingLayer.tsx | 17 +++++++---------- 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/TimelineHeaderRow/TimelineViewingLayer.test.js b/packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/TimelineHeaderRow/TimelineViewingLayer.test.js index 2ba24a0fce..e909aa6207 100644 --- a/packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/TimelineHeaderRow/TimelineViewingLayer.test.js +++ b/packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/TimelineHeaderRow/TimelineViewingLayer.test.js @@ -50,7 +50,7 @@ describe('', () => { it('sets _root to the root DOM node', () => { expect(instance._root).toBeDefined(); - expect(wrapper.find('.TimelineViewingLayer').getDOMNode()).toBe(instance._root); + expect(wrapper.find('.TimelineViewingLayer').getDOMNode()).toBe(instance._root.current); }); describe('uses DraggableManager', () => { @@ -75,10 +75,17 @@ describe('', () => { it('returns the dragging bounds from _getDraggingBounds()', () => { const left = 10; const width = 100; - instance._root.getBoundingClientRect = () => ({ left, width }); + instance._root.current.getBoundingClientRect = () => ({ left, width }); expect(instance._getDraggingBounds()).toEqual({ width, clientXLeft: left }); }); + it('throws error on call to _getDraggingBounds() on unmounted component', () => { + wrapper.unmount(); + expect(instance._getDraggingBounds).toThrow( + 'Component must be mounted in order to determine DraggableBounds' + ); + }); + it('updates viewRange.time.cursor via _draggerReframe._onMouseMove', () => { const value = 0.5; const cursor = mapFromSubRange(viewStart, viewEnd, value); diff --git a/packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/TimelineHeaderRow/TimelineViewingLayer.tsx b/packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/TimelineHeaderRow/TimelineViewingLayer.tsx index 3249592c9b..c36f94514e 100644 --- a/packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/TimelineHeaderRow/TimelineViewingLayer.tsx +++ b/packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/TimelineHeaderRow/TimelineViewingLayer.tsx @@ -122,7 +122,7 @@ function getMarkers( */ export default class TimelineViewingLayer extends React.PureComponent { _draggerReframe: DraggableManager; - _root: Element | TNil; + _root: React.RefObject; constructor(props: TimelineViewingLayerProps) { super(props); @@ -134,7 +134,7 @@ export default class TimelineViewingLayer extends React.PureComponent { - this._root = elm; - }; - _getDraggingBounds = (): DraggableBounds => { - if (!this._root) { - throw new Error('invalid state'); + const current = this._root.current; + if (!current) { + throw new Error('Component must be mounted in order to determine DraggableBounds'); } - const { left: clientXLeft, width } = this._root.getBoundingClientRect(); + const { left: clientXLeft, width } = current.getBoundingClientRect(); return { clientXLeft, width }; }; @@ -205,7 +202,7 @@ export default class TimelineViewingLayer extends React.PureComponent