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-8744] - Reduce Cypress flakiness in Placement Group deletion tests #11107

Conversation

jdamore-linode
Copy link
Contributor

@jdamore-linode jdamore-linode commented Oct 15, 2024

Description 📝

This PR attempts to address a specific trigger of flakiness in the Cypress Placement Group deletion tests, resulting in this failure:

CypressError: Timed out retrying after 80050ms: `cy.click()` failed because the page updated while this command was executing. Cypress tried to locate elements based on this query:

(...)

Recent improvements to our React Query useAllLinodesQuery hook allow components to display cached data while waiting for fresh data to be fetched. In the case of the Placement Group deletion tests, however, this introduced flakiness stemming from re-renders that occur once the HTTP request resolves. This appears to affect the tests only and doesn't impact real users as far as I'm aware.

This works around the React Query behavior by introducing calls to cy.wait() for various requests to the Linodes instances endpoint, and by opening the Placement Group deletion modal, closing it, and then re-opening it to ensure that all relevant HTTP requests have resolved by the time the modal has opened, reducing the risk of re-renders occurring during Cypress interactions[1].

  1. This is because the Placement Group landing page fetches Linodes upon opening a Placement Group's deletion modal, but has state to keep track of which Placement Group was most recently selected. As a result of this design, opening the deletion modal a second time does not trigger another HTTP request, allowing us to work around the re-render issue.

Changes 🔄

  • cy.wait for various requests to the Linodes instances endpoint to resolve
  • Open, close, and re-open the deletion modal before attempting to interact with it to reduce the risk of React re-renders during Cypress interactions
  • Remove some unneeded assertions for the last test (this has nothing to do with test flake)

Target release date 🗓️

N/A

How to test 🧪

⚠️ Note that this test already had very slight flakiness issues before the changes to useAllLinodesQuery were added. This PR only seeks to address the recent increase flakiness, so you may still encounter some flakiness when running the tests. You can disregard any failures that do not mention cy.click failing.

When I ran this spec on repeat against develop last week, I observed that at least one test failed 90% of the time (although I don't think it has been that bad in CI). When I ran the spec 10 times on repeat with these changes in place, I observed only 1 failure (and it was not a cy.click() failure).

yarn cy:run -s "cypress/e2e/core/placementGroups/delete-placement-groups.spec.ts"

As an Author I have considered 🤔

Check all that apply

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 Splitting feature into small PRs
  • ➕ Adding a changeset
  • 🧪 Providing/Improving test coverage
  • 🔐 Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

@jdamore-linode jdamore-linode requested a review from a team as a code owner October 15, 2024 23:11
@jdamore-linode jdamore-linode requested review from AzureLatte and removed request for a team October 15, 2024 23:11
Comment on lines -521 to -537

// Unassign each Linode.
cy.get('[data-qa-selection-list]').within(() => {
// Select the first Linode to unassign
const mockLinodeToUnassign = mockPlacementGroupLinodes[0];

cy.findByText(mockLinodeToUnassign.label)
.should('be.visible')
.closest('li')
.within(() => {
ui.button
.findByTitle('Unassign')
.should('be.visible')
.should('be.enabled')
.click();
});
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These interactions/assertions aren't needed for this test since the point is to confirm that the error message doesn't persist after re-opening the deletion dialog.

@jdamore-linode jdamore-linode requested a review from a team as a code owner October 16, 2024 00:01
@jdamore-linode jdamore-linode requested review from mjac0bs and hkhalil-akamai and removed request for a team October 16, 2024 00:01
@bnussman-akamai bnussman-akamai added the e2e Indicates that a PR touches Cypress tests in some way label Oct 16, 2024
Copy link
Contributor

@hkhalil-akamai hkhalil-akamai left a comment

Choose a reason for hiding this comment

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

This is a huge improvement in the flakiness of this test. I wonder what the remaining source of flakiness is...

Nice work @jdamore-linode!! 🎉

This PR (passed 9/10 times) Develop (passed 4/10 times)
Screenshot 2024-10-16 at 11 34 54 AM Screenshot 2024-10-16 at 11 48 56 AM

@@ -498,7 +568,7 @@ describe('Placement Group deletion', () => {
'not.exist'
);

// Click "Delete" button next to the mock Placement Group to reopen the dialog
// Click "Delete" button next to the mock Placement Group to reopen the dialog.
Copy link
Contributor

Choose a reason for hiding this comment

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

Love the attention to detail 😹

@hkhalil-akamai
Copy link
Contributor

Manually approved the GH Actions pipeline, @jdamore-linode you may have been removed from the GH org.

Copy link

Coverage Report:
Base Coverage: 86.96%
Current Coverage: 86.96%

Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

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

Even though it's obviously not ideal to have workarounds, the thorough in-line comments are great to explain what's going on here.

Thanks for investigating and improving this one. 🚀 (I ran this locally with a repeat 20 and only experienced one test failure, and it was on a timeout span element that evidently existed when it shouldn't have.)

@mjac0bs mjac0bs added the Approved Multiple approvals and ready to merge! label Oct 16, 2024
@jdamore-linode jdamore-linode merged commit f534be7 into linode:develop Oct 16, 2024
22 of 23 checks passed
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! e2e Indicates that a PR touches Cypress tests in some way
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants