HARP-13852 Adding unit tests for MapControls toggleTilt #2069

nzjony · 2021-01-22T14:37:16Z

No description provided.

nzjony · 2021-01-22T14:44:35Z

@here/harp-map-controls/test/MapControlsTest.ts

-                now: () => {}
+                now: () => {
+                    // Time in ms, i.e. 20ms gives us a FPS of 50.
+                    return (time += 20);


I tried incrementing by 1, but it is just a waste of resources, so this is a good value which approaches normal usage

codecov · 2021-01-22T15:13:47Z

Codecov Report

Merging #2069 (af400c5) into master (c4790fb) will decrease coverage by 0.09%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##           master    #2069      +/-   ##
==========================================
- Coverage   66.81%   66.71%   -0.10%     
==========================================
  Files         297      297              
  Lines       26378    26353      -25     
  Branches     5969     5958      -11     
==========================================
- Hits        17624    17582      -42     
- Misses       8754     8771      +17

Impacted Files	Coverage Δ
@here/harp-mapview/lib/CameraMovementDetector.ts	`93.05% <80.00%> (+0.09%)`	⬆️
@here/harp-test-utils/lib/WebGLStub.ts	`100.00% <100.00%> (ø)`
...here/harp-datasource-protocol/lib/TechniqueAttr.ts	`60.00% <0.00%> (-30.00%)`	⬇️
@here/harp-datasource-protocol/lib/DecodedTile.ts	`50.96% <0.00%> (-4.09%)`	⬇️
...vectortile-datasource/lib/VectorTileDataEmitter.ts	`60.22% <0.00%> (-4.05%)`	⬇️
@here/harp-datasource-protocol/lib/Techniques.ts	`71.69% <0.00%> (-0.95%)`	⬇️
@here/harp-mapview/lib/TileObjectsRenderer.ts	`44.20% <0.00%> (-0.80%)`	⬇️
@here/harp-mapview/lib/Statistics.ts	`68.88% <0.00%> (-0.27%)`	⬇️
.../harp-datasource-protocol/lib/StyleSetEvaluator.ts	`89.05% <0.00%> (-0.26%)`	⬇️
@here/harp-mapview/lib/MapObjectAdapter.ts	`87.50% <0.00%> (-0.22%)`	⬇️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 99b41a3...8dac926. Read the comment docs.

@here/harp-mapview/lib/CameraMovementDetector.ts

atomicsulfate · 2021-01-25T11:06:22Z

@here/harp-map-controls/test/MapControlsTest.ts

+                const checkGoesToZero = () => {
+                    const mapViewTiltRad = THREE.MathUtils.degToRad(mapView.tilt);
+
+                    // Resolve when we reach 0
+                    if (mapViewTiltRad <= Number.EPSILON) {
+                        mapView.removeEventListener(MapViewEventNames.AfterRender, checkGoesToZero);
+                        resolvePromise();
+                    } else if (previousTilt === mapViewTiltRad) {
+                        rejectPromise();
+                    }
+                    previousTilt = mapViewTiltRad;
+                };


This and checkReachesTiltAngle can be done with a single function. Just pass the target angle as parameter.

Yes, great idea!

atomicsulfate · 2021-01-25T11:15:23Z

@here/harp-map-controls/test/MapControlsTest.ts

+                    }
+                    previousTilt = mapViewTiltRad;
+                };
+                mapView.addEventListener(MapViewEventNames.AfterRender, checkReachesTiltAngle);


I would listen to MapViewEventNames.MovementFinished instead, that way we only need to check the tilt once at the end of the animation. Then return the reached tilt angle as Promise result and check the value in the assertion as I mention next.

Much cleaner, thanks for the tip!

atomicsulfate · 2021-01-25T11:20:51Z

@here/harp-map-controls/test/MapControlsTest.ts

+                    previousTilt = mapViewTiltRad;
+                };
+                mapView.addEventListener(MapViewEventNames.AfterRender, checkReachesTiltAngle);
+                await tiltCamera;


Suggested change

await tiltCamera;

expect(tiltCamera).to.eventually.be.closeTo(targetTilt, Number.EPSILON).then( // add here the second part of the test);

Using chai as promised the test expectation is clearer and the error messages will be more explicit. You need to import chai as promised (see TileLoaderTest).

atomicsulfate · 2021-01-25T11:22:08Z

@here/harp-map-controls/test/MapControlsTest.ts

+                    previousTilt = mapViewTiltRad;
+                };
+                mapView.addEventListener(MapViewEventNames.AfterRender, checkGoesToZero);
+                await tiltBackToZero;


Suggested change

await tiltBackToZero;

return expect(tiltBackToZero).to.eventually.be.closeTo(0, Number.EPSILON);

I have changed to use expect.

I don't like the return syntax, because otherwise the test gets more nested and it makes readability / maintenance harder IMO, also, await is used a lot in existing tests, so I think this is not some exception.

nzjony · 2021-01-28T12:48:31Z

retest this please

nzjony · 2021-01-28T15:15:17Z

retest this please

nzjony · 2021-01-29T10:13:52Z

retest this please

nzjony · 2021-01-29T14:12:39Z

rebase please

Signed-off-by: Jonathan Stichbury <2533428+nzjony@users.noreply.github.com>

github-actions · 2021-01-29T14:13:22Z

Rebase succeeded.

nzjony · 2021-01-29T15:36:28Z

retest this please

nzjony · 2021-02-01T09:22:47Z

retest this please

nzjony · 2021-02-01T09:44:48Z

retest this please

nzjony · 2021-02-01T09:53:44Z

@harpgl-bot retest this please

nzjony · 2021-02-01T10:03:16Z

rebase please

github-actions · 2021-02-01T10:03:54Z

Rebase succeeded.

nzjony force-pushed the HARP-13852_unittest branch from 06ce97e to dd5947f Compare January 22, 2021 14:42

nzjony commented Jan 22, 2021

View reviewed changes

nzjony force-pushed the HARP-13852_unittest branch from dd5947f to af400c5 Compare January 22, 2021 15:50

nzjony requested a review from atomicsulfate January 22, 2021 16:26

atomicsulfate reviewed Jan 25, 2021

View reviewed changes

nzjony force-pushed the HARP-13852_unittest branch from af400c5 to 0a4927f Compare January 25, 2021 12:34

atomicsulfate self-requested a review January 25, 2021 13:45

atomicsulfate previously approved these changes Jan 25, 2021

View reviewed changes

nzjony enabled auto-merge (squash) January 28, 2021 12:48

HARP-13852 Unit test for toggleTilt animation

8dac926

Signed-off-by: Jonathan Stichbury <2533428+nzjony@users.noreply.github.com>

github-actions bot dismissed atomicsulfate’s stale review via 8dac926 January 29, 2021 14:13

github-actions bot force-pushed the HARP-13852_unittest branch from 0a4927f to 8dac926 Compare January 29, 2021 14:13

atomicsulfate self-requested a review January 29, 2021 14:30

atomicsulfate approved these changes Jan 29, 2021

View reviewed changes

nzjony disabled auto-merge February 1, 2021 15:42

nzjony merged commit f638162 into master Feb 1, 2021

nzjony deleted the HARP-13852_unittest branch February 1, 2021 15:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HARP-13852 Adding unit tests for MapControls toggleTilt #2069

HARP-13852 Adding unit tests for MapControls toggleTilt #2069

nzjony commented Jan 22, 2021

nzjony Jan 22, 2021

codecov bot commented Jan 22, 2021 •

edited

Loading

atomicsulfate Jan 25, 2021

nzjony Jan 25, 2021

atomicsulfate Jan 25, 2021

nzjony Jan 25, 2021

atomicsulfate Jan 25, 2021

atomicsulfate Jan 25, 2021 •

edited

Loading

nzjony Jan 25, 2021

nzjony commented Jan 28, 2021

nzjony commented Jan 28, 2021

nzjony commented Jan 29, 2021

nzjony commented Jan 29, 2021

github-actions bot commented Jan 29, 2021

nzjony commented Jan 29, 2021

nzjony commented Feb 1, 2021

nzjony commented Feb 1, 2021

nzjony commented Feb 1, 2021

nzjony commented Feb 1, 2021

github-actions bot commented Feb 1, 2021

	await tiltCamera;
	expect(tiltCamera).to.eventually.be.closeTo(targetTilt, Number.EPSILON).then( // add here the second part of the test);

	await tiltBackToZero;
	return expect(tiltBackToZero).to.eventually.be.closeTo(0, Number.EPSILON);

HARP-13852 Adding unit tests for MapControls toggleTilt #2069

HARP-13852 Adding unit tests for MapControls toggleTilt #2069

Conversation

nzjony commented Jan 22, 2021

nzjony Jan 22, 2021

Choose a reason for hiding this comment

codecov bot commented Jan 22, 2021 • edited Loading

Codecov Report

atomicsulfate Jan 25, 2021

Choose a reason for hiding this comment

nzjony Jan 25, 2021

Choose a reason for hiding this comment

atomicsulfate Jan 25, 2021

Choose a reason for hiding this comment

nzjony Jan 25, 2021

Choose a reason for hiding this comment

atomicsulfate Jan 25, 2021

Choose a reason for hiding this comment

atomicsulfate Jan 25, 2021 • edited Loading

Choose a reason for hiding this comment

nzjony Jan 25, 2021

Choose a reason for hiding this comment

nzjony commented Jan 28, 2021

nzjony commented Jan 28, 2021

nzjony commented Jan 29, 2021

nzjony commented Jan 29, 2021

github-actions bot commented Jan 29, 2021

nzjony commented Jan 29, 2021

nzjony commented Feb 1, 2021

nzjony commented Feb 1, 2021

nzjony commented Feb 1, 2021

nzjony commented Feb 1, 2021

github-actions bot commented Feb 1, 2021

codecov bot commented Jan 22, 2021 •

edited

Loading

atomicsulfate Jan 25, 2021 •

edited

Loading