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

feat: Add timeout period to retry error message #9123

Merged
merged 7 commits into from
Dec 16, 2020

Conversation

sainthkh
Copy link
Contributor

@sainthkh sainthkh commented Nov 9, 2020

User facing changelog

Retry timeout error message now contains timeout period.

Additional details

  • Why was this change necessary? => It wasn't clear how long the timeout was.
  • What is affected by this change? => N/A
  • Any implementation details to explain? => It simply shows the difference.

How has the user experience changed?

Before:

Screenshot from 2020-11-09 16-11-04

After:

Screenshot from 2020-11-09 16-09-56

Questions

I intentionally stopped the CI because there are some problems to solve.

  • Is error message OK? Maybe there might be better copy.
  • Should we should the timeout period number every time? If it's the default, should we hide it?
  • Maybe it's not an important information and just closing the issue might be the solution.

Without these questions cleared, editing error messages in the tests is meaningless.

PR Tasks

  • Have tests been added/updated?
  • Has the original issue or this PR been tagged with a release in ZenHub?

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Nov 9, 2020

Thanks for taking the time to open a PR!

@sainthkh sainthkh changed the title Add timeout period to retry error message feat: Add timeout period to retry error message Nov 9, 2020
@jennifer-shehane
Copy link
Member

I'm planning to ask some of the team members about the wording here.

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.

Sorry for the delay, just wanted to verify the wording before you invest more time into the PR. Should be good to move forward with the suggested changes.

packages/driver/src/cypress/error_messages.js Outdated Show resolved Hide resolved
@sainthkh sainthkh force-pushed the issue-5781 branch 2 times, most recently from 1794f80 to ce6188e Compare November 20, 2020 05:24
@sainthkh sainthkh marked this pull request as ready for review November 20, 2020 05:41
@sainthkh
Copy link
Contributor Author

Flaky failures.

@sainthkh
Copy link
Contributor Author

Rebased and merged conflicts. Flaky failure.

@jennifer-shehane jennifer-shehane self-requested a review December 14, 2020 10:02
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.

I'm sorry. I thought I commented in here, but I must have forgotten. We decided to go back to the colons : because after seeing the changes there are some error messages that are strangely capitalized like.

Timed out retrying after 4000ms. cy.click() failed...
Timed out retrying after 50ms. expected '<button>'...

So they should be:

Timed out retrying after 4000ms: cy.click() failed...
Timed out retrying after 50ms: expected '<button>'

@jennifer-shehane jennifer-shehane self-requested a review December 15, 2020 07:18
packages/server/__snapshots__/3_config_spec.js Outdated Show resolved Hide resolved
packages/server/__snapshots__/5_stdout_spec.js Outdated Show resolved Hide resolved
packages/server/__snapshots__/5_stdout_spec.js Outdated Show resolved Hide resolved
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.

"Timed out Retrying" should indicate timeout value
2 participants