core(test): Add new test for map init config #3205

jolevesq · 2025-12-12T14:38:06Z

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes # (issue)

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce.

https://jolevesq.github.io/geoview/tests.html

Checklist:

I have build (rush build) and deploy (rush host) my PR
I have connected the issues(s) to this PR
My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
My changes generate no new warnings
I have created new issue(s) related to the outcome of this PR is needed
~~I have made corresponding changes to the documentation~~
~~I have added tests that prove my fix is effective or that my feature works~~
~~New and existing unit tests pass locally with my changes~~

This change is

Alex-NRCan

@Alex-NRCan reviewed 10 of 10 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @jolevesq)

packages/geoview-core/public/templates/tests.html line 345 at r1 (raw file):

      <div
        id="map4"
        style="width: 1000px; margin: auto"

What's the reason for the width:1000px and margin:auto for this map4?

packages/geoview-core/public/templates/tests.html line 581 at r1 (raw file):

          // After maps initialize, hide all except Run All Tests
          setTimeout(function () {

Hook on the cgpv.onMapInit() instead of setTimeout 2 seconds?

packages/geoview-core/public/templates/tests.html line 682 at r1 (raw file):

      // Function to update the global statistics in "Run All Tests" tab
      function updateGlobalStats() {

Is this the code that was in codedoc.js? If so, get rid of the code in codedoc.js?

packages/geoview-test-suite/src/tests/testers/map-config-tester.ts line 106 at r1 (raw file):

        // Verify the number of features in the data table
        test.addStep('Verifying number of features in data table...');
        if (Array.isArray(allFeaturesData) && allFeaturesData.length > 0 && allFeaturesData[0] && 'features' in allFeaturesData[0]) {

We could add an 'Test.assertIsArray()' function that verifies if a variable is an array, seems like it'd be useful to have and notably here.

packages/geoview-test-suite/src/tests/testers/map-config-tester.ts

+    if (value === null || value === undefined) {
+      delete current[finalKey];
+    } else {
+      current[finalKey] = value;


jolevesq

@jolevesq reviewed 11 files and all commit messages, made 3 comments, and resolved 1 discussion.
Reviewable status: 16 of 27 files reviewed, 5 unresolved discussions (waiting on @Alex-NRCan).

packages/geoview-core/public/templates/tests.html line 345 at r1 (raw file):

Previously, Alex-NRCan (Alex) wrote…

What's the reason for the width:1000px and margin:auto for this map4?

Do the map always have same size to help with extent assert

packages/geoview-test-suite/src/tests/testers/map-config-tester.ts line 106 at r1 (raw file):

Previously, Alex-NRCan (Alex) wrote…

We could add an 'Test.assertIsArray()' function that verifies if a variable is an array, seems like it'd be useful to have and notably here.

Done.

packages/geoview-core/src/api/event-processors/event-processor-children/feature-info-event-processor.ts line 136 at r4 (raw file):

    if (FeatureInfoEventProcessor.findLayerDataFromLayerDataArray(mapId, layerPath) === undefined) {
      logger.logWarning(`Trying to set selected layer path to '${layerPath}' which is not in the layer data array for mapId '${mapId}'`);

@Alex-NRCan Do we want a ifExist function? An exception?

jolevesq

@jolevesq made 2 comments.
Reviewable status: 15 of 27 files reviewed, 5 unresolved discussions (waiting on @Alex-NRCan).

packages/geoview-core/public/templates/tests.html line 581 at r1 (raw file):

Previously, Alex-NRCan (Alex) wrote…

Hook on the cgpv.onMapInit() instead of setTimeout 2 seconds?

Done.

packages/geoview-core/public/templates/tests.html line 682 at r1 (raw file):

Previously, Alex-NRCan (Alex) wrote…

Is this the code that was in codedoc.js? If so, get rid of the code in codedoc.js?

Done.

Alex-NRCan

@Alex-NRCan reviewed 21 files and all commit messages, made 6 comments, and resolved 4 discussions.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @jolevesq).

packages/geoview-core/src/api/event-processors/event-processor-children/feature-info-event-processor.ts line 136 at r4 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

@Alex-NRCan Do we want a ifExist function? An exception?

Hmm not a black/white answer. Personally, I like it when the framework throws exceptions, forcing the callers to either use IfExists or call a 'hasXXXX', makes for more robust code/behavior/expectations. However, I know that a lot of event processors are used via ui hooks and sometimes it's just easier to just 'skip when none found'.
So, for the event processors I think we can be more flexible and not always throw exceptions. I guess it can be a case-by-case basis.. the important thing I'd say is to try to have the jsdoc be clear when an exception can be thrown or not. That way, the caller knows.

packages/geoview-test-suite/src/tests/core/test.ts line 432 at r5 (raw file):

    // Redirect using a primitive comparer with optional rounding
    this.assertIsArrayEqualComparer(actualValue, expectedValue, (value1: T, value2: T): boolean => {
      // If rounding is specified and both values are numbers, round them first

Here, just redirect the call to assertIsEqual()? That way the calls to this.#roundToPrecision and possibly future logic like that is only coded in one location?

packages/geoview-core/src/geo/map/map-viewer.ts line 812 at r5 (raw file):

   * @param {MapSingleClickEvent} clickCoordinates - The clicked coordinates to emit.
   */
  emitMapSingleClick(clickCoordinates: MapSingleClickEvent): void {

This emit should be private indeed and the circular reference calls should be fixed. Good thing to have the TODO here.
There is a discussion to be had and work to be done in all the event processors.

packages/geoview-test-suite/src/tests/testers/map-config-tester.ts line 228 at r5 (raw file):

        test.addStep('Verifying navBar has default controls...');
        const navBarComponents = UIEventProcessor.getNavBarComponents(mapId);
        Test.assertIsDefined('navBarComponents', navBarComponents);

I'm not sure it's necessary to test if isDefined if we're checking for IsArrayEqual below? Maybe in assertIsArrayEqual, when we pass an actual array in second parameter and the array being checked is undefined, assertIsArrayEqual should check for assertIsArrayEqual automatically? (Saving an assert check at the top level like here)
Note to keep in mind, I do think that if assertIsArrayEqual is called with 2 'undefined' arrays then maybe that should be equal though.

packages/geoview-test-suite/src/tests/testers/map-config-tester.ts line 328 at r5 (raw file):

      async (test) => {
        // Create overlay objects configuration
        const overlayObjectsConfig = {

Maybe it'd be nice to have this variable be moved with the other constants in GVAbstractTester or as a parameter to testOverlayObjectsPointMarkers instead 🤔 ? Of course that also affects the tests of things like find((marker: { id: string }) => marker.id === 'ottawa' so 'ottawa' for example also has to be in the constants and the test assertIsArrayLengthEqual(cities, 2) should probably be dynamic instead of a straight '2'.

Minor: Seeing how 'cities' in mandatory property, would it be worthwhile to have the variable 'TypeScript typed' as well? Or maybe the object { pointMarkers: } can be created here with just the cities stored in the constants / received via parameters?

packages/geoview-test-suite/src/tests/testers/map-config-tester.ts line 405 at r5 (raw file):

        // Test zooming to minimum allowed zoom (6)
        test.addStep('Testing zoom to minimum allowed level (6)...');
        mapViewer.setZoomLevel(6);

Maybe we improve the 'setZoomLevel function to return a Promise so that we can await on it?

Quick: the 'await delay(500)' could be inside the setZoomLevel function
in setZoomLevel, we could listen, once, on the 'rendercomplete' event and resolve a Promise and it's that Promise that's returned, something like:
map.once('rendercomplete', () => {
// zoom fully rendered
resolve();
});

jolevesq

@jolevesq reviewed 2 files and all commit messages, and made 5 comments.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @Alex-NRCan).

packages/geoview-core/src/api/event-processors/event-processor-children/feature-info-event-processor.ts line 136 at r4 (raw file):

Previously, Alex-NRCan (Alex) wrote…

Hmm not a black/white answer. Personally, I like it when the framework throws exceptions, forcing the callers to either use IfExists or call a 'hasXXXX', makes for more robust code/behavior/expectations. However, I know that a lot of event processors are used via ui hooks and sometimes it's just easier to just 'skip when none found'.
So, for the event processors I think we can be more flexible and not always throw exceptions. I guess it can be a case-by-case basis.. the important thing I'd say is to try to have the jsdoc be clear when an exception can be thrown or not. That way, the caller knows.

Ok, so I will only let the warning for now.

packages/geoview-test-suite/src/tests/core/test.ts line 432 at r5 (raw file):

Previously, Alex-NRCan (Alex) wrote…

Here, just redirect the call to assertIsEqual()? That way the calls to this.#roundToPrecision and possibly future logic like that is only coded in one location?

Done

packages/geoview-test-suite/src/tests/testers/map-config-tester.ts line 228 at r5 (raw file):

Previously, Alex-NRCan (Alex) wrote…

I'm not sure it's necessary to test if isDefined if we're checking for IsArrayEqual below? Maybe in assertIsArrayEqual, when we pass an actual array in second parameter and the array being checked is undefined, assertIsArrayEqual should check for assertIsArrayEqual automatically? (Saving an assert check at the top level like here)
Note to keep in mind, I do think that if assertIsArrayEqual is called with 2 'undefined' arrays then maybe that should be equal though.

You re right that it is not really needed... just find it more explicit. I can remove if you think it helps

packages/geoview-test-suite/src/tests/testers/map-config-tester.ts line 328 at r5 (raw file):

Previously, Alex-NRCan (Alex) wrote…

Maybe it'd be nice to have this variable be moved with the other constants in GVAbstractTester or as a parameter to testOverlayObjectsPointMarkers instead 🤔 ? Of course that also affects the tests of things like find((marker: { id: string }) => marker.id === 'ottawa' so 'ottawa' for example also has to be in the constants and the test assertIsArrayLengthEqual(cities, 2) should probably be dynamic instead of a straight '2'.

Minor: Seeing how 'cities' in mandatory property, would it be worthwhile to have the variable 'TypeScript typed' as well? Or maybe the object { pointMarkers: } can be created here with just the cities stored in the constants / received via parameters?

I am a bit confused for this one... because this info is only useful for this particular test I put it there. In your test you have the suite send the parameters... is it not clearer when test has its own const inside. If they need to be reuse it make sense to move them but when it is only for one test???

packages/geoview-test-suite/src/tests/testers/map-config-tester.ts line 405 at r5 (raw file):

Previously, Alex-NRCan (Alex) wrote…

Maybe we improve the 'setZoomLevel function to return a Promise so that we can await on it?

Quick: the 'await delay(500)' could be inside the setZoomLevel function

in setZoomLevel, we could listen, once, on the 'rendercomplete' event and resolve a Promise and it's that Promise that's returned, something like:
map.once('rendercomplete', () => {
// zoom fully rendered
resolve();
});

Done

Alex-NRCan

@Alex-NRCan made 8 comments and resolved 5 discussions.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @jolevesq).

packages/geoview-test-suite/src/tests/testers/map-config-tester.ts line 328 at r5 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

I am a bit confused for this one... because this info is only useful for this particular test I put it there. In your test you have the suite send the parameters... is it not clearer when test has its own const inside. If they need to be reuse it make sense to move them but when it is only for one test???

If it's just for one test and very restricted to it, it's okay. I wonder if we shouldn't have more parameters, in general, in our testers, maybe not.

packages/geoview-test-suite/src/tests/testers/map-tester.ts line 480 at r5 (raw file):

        // Wait for basemap change
        await delay(1000);

What is it waiting for exactly? The basemap isn't set after a setBasemap? It seems the activeBasemap should be set as it's the first thing happening in 'setBasemap' 🤔

packages/geoview-test-suite/src/tests/testers/map-tester.ts line 521 at r5 (raw file):

          test.addStep('Switching to LCC projection (3978)...');
          await MapEventProcessor.setProjection(this.getMapId(), 3978);
          await delay(500);

Can we write the reason for the delay here and for every delay where no comment is written. Ideally, if the reason can be explained, we should fix it in the core framework. Building these tests help us with that.

packages/geoview-test-suite/src/tests/testers/map-tester.ts line 526 at r5 (raw file):

        test.addStep('Zooming to British Columbia extent...');
        await this.getMapViewer().zoomToLonLatExtentOrCoordinate(bcExtent);
        await delay(500);

Same comment

packages/geoview-test-suite/src/tests/testers/map-tester.ts line 586 at r5 (raw file):

        // If listener not enabled, enable it
        test.addStep(`Enabling click listener for layer '${layerPath}'...`);
        if (!featureInfoLayerSet.isClickListenerEnabled(layerPath)) {

This will change in my PR to gvLayer.get/setQueryable().

packages/geoview-test-suite/src/tests/testers/map-tester.ts line 660 at r5 (raw file):

        // If listener not enabled, enable it
        test.addStep(`Enabling hover listener for layer '${layerPath}'...`);
        if (!hoverFeatureInfoLayerSet.isHoverListenerEnabled(layerPath)) {

Same comment, will need conflict resolving with my PR

packages/geoview-test-suite/src/tests/testers/map-tester.ts line 687 at r5 (raw file):

        Test.assertIsEqual(result.enabledAfter, false);

        // GV: Need to wait before calling next test or the first map click return empty features

This delay shouldn't be there, because there's no guarantee the 'next test' will have a map click. We should understand why the delay was necessary and fix it. If it has to do with a map click it should at least be closer to where the map click happens, but ideally should just be fixed and removed.

packages/geoview-test-suite/src/tests/testers/map-tester.ts line 732 at r5 (raw file):

        // await delay(2000);
        simulateMapClick(firstClickCoords);
        await delay(1000);

Here, I'm assuming the delay depends on the duration of the query and it's only when the query finishes that the selectedLayerPath is set. We should probably remove the delay(1000) here and listen on the onQueryEnded event of the featureInfoLayerSet. That way, if the query takes for example 1001ms the test passes instead of failing.

jolevesq

@jolevesq reviewed 11 files and made 7 comments.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @Alex-NRCan).

packages/geoview-test-suite/src/tests/testers/map-tester.ts line 480 at r5 (raw file):

Previously, Alex-NRCan (Alex) wrote…

What is it waiting for exactly? The basemap isn't set after a setBasemap? It seems the activeBasemap should be set as it's the first thing happening in 'setBasemap' 🤔

Done.

packages/geoview-test-suite/src/tests/testers/map-tester.ts line 521 at r5 (raw file):

Previously, Alex-NRCan (Alex) wrote…

Can we write the reason for the delay here and for every delay where no comment is written. Ideally, if the reason can be explained, we should fix it in the core framework. Building these tests help us with that.

Removed

packages/geoview-test-suite/src/tests/testers/map-tester.ts line 526 at r5 (raw file):

Previously, Alex-NRCan (Alex) wrote…

Same comment

Need one for the dom to update

packages/geoview-test-suite/src/tests/testers/map-tester.ts line 586 at r5 (raw file):

Previously, Alex-NRCan (Alex) wrote…

This will change in my PR to gvLayer.get/setQueryable().

I will let you modify it

packages/geoview-test-suite/src/tests/testers/map-tester.ts line 660 at r5 (raw file):

Previously, Alex-NRCan (Alex) wrote…

Same comment, will need conflict resolving with my PR

ok

packages/geoview-test-suite/src/tests/testers/map-tester.ts line 687 at r5 (raw file):

Previously, Alex-NRCan (Alex) wrote…

This delay shouldn't be there, because there's no guarantee the 'next test' will have a map click. We should understand why the delay was necessary and fix it. If it has to do with a map click it should at least be closer to where the map click happens, but ideally should just be fixed and removed.

Moved it in the test where it belong

packages/geoview-test-suite/src/tests/testers/map-tester.ts line 732 at r5 (raw file):

Previously, Alex-NRCan (Alex) wrote…

Here, I'm assuming the delay depends on the duration of the query and it's only when the query finishes that the selectedLayerPath is set. We should probably remove the delay(1000) here and listen on the onQueryEnded event of the featureInfoLayerSet. That way, if the query takes for example 1001ms the test passes instead of failing.

Yes and no.... using the the event works but not for first click.... I added comment

Alex-NRCan

@Alex-NRCan reviewed 3 files and resolved 8 discussions.
Reviewable status: 26 of 29 files reviewed, all discussions resolved (waiting on @jolevesq).

Alex-NRCan

@Alex-NRCan reviewed 4 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @jolevesq).

packages/geoview-core/src/geo/map/map-viewer.ts line 713 at r7 (raw file):

   */
  setMapZoomLevel(zoom: number): Promise<void> {
    this.getView().setZoom(zoom);

Maybe worth trying to set zoom to a zoom level that's already what the map is at.. I'm curious if the 'rendercomplete' event is raised - I'm assuming it might not. So we should maybe check if the current map zoom is the same as the zoom in parameter and if so, just return Promise.resolve();.

packages/geoview-core/src/geo/map/map-viewer.ts line 829 at r7 (raw file):

   */
  simulateMapClick(clickCoordinates: MapSingleClickEvent): void {
    // Update store... this will emit the event

Is the comment "this will emit the event" still valid here? I don't think it raises an event unless it's working on WCAG crosshair mode?

jolevesq

@jolevesq reviewed 2 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Alex-NRCan).

packages/geoview-core/src/geo/map/map-viewer.ts line 713 at r7 (raw file):

Previously, Alex-NRCan (Alex) wrote…

Maybe worth trying to set zoom to a zoom level that's already what the map is at.. I'm curious if the 'rendercomplete' event is raised - I'm assuming it might not. So we should maybe check if the current map zoom is the same as the zoom in parameter and if so, just return Promise.resolve();.

You are right, if same zoomlevel, the rendercomplete is never triggered.. good catch!

packages/geoview-core/src/geo/map/map-viewer.ts line 829 at r7 (raw file):

Previously, Alex-NRCan (Alex) wrote…

Is the comment "this will emit the event" still valid here? I don't think it raises an event unless it's working on WCAG crosshair mode?

It will, not in the processor but the line below this one

Alex-NRCan

@Alex-NRCan reviewed 1 file and all commit messages, and resolved 2 discussions.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @jolevesq).

Alex-NRCan

@Alex-NRCan reviewed 1 file and all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @jolevesq).

jolevesq

@jolevesq reviewed 4 files and all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @jolevesq).

Alex-NRCan reviewed Dec 12, 2025

View reviewed changes

github-advanced-security bot found potential problems Dec 15, 2025

View reviewed changes

jolevesq force-pushed the newtest branch from ee93cb5 to bf371c9 Compare December 15, 2025 21:43

jolevesq commented Dec 17, 2025

View reviewed changes

Alex-NRCan reviewed Dec 18, 2025

View reviewed changes

jolevesq commented Dec 18, 2025

View reviewed changes

Alex-NRCan reviewed Dec 18, 2025

View reviewed changes

jolevesq commented Dec 18, 2025

View reviewed changes

Alex-NRCan reviewed Dec 18, 2025

View reviewed changes

jolevesq commented Dec 19, 2025

View reviewed changes

jolevesq marked this pull request as ready for review December 19, 2025 13:26

Canadian-Geospatial-Platform locked and limited conversation to collaborators Dec 19, 2025

Canadian-Geospatial-Platform unlocked this conversation Dec 19, 2025

jolevesq force-pushed the newtest branch from d8bdfb9 to 1ec3d0c Compare December 19, 2025 13:29

Canadian-Geospatial-Platform locked and limited conversation to collaborators Dec 19, 2025

Canadian-Geospatial-Platform unlocked this conversation Dec 19, 2025

Alex-NRCan approved these changes Dec 19, 2025

View reviewed changes

Johann Levesque and others added 10 commits December 19, 2025 13:37

core(test): Add new test for mamp init config

05da109

Add new test

36dd525

add new tests

862230e

fix bug with tab index key

e1666e5

add new tests

9a8f818

fix test

17c3b36

fix comment

024cb38

fix comment

f12969d

fix map click

79fd8f0

fix promise

ec2031b

jolevesq added 4 commits December 19, 2025 13:37

Fix comment

f4d63e9

fix navbar orientation

418e8c8

fix schema

0e9f097

fix minZooom test

4d80523

jolevesq force-pushed the newtest branch from 81b5fdc to 4d80523 Compare December 19, 2025 18:37

jolevesq commented Dec 19, 2025

View reviewed changes

Canadian-Geospatial-Platform locked and limited conversation to collaborators Dec 19, 2025

jolevesq merged commit 207ecf6 into Canadian-Geospatial-Platform:develop Dec 19, 2025
4 checks passed

core(test): Add new test for map init config #3205

core(test): Add new test for map init config #3205

Uh oh!

Conversation

jolevesq commented Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

How Has This Been Tested?

Checklist:

Uh oh!

Alex-NRCan left a comment

Choose a reason for hiding this comment

Uh oh!

Check warning

Uh oh!

Uh oh!

jolevesq left a comment

Choose a reason for hiding this comment

Uh oh!

jolevesq left a comment

Choose a reason for hiding this comment

Uh oh!

Alex-NRCan left a comment

Choose a reason for hiding this comment

Uh oh!

jolevesq left a comment

Choose a reason for hiding this comment

Uh oh!

Alex-NRCan left a comment

Choose a reason for hiding this comment

Uh oh!

jolevesq left a comment

Choose a reason for hiding this comment

Uh oh!

Alex-NRCan left a comment

Choose a reason for hiding this comment

Uh oh!

Alex-NRCan left a comment

Choose a reason for hiding this comment

Uh oh!

jolevesq left a comment

Choose a reason for hiding this comment

Uh oh!

Alex-NRCan left a comment

Choose a reason for hiding this comment

Uh oh!

Alex-NRCan left a comment

Choose a reason for hiding this comment

Uh oh!

jolevesq left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jolevesq commented Dec 12, 2025 •

edited

Loading