chore(web-forms#820): clean up common package#1673
Conversation
|
|
Converted to draft because this should not be merged until after the 2026.2 release. |
latin-panda
left a comment
latin-panda
left a comment
There was a problem hiding this comment.
Wow, so many files were deleted! Just a couple of suggestions below.
There was a problem hiding this comment.
I haven't checked the other forms, but this one has additional attachments we'll likely need for testing.
Can you please add them to the /attachments folder? They use different file extensions intentionally.
There was a problem hiding this comment.
While the form does reference these files there is no visual or functional test that checks that they load so I left them out intentionally. I'd rather not add files which aren't used or useful.
... we'll likely need for testing.
Do you mean they'll be needed in the future? Are you thinking manual or automatic testing?
There was a problem hiding this comment.
I mean automated tests. The display of all select appearances is quite complex and took a lot of effort to get right, and we should have e2e tests; we just haven't added them yet. 😣
There was a problem hiding this comment.
This would be a screenshot/visual e2e test I assume? I don't know how you would test this functionally...
There was a problem hiding this comment.
Ok.. I've added screenshot tests for 4 of the image select types and added them back as attachments. I extracted the screenshot code from the maps control so it can be reused, so it's worth reviewing.
There was a problem hiding this comment.
Is this only used for e2e tests? Maybe it's better to move it inside /e2e for now.
garethbowen
force-pushed
the
rename-reamde
branch
from
d167943 to
9863a45
Compare
latin-panda
left a comment
latin-panda
left a comment
There was a problem hiding this comment.
Thank you for adding the new test coverage for select labels! ✨
2 participants
Closes getodk/web-forms#820
What has been done to verify that this works as intended?
CI
Local dev
Why is this the best possible solution? Were any other approaches considered?
The demo app has been moved to a new repo: https://github.com/getodk/web-forms-demo
These files are being moved into a different repo in this PR: getodk/sample-forms#7
This means these files are no longer needed here except for where they are used in tests. Where they were used by xforms-engine integration tests I have moved them into the xforms-engine package. Where they were used by e2e tests I've moved them into the web-forms package.
I have created the bare minimum app for testing purposes, and removed utility files that were doing things like scanning for forms.
This is much simpler and easier to maintain and therefore more appropriate for the now much smaller number of files being managed.
How does this change impact users? Describe intentional behavior changes from code updates. What are the regression risks?
It shouldn't.
Does this change require updates to user documentation? If so, please file an issue here and include the link below.
No.