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

Changing the timeout logic. #465

Merged
merged 1 commit into from
Jun 14, 2023

Conversation

liambutler
Copy link

Resolves issue #464

By moving away from a retry countdown and instead using a timestamp comparison on each iteration, waitUntil() should timeout closer to the user-defined timeout.

@liambutler
Copy link
Author

Hi @NoriSte, how does this look to you?

I've tested against my own project using Cypress and all was fine, 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

@NoriSte
Copy link
Owner

NoriSte commented Jun 8, 2023

Hey Liam!!!
First of all: sorry for the radio silence 🙏 I am super busy this period but I know the feeling of not receiving responses after participating in OSS projects...

Anyway: I just pushed on the master branch the commits that allow you to run everything properly and to add new tests dedicated to the "new" functionality. If you rebase your branch over master everything should work. Thank you and sorry for the delay (and sorry in advance also for the future delay)

@liambutler
Copy link
Author

Hi Stafano!

Absolutely no need to apologise ☺️

I've rebased, run all of the existing tests, and they all pass.

Screenshot 2023-06-08 at 14 08 41

Given that the existing tests already cover the core functionality (re-running until timeout), I can't think of any useful tests to add. That said, if you can think of any scenarios that you believe would be beneficial, I'm happy to add them.

@NoriSte
Copy link
Owner

NoriSte commented Jun 8, 2023

I can't think of any useful tests to add

The tests could be

  1. Should run once a passed checkFunction that takes more than the timeout to be executed.
  2. Should run 4 times a passed checkFunction that takes 2 second with an interval of 1 second and a timeout of 10 seconds

These tests should fail if written and added on the masterbranch, and succeed on your branch.

That said, if you can think of any scenarios that you believe would be beneficial, I'm happy to add them.

Sure, let me know if you prefer to give up since you already dedicated some time here 😊

@liambutler
Copy link
Author

Sure, let me know if you prefer to give up since you already dedicated some time here

Great suggestions! I'll look into adding those 😊

@liambutler
Copy link
Author

Hi again!

I've added a couple of scenarios that fail on master and pass on my branch. Happy to add more if any edge cases have occurred to you

…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

Hi again!

I've added a couple of scenarios that fail on master and pass on my branch. Happy to add more if any edge cases have occurred to you

Thaaank you so much I just force-pushed a commit message which includes all the details 😊

@NoriSte
Copy link
Owner

NoriSte commented Jun 14, 2023

@all-contributors please add @liambutler for code and test

@allcontributors
Copy link
Contributor

@NoriSte

I've put up a pull request to add @liambutler! 🎉

@NoriSte NoriSte merged commit 146a775 into NoriSte:master Jun 14, 2023
@NoriSte
Copy link
Owner

NoriSte commented Jun 14, 2023

🎉 This PR has been released in version 2.0.0 🎉

@NoriSte
Copy link
Owner

NoriSte commented Jun 14, 2023

Thank you so much @liambutler !!!! 🤗

@NoriSte
Copy link
Owner

NoriSte commented Jun 14, 2023

Uhm... I made only a mistake... by force-pushing the commit you do not result as the author anymore. So even if I added you to the "all-contributor" file, the contribution does not show up in your timeline 😞 I'm sorry, I was not aware of it 😞

@liambutler
Copy link
Author

Uhm... I made only a mistake... by force-pushing the commit you do not result as the author anymore. So even if I added you to the "all-contributor" file, the contribution does not show up in your timeline 😞 I'm sorry, I was not aware of it 😞

Not a problem at all! I'm just glad it's fixed ☺️

@liambutler liambutler deleted the fix/change-timeout-logic branch June 20, 2023 07:36
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.

2 participants