Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PB-1263: add handleLegacyParam function tests #1161

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

sommerfe
Copy link
Contributor

@sommerfe sommerfe commented Dec 5, 2024

@sommerfe sommerfe self-assigned this Dec 5, 2024
Copy link

cypress bot commented Dec 5, 2024

web-mapviewer    Run #4155

Run Properties:  status check passed Passed #4155  •  git commit c607a7a6d9: PB-1263: add handleLegacyParam function tests
Project web-mapviewer
Branch Review feat-pb-1263-add-handlelegacyparam-function-tests
Run status status check passed Passed #4155
Run duration 05m 34s
Commit git commit c607a7a6d9: PB-1263: add handleLegacyParam function tests
Committer Felix Sommer
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 21
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 218
View all changes introduced in this branch ↗︎

@sommerfe sommerfe force-pushed the feat-pb-1263-add-handlelegacyparam-function-tests branch 3 times, most recently from 02a0ea4 to 305f1f6 Compare December 9, 2024 09:57
@sommerfe sommerfe requested review from pakb and ltkum December 9, 2024 10:08
@sommerfe sommerfe marked this pull request as ready for review December 9, 2024 10:08
Copy link
Contributor

@pakb pakb left a comment

Choose a reason for hiding this comment

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

for if tests are covering sufficient ground, I'll let @ltkum gives his opinion on the matter as he was the one writing most of it

Copy link
Contributor

Choose a reason for hiding this comment

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

const params = new Map()
            const param = 'zoom'
            let legacyValue = 10
            let newQuery = {}
            let latlongCoordinates = []
            let legacyCoordinates = []
            let cameraPosition = [null, null, null, null, null, null]

            handleLegacyParam(
                params,
                param,
                legacyValue,
                fakeStore,
                newQuery,
                latlongCoordinates,
                legacyCoordinates,
                cameraPosition
            )

This portion of code is repeated as many times as there are tests with very small variations, can you make a function out of it, taking an object with all params as input? And have all the default value set in the function and only alter what you need for the test

@sommerfe sommerfe force-pushed the feat-pb-1263-add-handlelegacyparam-function-tests branch 7 times, most recently from 591755d to 154de0a Compare December 18, 2024 14:46
Copy link
Contributor

@ltkum ltkum left a comment

Choose a reason for hiding this comment

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

It's a good foundation, but I think we can simplify this a bit, and also extend the coverage at the same time

@@ -56,7 +56,7 @@ const handleLegacyParam = (
if (!(projection instanceof SwissCoordinateSystem)) {
newValue = LV95.transformCustomZoomLevelToStandard(legacyValue)
if (projection instanceof CustomCoordinateSystem) {
newValue = projection.transformStandardZoomLevelToCustom(newValue)
newValue = projection.transformStandardZoomLevelToCustom(newValue) // FIXME: the function transformStandardZoomLevelToCustom is not implemented in CustomCoordinateSystem
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently in the mapviewer, we have the following coordinates systems :
LV95 & LV03 : swissCoordinatesSystems which inherits CustomCoordinateSystem, and in which the function is implemented
WGS84 & webmercator which inherits StandardCoordinateSystem
We should not call for the function outside of a specific projection. I would remove this FIXME.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do i understand this right, that this function will never be called ? so then we should remove either the whole if statement or replace this function call directly with a throw Error call

Comment on lines 131 to 104
it('N', () => {
const param = 'N'
const legacyCoordinates = [0, 0]
handleLegacyParams({ param, legacyCoordinates })

expect(legacyCoordinates).to.deep.equal([0, legacyValue])
Copy link
Contributor

Choose a reason for hiding this comment

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

this can also work if the array is still empty, and we should make sure it works in both case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so if legacyCoordinates = []
then the result is [ undefined, 10 ]
is this valid ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if it is ['undefined', 10], as when I tried it quickly it gave me in the console something of the form : [, 10], but if you do legacyCoordinates[0], it should give you undefined, yes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried it and it fills the index 0 with undefined, how did you get [, 10] ? is this an empty string then or what is at index 0
so should i add a test to see if the result is [undefined, 10] ?

Comment on lines +220 to +180
const layerVisibility = 'layers_visibility'
const layerOpacity = 'layers_opacity'
const layerTimestamp = 'layers_timestamp'
Copy link
Contributor

Choose a reason for hiding this comment

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

if we want to check it's transformed correctly, we should also make tests which checks that the values are transformed correctly. This means :
we need to set the visibility to 'true'
we need to check that if we set the opacity to a valid value (a value between 0 and 1 as a string)
we need to add a timeConfig to the layerConfig and a timestamp to check that it can be added to the layer

but it is also good to check that if we set invalid values, it work as expected and ignores those values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this has to be done in a unit test of the transformLayerIntoUrlString function and does not belong to this test
do you want me to create also a unit test for the transformLayerIntoUrlString function ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, handleLegacyParameter uses transformLayerIntoUrlString anyway, but we should also test the transformLayerIntoUrlString if it's not already tested.

@sommerfe sommerfe force-pushed the feat-pb-1263-add-handlelegacyparam-function-tests branch from 154de0a to ad67651 Compare December 19, 2024 11:33
@sommerfe sommerfe force-pushed the feat-pb-1263-add-handlelegacyparam-function-tests branch from ad67651 to c607a7a Compare December 20, 2024 08:18
@sommerfe sommerfe requested a review from ltkum December 20, 2024 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants