-
Notifications
You must be signed in to change notification settings - Fork 50
fix: flaky behavior of previously added test #2893
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
Conversation
genehynson
left a comment
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.
nice - glad it was an easy fix. The CI ran the test 20x and it passed every time so I'm good with this change. Just fix the typo dshboard -> dashboard
| cy.getByTestID('nav-item-dashboards').click() | ||
| cy.getByTestID('dashboard-card--name').click() | ||
| cy.getByTestID('page-title').type('dashboard') // dashboard name added to prevent failure due to downloading JSON with a different name | ||
| cy.getByTestID('page-title').type('dshboard') // dashboard name added to prevent failure due to downloading JSON with a different name |
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.
the dshboard name caused issues last time for OSS, can we just name it dashboard please? See where I fixed it previously here: #2808
|
|
On the first CI run, the test failed several times. I will try to make further adjustments to ensure stability of the test. |
a256948 to
a15ef37
Compare
genehynson
left a comment
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 don't want to skip this test in firefox
|
|
||
| // Skipping until https://github.com/influxdata/ui/issues/2864 is resolved by Bonitoo | ||
| it.skip('creates a dashboard and downloads JSON', () => { | ||
| it('creates a dashboard and downloads JSON', {browser: '!firefox'}, () => { |
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.
why are we skipping this in firefox?
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 tried to solve the flaky behavior of the test. I managed to fix the behavior in Chrome, but Firefox kept failing from time to time. I changed the readfile several times to try out possible fixes, but none worked. Because there is no other way to ensure that the file downloaded, I decided to skip this test in Firefox to ensure it's stability. UI itself shouldn't be the problem, since I was unable to make the problem appear manually.
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.
If we're going to skip this test in Firefox, I'd like to see specific examples of the failure in Firefox that showcase the issue and an explanation on why there is no work around. This is the first Cypress test in the hundreds that we have that does not work in Firefox. Could you please provide this info?
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.
So far as I can tell, there is no work around. Cy.readfile is the only option to check content of the cypress/downloads folder. Cy.fixture searches for files form the cypress/fixtures/ folder.
The download process is handled internally in the browser (via blob). Because of that, checking the initiation of the download process is not an option eighter.
Also, there is not a notification which would indicate that the file downloaded successfully, like with the second export option (copy to clipboard).
In Firefox, the JSON file occasionally downloaded without it's content (NULL). Because of that, I added ".should('not.be.null')", to make the test fail due to a failure caused by the UI. Unfortunately, the problem persisted. When this error appears, cy.readfile is unable to read the mentioned file.
Similar problem appeared in Chrome, when I moved the removal of previously downloaded file to "beforeEach".
Putting it back in "before" fixed this behavior for Chrome.
I've been running the dashboardsIndex test for a few hours to provide you with some screenshots of the failure, but it hasn't crashed a single time. I will try to commit a version without excluding Firefox.
1a9507c to
1b73d2f
Compare
genehynson
left a comment
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.
thanks for getting this working!
|
Closes: #2864 |
This PR fixes flaky behavior of the test added in #2748.
Removal of the downloaded file has been moved back to before from beforeEach.
I ran the test several times, both in Chrome and Firefox headless mode. All the runs were completed successfully.