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

Break up e2e Error UI tests and fix flakiness #7691

Closed
wants to merge 7 commits into from

Conversation

chrisbreiding
Copy link
Contributor

@chrisbreiding chrisbreiding commented Jun 12, 2020

User facing changelog

N/A - Internal only

Additional details

  • Breaks up the e2e tests into multiple files so they don't take too long and time out.
  • Fixes an issue where one of the tests plus firefox garbage collection caused a failure.
  • Adds better error output, so if a test do fail, it's easier to find which one it is.

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Jun 12, 2020

Thanks for taking the time to open a PR!

@cypress
Copy link

cypress bot commented Jun 12, 2020



Test summary

7652 0 119 0


Run details

Project cypress
Status Passed
Commit 188bbce
Started Jun 19, 2020 1:17 PM
Ended Jun 19, 2020 1:24 PM
Duration 07:02 💡
OS Linux Debian - 10.1
Browser Multiple

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@chrisbreiding chrisbreiding removed the request for review from jennifer-shehane June 12, 2020 14:25
@chrisbreiding chrisbreiding marked this pull request as draft June 12, 2020 14:25
@chrisbreiding
Copy link
Contributor Author

Looks like the timeouts were hiding some legitimate failures/flakiness. If I have time, I'll look into fixing them. Or if the isolated runner PR gets in first, will just move these tests to that.

@chrisbreiding chrisbreiding marked this pull request as ready for review June 18, 2020 20:13
@chrisbreiding chrisbreiding changed the title Break up e2e Error UI tests so they don't hang Break up e2e Error UI tests and fix flakiness Jun 18, 2020
@brian-mann
Copy link
Member

The isolated runner PR has been in for about a week now - get with @bkucera on transferring these over.

@chrisbreiding
Copy link
Contributor Author

I experimented with moving them to the isolated runner. Unfortunately, it doesn't look like it will work for these tests as it currently stands. Since it evals the test function, the errors don't end up with a stack trace. It might be possible to get it to work with some updates to the isolated runner.

In any case, it would be valuable to merge the changes in this PR now as they are, since the flakiness of these tests are slowing down other PRs. I can take another look at moving them to the isolated runner next week.

Copy link
Member

@jennifer-shehane jennifer-shehane left a comment

Choose a reason for hiding this comment

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

It'd be a bit nicer if the sets of tests had some kind of logical grouping instead of 1, 2, 3 - it's a bit unclear if I needed to add a new test where to put it. But, this should be an improvement overall. 👍

@jennifer-shehane jennifer-shehane dismissed their stale review June 22, 2020 05:19

sorry, have some more things to say

Copy link
Member

@jennifer-shehane jennifer-shehane left a comment

Choose a reason for hiding this comment

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

There are a couple of failures in the 8_errors_ui now after these changes. And...they're a bit confusing to debug I think. Maybe I don't understand the structure well enough. The message is below.

  1. So, what was the actual state of this test? Did it pass when it was supposed to fail or did it fail when it was supposed to pass?
  2. Is there any way to print which test file failed? It'd be a lot quicker to debug if I could jump straight to the file? Right now I look up onResponse assertion failure and there are 4 matches, so then I have to track down which file/line this one was.

Screen Shot 2020-06-22 at 11 48 35 AM

@jennifer-shehane jennifer-shehane mentioned this pull request Jun 24, 2020
4 tasks
@chrisbreiding
Copy link
Contributor Author

Closing in favor of #7831

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flaky internal test: Timeout of 180000ms exceeded in 8_error_ui_spec
3 participants