chore: automated tests for select one for map #509

latin-panda · 2025-10-03T20:06:13Z

Related to #305

I have verified this PR works in these browsers (latest versions):

What else has been done to verify that this works as intended?

Running CI multiple times

Why is this the best possible solution? Were any other approaches considered?

The map feature is highly visual, and testing it at the unit level would require extensive mocking of OpenLayers functions. It would significantly reduce the value of the tests since the real rendering logic would be bypassed.

Using E2E tests allows us to verify the actual behavior of the map, including OpenLayers’ rendering and our integration code, without relying on mocks. This provides the most accurate validation of how the map appears and behaves in the real application.

We use snapshots because many interactions can’t be queried with selectors, as the map and its features are rendered on a canvas. This approach allows us to verify the display of features (points, lines, and shapes) and their states (unselected, selected, saved), as well as zooming and panning behavior.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

NA

Do we need any specific form for testing your changes? If so, please attach one.

NA

What's changed

Adds e2e tests for select one from maps, verifying the main functionality of the map features and handling geolocation access denial.
Enhances the All Questions form's map attachment by incorporating point, line, and shape features in a simplified city layout, making visual test assessments easier.
Uses snapshots to verify map states, automatically generated by Playwright (docs).
Uses the mouse event instead of click to select features and pan the map, because click doesn't work for this element.
Uploads Playwright results to facilitate debugging
Extract code that converts from BaseItem to GeoJson and add a unit test. The logic remains the same; just moved the code to a new file.

…t-one-for-map

into ui-select-one-for-map

…lect-one-for-map

…culation

latin-panda · 2025-10-08T14:39:23Z

🎉🎉🎉🎉

latin-panda · 2025-10-08T18:38:54Z

packages/web-forms/src/components/common/map/createFeatureCollectionAndProps.ts

@@ -0,0 +1,110 @@
+import type { SelectItem } from '@getodk/xforms-engine';


The logic remains the same; I've just moved it to a new file.

Nice refactor!

latin-panda · 2025-10-08T19:18:54Z

@garethbowen What do you think about these tests? We could also consider adding visual tests for the markdown feature.

The biggest challenge was getting them to pass in CI. Browsers behave differently in the CI, so I had to disable headless mode and add a few tweaks (you’ll see them in the diff). Because of the map’s long animations (up to 1 sec each time), it also needs to wait for the rendering to finish, which makes the tests slower.

Each browser shows tiny rendering differences (e.g., font weight and map centering, etc). To focus only on meaningful UI regressions, I've added separate snapshots for each browser (the community recommendation is to have snapshots per browser and per OS, which I didn't add the latter). We can look into aligning browsers later, but it's a low priority at the moment. There's a threshold of 5000 pixels for errors, which is enough to help us notice when a feature is displaying the selected design instead of the saved design, for example.

I consider these valuable tests to have, even if they are kinda slow and just validate a small set of cases (the QA currently can take more than a week to fully verify the map feature in Collect). We could consider running these tests in a single browser instead of three, or moving them to a separate suite that runs on demand.

garethbowen · 2025-10-08T20:19:49Z

Because of the map’s long animations (up to 1 sec each time), it also needs to wait for the rendering to finish, which makes the tests slower.

I think you can disable animations which would speed this up: https://playwright.dev/docs/api/class-pageassertions#page-assertions-to-have-screenshot-1 . Alternatively we could use prefers-reduced-motion which would also be an accessibility win. I think it's ok because you're not actually testing the animation (just waiting for it to finish) and will speed up test time a lot!

I consider these valuable tests to have

100% agree. Let's keep the suite small so it completes reasonably quickly. I see you're already using the "all question types" form, so we could screenshot more parts of that one form and catch most regressions with a single test.

consider running these tests in a single browser instead of three

I think that's sensible if we get frustrated by the suite. Ideally we'd have one on iOS with Safari, one on Android with Chrome, and one on linux with Firefox!

moving them to a separate suite that runs on demand

I don't like on demand because I keep forgetting to run them! 😂 If it becomes an issue, we could choose to run the suite only when a PR is created.

garethbowen · 2025-10-08T20:33:24Z

packages/web-forms/playwright.config.ts

-		/* Only on CI systems run the tests headless */
-		headless: !!process.env.CI,
+		// Turning off headless for visual tests (to compare snapshots) in CI
+		headless: false,


What impact might this have on the non-screenshot tests? I assume it'll slow them down a bit too? Maybe we should split this off as a separate suite?

Yes, it's slower because it requires more resources. Also UI animations and transitions will take longer.

I agree to splitting the suite

garethbowen · 2025-10-08T20:45:39Z

packages/web-forms/e2e/page-objects/controls/MapControl.ts

+		}
+
+		await expect(map).toHaveScreenshot(snapshotName, {
+			maxDiffPixels: 5000,


This number sounds huge until you realise the image is huge. I'm still surprised that the variation can be this big. Did you have a look at the diffs to figure out what is changing?

I think it'd make me feel better if it was a ratio, eg

Suggested change

maxDiffPixels: 5000,

maxDiffPixelRatio: 0.005,

This is roughly equivalent but if we got a smaller image (eg: mobile resolution tests) then the error threshold would scale down too.

Yes, I compared the diff, and the large difference mainly comes from the map. For example, if the zoom level ends up being off by just a couple of pixels, the map lines and tiles shift slightly, which makes the screenshot comparison detect a big pixel difference, even though the UI looks the same to humans 😅

That’s why I moved the test points from Europe to South Australia, that area has fewer visual map details (just a few roads and a small town), so there’s less variation between runs.

For other question types (not map-related), I’d expect the pixel difference to be much smaller, around 100 pixels or so.

I also tried adjusting maxDiffPixelRatio with small values like 0.0XX, but it only seemed to work with values like 0.1, 0.2, etc.

garethbowen

Great work! Really impressive.

It looks like the webkit scenario build is about 30 seconds (or 16%) slower which is ok IMO.

At some point I think we should split this out as a separate suite and build out a screenshot smoke test that touches everything at a high level, but this isn't blocking.

latin-panda · 2025-10-09T11:09:25Z

I think you can disable animations which would speed this up: https://playwright.dev/docs/api/class-pageassertions#page-assertions-to-have-screenshot-1 . Alternatively we could use prefers-reduced-motion

I tried the first approach before, but it doesn’t disable OpenLayers animations, I believe their animations is calculated with JS. I didn’t know about prefers-reduced-motion, I’ll give that a try when adding the Geo Shape e2e tests.

At some point I think we should split this out as a separate suite and build out a screenshot smoke test that touches everything at a high level, but this isn't blocking.

100%! As you mentioned, a smoke test on the all question types form, for mobile and desktop.

Alright! I will add more visual tests when working on the Geo Shape and Geo Trace features, on that work

I'll try prefers-reduced-motion
Split the suite
Add mobile e2e tests for maps

For now, I'll merge this PR. Thanks for the feedback! 🤩

…elect-one-for-map # Conflicts: # packages/web-forms/src/components/common/map/AsyncMap.vue # packages/web-forms/src/components/common/map/MapBlock.vue

latin-panda added 30 commits August 21, 2025 00:31

Merge branch 'main' of https://github.com/getodk/web-forms into selec…

517792e

…t-one-for-map

Merge branch 'main' of https://github.com/getodk/web-forms into selec…

ef0cb3a

…t-one-for-map

engine support for metadata in itemset nodes

0cd76eb

Merge branch 'main' of https://github.com/getodk/web-forms into selec…

68da647

…t-one-for-map

Async map bundle loading

16f3cde

typing

66e9764

Map Component

a7dea73

Adds new icons

4823ebf

removes cache buste and timeout

1e9feec

Map UI features: properties popup and status bar

b009dc7

Merge branch 'main' of https://github.com/getodk/web-forms into selec…

93cc3a8

…t-one-for-map

Merge branch 'select-one-for-map' of https://github.com/getodk/web-forms

525d890

into ui-select-one-for-map

fixes styles and icons based on feedback

01a4fa5

Merge branch 'main' of https://github.com/getodk/web-forms into ui-se…

78872b2

…lect-one-for-map

Merge branch 'main' of https://github.com/getodk/web-forms into ui-se…

dcf7902

…lect-one-for-map

Reverts changes in engine. We decided another solution

313e751

Applies latest designer feedback

6c0ab6f

fixes lint, handles esc key

4b0a7e4

Makes lines less thick

2467720

stacking errors

294ddd5

Adds map background for full screen

a4d3056

Adds support for properties in the engine

087645f

Fixes scenario tests

34ebdb4

Integration engine and client

62562bf

Refactored component input types and fixed view center when no features

7ff1896

Simplified map ref and source get

f1494a6

Flat and prefixed properties to avoid OL conflicts, fixes polygon cal…

c7657c2

…culation

fixes click binding for disable question

07d288d

fixes lint

40b9ea4

Feedback for async map and properties components

d944129

latin-panda added 7 commits October 8, 2025 19:01

take-8: browser rendering settings

e173313

take-8: styling fix

9cfe196

take-8: revert style changes

32f01bd

take-8: revert style changes

29380fe

take-8: fullscreen adjustment

93e87cb

add back permissions

89e42b1

add back permissions

920d995

latin-panda added 7 commits October 8, 2025 21:50

Testing minimum args needed - Take 1

6b30bf5

Cleaning

b9ed806

more cleaning

0cf1f23

test: Can we do less css changes?

4d838d5

test-2: Can we do less css changes?

1466ee6

test-3: Can we do less css changes?

3fc2c6b

test-4: Can we do less css changes?

5149154

latin-panda marked this pull request as ready for review October 8, 2025 17:51

unit test for conversion of ODK features to GeoJSON

719525d

latin-panda commented Oct 8, 2025

View reviewed changes

fixes format

0629351

latin-panda requested a review from garethbowen October 8, 2025 19:19

garethbowen reviewed Oct 8, 2025

View reviewed changes

garethbowen approved these changes Oct 8, 2025

View reviewed changes

Base automatically changed from ui-select-one-for-map to main October 13, 2025 11:48

Merge branch 'main' of https://github.com/getodk/web-forms into e2e-s…

ce3045b

…elect-one-for-map # Conflicts: # packages/web-forms/src/components/common/map/AsyncMap.vue # packages/web-forms/src/components/common/map/MapBlock.vue

latin-panda merged commit 5b5971e into main Oct 13, 2025
47 checks passed

latin-panda deleted the e2e-select-one-for-map branch October 13, 2025 12:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

chore: automated tests for select one for map #509

chore: automated tests for select one for map #509

Uh oh!

latin-panda commented Oct 3, 2025 •

edited

Loading

Uh oh!

latin-panda commented Oct 8, 2025

Uh oh!

latin-panda Oct 8, 2025

Uh oh!

garethbowen Oct 8, 2025

Uh oh!

latin-panda commented Oct 8, 2025

Uh oh!

garethbowen commented Oct 8, 2025 •

edited

Loading

Uh oh!

garethbowen Oct 8, 2025

Uh oh!

latin-panda Oct 9, 2025 •

edited

Loading

Uh oh!

garethbowen Oct 8, 2025

Uh oh!

latin-panda Oct 9, 2025

Uh oh!

garethbowen left a comment

Uh oh!

latin-panda commented Oct 9, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		@@ -0,0 +1,110 @@
		import type { SelectItem } from '@getodk/xforms-engine';

chore: automated tests for select one for map #509

chore: automated tests for select one for map #509

Uh oh!

Conversation

latin-panda commented Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

I have verified this PR works in these browsers (latest versions):

What else has been done to verify that this works as intended?

Why is this the best possible solution? Were any other approaches considered?

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Do we need any specific form for testing your changes? If so, please attach one.

What's changed

Uh oh!

latin-panda commented Oct 8, 2025

Uh oh!

latin-panda Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

garethbowen Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

latin-panda commented Oct 8, 2025

Uh oh!

garethbowen commented Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

garethbowen Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

latin-panda Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garethbowen Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

latin-panda Oct 9, 2025

Choose a reason for hiding this comment

Uh oh!

garethbowen left a comment

Choose a reason for hiding this comment

Uh oh!

latin-panda commented Oct 9, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

latin-panda commented Oct 3, 2025 •

edited

Loading

garethbowen commented Oct 8, 2025 •

edited

Loading

latin-panda Oct 9, 2025 •

edited

Loading