Refactor: Replace data-heap-event with standardized data-test for Cypress and Telemetry
#1995
Conversation
rashidakanchwala
requested review from
ravi-kumar-pilla and
jitu5
as code owners
rashidakanchwala
changed the title
rashidakanchwala
changed the title
data-test attribute to be used for Cypress and Telemetry
rashidakanchwala
changed the title
data-test attribute to be used for Cypress and Telemetrydata-heap-event with standardized data-test for Cypress and Telemetry
| @@ -3,7 +3,7 @@ | |||
| describe('Experiment Tracking Primary Toolbar', () => { | |||
| it('verifies that users can hide/show the side menu. #TC-38', () => { | |||
| // Alias | |||
| cy.get('[data-test="btnToggleMenu"]').as('btnToggleMenu'); | |||
| cy.get('[data-test*="sidebar-experiments-visible-btn-"]').as('btnToggleMenu'); | |||
There was a problem hiding this comment.
Hey is there a special meaning for *=?
There was a problem hiding this comment.
Good question -- I have added 'state' to the data-test attribute to help with Heap Analytics by indicating whether the sidebar is collapsed or expanded. This information isn't required for Cypress testing. When I use data-test*, it allows me to get the element that partially matches the data-test name.
So the original data-test attribute = sidebar-experiments-visible-btn-true
There was a problem hiding this comment.
@jitu5 -- see above.
Maybe I need to document this somewhere.
| @@ -41,7 +41,7 @@ describe('Experiment Tracking', () => { | |||
|
|
|||
| // Alias | |||
| cy.get('.details-metadata__notes').as('metadataNotes'); | |||
There was a problem hiding this comment.
i wonder how we decide which element to target data-test, which one to target classname ? Cause it seems like we currently mix them both. Maybe not for this ticket but a bigger refactor ticket where we think about what we really want to track etc?
There was a problem hiding this comment.
So, data-test is used pervasively to test e2e clicks on Cypress. I am thinking of using the same for Heap. Once we release this PR and start getting better heap data -- we will be able to observe better. For now, it does track all the things that are clickable (or maybe other interactions) by users
|
Hi @rashidakanchwala , the refactor itself looks great. Since I am not sure on how to validate the telemetry info, i.e., ( whether the expected events are pushing data to the dashboard ), it would be great to have a doc for telemetry (in future) which describes -
Thank you |
There was a problem hiding this comment.
Happy to see a standard for cypress component identification. Great work @rashidakanchwala !!
Since e2e tests are passing and I am not sure on testing the heap events, the refactor looks good to me.
There was a problem hiding this comment.
thank you @rashidakanchwala, all the names changes look good to me, thank you for changing them
also love the idea from @ravi-kumar-pilla to have somewhere to explain about how we're collecting and previewing data
There was a problem hiding this comment.
@rashidakanchwala. This is really cool. Thanks for the cleanup.
Description
This is a major refactor PR that removes all
data-heap-eventattributes and replaces them withdata-testattributes.I also noticed that our
data-testattribute naming was not following any convention. Therefore, in this PR, I have introduced the following naming convention:< parent component >-< component >-<ui element (button, input, dropdown)>-< state : optional (true/false) >
Development notes
I noticed that sometimes
data-testattributes were defined at the UI component level. This caused multiple components using that UI component to have the samedata-testattribute. I have changed this so thatdata-testattributes are now always defined at the component level. In cases where the component is shared, such as theprimary toolbar, thedata-testattribute is defined at the highest component level.The state isn't required for Cypress Testing but it is for Heap Analytics. Hence I use 'data-test * =' in Cypress testing so it allows me to match the attribute partially.
QA notes
Checklist
RELEASE.mdfile