Add data-ms-id test automation attributes - #12857
MarkBremer wants to merge 1 commit into
Conversation
…om 2026-03-11_cy-data)
|
In order to contribute to the MapStore project, the CLA (Contributor License agreement) should be sent signed to GeoSolutions. Please consult contributing rules at: https://github.com/geosolutions-it/MapStore2/wiki/Contributing-to-MapStore#contributing-code |
allyoucanmap
left a comment
There was a problem hiding this comment.
Hi @MarkBremer, thanks for your contribution. In general we expect to receive first a PR on the master branch then after approval and merge other backport PRs may be open to include same fixes on stable branches. I worked on a separate branch https://github.com/allyoucanmap/MapStore2/tree/5.0.x-data-ms-id starting from your commit to provide feedback and explain needed changes for this PR and related ones.
As last commit I included documentation to summarize how the attribute should be applied, see allyoucanmap@910b24b (in particulate docs/developer-guide/test-automation-identifiers.md).
One final aspect not included in the changes that needs to be discussed is the naming convention on the identifier, that currently does not seem consistent, below a table proposal for possible changes:
| current | proposed | kind |
|---|---|---|
Layer download (components/data/download/) |
||
dataset-export-box |
layer-download-dialog |
vocabulary |
dataset-export-box-close |
layer-download-dialog-close |
vocabulary |
dataset-export-box-export |
layer-download-submit |
vocabulary |
dataset-export-select-format |
layer-download-format |
vocabulary |
dataset-export-select-reference-system |
layer-download-srs |
vocabulary |
dataset-export-toggle-advanced |
layer-download-advanced-toggle |
vocabulary |
Resource details (ResourcesCatalog/) |
||
dataset-view-sidepanel-close |
resource-details-close |
vocabulary |
dataset-view-sidepanel-title |
resource-details-title |
vocabulary |
dataset-view-sidepanel-tab-{tab.id} |
resource-details-tab-{tab.id} |
vocabulary |
dataset-view-sidepanel-tab-content |
resource-details-tab-{tab.id}-content |
defect |
details-info-fields |
resource-details-fields |
vocabulary |
Resource cards (ResourcesCatalog/) |
||
dataset-card-{idx} |
resource-card-{resource.id} |
defect |
{prefix}-title |
resource-card-{id}-title |
vocabulary |
{prefix}-link |
resource-card-{id}-link |
vocabulary |
{prefix}-properties |
resource-card-{id}-details |
vocabulary |
{prefix}-btn-favorite |
resource-card-{id}-favorite |
vocabulary |
{prefix}-actions |
resource-card-{id}-actions |
vocabulary |
{prefix}-actions-delete |
resource-card-{id}-delete |
vocabulary |
{prefix}-actions-clone (GeoNode injector) |
resource-card-{id}-clone |
vocabulary |
Service catalog (components/catalog/, MetadataExplorer) |
||
catalog-search |
keep | keep |
catalog-search-button |
keep | keep |
catalog-panel |
keep | keep |
catalog-add-service |
keep | keep |
catalog-save |
keep | keep |
catalog-cancel |
keep | keep |
catalog-delete |
keep | keep |
catalog-service-url |
keep | keep |
catalog-service-type |
keep | keep |
catalog-service-title |
keep | keep |
catalog-add-first-result |
catalog-add-result |
vocabulary |
Resources search (ResourcesSearch) |
||
catalogue-search |
resources-search |
vocabulary |
| TOC | ||
toc-add-layer |
keep | keep |
edit-style-btn-{tab.glyph} |
toc-settings-tab-{tab.id} |
defect |
btn-print |
print-submit |
vocabulary |
print-form-{label} |
print-{property} |
defect |
| Cookie banner | ||
cookie-overlay |
keep | keep |
cookie-banner-maintext |
keep | keep |
cookie-accept-button |
keep | keep |
cookie-privacy-link |
keep | keep |
cookie-privacy-text |
keep | keep |
| Sidebar | ||
sidebar-btn-{tool.name} |
keep | keep |
| Widgets | ||
add-widget-text-title |
widget-text-title |
vocabulary |
add-widget-text-description |
widget-text-description |
vocabulary |
| Other | ||
measure-container |
keep | keep |
drawer-menu-button |
drawer-menu-toggle |
vocabulary |
| useEffect(() => { | ||
| if (!enabled) { | ||
| return; | ||
| } | ||
| const dialogRoot = document.querySelector('#mapstore-export'); | ||
| dialogRoot?.setAttribute('data-ms-id', 'dataset-export-box'); | ||
|
|
||
| const formatInput = document.querySelector('#mapstore-export .mapstore-downloadoptions:not(.downloadMode) .Select-input input'); | ||
| formatInput?.setAttribute('data-ms-id', 'dataset-export-select-format'); | ||
| }, [enabled, downloadOptions, service, showLoader]); | ||
|
|
There was a problem hiding this comment.
We should avoid DOM manipulation to apply an attribute to a react component, in this commit allyoucanmap@7586026 you can find the fix to avoid usage of useEffect
| useEffect(() => { | ||
| if (!advancedOptionsOpened) { | ||
| return; | ||
| } | ||
| const srsInput = document.querySelector('.mapstore-downloadwpsoptions-advanced .mapstore-downloadwpsoptions-advanced-menuitem .Select-input input'); | ||
| srsInput?.setAttribute('data-ms-id', 'dataset-export-select-reference-system'); | ||
| }, [advancedOptionsOpened, selectedSrs, srsList]); |
There was a problem hiding this comment.
same as previous comment, avoid useEffect to manipulate DOM, see allyoucanmap@7586026
| quality: 0.8 | ||
| }).then((linkBase64) => { | ||
| resolve({ data: { link: linkBase64 } }); | ||
| <div data-ms-id="add-widget-text-description"> |
There was a problem hiding this comment.
The CompactRichTextEditor is a generic component and we should avoid to add an id specific for one of the usage, it would be better to add the id to the caller instead see allyoucanmap@9f8470d
| const tabCyDataMap = { | ||
| info: 'dataset-view-sidepanel-tab-info', | ||
| location: 'dataset-view-sidepanel-tab-location', | ||
| locations: 'dataset-view-sidepanel-tab-location', | ||
| assets: 'dataset-view-sidepanel-tab-assets', | ||
| data: 'dataset-view-sidepanel-tab-data', | ||
| related: 'dataset-view-sidepanel-tab-data', | ||
| share: 'dataset-view-sidepanel-tab-share', | ||
| settings: 'dataset-view-sidepanel-tab-settings' | ||
| }; | ||
|
|
||
| const getTabCyData = (tab = {}) => { | ||
| const normalizedId = `${tab?.id || ''}`.toLowerCase(); | ||
| if (tabCyDataMap[normalizedId]) { | ||
| return tabCyDataMap[normalizedId]; | ||
| } | ||
| const normalizedLabel = `${tab?.label || ''}`.toLowerCase(); | ||
| return tabCyDataMap[normalizedLabel] || null; | ||
| }; | ||
|
|
There was a problem hiding this comment.
This change is hardcoding values only available in geonode, we should not also use the label for the Id, see changes here: allyoucanmap@60c1c10
| const resolvedDataMsId = dataMsId | ||
| || (cardMsIdPrefix && ['heart', 'heart-o'].includes(glyph) ? `${cardMsIdPrefix}-btn-favorite` : null) | ||
| || (cardMsIdPrefix && glyph === 'details' ? `${cardMsIdPrefix}-properties` : null); | ||
|
|
There was a problem hiding this comment.
We should avoid to rely on glyphs to generate the id, also the data-ms-id should be applied to the injected component not the container, see allyoucanmap@880ef66
| toggleAttributes, | ||
| menuItemAttributes, | ||
| dataMsId |
There was a problem hiding this comment.
We don't need toggleAttributes and menuItemAttributes, dataMsId is enough in this case, see allyoucanmap@61685b7
| ...tool, | ||
| cfg: { | ||
| ...(tool.cfg || {}), | ||
| 'data-ms-id': (tool.cfg && tool.cfg['data-ms-id']) || (tool.name ? `sidebar-btn-${tool.name}` : `tooltip-btn-${index}`) |
There was a problem hiding this comment.
We should avoid to spread the 'data-ms-id' inside the cfg, it should be enough to pass the dataMsId to the web/client/components/sidebarmenu/SidebarElement.jsx, see allyoucanmap@fc45ac8.
This particular fix needs to be double checked to verify if all injected button are using correctly the SidebarElement
| if (item.type === 'icon') { | ||
| return ( | ||
| <Text key={idx} fontSize="sm" className={item.variant ? `ms-${item.variant}-text` : ''} > | ||
| <Text key={idx} fontSize="sm" className={`${item.variant ? `ms-${item.variant}-text` : ''} ${item.className || ''}`} {...(item['data-ms-id'] ? {'data-ms-id': item['data-ms-id']} : {})}> |
There was a problem hiding this comment.
The className here seems unrelated change, it's better to remove it, see allyoucanmap@dbefc00
| <Button | ||
| bsStyle="primary" | ||
| className="download-button" | ||
| {...{ 'data-ms-id': 'dataset-export-box-export' }} |
There was a problem hiding this comment.
On multiple instances there is the usage of spread operator {...{ 'data-ms-id': '<id>' }}, it would be better us use directly the prop data-ms-id="<id>", see allyoucanmap@67f22dc
| const label = field.labelId ? <Message msgId={field.labelId} /> : field.label; | ||
| return isStyleLabel(field.style) && field.href | ||
| ? (<a href={field.href} target={field.target}>{label}</a>) | ||
| ? (<a href={field.href} target={field.target} {...(field['data-ms-id'] ? {'data-ms-id': field['data-ms-id']} : {})}>{label}</a>) |
There was a problem hiding this comment.
I would prefer to make consistent usage of data-ms-id and dataMsId, the rule should be data-ms-id for direct prop on react component and dataMsId for other cases, configuration and passed props, see allyoucanmap@2688b28
Description
Selected UI elements now have a data-ms-id attribute to provide stable identifiers for automated tests and make test automation more reliable.
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (check one with "x", remove the others)
Issue
What is the current behavior?
#12203
What is the new behavior?
Selected UI elements are now annotated with a data-ms-id attribute, providing stable identifiers that can be used by automated tests.
Breaking change
Does this PR introduce a breaking change? (check one with "x", remove the other)
Other useful information