Skip to content
This repository has been archived by the owner on Mar 8, 2023. It is now read-only.

HARP-12989 Fix extra console entries in tests #1972

Merged
merged 6 commits into from
Nov 19, 2020
Merged

HARP-12989 Fix extra console entries in tests #1972

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b6e4704
HARP-12989 Fix extra console entries in tests
nzjony Nov 16, 2020
b5ddf2e
HARP-12989 Fixing deprecated warning for min/max ZoomLevel
nzjony Nov 16, 2020
48e8399
HARP-12989 Removing some more loggers
nzjony Nov 17, 2020
2b056c8
Code review fixes
nzjony Nov 17, 2020
688399a
More code review fixes
nzjony Nov 18, 2020
ecd16a6
Wait for mapView to finish loading before disposing
nzjony Nov 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
111 changes: 59 additions & 52 deletions @here/harp-mapview-decoder/test/TileDataSourceTest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {
webMercatorTilingScheme
} from "@here/harp-geoutils";
import { DataSource, MapView, Statistics, Tile, TileLoaderState } from "@here/harp-mapview";
import { errorOnlyLoggingAroundFunction } from "@here/harp-test-utils";
import { assert, expect } from "chai";
import * as sinon from "sinon";

Expand Down Expand Up @@ -301,72 +302,78 @@ describe("TileDataSource", function() {
});

it("supports deprecated minZoomLevel and maxZoomLevel in constructor", function() {
const testedDataSource = new TileDataSource(new TileFactory(Tile), {
styleSetName: "",
tilingScheme: webMercatorTilingScheme,
dataProvider: new MockDataProvider(),
decoder: createMockTileDecoder(),
minZoomLevel: 3,
maxZoomLevel: 17
errorOnlyLoggingAroundFunction("DataSource", () => {
const testedDataSource = new TileDataSource(new TileFactory(Tile), {
styleSetName: "",
tilingScheme: webMercatorTilingScheme,
dataProvider: new MockDataProvider(),
decoder: createMockTileDecoder(),
minZoomLevel: 3,
maxZoomLevel: 17
});

assert.equal(testedDataSource.minZoomLevel, 3);
assert.equal(testedDataSource.minDataLevel, 3);
assert.equal(testedDataSource.maxZoomLevel, 17);
assert.equal(testedDataSource.maxDataLevel, 17);
});

assert.equal(testedDataSource.minZoomLevel, 3);
assert.equal(testedDataSource.minDataLevel, 3);
assert.equal(testedDataSource.maxZoomLevel, 17);
assert.equal(testedDataSource.maxDataLevel, 17);
});

it("supports setting of theme", async function() {
const mockDecoder = createMockTileDecoder();
const testedDataSource = new TileDataSource(new TileFactory(Tile), {
styleSetName: "tilezen",
tilingScheme: webMercatorTilingScheme,
dataProvider: new MockDataProvider(),
decoder: mockDecoder,
minZoomLevel: 3,
maxZoomLevel: 17
errorOnlyLoggingAroundFunction("DataSource", async () => {
const testedDataSource = new TileDataSource(new TileFactory(Tile), {
styleSetName: "tilezen",
tilingScheme: webMercatorTilingScheme,
dataProvider: new MockDataProvider(),
decoder: mockDecoder,
minZoomLevel: 3,
maxZoomLevel: 17
});

testedDataSource.attach(createMockMapView());

const styles: StyleSet = [
{
styleSet: "tilezen",
technique: "none"
}
];

await testedDataSource.setTheme({
styles
});

assert(mockDecoder.configure.calledOnce);
assert(mockDecoder.configure.calledWith(sinon.match({ styleSet: styles })));
});

testedDataSource.attach(createMockMapView());

const styles: StyleSet = [
{
styleSet: "tilezen",
technique: "none"
}
];

await testedDataSource.setTheme({
styles
});

assert(mockDecoder.configure.calledOnce);
assert(mockDecoder.configure.calledWith(sinon.match({ styleSet: styles })));
});

it("supports setting of languages", async function() {
const mockDecoder = createMockTileDecoder();
const testedDataSource = new TileDataSource(new TileFactory(Tile), {
styleSetName: "tilezen",
tilingScheme: webMercatorTilingScheme,
dataProvider: new MockDataProvider(),
decoder: mockDecoder,
minZoomLevel: 3,
maxZoomLevel: 17,
languages: ["de"]
});
errorOnlyLoggingAroundFunction("DataSource", async () => {
const testedDataSource = new TileDataSource(new TileFactory(Tile), {
styleSetName: "tilezen",
tilingScheme: webMercatorTilingScheme,
dataProvider: new MockDataProvider(),
decoder: mockDecoder,
minZoomLevel: 3,
maxZoomLevel: 17,
languages: ["de"]
});

await testedDataSource.connect();
await testedDataSource.connect();

expect(mockDecoder.configure.calledOnce).to.be.true;
expect(mockDecoder.configure.calledWith(sinon.match({ languages: ["de"] }))).to.be.true;
expect(mockDecoder.configure.calledOnce).to.be.true;
expect(mockDecoder.configure.calledWith(sinon.match({ languages: ["de"] }))).to.be.true;

testedDataSource.attach(createMockMapView());
testedDataSource.attach(createMockMapView());

testedDataSource.setLanguages(["de", "en"]);
testedDataSource.setLanguages(["de", "en"]);

expect(mockDecoder.configure.calledTwice).to.be.true;
expect(mockDecoder.configure.calledWith(sinon.match({ languages: ["de", "en"] }))).to.be
.true;
expect(mockDecoder.configure.calledTwice).to.be.true;
expect(mockDecoder.configure.calledWith(sinon.match({ languages: ["de", "en"] }))).to.be
.true;
});
});
});
4 changes: 2 additions & 2 deletions @here/harp-mapview/lib/MapView.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1093,7 +1093,7 @@ export class MapView extends EventDispatcher {

// Must be initialized before setupCamera, because the VisibleTileSet is created as part
// of the setupCamera method and it needs the TaskQueue instance, same for environment
this.m_taskScheduler = new MapViewTaskScheduler(this.maxFps);
this.m_taskScheduler = new MapViewTaskScheduler(this.maxFps, this);
this.m_sceneEnvironment = new MapViewEnvironment(this, options);

// setup camera with initial position
Expand Down Expand Up @@ -2158,7 +2158,7 @@ export class MapView extends EventDispatcher {
dataSource.setLanguages(this.m_languages);

if (theme !== undefined && theme.styles !== undefined) {
dataSource.setTheme(theme);
await dataSource.setTheme(theme);
}

this.m_connectedDataSources.add(dataSource.name);
Expand Down
14 changes: 13 additions & 1 deletion @here/harp-mapview/lib/MapViewEnvironment.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,13 +47,16 @@ export class MapViewEnvironment {
private m_skyBackground?: SkyBackground;
private m_createdLights?: THREE.Light[];
private m_overlayCreatedLights?: THREE.Light[];
private readonly m_backgroundDSPromise: Promise<void> | undefined;
private readonly m_backgroundDataSource?: BackgroundDataSource;

constructor(private readonly m_mapView: MapView, options: MapViewEnvironmentOptions) {
this.m_fog = new MapViewFog(this.m_mapView.scene);
if (options.addBackgroundDatasource !== false) {
this.m_backgroundDataSource = new BackgroundDataSource();
this.m_mapView.addDataSource(this.m_backgroundDataSource);
this.m_backgroundDSPromise = this.m_mapView.addDataSource(this.m_backgroundDataSource);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to capture this and make it external so that it can be awaited in the test to ensure that it is connected before we continue test execution.

} else {
this.m_backgroundDSPromise = undefined;
}
if (
options.backgroundTilingScheme !== undefined &&
Expand All @@ -72,6 +75,15 @@ export class MapViewEnvironment {
return this.m_fog;
}

/**
* Required to be able to wait for this DataSource to be added, if not, the MapViewTest.ts will
* complain because we then call update() once the MapView is disposed.
* @internal
*/
get backgroundDataSourcePromise(): Promise<void> | undefined {
return this.m_backgroundDSPromise;
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can and should avoid exposing this implementation detail, see my comments in MapViewTest.

updateBackgroundDataSource() {
if (this.m_backgroundDataSource) {
this.m_backgroundDataSource.updateStorageLevelOffset();
Expand Down
15 changes: 11 additions & 4 deletions @here/harp-mapview/lib/MapViewTaskScheduler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
import { PerformanceTimer, Task, TaskQueue } from "@here/harp-utils";
import THREE = require("three");

import { TileTaskGroups } from "./MapView";
import { MapView, TileTaskGroups } from "./MapView";
import { PerformanceStatistics } from "./Statistics";

const DEFAULT_MAX_FPS = 60;
Expand All @@ -17,7 +17,7 @@ export class MapViewTaskScheduler extends THREE.EventDispatcher {
private readonly m_taskQueue: TaskQueue;
private m_throttlingEnabled: boolean = false;

constructor(private m_maxFps: number = DEFAULT_MAX_FPS) {
constructor(private m_maxFps: number = DEFAULT_MAX_FPS, private readonly m_mapView: MapView) {
nzjony marked this conversation as resolved.
Show resolved Hide resolved
super();
this.m_taskQueue = new TaskQueue({
groups: [TileTaskGroups.FETCH_AND_DECODE, TileTaskGroups.CREATE],
Expand Down Expand Up @@ -77,6 +77,10 @@ export class MapViewTaskScheduler extends THREE.EventDispatcher {
let numItemsLeft = this.taskQueue.numItemsLeft();
currentFrameEvent?.setValue("TaskScheduler.numPendingTasks", numItemsLeft);

// Needed for tests that dispose but that still have running tasks after MapView disposed.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise the tasks are executed on a mapView that has disposed and the update method sends long of messages to the console.

const mapViewAlive = () => {
return !this.m_mapView.disposed;
};
if (this.throttlingEnabled) {
// get the available time in this frame to achieve a max fps rate
let availableTime = this.spaceInFrame(frameStartTime);
Expand All @@ -90,6 +94,9 @@ export class MapViewTaskScheduler extends THREE.EventDispatcher {
counter++;
// create a processing condition for the tasks
function shouldProcess(task: Task) {
if (!mapViewAlive()) {
return false;
}
// if there is a time estimate use it, otherwise default to 1 ms
// TODO: check whats a sane default, 1 seems to do it for now
availableTime -=
Expand Down Expand Up @@ -132,12 +139,12 @@ export class MapViewTaskScheduler extends THREE.EventDispatcher {
//if throttling is disabled, process all pending tasks
this.m_taskQueue.processNext(
TileTaskGroups.CREATE,
undefined,
mapViewAlive,
this.m_taskQueue.numItemsLeft(TileTaskGroups.CREATE)
);
this.m_taskQueue.processNext(
TileTaskGroups.FETCH_AND_DECODE,
undefined,
mapViewAlive,
this.m_taskQueue.numItemsLeft(TileTaskGroups.FETCH_AND_DECODE)
);
}
Expand Down
Loading