-
Notifications
You must be signed in to change notification settings - Fork 11
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
base: develop
Are you sure you want to change the base?
PB-1263: add handleLegacyParam function tests #1161
Conversation
web-mapviewer Run #4155
Run Properties:
|
Project |
web-mapviewer
|
Branch Review |
feat-pb-1263-add-handlelegacyparam-function-tests
|
Run status |
Passed #4155
|
Run duration | 05m 34s |
Commit |
c607a7a6d9: PB-1263: add handleLegacyParam function tests
|
Committer | Felix Sommer |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
0
|
Pending |
21
|
Skipped |
0
|
Passing |
218
|
View all changes introduced in this branch ↗︎ |
02a0ea4
to
305f1f6
Compare
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.
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
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.
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
591755d
to
154de0a
Compare
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.
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 |
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.
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.
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.
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
src/router/__tests__/legacyPermalinkManagement.routerPlugin.spec.js
Outdated
Show resolved
Hide resolved
src/router/__tests__/legacyPermalinkManagement.routerPlugin.spec.js
Outdated
Show resolved
Hide resolved
src/router/__tests__/legacyPermalinkManagement.routerPlugin.spec.js
Outdated
Show resolved
Hide resolved
src/router/__tests__/legacyPermalinkManagement.routerPlugin.spec.js
Outdated
Show resolved
Hide resolved
it('N', () => { | ||
const param = 'N' | ||
const legacyCoordinates = [0, 0] | ||
handleLegacyParams({ param, legacyCoordinates }) | ||
|
||
expect(legacyCoordinates).to.deep.equal([0, legacyValue]) |
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.
this can also work if the array is still empty, and we should make sure it works in both case.
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.
so if legacyCoordinates = []
then the result is [ undefined, 10 ]
is this valid ?
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 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.
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 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] ?
const layerVisibility = 'layers_visibility' | ||
const layerOpacity = 'layers_opacity' | ||
const layerTimestamp = 'layers_timestamp' |
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.
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.
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 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 ?
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.
Well, handleLegacyParameter uses transformLayerIntoUrlString anyway, but we should also test the transformLayerIntoUrlString if it's not already tested.
154de0a
to
ad67651
Compare
ad67651
to
c607a7a
Compare
Test link