-
Notifications
You must be signed in to change notification settings - Fork 94
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
Cypress: Avoid failing tests due to page reload conditions causing exceptions #1908
Conversation
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.
Looks good to me.
I don't think there are any exceptions during these steps that we would miss.
We need to make sure to check the actual test is done before running the after step. So tests should end with an assertion that checks if the last step has really completed and succeeded. But i think that's something we want independently of this change.
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.
removed duplicate
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 I had a look at the tests to check if they actually check things are done. All tests follow the share link to test it and make sure the editor renders fine. So that's cool.
But turns out that one test logs out already in order to test the public share link. This seems to be the only test that blows up in the after function. I suspect that's because we attempt to logout twice.
Catching the exception created by the duplicate logout attempt does not seem like the right approach to me. I think we should refactor the test instead to only loggout when needed.
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.
Ah... i was confused. it is a different test that does the logout. so that's not what's failing here. Sorry for the noise.
Signed-off-by: Julius Härtl <jus@bitgrid.net>
246ae8b
to
44f29af
Compare
This should hopefully avoid failures like in https://github.com/nextcloud/text/runs/3961812279 where the page navigation in the after all hook caused an exception that is thrown which then marks a test as failed. Instead we just ignore that since the page is navigating away.