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

Activity Log settings e2e #2753

Merged
merged 5 commits into from
Jul 9, 2024
Merged

Activity Log settings e2e #2753

merged 5 commits into from
Jul 9, 2024

Conversation

balanza
Copy link
Member

@balanza balanza commented Jul 8, 2024

Description

Provide e2e tests for the Activity log section on the settings page.

This PR includes a refactor to the production code (eace365) that simplifies focusing on elements by segmenting the HTML with <section> (no UI change is involved).

@balanza balanza force-pushed the actity-log-settings-e2e branch from 2f03212 to 4c09d30 Compare July 8, 2024 08:08
@balanza balanza enabled auto-merge (squash) July 8, 2024 08:08
@balanza balanza disabled auto-merge July 8, 2024 08:08
@balanza balanza enabled auto-merge (squash) July 8, 2024 08:08
nelsonkopliku
nelsonkopliku previously approved these changes Jul 8, 2024
Copy link
Member

@nelsonkopliku nelsonkopliku left a comment

Choose a reason for hiding this comment

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

Looks good!

Just a doubt about whether we are doing the same thing in beforeEach and in each test.

cy.get('@initialRetentionTime').then((text) =>
section()
.get('[aria-label="retention-time"]')
.should('have.text', text)
Copy link
Member

Choose a reason for hiding this comment

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

how does this change from what we do in beforeEach? Are they both needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

In the beforeEach clause we read the initial text, here we ensure the test starts from there.

It's indeed a step that never fails, I mostly added it for completing the test case (start from a value x, get another value y). Should we remove it? I see no harm, I wonder if it can work as a "safety net" for regressions.

Copy link
Member

Choose a reason for hiding this comment

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

No need to remove, just wanted to understand the rationale, and make sure it was not accidentally like that.

test/e2e/cypress/e2e/settings.cy.js Show resolved Hide resolved
@@ -677,4 +677,133 @@ context('Settings page', () => {
});
});
});

describe('Activity log', () => {
// Elements must be filtered to match only the ones relate to the Activity Logs.
Copy link
Member

Choose a reason for hiding this comment

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

interesting approach.
Another way that I find helpful in order to get specific elements and avoid weird css selectors is using custom classes/labels/names (take a look at SUMA Credentials Management section)
No need to change anything, just wanting to share.

Copy link
Member Author

Choose a reason for hiding this comment

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

First and foremost, I was experimenting thus I'm not sure my approach is good.

We have to face 2 orders of problems here:

  1. to select the element in the right section
  2. to select an element that might have been re-rendered.

Regarding 1, maybe selecting even cypress docs says that selecting by text is not a best practice (https://docs.cypress.io/guides/references/best-practices#Text-Content). Perhaps we can assign a specific class/ID to every section? Quirk: models don't fall under any section 🙃
I'm considering a refactor, though.

About 2, I found no other option than to use helper functions. I wonder if it's a huge burden for performance.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I like that piece of document. I also find using highly brittle selectors that are subject to change (which is not the case here) a pain 🙈
All good from my side

test/e2e/cypress/e2e/settings.cy.js Show resolved Hide resolved
@balanza balanza disabled auto-merge July 8, 2024 16:10
@balanza balanza requested a review from nelsonkopliku July 8, 2024 16:29
@balanza balanza dismissed nelsonkopliku’s stale review July 8, 2024 16:30

let's review your comments togheter

@balanza balanza force-pushed the actity-log-settings-e2e branch from 124ad3d to dc9b268 Compare July 9, 2024 08:57
@balanza balanza enabled auto-merge (squash) July 9, 2024 08:57
@balanza balanza force-pushed the actity-log-settings-e2e branch from dc9b268 to 7080165 Compare July 9, 2024 09:18
@balanza balanza force-pushed the actity-log-settings-e2e branch from 7080165 to bb74f54 Compare July 9, 2024 09:19
@balanza balanza merged commit a738fc0 into main Jul 9, 2024
5 checks passed
@balanza balanza deleted the actity-log-settings-e2e branch July 9, 2024 09:19
@nelsonkopliku nelsonkopliku added enhancement New feature or request test labels Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request test
Development

Successfully merging this pull request may close these issues.

2 participants