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

Timeout can take longer than defined #464

Closed
liambutler opened this issue May 15, 2023 · 6 comments
Closed

Timeout can take longer than defined #464

liambutler opened this issue May 15, 2023 · 6 comments

Comments

@liambutler
Copy link

Love this plugin, has been so useful! However, there's something that was puzzling me for ages.

I couldn't understand why my waitUntil() function was set to time out after 60000ms, but was taking longer than two minutes to fail.

It's because this line doesn't factor in the time that it takes to perform an action.

So if I have the following config:

      {
        timeout: 60000,
        interval: 1000,
        errorMsg: `Could not find ${id} in ${documentType} after 60s`,
      }

and I'm looping an API call that takes 1s to resolve, it'll perform that call 60 times, with a 1 second interval between. Hence a one minute timeout takes two minutes.

@NoriSte
Copy link
Owner

NoriSte commented May 15, 2023

Oh, you are right!

I'm pretty busy these days but I try to jump on a fix as soon as possible!! Thank you so much for digging into the problem 😊

@liambutler
Copy link
Author

Hey @NoriSte, I believe that I have come up with a fix for this. Would it be possible for me to be granted access so I can raise a PR?

@NoriSte
Copy link
Owner

NoriSte commented May 22, 2023

Hey @NoriSte, I believe that I have come up with a fix for this. Would it be possible for me to be granted access so I can raise a PR?

Thank you!! Dumb question: doesn't GitHub allow you to follow the standard contribution process? That is

  1. You fork the repo
  2. You create a new branch in the fork
  3. You open a PR in this repo by comparing your fork's branch to this repo's master branch

?

@liambutler
Copy link
Author

Not a dumb question at all! Thanks for patiently explaining it to me. This is my first time pushing a fix to an open repo

I think it's because I cloned rather than forked. Let me try again :)

liambutler added a commit to liambutler/cypress-wait-until that referenced this issue May 22, 2023
@liambutler
Copy link
Author

Figured it out! PR here: #465

I've tested against my own project using Cypress, but I wasn't able to get results from running npm test. Cypress wasn't happy because the repo has the old cypress.json rather than the cypress.config.js favoured by v10 and above

liambutler added a commit to liambutler/cypress-wait-until that referenced this issue Jun 8, 2023
NoriSte added a commit to liambutler/cypress-wait-until that referenced this issue Jun 14, 2023
…tion

Before this commit, the number of retries was calculated ignoring the checkFunction duration.

As a result, a 5s timeout with a 0.5s interval resulted in 10 retries.
If the checkFunction takes 10 seconds, 10 retries mean waiting for 100 seconds, violating the 5s timeout.

This commit fixes the problem by checking, after each checkFunction invocation, what is the elapsed time and stops retrying in case the timeout is over.

BREAKING CHANGE: The timeout is now respected even if checkFunction takes a long time. As a result,
you could face that your checkFunction runs less times.

fix NoriSte#464
@NoriSte
Copy link
Owner

NoriSte commented Jun 14, 2023

🎉 This issue has been resolved in version 2.0.0 🎉

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

No branches or pull requests

2 participants