Skip to content

Commit f340a5d

Browse files
authored
Fix most tests that break with es2020 output. (#5991)
The migration to spec_bundle() will require we configure the TypeScript compiler to output to JavaScript 'es2020'. There is one pattern of code that we often use that will break: in my_utilities.ts: ``` export function myFuncToSpyOn() { // Some code. } ``` in my_test.ts: ``` import * as my_utilities from './my_utilities' spyOn(my_utilities, 'myFuncToSpyOn').and.returnValue(1); ``` The Jasmine library will complain: `Error: <spyOn> : myFuncToSpyOn is not declared writable or has no setter` Jasmine is aware of the issue but can't provide a fix: * jasmine/jasmine#1942 * https://jasmine.github.io/pages/faq.html#module-spy Instead they suggest we refactor our code so that the functions being spied on are wrapped in some Class or other Object. This PR does exactly that, for the most part doing the most simple of wrapping: now in my_utilities.ts: ``` // No longer exported. function myFuncToSpyOn() { // Some code. } export const MyUtilities = { myFuncToSpyOn } ``` now in my_test.ts: ``` import {MyUtilities} from './my_utilities' spyOn(MyUtilities, 'myFuncToSpyOn').and.returnValue(1); ``` The change has a bit of bigger blast radius because not only do the tests need to be changed but so does every other file (test or not) that are using the utility functions.
1 parent 3f018be commit f340a5d

19 files changed

+109
-86
lines changed

tensorboard/webapp/widgets/line_chart_v2/lib/coordinator.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ limitations under the License.
1515

1616
import {Rect, Scale, ScaleType} from './internal_types';
1717
import {createScale} from './scale';
18-
import {convertRectToExtent} from './utils';
18+
import {ChartUtils} from './utils';
1919

2020
/**
2121
* A stateful convenient utility around scale for converting coordinate systems.
@@ -109,7 +109,7 @@ export class Coordinator {
109109
dataCoordinate: [number, number]
110110
): [number, number] {
111111
const rect = rectInUiCoordinate;
112-
const domain = convertRectToExtent(this.currentViewBoxRect);
112+
const domain = ChartUtils.convertRectToExtent(this.currentViewBoxRect);
113113
return [
114114
this.xScale.forward(
115115
domain.x,

tensorboard/webapp/widgets/line_chart_v2/lib/renderer/renderer_test.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ import {Polyline} from '../internal_types';
1717
import {assertSvgPathD} from '../testing';
1818
import {ThreeCoordinator} from '../threejs_coordinator';
1919
import {SvgRenderer} from './svg_renderer';
20-
import {ThreeRenderer} from './threejs_renderer';
20+
import {TEST_ONLY, ThreeRenderer} from './threejs_renderer';
2121

2222
describe('line_chart_v2/lib/renderer test', () => {
2323
const SVG_NS = 'http://www.w3.org/2000/svg';
@@ -326,7 +326,7 @@ describe('line_chart_v2/lib/renderer test', () => {
326326

327327
beforeEach(() => {
328328
scene = new THREE.Scene();
329-
spyOn(THREE, 'Scene').and.returnValue(scene);
329+
spyOn(TEST_ONLY.ThreeWrapper, 'createScene').and.returnValue(scene);
330330

331331
const canvas = document.createElement('canvas');
332332
const coordinator = new ThreeCoordinator();

tensorboard/webapp/widgets/line_chart_v2/lib/renderer/svg_renderer.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ limitations under the License.
1414
==============================================================================*/
1515

1616
import {Point, Polyline, Rect} from '../internal_types';
17-
import {arePolylinesEqual} from '../utils';
17+
import {ChartUtils} from '../utils';
1818
import {
1919
CirclePaintOption,
2020
LinePaintOption,
@@ -110,7 +110,7 @@ export class SvgRenderer implements ObjectRenderer<CacheValue> {
110110
(dom) => {
111111
if (
112112
!cachedLine?.data ||
113-
!arePolylinesEqual(polyline, cachedLine?.data)
113+
!ChartUtils.arePolylinesEqual(polyline, cachedLine?.data)
114114
) {
115115
const data = this.createPathDString(polyline);
116116
dom.setAttribute('d', data);

tensorboard/webapp/widgets/line_chart_v2/lib/renderer/threejs_renderer.ts

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ import * as THREE from 'three';
1717
import {hsl, interpolateHsl} from '../../../../third_party/d3';
1818
import {Point, Polyline, Rect} from '../internal_types';
1919
import {ThreeCoordinator} from '../threejs_coordinator';
20-
import {arePolylinesEqual, isOffscreenCanvasSupported} from '../utils';
20+
import {ChartUtils} from '../utils';
2121
import {
2222
CirclePaintOption,
2323
LinePaintOption,
@@ -260,9 +260,15 @@ function updateObject(
260260
return true;
261261
}
262262

263+
const ThreeWrapper = {
264+
createScene: () => {
265+
return new THREE.Scene();
266+
},
267+
};
268+
263269
export class ThreeRenderer implements ObjectRenderer<CacheValue> {
264270
private readonly renderer: THREE.WebGLRenderer;
265-
private readonly scene = new THREE.Scene();
271+
private readonly scene = ThreeWrapper.createScene();
266272
private backgroundColor: string = '#fff';
267273

268274
constructor(
@@ -271,7 +277,10 @@ export class ThreeRenderer implements ObjectRenderer<CacheValue> {
271277
devicePixelRatio: number,
272278
onContextLost?: EventListener
273279
) {
274-
if (isOffscreenCanvasSupported() && canvas instanceof OffscreenCanvas) {
280+
if (
281+
ChartUtils.isOffscreenCanvasSupported() &&
282+
canvas instanceof OffscreenCanvas
283+
) {
275284
// THREE.js require the style object which Offscreen canvas lacks.
276285
(canvas as any).style = (canvas as any).style || {};
277286
}
@@ -347,7 +356,7 @@ export class ThreeRenderer implements ObjectRenderer<CacheValue> {
347356
if (
348357
width !== prevWidth ||
349358
!prevPolyline ||
350-
!arePolylinesEqual(prevPolyline, polyline)
359+
!ChartUtils.arePolylinesEqual(prevPolyline, polyline)
351360
) {
352361
updateThickPolylineGeometry(geometry, polyline, width);
353362
}
@@ -497,3 +506,7 @@ export class ThreeRenderer implements ObjectRenderer<CacheValue> {
497506
this.renderer.dispose();
498507
}
499508
}
509+
510+
export const TEST_ONLY = {
511+
ThreeWrapper,
512+
};

tensorboard/webapp/widgets/line_chart_v2/lib/utils.ts

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ limitations under the License.
1515

1616
import {Extent, Polyline, Rect} from './internal_types';
1717

18-
export function convertRectToExtent(rect: Rect): Extent {
18+
function convertRectToExtent(rect: Rect): Extent {
1919
return {
2020
x: [rect.x, rect.x + rect.width],
2121
y: [rect.y, rect.y + rect.height],
@@ -39,15 +39,15 @@ let cachedIsWebGl2Supported = false;
3939
}
4040
}
4141

42-
export function isWebGl2Supported(): boolean {
42+
function isWebGl2Supported(): boolean {
4343
return cachedIsWebGl2Supported;
4444
}
4545

46-
export function isOffscreenCanvasSupported(): boolean {
46+
function isOffscreenCanvasSupported(): boolean {
4747
return self.hasOwnProperty('OffscreenCanvas');
4848
}
4949

50-
export function arePolylinesEqual(lineA: Polyline, lineB: Polyline) {
50+
function arePolylinesEqual(lineA: Polyline, lineB: Polyline) {
5151
if (lineA.length !== lineB.length) {
5252
return false;
5353
}
@@ -60,11 +60,19 @@ export function arePolylinesEqual(lineA: Polyline, lineB: Polyline) {
6060
return true;
6161
}
6262

63-
export function areExtentsEqual(extentA: Extent, extentB: Extent): boolean {
63+
function areExtentsEqual(extentA: Extent, extentB: Extent): boolean {
6464
return (
6565
extentA.x[0] === extentB.x[0] &&
6666
extentA.x[1] === extentB.x[1] &&
6767
extentA.y[0] === extentB.y[0] &&
6868
extentA.y[1] === extentB.y[1]
6969
);
7070
}
71+
72+
export const ChartUtils = {
73+
convertRectToExtent,
74+
isWebGl2Supported,
75+
isOffscreenCanvasSupported,
76+
arePolylinesEqual,
77+
areExtentsEqual,
78+
};

tensorboard/webapp/widgets/line_chart_v2/line_chart_component.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ import {
4343
ScaleType,
4444
} from './lib/public_types';
4545
import {createScale} from './lib/scale';
46-
import {isOffscreenCanvasSupported} from './lib/utils';
46+
import {ChartUtils} from './lib/utils';
4747
import {WorkerChart} from './lib/worker/worker_chart';
4848
import {
4949
computeDataSeriesExtent,
@@ -385,7 +385,8 @@ export class LineChartComponent
385385
}
386386

387387
const useWorker =
388-
rendererType !== RendererType.SVG && isOffscreenCanvasSupported();
388+
rendererType !== RendererType.SVG &&
389+
ChartUtils.isOffscreenCanvasSupported();
389390
const klass = useWorker ? WorkerChart : ChartImpl;
390391
this.lineChart = new klass(params);
391392
}

tensorboard/webapp/widgets/line_chart_v2/line_chart_internal_utils.ts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ import {
1818
DataSeriesMetadataMap,
1919
RendererType,
2020
} from './lib/public_types';
21-
import {isWebGl2Supported} from './lib/utils';
21+
import {ChartUtils} from './lib/utils';
2222

2323
/**
2424
* Returns extent, min and max values of each dimensions, of all data series points.
@@ -79,7 +79,9 @@ export function getRendererType(
7979
case RendererType.SVG:
8080
return RendererType.SVG;
8181
case RendererType.WEBGL:
82-
return isWebGl2Supported() ? RendererType.WEBGL : RendererType.SVG;
82+
return ChartUtils.isWebGl2Supported()
83+
? RendererType.WEBGL
84+
: RendererType.SVG;
8385
default:
8486
const _ = preferredRendererType as never;
8587
throw new Error(`Unknown rendererType: ${preferredRendererType}`);

tensorboard/webapp/widgets/line_chart_v2/line_chart_internal_utils_test.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ limitations under the License.
1616
import {RendererType, ScaleType} from './lib/public_types';
1717
import {createScale} from './lib/scale';
1818
import {buildMetadata, buildSeries} from './lib/testing';
19-
import * as libUtils from './lib/utils';
19+
import {ChartUtils} from './lib/utils';
2020
import {
2121
computeDataSeriesExtent,
2222
getRendererType,
@@ -465,12 +465,12 @@ describe('line_chart_v2/line_chart_internal_utils test', () => {
465465
});
466466

467467
it('returns webgl if webgl2 is supported', () => {
468-
spyOn(libUtils, 'isWebGl2Supported').and.returnValue(true);
468+
spyOn(ChartUtils, 'isWebGl2Supported').and.returnValue(true);
469469
expect(getRendererType(RendererType.WEBGL)).toBe(RendererType.WEBGL);
470470
});
471471

472472
it('returns svg if webgl2 is not supported', () => {
473-
spyOn(libUtils, 'isWebGl2Supported').and.returnValue(false);
473+
spyOn(ChartUtils, 'isWebGl2Supported').and.returnValue(false);
474474
expect(getRendererType(RendererType.WEBGL)).toBe(RendererType.SVG);
475475
});
476476
});

tensorboard/webapp/widgets/line_chart_v2/sub_view/line_chart_axis_utils.ts

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ function getNumLeadingZerosInFractional(value: number): number {
3939
return 0;
4040
}
4141

42-
export function getStandardTicks(
42+
function getStandardTicks(
4343
scale: Scale,
4444
formatter: Formatter,
4545
maxMinorTickCount: number,
@@ -57,7 +57,7 @@ export function getStandardTicks(
5757
};
5858
}
5959

60-
export function getTicksForTemporalScale(
60+
function getTicksForTemporalScale(
6161
scale: Scale,
6262
formatter: Formatter,
6363
maxMinorTickCount: number,
@@ -87,7 +87,7 @@ export function getTicksForTemporalScale(
8787
};
8888
}
8989

90-
export function getTicksForLinearScale(
90+
function getTicksForLinearScale(
9191
scale: LinearScale,
9292
formatter: Formatter,
9393
maxMinorTickCount: number,
@@ -181,7 +181,7 @@ const canvasForMeasure = document.createElement('canvas').getContext('2d');
181181
* @param marginBetweenAxis Optional required spacing between labels.
182182
* @returns Filtered minor ticks based on their visibilities.
183183
*/
184-
export function filterTicksByVisibility(
184+
function filterTicksByVisibility(
185185
minorTicks: MinorTick[],
186186
getDomPos: (tick: MinorTick) => number,
187187
axis: 'x' | 'y',
@@ -223,3 +223,10 @@ export function filterTicksByVisibility(
223223
return true;
224224
});
225225
}
226+
227+
export const AxisUtils = {
228+
getStandardTicks,
229+
getTicksForTemporalScale,
230+
getTicksForLinearScale,
231+
filterTicksByVisibility,
232+
};

0 commit comments

Comments
 (0)