-
Notifications
You must be signed in to change notification settings - Fork 20
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
feat(RHINENG-8253): Update risk label in cluster page #706
Conversation
/retest |
I've restarted like a million times all is working... |
7dc087f
to
7250657
Compare
cypress/utils/table.js
Outdated
@@ -89,6 +89,8 @@ function checkFiltering( | |||
checkTableHeaders(tableHeaders); | |||
} else { | |||
cy.get('[data-ouia-component-id=loading-skeleton]').should('not.exist'); | |||
// eslint-disable cypress/no-unnecessary-waiting | |||
cy.wait(1000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a very good example of where we actually don't need unnecessary waiting :-)
Can we think of why our tests are timing out or not able to find the expected data under the columns? Is it the fact that the table is still in the loading state? If so, what should appear if we consider the table loaded? Instead of waiting generic 1 second, can we just wait for some of the first row data to appear?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I wanted to give it a try if it eventually helps something, looks like it doesn't
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Loading state I've fixed few lines below by adding this
cy.get('[data-ouia-component-id=loading-skeleton]').should('not.exist');
Waits until the skeleton is gone
cypress.config.js
Outdated
@@ -18,4 +18,5 @@ module.exports = defineConfig({ | |||
return config; | |||
}, | |||
}, | |||
retries: 2, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this was introduced earlier already on master. But I believe this is a general anti-pattern when dealing with flaky tests. I strongly discourage from using any retries since we will have even less insights into why our tests or our UI is flaky.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the end this usually didn't help with anything, if it failed once then it failed in the retries as well. But I agree it shouldn't be there
🎉 This PR is included in version 1.42.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
RHINENG-8253