-
Notifications
You must be signed in to change notification settings - Fork 197
HARP-12989 Fix extra console entries in tests #1972
Conversation
Signed-off-by: Jonathan Stichbury <[email protected]>
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); |
There was a problem hiding this comment.
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 await
ed in the test to ensure that it is connected before we continue test execution.
@@ -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. |
There was a problem hiding this comment.
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.
retest this please |
Signed-off-by: Jonathan Stichbury <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #1972 +/- ##
==========================================
+ Coverage 65.89% 65.97% +0.07%
==========================================
Files 293 293
Lines 26238 26257 +19
Branches 5945 5947 +2
==========================================
+ Hits 17290 17323 +33
+ Misses 8948 8934 -14
Continue to review full report at Codecov.
|
Signed-off-by: Jonathan Stichbury <[email protected]>
Signed-off-by: Jonathan Stichbury <[email protected]>
@atomicsulfate , thanks for the review, I have resolved them. |
/** | ||
* 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; | ||
} | ||
|
There was a problem hiding this comment.
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.
@@ -1434,6 +1453,8 @@ describe("MapView", function() { | |||
}; | |||
} | |||
mapView = new MapView({ canvas, theme: {} }); | |||
await mapView.getTheme(); | |||
await mapView.sceneEnvironment.backgroundDataSourcePromise; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed, I checked locally and only waiting for the theme it's enough to get rid of the errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will recheck, because I tried this too and got an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wrong, have fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's many more tests in MapViewTest that trigger an update after disposal of the MapView (e.g. all those called "Correctly sets..."). I checked some and they're fixed just by waiting for the theme after the MapView construction.
Signed-off-by: Jonathan Stichbury <[email protected]>
I don't see any cases of this with my latest change set... can you try again with my latest changes please? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's many more tests in MapViewTest that trigger an update after disposal of the MapView (e.g. all those called "Correctly sets..."). I checked some and they're fixed just by waiting for the theme after the MapView construction.
I don't see any cases of this with my latest change set... can you try again with my latest changes please?
You can see them in Travis for your build:
https://travis-ci.com/github/heremaps/harp.gl/jobs/442111620#L7504
https://travis-ci.com/github/heremaps/harp.gl/jobs/442111620#L4235
Signed-off-by: Jonathan Stichbury <[email protected]>
Thanks, weird, I couldn't reproduce them locally, I guess because I use a different node version. I have pushed a fix and will check the travis-ci. |
retest this please |
No description provided.