Skip to content
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

test: [M3-8914] - POC: Centralized Locators for Linode Page in Cypress UI Automation Framework #11594

Conversation

subsingh-akamai
Copy link
Contributor

@subsingh-akamai subsingh-akamai commented Feb 3, 2025

Description 📝

The objective of this proof of concept (PoC) task is to enhance the UI automation framework in Cypress by implementing centralized locators for the Linode page. This will involve:

  • Identifying all the locators used on the Linode page used in test cypress/e2e/core/linodes/smoke-linode-landing-table.spec.ts
  • Creating a centralized repository or file to store these locators.
  • Refactoring existing Cypress test scripts to utilize the centralized locators and also did refactoring in test to use findByTestId() instead of using get().

Note - This is a new PR for #11455.

Below are guidelines which is proposed to use centralized locators

  • We will continue to use string directly in test for readability of test with cypress locator function like findByText(), findByTitle(), findByLabel(), findByTestId().
  • Wherever we use element attributes to identify elements we need to use centralized locators with cypress locator function like get()
  • Path - support/ui/locators
  • Locators should be defined as per below
    • Each page will have locators file which contains locators for that page. To avoid defining all locators object at once it is better divide into specific components of page like table, empty, non-empty page.
    • Locator should be written in a generic way so that it can be used in multiple tests like it should not have test specfic name and should uniquely locate element.
    • common-locators.ts - it contains common locators like side and top menu
    • linode-locators.ts - contains locators for (/linodes) page
  • To ensure consistency, we should refactor existing tests that use cy.get() to either use centralized locators or more readable locator functions like findByText(), findByTitle(), findByLabel(), or findByTestId().

Changes 🔄

As part of POC, I have updated of locators in test cypress/e2e/core/linodes/smoke-linode-landing-table.spec.ts
Below 2 files are created under cypress/support/ui/locators to store locators used in above test

  • common-locators.ts
  • linode-locators.ts

How to test 🧪

yarn cy:run -s cypress/e2e/core/linodes/smoke-linode-landing-table.spec.ts

Verification steps

When test executed using this command yarn cy:run -s cypress/e2e/core/linodes/smoke-linode-landing-table.spec.tsall tests should pass successfully.

image

As an Author, to speed up the review process, I considered 🤔

👀 Doing a self review
❔ Our contribution guidelines
➕ Adding a changeset
🔐 Removing all sensitive information from the code and PR description


  • I have read and considered all applicable items listed above.

@subsingh-akamai subsingh-akamai requested a review from a team as a code owner February 3, 2025 11:32
@subsingh-akamai subsingh-akamai requested review from dmcintyr-akamai and removed request for a team February 3, 2025 11:32
@subsingh-akamai subsingh-akamai requested a review from a team as a code owner February 3, 2025 11:43
@subsingh-akamai subsingh-akamai requested review from coliu-akamai, hasyed-akamai, bnussman-akamai and jdamore-linode and removed request for a team February 3, 2025 11:43
@subsingh-akamai
Copy link
Contributor Author

Hi @bnussman and @jdamore-linode - I have created this new PR for POC of centralized locators and added you as reviewer (PR #11455).

Copy link

github-actions bot commented Feb 3, 2025

Coverage Report:
Base Coverage: 79.24%
Current Coverage: 79.24%

Copy link
Contributor

@coliu-akamai coliu-akamai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you! confirmed tests passed ✅

@coliu-akamai coliu-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Feb 4, 2025
Copy link
Contributor

@jdamore-linode jdamore-linode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Subodh! This is a nice improvement since I last looked at #11455, and I think serves as a good demonstration and starting point to explore this pattern.

I'm still not totally sold on the DX aspect of this change, namely some of the things we've already discussed in the previous PR:

  • I still wonder if discoverability of these locators is an issue and whether development velocity is improved or hindered as a result. My instinct is that locators could hinder test development and test troubleshooting because of the extra overhead of having to manage/find these locators and relate them to test failure messages
  • What problem are we solving with these that isn't solved by the ui helper objects? When should we lean for these rather than UI helpers? My instinct is that if we're using certain selectors so often throughout the tests that we feel the need to do something about it, a UI helper would be a good choice. And if we're using selectors here and there without a lot of repetition, do we really want to bother with factoring those out?

I think this is a good starting point and that we can merge this and see how it impacts DX, but I think our next steps should be to explore writing new tests using locators to see what that's like from a development and review point of view 👍

Comment on lines 3 to 15
// Top menu items
export const topMenuItemsLocator = {
// Create Button
addNewMenuButton: '[data-qa-add-new-menu-button="true"]',
// Notifications Button
notificationsButton: '[aria-label="Notifications"]',
// Search Icon
searchIcon: '[data-qa-search-icon="true"]',
// Search Text Field
searchInput: '[data-qa-main-search]',
// Toogle side menu bar button
toggleSideMenuButton: '[aria-label="open menu"]',
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Top menu items
export const topMenuItemsLocator = {
// Create Button
addNewMenuButton: '[data-qa-add-new-menu-button="true"]',
// Notifications Button
notificationsButton: '[aria-label="Notifications"]',
// Search Icon
searchIcon: '[data-qa-search-icon="true"]',
// Search Text Field
searchInput: '[data-qa-main-search]',
// Toogle side menu bar button
toggleSideMenuButton: '[aria-label="open menu"]',
};
/** Top menu items. */
export const topMenuItemsLocator = {
/** Top menu create button. */
addNewMenuButton: '[data-qa-add-new-menu-button="true"]',
/** Top menu notifications button. */
notificationsButton: '[aria-label="Notifications"]',
/** Top menu search icon. */
searchIcon: '[data-qa-search-icon="true"]',
/** Top menu search field. */
searchInput: '[data-qa-main-search]',
/** Top menu navigation toggle. */
toggleSideMenuButton: '[aria-label="open menu"]',
};

We can get a nice DX enhancement by using doc comments for these objects and properties, which might be good for discoverability:

Before After
Screenshot 2025-02-04 at 3 45 58 PM Screenshot 2025-02-04 at 3 44 43 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you suggessting doc comments, I have added same in this commit 793d6d6

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, UI helpers primarily focus on element interactions, such as buttons.ts and drawer.ts, which is great for implementing generic element interactions.

However, this POC was specifically aimed at centralizing locators to prevent defining them across multiple tests.

Ideally, test-specific DOM locators should be maintained in a centralized location to enhance reusability across tests, ultimately reducing development effort in the long run.

I completely agree that discoverability of these locators remains a challenge due to Cypress error traces.

Copy link
Contributor

@hasyed-akamai hasyed-akamai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @subsingh-akamai !

Code Review ✅
All Test Passed ✅

@linode-gh-bot
Copy link
Collaborator

Cloud Manager UI test results

🔺 2 failing tests on test run #4 ↗︎

❌ Failing✅ Passing↪️ Skipped🕐 Duration
2 Failing492 Passing2 Skipped111m 12s

Details

Failing Tests
SpecTest
lke-create.spec.tsLKE Cluster Creation » can create an LKE cluster
lke-create.spec.tsLKE Cluster Creation with APL enabled » can create an LKE cluster with APL flag enabled

Troubleshooting

Use this command to re-run the failing tests:

yarn cy:run -s "cypress/e2e/core/kubernetes/lke-create.spec.ts"

@subsingh-akamai subsingh-akamai merged commit 03462e7 into linode:develop Feb 6, 2025
21 of 23 checks passed
Copy link

cypress bot commented Feb 6, 2025

Cloud Manager E2E    Run #7186

Run Properties:  status check failed Failed #7186  •  git commit 03462e7fa8: test: [M3-8914] - POC: Centralized Locators for Linode Page in Cypress UI Automa...
Project Cloud Manager E2E
Branch Review develop
Run status status check failed Failed #7186
Run duration 31m 42s
Commit git commit 03462e7fa8: test: [M3-8914] - POC: Centralized Locators for Linode Page in Cypress UI Automa...
Committer subsingh-akamai
View all properties for this run ↗︎

Test results
Tests that failed  Failures 1
Tests that were flaky  Flaky 4
Tests that did not run due to a developer annotating a test with .skip  Pending 2
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 499
View all changes introduced in this branch ↗︎

Tests for review

Failed  cypress/e2e/core/linodes/smoke-linode-landing-table.spec.ts • 1 failed test

View Output Video

Test Artifacts
linode landing checks for non-empty state with restricted user > checks restricted user with read access has no access to create linode and can see existing linodes Screenshots Video
Flakiness  linodes/linode-config.spec.ts • 1 flaky test

View Output Video

Test Artifacts
Linode Config management > End-to-End > Boots a config Screenshots Video
Flakiness  stackscripts/create-stackscripts.spec.ts • 1 flaky test

View Output Video

Test Artifacts
Create stackscripts > creates a StackScript with Any/All target image Screenshots Video
Flakiness  linodes/search-linodes.spec.ts • 1 flaky test

View Output Video

Test Artifacts
Search Linodes > create a linode and make sure it shows up in the table and is searchable in main search tool Screenshots Video
Flakiness  images/create-image.spec.ts • 1 flaky test

View Output Video

Test Artifacts
create image (e2e) > create image from a linode Screenshots Video

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge! Testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants